-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TCK Challenge] SerializersCustomizationCDITest assumes different behaviour than non-CDI SerializersCustomizationTest #344
Comments
Yeah, the behavior should be the same whereas you run on CDI environment or plain Java SE mode. To find the correct behavior, the specification might help or the related part of the RFC mentioned in the spec. |
@struberg one question, you said that:
But in the link I see Regarding your comment about what would happen if you add
It should look like:
|
hi! The difference is that the non-CDI version scans over the events UNTIL it hits START_OBJECT.
Which is imo correct as the beginning of the json content for the "animals" attribute is a START_ARRAY. This is fundamentally different to the CDI version which expects START_OBJECT as first event. |
Now back to the 2nd question: My intention was not that the "size" attribute should be part of the Java model but ONLY in JSON. It should still map to a
In this case the Maybe the example is more realistic if we have a json with an |
Thanks for clarification @struberg. Basically you want to have the same logic we use in I agree, although the JSON comes from the TCK test, so I don't expect it is going to change. |
+1, if CDI and standalone tests are not 1-1 (at injection exception) it is a TCK bug which must be fixed in next TCK delivery and test must be excluded in between. An implementation passing this does not respect the spec which expects both to behave the same JSON wide. @struberg do you want to PR the fix? |
I want to have the same logic we have in the non-CDI AnimalListDeserializer also in the CDI version. Because I honestly believe that START_OBJECT is plain wrong. The Object which gets converted is the json array. So the correct event is START_ARRAY. Having it the same like in the non-CDI version at least makes the TCK pass for both impls. I'll ship a PR which allows both. |
The SerializersCustomizationCDITest did assume a different behaviour than the non-CDI SerializersCustomizationTest.
It seems that the deserialiser in SerializersCustomizationCDITest is expecting to see a START_OBJECT of the actual items instead of the actual current JsonValue event which is a START_ARRAY.
https://github.com/jakartaee/jsonb-api/blob/master/tck/src/main/java/ee/jakarta/tck/json/bind/cdi/customizedmapping/serializers/model/serializer/AnimalListDeserializerInjected.java#L44
This is different to the (imo correct) implementation in the deserialiser of the non-CDI TCK test which expects to see the START_ARRAY.
https://github.com/jakartaee/jsonb-api/blob/master/tck/src/main/java/ee/jakarta/tck/json/bind/customizedmapping/serializers/model/serializer/AnimalListDeserializer.java#L40
The reason why I think that START_ARRAY is correct is that only this way we can support more complex scenarios.
The current value to parse is
But imagine having a more complex JSON:
For this case the jsonParser.next() should imo point at the START_OBJECT just before the 'size' attribute, right?
The text was updated successfully, but these errors were encountered: