TCK cannot expect sorting on enum #831
Conversation
Signed-off-by: Nathan Rauh <[email protected]>
| @@ -36,9 +36,14 @@ public enum NumberType { | |||
| @jakarta.nosql.Column | |||
| private Short numBitsRequired; | |||
|
|
|||
| // Sorting on enum types is vendor-specific in Jakarta Data. | |||
| // Use numTypeOrdinal for sorting instead. | |||
There was a problem hiding this comment.
If you are already changing this entity why not put @Enumerated(STRING)
for semantic and that sounds better.
There was a problem hiding this comment.
If you are already changing this entity why not put
@Enumerated(STRING)for semantic and that sounds better.
If you like, I can add that too, but it isn't a solution on its own and doesn't fix the problem. Without the other changes I made here, the test would remain broken for providers that are neither JPA nor NoSQL.
There was a problem hiding this comment.
@njr-11 I know, but once you can change the tests, I recommend using String and updating the tests based on the String instead of int.
There was a problem hiding this comment.
@njr-11 I know, but once you can change the tests, I recommend using String and updating the tests based on the String instead of int.
I added Enumerated(STRING) for you under ba68544
but it is wrong to redo test assertions like you are suggesting in a .0.1 maintenance release. The scope of a fix in a maintenance should be limited to fixing the problem only, not refactoring tests or switching things around that aren't required to fix the bug.
There was a problem hiding this comment.
@njr-11 I know, but once you can change the tests, I recommend using String and updating the tests based on the String instead of int.
I added Enumerated(STRING) for you under ba68544 but it is wrong to redo test assertions like you are suggesting in a .0.1 maintenance release. The scope of a fix in a maintenance should be limited to fixing the problem only, not refactoring tests or switching things around that aren't required to fix the bug.
I agreed, and that is why I proposed to create those tests as a NoSQL extension:
There was a problem hiding this comment.
I agreed, and that is why I proposed to create those tests as a NoSQL extension:
Adding some NoSQL specific tests (and also some Jakarta Persistence specific ones) in this are is fine for the 1.1 release. It would be out of place for the 1.0.1 maintenance release and is not a substitute for fixing the bug that exists in 1.0 / 1.0.1 which this PR addresses.
| private NumberType numType; // enum of ONE | PRIME | COMPOSITE | ||
|
|
||
| @jakarta.nosql.Column | ||
| private int numTypeOrdinal; // ordinal value of numType |
There was a problem hiding this comment.
@njr-11 you changed to String, but you still using this one here: numTypeOrdinal.
Shall we remove this one and update the tests to work with the enum as String?
There was a problem hiding this comment.
@njr-11 you changed to String, but you still using this one here: numTypeOrdinal.
Shall we remove this one and update the tests to work with the enum as String?
To repeat what I said earlier, doing that "isn't a solution on its own and doesn't fix the problem. Without the other changes I made here, the test would remain broken for providers that are neither JPA nor NoSQL."
Fixes #830
This corrects the error in the TCK where it expects sorting on an enum attribute to be done in a particular way, which per the spec is vendor specific.
Because this fix is needed in the 1.0.1 maintenance release, it should be as minimal as possible to fix the TCK, which is achieved by adding an entity attribute for the ordinal value and switching a few places in the TCK to sort on that value instead of the enum.