[#617]User defined deserializer always should be used before standard…

[api-from-the-ion]

… ones.

Created a correction for #617. I little bit of explanations: the culprit is in the TypeDeserializers#assignableCases. Here, the EnumDeserializer will be created for any Enum type. But here we are missing some objects needed to create a user deserializer. So we have to create it earlier in the call stack, directly in the DeserializationModelCreator#deserializerChainInternal, before the call of DeserializationModelCreator#typeDeserializer.

This way it would be effective for all classes, not just the enums.

On the other hand, I looked on the SerializationModelCreator. Here one first looks in the cache, then for user defined serializer, then for user defined adapter. And only after that the standard serializer would be created. So now both DeserializationModelCreator and SerializationModelCreator will share the same logic on creation of custom / standard elements.

And I forget an Signed-off-by: Anton Pinsky <[email protected]> - could I change it?

[api-from-the-ion]

On commit for #616. I've changed both EnumSerializer and EnumDeserializer, of course. Now they build the map in the constructor, and use it. The map is filled according to the PropertyModel#getxxxName. So the annotation value has effect.

In EnumDeserializer I needed a whole jsonbContext to get the models. Because of this, I've changed the TypeDeserializerBuilder to set and get the JsonbContext as parameter. There is still a getter for JsonbConfigProperties, but we can remove it. If we do this, we have to change another three classes which use this getter.

[Verdent]

Hello, thank you for your contribution!

Would it be possible to ask to to update also the class copyright years? :-)

[Verdent]

As far as I can tell, some of the copyright failures are caused by my mistake some time ago and will fix them. As of the rest, I might kindly ask you to fix them.

/home/runner/work/yasson/yasson/src/main/java/org/eclipse/yasson/internal/ReflectionUtils.java: Copyright year is wrong; is 2022, should be 2023
/home/runner/work/yasson/yasson/src/test/java/org/eclipse/yasson/defaultmapping/generics/model/LowerBoundTypeVariableWithCollectionAttributeClass.java: Copyright year is wrong; is 2022, should be 2023
/home/runner/work/yasson/yasson/src/test/java/org/eclipse/yasson/defaultmapping/generics/GenericsTest.java: Copyright year is wrong; is 2022, should be 2023

These are the ones I will create PR for tomorrow.

[api-from-the-ion]

Copyright - done, even if it wasn't mine! ;-)

[Verdent]

Alright, I fixed the master and everything should be correct now! :-)

It sill look like there might be some copyright years remaining to fix.

[api-from-the-ion]

On commit for #616. ...

In EnumDeserializer I needed a whole jsonbContext to get the models. Because of this, I've changed the TypeDeserializerBuilder to set and get the JsonbContext as parameter. There is still a getter for JsonbConfigProperties, but we can remove it. If we do this, we have to change another three classes which use this getter.

The other possible change was to move the creation of EnumDeserializer out of the TypeDeserializers chain, direct into the DeserializationModelCreator. Then I wouldn't have to change the TypeDeserializerBuilder. But this looks like a more destructive change for me. Of course, it also could be done.

[api-from-the-ion]

Put every call of JsonbBuilder.create in the tests into try-with-resources.

There is a discussion about similar things here #586. I don't know if this should be done or not right now. Anyway, I just did it. It is only in the test code, so there we could close any resource of the created JSON instance and builder.

If there is an exception during auto-closing - I think it should break a test. But it also could be irrelevant for the test, so we could silently catch the exception. This also could be discussed.

[Verdent]

Thank you for doing changes to Yasson :-)

I have a few things to note here:

1. since this will be very likely get merged some time after the New Year, It will need to have updated copyright years for a changed files to 2024.
2. If possible, please list here all the issues it fixes
3. I am usually squashing everything when merging, to do not have 100 new commits, but rather one. Are you fine with that?

[Verdent]

I think this should be removed, if it is not needed

[api-from-the-ion]

Here I addressed your approach of making the multi-release JAR, for java pre-16 and above. So the Java 16 sources are now included by plugin and not by manipulation of the compiler plugin configuration. This worked only for test classes but not for main classes; therefore I commended this part out.

The question is: would we like to get the multi-release somehow through some official / right way (there are some now)? Or should we forget it?

I don't think that you got multi-release JAR before, just the compilation and creation of the different JARs for different versions. So maybe this multi-release wasn't a goal anyway? And shouldn't be reached?

As about this comment - I can remove it independently of the decision about the above topic.

[Verdent]

I think this is not needed, since it is added by compile by default

[api-from-the-ion]

As I wrote in commit comment this is because of JPMS: these are the classes in the root package, which is exported. So I get warnings from the compiler, which suggests adding an explicit constructor. I think the public one is OK, because we should be able to create the instances of these classes outside the packages?

[Verdent]

I think this is not needed, since it is added by compile by default

[api-from-the-ion]

See comment above.

[Verdent]

Hm... I think this annotation might be better placed on the Method level. (I will not mark it in other cases I see this.)

What do you think?

[api-from-the-ion]

The problem with suppression on the method level - you suppress all such things everywhere in the method. So if you put a new one, which you probably wouldn't like to be suppressed and paid attention to it - it also would be automatically suppressed in such case. So I try to put the thing like this directly at the point where it happens, so no other occurrences would be suppressed.

Sometimes I have to change code a little because of this. For example, in case of something would be cast without check directly at return of the method, but one can't put suppress on the return. So I have to extract the result of the cast into the new variable, where I can suppress the unchecked cast warning; and return this variable in the next line.

I think this is a healthy approach.

[Verdent]

This should never happen, if method isEnum is used.

[api-from-the-ion]

The CCE could happen here not because I cast the class to the enum class, but to some definite enum class E in line 43. Otherwise, I wouldn't have typed constants in the loop. If you think that this wouldn't happen or cause the exception - I can remove the try-catch block. Or if you think that it's OK to have here a CCE been thrown and not IAE.

[Verdent]

the same as wit Deserializer. When this map can be null?

[api-from-the-ion]

I already answered this above in deserializer.

[Verdent]

Why have you decided to create these methods? :-)

[api-from-the-ion]

In the tests, one always need a Jsonb object to test it. So there are these repeating parts in the test which creates this object one way or another. Sometimes it is in the try-with-resources block, sometimes one forgot about this.

So I extracted these repeating parts into the methods. These methods are the fixtures for the test. The test gets the Jsonb over the consumer and only have to implement the test logic itself in the consumer. No need to write this boilerplate code over and over again, with some unneeded combinations.

I choose to place these methods into this class because this class already has some common constants and methods which used all over the test code. If you think we need another class just for these static methods - I can do it.

[Verdent]

What exactly does this mean on the field?

[api-from-the-ion]

We are using the deprecated parameter (nillable) on the annotation (@JsonbProperty) here. Therefore, I have a suppressor on the field. I think this is an old test from the times when this parameter wasn't deprecated. So suppression is ok right now; this test would be removed as soon as the parameter would be also removed and wouldn't be just deprecated.

[api-from-the-ion]

Thank you for doing changes to Yasson :-)

I have a few things to note here:

1. Done this, the year is 2024 now
2. Here is the list of the tickets I closed in this PR.

#611 YassonParser does not implement JsonParser.currentEvent()
#616 Recognize or honor @JsonbProperty annotation when applied to enumerated types.
#617 @JsonbTypeDeserializer is ignored on Enums in 3.0.3; works in 2.0.4
#625 Polymorphic deserialization fails with a JsonValue property
#626 DocumentationExampleTest: tests for date and number formats not working on installations with a non-English default locale.
#627 Components (adapters, de- serializes) not really cached because of the bug in the ComponentMatcher
#630 YassonParser stream and skip methods breaks parser state and DeserializationContext contract

Besides, after fixing the main issue, I've done some refactoring and warning fixing. If you need this, I can add some rough description of them, building some groups. At maximum, I can just assemble the list of all comments in my pushes in this PR, except for some small corrections and forgotten classes. Just tell me how deep and long this list should be

3. If we squash, we would have just everything in one commit, not knowing which change belongs to which ticket or theme. This is probably the reason for the list above, this should be the commit comment? If this is OK for you - then this is also OK for me.

[api-from-the-ion]

I found a missing case in ReflectionUtils#getOptionalRawType (WildcardType). I closed this and wrote the test. Furthermore, I don't think that we need an extra ticket for this bug - there was an exception in this case.