Skip to content
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

Generate Jackson deserializers #42954

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Conversation

mariofusco
Copy link
Contributor

This pull request completes the work on Jackson's reflection-free serialization also implementing the deserialization part. I added a complete explanation of what this generates in the javadocs. it also moves the registration of the generated serializers in its own ObjectMapperCustomizer.

Once this will be merged I believe that we could have some discussion on how to extend and generalize this work making this improvement available also to other parts of Quarkus that need to perform some sort of json serialization.

/cc @geoand @franz1981 @metacosm

@geoand
Copy link
Contributor

geoand commented Sep 2, 2024

Nice work!

I assume you'll add some tests for this :)

@mariofusco
Copy link
Contributor Author

I assume you'll add some tests for this :)

I added one very simple test that I had in my former pull request and for some reason missed to port on this one https://github.com/quarkusio/quarkus/pull/42954/files#diff-3364b502d76016d13fc343c40b2f921523fce303002673812ad65d9064c70b40R661

For the rest the deserialization is already tested by the other tests performing a POST in the SimpleJsonTest class. I agree on the fact that this far from having an extensive coverage, but it would be probably better to add ad-hoc tests when a specific bug is reported as it happened with the interface one. However if you have any suggestion for other test cases that you would like to see covered please let me know.

This comment has been minimized.


String writtenType() {
return switch (fieldType.name().toString()) {
case "char", "java.lang.Character" -> "java.lang.String";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't supposed to map both to primitive char?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Maybe the name of the method is misleading, anyway this is the type that has to be used when a field is written into json. In Jackson (and indeed in json) there's no notion of a single char, so it has to be written as a String.

@geoand
Copy link
Contributor

geoand commented Sep 3, 2024

I assume you'll add some tests for this :)

I added one very simple test that I had in my former pull request and for some reason missed to port on this one https://github.com/quarkusio/quarkus/pull/42954/files#diff-3364b502d76016d13fc343c40b2f921523fce303002673812ad65d9064c70b40R661

For the rest the deserialization is already tested by the other tests performing a POST in the SimpleJsonTest class. I agree on the fact that this far from having an extensive coverage, but it would be probably better to add ad-hoc tests when a specific bug is reported as it happened with the interface one. However if you have any suggestion for other test cases that you would like to see covered please let me know.

Which other tests are you referring to here?

Also, unless this feature is already being used in one of the integration tests, we have zero coverage for the native case.

@mariofusco
Copy link
Contributor Author

Which other tests are you referring to here?

There are a few tests performing a POST in the SimpleJsonTest class, see

https://github.com/quarkusio/quarkus/blob/d2f8463972c4da33d2fa4d8d1a2c8e5e1e630d36/extensions/resteasy-reactive/rest-jackson/deployment/src/test/java/io/quarkus/resteasy/reactive/jackson/deployment/test/SimpleJsonTest.java#L53C18-L53C22


and so on. For all of them when the test cases are executed within SimpleJsonWithReflectionFreeSerializersTest that extends it, all the deserializations are performed through these new generated deserializers.

Also, unless this feature is already being used in one of the integration tests, we have zero coverage for the native case.

This is true, and it's also true for the formerly introduced serializers. Can you please point me on where I could add those tests for native?

@geoand
Copy link
Contributor

geoand commented Sep 3, 2024

and so on. For all of them when the test cases are executed within SimpleJsonWithReflectionFreeSerializersTest that extends it, all the deserializations are performed through these new generated deserializers.

Cool

This is true, and it's also true for the formerly introduced serializers. Can you please point me on where I could add those tests for native?

Feel free to add the setting to any of the modules in integration-tests that uses quarkus-rest-jackson. We probably don't want to add a new module because that stresses CI a lot...

move generated serializers registration in its own ObjectMapperCustomizer

add test

add integration tests
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪🏽

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 3, 2024
@mariofusco
Copy link
Contributor Author

💪🏽

@geoand as you already saw I added a couple of tests in the rest-client-reactive-multipart module, maybe a bit misleading but I couldn't find a better place without creating a brand new module, which is probably overkilling as you wrote.

The tests are only 2 for now, but they cover both serialization and deserialization and also the problem that I had with the interface. I will incrementally add new tests there when we will add new features and/or fix other bugs.

Copy link

quarkus-bot bot commented Sep 3, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit bd57e87.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand geoand merged commit b41e186 into quarkusio:main Sep 3, 2024
32 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 3, 2024
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 3, 2024
case "java.lang.Short" -> "short";
case "java.lang.Long" -> "long";
case "java.lang.Double" -> "double";
case "java.lang.Float" -> "float";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are missing Boolean here =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants