Could not decode "Pellet" to enum and others

[mikfire]

When I start brewtarget, I get several hundred errors like:

[11:46:51.246] (1di2uj4mv4) ERROR : int {anonymous}::stringToEnum(const ObjectStore::TableDefinition&, const ObjectStore::TableField&, const QVariant&) Could not decode "Both" to enum when mapping column htype to property type for hop so using 0  [database/ObjectStore.cpp:241]
[11:46:51.246] (1di2uj4mv4) ERROR : int {anonymous}::stringToEnum(const ObjectStore::TableDefinition&, const ObjectStore::TableField&, const QVariant&) Could not decode "Pellet" to enum when mapping column form to property form for hop so using 0  [database/ObjectStore.cpp:241]
[11:46:51.246] (1di2uj4mv4) ERROR : int {anonymous}::stringToEnum(const ObjectStore::TableDefinition&, const ObjectStore::TableField&, const QVariant&) Could not decode "Aroma" to enum when mapping column htype to property type for hop so using 0  [database/ObjectStore.cpp:241]

I am guessing I get at least one per hop in the database, but it may be as much as three.

All of the hops are now classified as boil, bittering and leaf for use, type and form, respectively. This is also being immediately written to the database, so cleaning it up is likely to be annoying.

This happens in both the develop branch and the v3.0.5 tag. I have tried this with my existing databases as well as wiping everything clean (ie, rm -rf ~/.config/brewtarget) and starting brewtarget.

This issue does not occur in v3.0.4. I will attempt a bisect to further figure this out.

[matty0ung]

Ah, hmm, sorry, this is probably my fault. I'll have a look. I think I may have inadvertently brought across bits of the BeerJson work that are not yet bullet-proof. (BeerJson adds a bunch of things that we don't currently have. I've been working on Hop as my first "test case".)

[matty0ung]

I see it now. I must have been having a brainstorm. New code has additional/modified enums that ain't going to work without the DB upgrade. I think I can manually revert back the bits that came across prematurely. (Meanwhile, if you want to see BeerJson import and export sort of working for Hops, it's live in Brewken. Still need more testing on the export, but I've had the import pulling in 100 records at a time.)

[mikfire]

So I merged 5c04c46 and the errors persist. I believe the problem is that we store the hop type (for example) as "Aroma" in the database but Hop::typeStringMapping is expecting "aroma". The change in case is causing the problem.

The next issue is that if I fix things, I still get the error because somewhere in my database I now have hops with a form of "extract" and "pellet". I could probably dig into my database and fix those. But that would not be as much fun.

My best idea is to stop storing this data as a string in the database. Yes, it makes it difficult for a human to read the raw data. But humans are not supposed to be reading the raw data. I really dislike this translation to and from strings. It is incredibly fragile and breaks over the smallest things, like changes in case. This is a large change, requiring schema changes, etc.

My second idea is if we are going to insist on storing the data as a string and then converting it, then we need to quash case before we do the conversion to enum. This is a larger change than it may seem, and my first few tries at it have not worked.

My third, and least best but easiest to implement, idea is to modifying the stringToEnum to first search for the string as given and then search for the squashed-case string if the first search didn't work. I actually have this idea working, and it is a very tiny change. It is more hiding the problem then fixing it, but it works.

[matty0ung]

Ah, bother. It was too much to hope this would be a quick fix.

I like your idea of doing the quash case as a fallback. Presumably it can all be handled in the code in utils/EnumStringMapping.cpp? Most of the time most people would never need the fallback, but it would save us in cases such as the current one. It is a bit of a hack, but, as you say, it's also the smallest change.

More generally, it's a good question about how to store enums in the database.

It seems there are arguments for and against every approach - eg debugging is harder with raw enums and you have to be disciplined about never changing the underlying int values. At the same time, the points you make about strings are entirely valid.

Up to now, where I have been generally trying to head to is to have a standard enum to string mapping stored in the class, that can be used for DB storage, BeerJSON, and logging. (BeerXML would need its own mapping because its serialisations are different from BeerJSON.) Eg see https://github.com/Brewken/brewken/blob/bb9d3cd01b5a3c9ef4bb5e35d1bf5a5003f3103a/src/model/Hop.h#L97 and https://github.com/Brewken/brewken/blob/bb9d3cd01b5a3c9ef4bb5e35d1bf5a5003f3103a/src/model/Hop.cpp#L34.

My thinking was we've got to have the mapping anyway for BeerJSON and (to a lesser extent) logging, so why not make the DB the same. You'll see at https://github.com/Brewken/brewken/blob/bb9d3cd01b5a3c9ef4bb5e35d1bf5a5003f3103a/src/database/DatabaseSchemaHelper.cpp#L545 that I was intending to do the case fixing on existing data at the same time as adding new fields etc to the DB for BeerJson.

(In passing I had a crazy idea about generating all the boillerplate code for the model so that we avoid bugs such as this

brewtarget/src/model/Salt.cpp

Line 58 in 40fc2af

m_amount_is_weight{namedParameterBundle(PropertyNames::Salt::isAcid ).toBool()},

, which I just came across today. On balance, my current feeling is doing automating the code generation would be more trouble/complexity than it's worth, and that we'll fix these bugs by improving code layout in the model classes. If the model were a lot bigger, I might pursue it though.)

[mikfire]

For being a horrible solution, squashing the case works astonishingly well and is about a 7 line change in util/EnumStringMapping.cpp. It is only invoked when the string can't be found otherwise, so shouldn't be much overhead until we move more towards beerjson. At which point, we can switch the order and still not invoke too much penalty.

As for the string v. enum, there isn't a good solution. You either accept the fragility of strings, or you accept more pain when tracing/debugging. I've tried both in the config file, and my opinion is neither worked well.

And I would prefer to avoid the automatic code generation. I am not a terribly proficient c++ coder, and reading your code is a challenge enough for me as it is. I would rather avoid making it more magic.

[matty0ung]

😃 Agreed on no more magic! It's often tempting to solve problems by adding just one more layer of abstraction, but there's a line between code that is "elegant" and that is "trying to be too clever by half".

I'm hoping BeerJSON support will be the last "big" change by the way - at least for the foreseeable future. Reading and writing actual JSON is not too tricky, but extending our data model to support all the new things (and new ways of measuring things) that BeerJSON brings has turned out to be quite a chunky task, even with the foundations of the BeerXML and Database layer work (which are both useful leg-ups). It will be neat when it's done, but I'm keen afterwards to get back to working in smaller increments!

[mikfire]

Oh god. And I am digging deep into the water chemistry table, trying to figure out why things that are weights are showing as things that are volumes and there's one poor neuron trying to say I already know the problem....

Which is to say, I just found that bug too. And that whole section is three kinds of messed up.

[matty0ung]

This should be fixed in the 3.0.6 release.