FIX: exception on setting enum based filter. #310
Conversation
* Instead of returning the enum object itself, we must return it's
ordinal value, as the database has an integer field.
|
This fixes #308 |
There was a problem hiding this comment.
It is always best to avoid using the ordinal of an Enum as a key or identifier. If an Enum is modified in future, then the ordinals can change and suddenly you get different Enums when resolving from the previous ordinals.
Instead it is better to statically assign a value to each enum and use that, for an example, see: https://github.com/facebook/rocksdb/blob/master/java/src/main/java/org/rocksdb/CompactionPriority.java#L11
|
You are right, this is fragile. It's also an antipattern which is repeated across quite a lot of DROID. I'll have a look at giving the enums a fixed ID, but a quick look shows a lot of places in DROID this happens. |
|
OK - this isn't a trivial change. It's easy enough to give the enums in that method a "getID()" method and assign a permanent one on construction. But then it has to match what the SQL puts in the database, and this uses methods generic to enum (i.e. ordinal()) to convert an enum value into an int to store in the db. Conversion the other way around needs similar treatment, and all the filter dialogues and other things that touch this would need to be updated so they all use the same values. The biggest change would be to the SQL conversion code, as all it knows is it's got an enum, not a whole load of different enums all with getID() methods on them... |
|
@nishihatapalmer okay, so I will merge it as is. |
|
@adamretter It's a good point though - this is really fragile. I'll raise an issue so we don't forget about it. It's something that should probably be done, but quite carefully! |
|
Raised issue: #325 to cover the use of ordinal values in DROID in general. |
ordinal value, as the database has an integer field.