[TCK Challenge] SerializersCustomizationCDITest assumes different behaviour than non-CDI SerializersCustomizationTest

[struberg]

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

{ "animals" : [ { "type" : "cat", "cuddly" : true, "age" : 5, "furry" : true, "name" : "Garfield" , "weight" : 10.5}, { "type" : "dog", "barking" : true, "age" : 3, "furry" : false, "name" : "Milo", "weight" : 5.5}, { "type" : "animal", "age" : 6, "furry" : false, "name" : "Tweety", "weight" : 0.5} ] }

But imagine having a more complex JSON:

{
"animals": 
    {
    "size":3,
    "values": [ { "type" : "cat", "cuddly" : true, "age" : 5, "furry" : true, "name" : "Garfield" , "weight" : 10.5}, { "type" : "dog", "barking" : true, "age" : 3, "furry" : false, "name" : "Milo", "weight" : 5.5}, { "type" : "animal", "age" : 6, "furry" : false, "name" : "Tweety", "weight" : 0.5} ]
    }
}

For this case the jsonParser.next() should imo point at the START_OBJECT just before the 'size' attribute, right?

[jeanouii]

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.

[jbescos]

@struberg one question, you said that:

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

But in the link I see JsonParser.Event.START_OBJECT. So both (SE and CDI) are consistent.

Regarding your comment about what would happen if you add size, then AnimalShelterWithSerializer that currently looks like this:

public class AnimalShelterWithSerializer {
    @JsonbTypeSerializer(AnimalListSerializer.class)
    @JsonbTypeDeserializer(AnimalListDeserializer.class)
    private List<Animal> animals = new ArrayList<>();
...

It should look like:

public class AnimalShelterWithSerializer {
    private NewClass animals;
...
public class NewClass {
    private int size;
    @JsonbTypeSerializer(AnimalListSerializer.class)
    @JsonbTypeDeserializer(AnimalListDeserializer.class)
    private List<Animal> values = new ArrayList<>();

AnimalListDeserializer is to deserialize List<Animal> only.

[struberg]

hi! The difference is that the non-CDI version scans over the events UNTIL it hits START_OBJECT.

        while (jsonParser.hasNext()) {
            JsonParser.Event event = jsonParser.next();
            while (event == JsonParser.Event.START_OBJECT) {
                animals.add(animalDeserializer.deserialize(jsonParser,
                                                           deserializationContext, type));
                event = jsonParser.next();
            }
        }

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.

[struberg]

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

@JsonbTypeDeserializer(AnimalListDeserializer.class)
 private List<Animal> values;

In this case the AnimalListDeserializer has to cope with the size attribute.

Maybe the example is more realistic if we have a json with an "species":"Dog" attribute to fill instances of Dog extends Animal into the list?

[jbescos]

Thanks for clarification @struberg. Basically you want to have the same logic we use in AnimalListDeserializer for CDI.

I agree, although the JSON comes from the TCK test, so I don't expect it is going to change.

[rmannibucau]

+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?

[struberg]

Thanks for clarification @struberg. Basically you want to have the same logic we use in AnimalListDeserializer for CDI.

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.

[struberg]

What happened to this challenge?

[jbescos]

@struberg this is not the official repository of TCKs. We just copy-paste the tests so we eagerly know whether we are breaking the spec when we make any commit. I think I didn't express it clearly in this comment.

Could you forward this issue here, please?: https://github.com/jakartaee/platform-tck

[struberg]

It that's the case that the TCK doesn't get maintained over here anymore, then we should please delete the whole directory and add the link to the proper location to the readme!

[struberg]

And back again: the platform spec EXPLICITLY SAYS: "TCK Challenges MUST be filed via the specification project’s issue tracker using the label challenge and include the following information:"

The "specification project" is this very project here! I read this as that we still handle it the way we've done this the last 20 years: the EE umbrella project only is for umbrella challenges, and the specs handle their own challenges themselves. Is this interpretation correct @m0mus ?

[m0mus]

@struberg yes, you are right.