eclipse-ee4j / yasson

Eclipse Yasson project
https://projects.eclipse.org/projects/ee4j.yasson
Other
204 stars 96 forks source link

PropertyNamingStrategy is not considered for JsonbCreators #583

Closed bmarwell closed 1 year ago

bmarwell commented 1 year ago

Describe the bug

PropertyNamingStrategy is not considered for records

To Reproduce

Try to read a JSON file and see this error:

jakarta.json.bind.JsonbException: JsonbCreator parameter firstName is missing in json document.

Expected behavior

SHould have used first_name instead.

System information:

Additional context

Example on this commit: https://github.com/bmarwell/jaxrs-test-showcase/pull/61/commits/71348e8d153300a4699ec33314bb577ec1c87228

bmarwell commented 1 year ago

Looking at ObjectDeserializer::deserialzeNext, it seems that getClassModel().getClassCustomization().getCreator() did not get the underscore property names.

This is being createad in MappingContext::createParseClassModelFunction which has the var ClassCustomization customization already containing no creator customization.

That in return is created by AnnotationIntrospector::getCreator. The method JsonbCreator createJsonbCreator checks for @JsonbProperty, but never considers the naming Strategy: https://github.com/eclipse-ee4j/yasson/blob/6e7ec79c54218a05342cbb65f5b150cc5892b7c2/src/main/java/org/eclipse/yasson/internal/AnnotationIntrospector.java#L208

In this line it should have considered the already known PropertyModel, just call model.get(parameter.getName()) to receive the JSON name.

bmarwell commented 1 year ago

Works in Apache Johnzon. When no @JsonbProperty is defined on a @JsonbCreator, the naming strategy is being considered.

bmarwell commented 1 year ago

Example project with builds of:

... is here: https://github.com/bmarwell/jsonb-creator-property-naming

Romain fixed it in johnzen literally minutes after talking to him 😆 => https://github.com/apache/johnzon/commit/b25243b23f36f257e47d8fce9b914bb5c882889c

Verdent commented 1 year ago

Hi @bmarwell , I am finally having a little bit of time to take a look into this :-) Thank you for bringing this up and making fix for it as well.

Verdent commented 1 year ago

Fix merged