On the new XML methods

[mikfire]

The changes I made with respect to the schema classes was done with the very explicit intent that all of the work to be done to support a new column (or table) could be easily managed in at most two or three files.

In the past, I've had to go chasing through 8 files and a god awful number of arrays, hashes and methods to add a single column. It sucked. I would really like to avoid recreating that situation. And it seems we are with the new work in BeerXML. An XML property has to be defined both in the appropriate schema file, and defined again in BeerXML.

It is entirely possible that I misunderstanding the new code and simply reacting because it seems far more complex than what we had. The first question I am asking, then, is am I understanding what I am reading correctly?

If I am understanding this properly, I think we need to clean this up. My 30 minute read of the code strongly suggests the right approach would be to completely isolate the XML stuff into the BeerXML class. The kxmlprop definitions would be removed and that field dropped from the PropertySchema.

Am I understanding what I am reading correctly, and is the new import code worth the effort?

[matty0ung]

I think we actually broadly agree. There is a follow-up piece of work I want to get to, which is to change the BeerXML export code to use the same data structures as the import code, at which point the kxml constants will no longer be required, and all the XML stuff will be in one place. I didn't do it in the original pull request because (a) it was already quite big and (b) it was useful to be able to check that data exported with the existing code imported fine with the new code. Now that pull request got approved, I'll come back to the rest of the XML work in the near future I hope.

As to whether the new import code was worth the effort, I'm not the most objective person to form a judgement(!) but I'm pleased that:

• after more than one person said BeerXML could not be validated against an XSD file, I managed to do just this (and, as a result, even detect typo errors in some of the sample BeerXML files hosted on www.beerxml.com)
• the new code tells you about bad XML (often including the problem line number in the input file) instead of silently crashing
• the new code tells you what it imported and what it skipped over instead of just returning to the GUI with no message
• once the save counterpart to the new code is written, various restrictions on enum names can totally disappear (see comments in the code such as // NOTE: way bad. We have a duplicate enum (Aroma) and // NOTE: BeerXML expects a space in "Very High", but not possible with enum. What to do?)

[mikfire]

I have thought a bit on splitting up the Database class.

I have some ideas if you are interested. Otherwise, I will wait and ask stupid questions after I've merged the PR.

[matty0ung]

Yes, definitely good to hear your ideas.

My initial approach has been to try to pull some of the class-specific knowledge out of the big central class. Eg, at the moment, to create a new Water, Hop, etc, you call Database::newWater(), Database::newHop(), etc which first creates a new row in the relevant table and then asks the class to construct a new instance of itself and remember what table it's stored in. This means the DB code needs to know default values for every property of every stored class. If we did things the other way around -- call new Water(), new Hop(), etc and then ask to store in the DB -- then the DB layer no longer needs to know default values and just needs to map properties to-and-from columns.

Similarly, at the moment, when we read a Water/Hop/etc out of the db, we pass QSqlRecord into the constructor, which means the constructor needs to know the DB column names. But since we already know the mapping of columns to properties, we could instead just supply the constructor with a hashmap of properties->values, so now the class only needs to use its own property names to construct an instance of itself from a DB records.

[mikfire]

I think you will find that the Database class is fairly ignorant of either the database schema or the object properties. To use your example, Database::newWater() calls with Database::copy(other) or Database::newNamedEntity(). Database::newNamedEntity() simply does insert into water with default. Database::copy(other) queries the database using select * from water where id = [other.key]. It then uses a for-loop to extract every column name and value from the query, does some bind magic and then inserts.

The default values are actually defined when we build the database. For example, in postgresql the salt table looks a little like this:

                                 Table "public.salt"
      Column      |  Type   | Collation | Nullable |             Default
------------------+---------+-----------+----------+----------------------------------
 id               | integer |           | not null | nextval('salt_id_seq'::regclass)
 addto            | integer |           |          | 0
 amount           | real    |           |          | 0
 amount_is_weight | boolean |           |          | true

Database has no clue of what the values are. All of that is hidden in TableSchema, and Database simply calls TableSchema::generateCreateTable() when it needs to. TableSchema handles things like using true or false (postgresql) or 0 and 1 (sqlite) for boolean values. It handles things like autoincrementing ID fields. The complexity you are hoping to remove kind of has been, I think?

The reason we need Database::newWater and Database::newHop and ... is because of the signals. We generate a unique signal (newWaterSignal and newHopSignal and ..., respectively) for each object. This reduces the overall load on the system by making sure only those things interested in a new Water get a signal. Generating one generic newNamedEntitySignal would cause every object in the system to have to see if it was interested. We did that for a bit. It was NOT pretty and I am going to be very resistant to going back.

If you can fix the signals, I would be very happy. Until we do that better, I believe the most you will be able to do is to move the complexity around.
1/2

[mikfire]

I will admit, I screwed up the constructors. I took a best practice of using static assignments a bit too far. I think there's a better solution. I've a design idea. Let me code it up and see if it works.

Mapping from the database columns to the object properties is work that has to be done, it has to be done somewhere and it can only be simplified so far. Everything else is trying to make it easier for the humans.

Doing the work in the object seem more natural to me. While it is a little upsetting for the object to know about a QSqlRecord, that is a constant format regardless of the underlying database. And it is, to me, less upsetting than Database knowing about the objects.

It also means the object can do what it wants or needs to do with the data -- if it needs to do something weird or odd, the weirdness and oddness is isolated. If we put it in Database, that will move the oddness and weirdness to a place that is already weird and odd enough.

[matty0ung]

I see what you're saying. I guess my reason for not wanting to pass a QSqlRecord to an object is that the object shouldn't need to know how it gets serialised or stored. We already have two different ways of "storing" an object (in a SQL DB, in XML) and we will one day also have JSON (and might conceivably at a future point even support a NoSQL DB). The object's properties are a public part of its interface, whereas things like table names and column names are a private implementation detail of one particular sort of storage. So it feels better for the "storage" parts of the code to say "here are your properties" than "here is the way I store your properties". (I know I'm oversimplifying here because the database is the master storage whereas XML and JSON are just import/export formats, but I still feel there's something in the general point about which direction the knowledge has to be in.)

All that said, I'm still at early stages of playing around with this, doing some CRUD operations for Water in templated classes, so I expect I'll have more thoughts when I try to do harder stuff with Recipe! I agree that it's unlikely we can magic away that much complexity but I think we may be able to get the same result with some smaller classes. But let's see!