Add support for Jackson's JsonPOJOBuilder #3688

misl · 2019-08-25T19:29:55Z

Added support to Jackson extension for immutable POJO models using @JsonPOJOBuilder.

Both model and builder are added as reflective classes
Also supports model and builder inheritance (add superclasses as well)
Added exclusion for @JsonDeserialize without builder field
Includes integration tests.

geoand · 2019-08-26T08:35:58Z

Thanks for the contribution!

I took the liebrty of adding thr coding in progress label since a NPE is being thrown as a result of the changes introduced by the PR.

geoand · 2019-08-27T08:41:02Z

@misl when you are done working on this, please squash your commits into a single one - thanks!

geoand · 2019-08-27T10:23:50Z

Also, I think that the PR needs to be rebased on master and update the native image CI configuration now that #3719 has been merged

misl · 2019-08-27T11:05:40Z

Also, I think that the PR needs to be rebased on master and update the native image CI configuration now that #3719 has been merged

Rebase was performed locally and is building right now. But what is expect from "update the native image CI configuration"?

geoand · 2019-08-27T11:07:46Z

Because you have added a new integration test module, you need to add something like this

misl · 2019-08-27T11:19:52Z

Thanks, added it locally. Will soon commit and push (as local build completed successfully)

misl · 2019-08-27T17:27:03Z

Sorry, but I need some help on this one. I have no clue what the vertx integration test compilation errors has to do with my changes.

geoand · 2019-08-27T17:35:42Z

I saw the same thing earlier in a different PR - but I think it has been fixed on master since so it's probably worth rebasing again

geoand · 2019-08-28T08:27:30Z

Sorry for this, but it needs another rebase - there have been a lot of CI changes the last 2 days

misl · 2019-08-28T15:19:52Z

Help, I need help again. Is this another error due to CI changes.

Sorry to say so, but I am starting to feel like I am wasting time on this.

geoand · 2019-08-28T15:26:14Z

I'm not on my machine now so I'll take a look in a few hours.

I understand the feeling, but I can assure you that you have experienced particular bad luck having your PR arrive in the middle of large CI changes.

Sorry again for the inconvenience

misl · 2019-08-28T16:49:37Z

Let's hope this is not an bad omen ;-)

BTW regarding CI. In my opinion things are scattered all over (overstatement) the repository. Especially for extensions. I needed to make changes in various locations:

the extension folder itself: extensions/jackson
Integration test: integration-tests/jackson
CI pipeline: azure-pipelines.yml
Personally I would prefer these things in a common extension root module (like extensions/jackson). The integration test could live there as well. The pipeline part can probably we auto generated (convention over configuration).

Like this there is less chance of merge conflicts. Newbies like me need less knowledge about the project structure. If (ever) extensions get separated from core Quarkus it would be a lot less work since all extension related stuff is in a single place. And possible improves development speed (less hopping around).

Sorry, this is just me always trying to improve on (working) things and bringing it rather blunt.

geoand · 2019-08-29T12:19:52Z

I've restarted the failing test multiple times - same result each time.
None of the other PRs are failing this test so maybe there is something wrong with this PR?

misl · 2019-08-29T15:38:21Z

Looking at the log I see no indication that the errors are caused by the Jackson extension. Looks more like the DynamoDB:

2019-08-29T11:15:10.9962932Z 11:15:09,561 INFO  [org.jbo.threads] JBoss Threads version 3.0.0.Beta5
2019-08-29T11:15:11.0949524Z [ERROR] Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 3.572 s <<< FAILURE! - in io.quarkus.it.dynamodb.DynamoDbFunctionalityTest
2019-08-29T11:15:11.0961581Z [ERROR] testDynamoDbAsync  Time elapsed: 0.022 s  <<< ERROR!
2019-08-29T11:15:11.0962235Z org.junit.jupiter.api.extension.TestInstantiationException: 
2019-08-29T11:15:11.0981185Z TestInstanceFactory [io.quarkus.test.junit.QuarkusTestExtension] failed to instantiate test class [io.quarkus.it.dynamodb.DynamoDbFunctionalityTest]: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
2019-08-29T11:15:11.0981944Z 	[error]: Build step io.quarkus.arc.deployment.ArcProcessor#validate threw an exception: javax.enterprise.inject.spi.DeploymentException: Found 2 deployment problems: 
2019-08-29T11:15:11.0994909Z [1] Unsatisfied dependency for type software.amazon.awssdk.services.dynamodb.DynamoDbAsyncClient and qualifiers [@Default]
2019-08-29T11:15:11.0995959Z 	- java member: io.quarkus.it.dynamodb.DynamoDBResource#asyncClient
2019-08-29T11:15:11.0997080Z 	- declared on CLASS bean [types=[io.quarkus.it.dynamodb.DynamoDBResource, java.lang.Object], qualifiers=[@Default, @Any], target=io.quarkus.it.dynamodb.DynamoDBResource]
2019-08-29T11:15:11.1008975Z [2] Unsatisfied dependency for type software.amazon.awssdk.services.dynamodb.DynamoDbClient and qualifiers [@Default]
2019-08-29T11:15:11.1009988Z 	- java member: io.quarkus.it.dynamodb.DynamoDBResource#client
2019-08-29T11:15:11.1024370Z 	- declared on CLASS bean [types=[io.quarkus.it.dynamodb.DynamoDBResource, java.lang.Object], qualifiers=[@Default, @Any], target=io.quarkus.it.dynamodb.DynamoDBResource]
2019-08-29T11:15:11.1025035Z 	at

I will attempt another rebase

misl · 2019-08-30T10:25:45Z

Finally, all tests passed :-)

geoand · 2019-08-30T10:41:56Z

👍

I'll check it out, but as I (think I) mentioned in a previous comment, the PR needs to be squashed to a single commit :)

geoand · 2019-08-30T11:40:00Z

Much better, thanks!

FroMage · 2019-08-30T14:15:52Z

@geoand, do you intend to review this?

geoand · 2019-08-30T14:20:58Z

@FroMage yup, absolutely

geoand

This is really good overall, great job!

I added a few comments, most of which are minor

extensions/jackson/deployment/src/main/java/io/quarkus/jackson/deployment/JacksonProcessor.java

...odel/src/main/java/io/quarkus/reproducer/jacksonbuilder/model/InheritedModelWithBuilder.java

extensions/jackson/deployment/src/main/java/io/quarkus/jackson/deployment/JacksonProcessor.java

integration-tests/jackson/Readme.md

.../src/test/java/io/quarkus/reproducer/jacksonbuilder/model/InheritedModelWithBuilderTest.java

integration-tests/jackson/service/src/main/docker/Dockerfile.jvm

geoand · 2019-08-31T11:21:37Z

Once done with the changes, please squash to a single commit.

Thanks!

misl · 2019-08-31T11:24:25Z

How to proceed from here?

Do you prefer to review the individual commits or should I first squash (+ rebase) again?

You already answered that. I will squash it now.

geoand · 2019-08-31T11:37:26Z

Go ahead please with squash and rebase - the PR is not too large to require review per commit I would say

geoand · 2019-08-31T12:25:30Z

Just a few last minor points I raised. Once they are addressed, I'll approve

extensions/jackson/deployment/src/main/java/io/quarkus/jackson/deployment/JacksonProcessor.java

- Use @JsonDeserialize (instead of @JsonPOJOBuilder) to add reflective classes. - Both model and builder are added - Added safeguard for @JsonDeserialize without builder - Transformed reproducer project into integration tests for jackson extension. - Added support for models and model builders with inheritance

geoand

Thank you very much for this!

geoand · 2019-08-31T13:55:13Z

extensions/jackson/deployment/src/main/java/io/quarkus/jackson/deployment/JacksonProcessor.java

+        IndexView index = combinedIndexBuildItem.getIndex();
+
+        // TODO: Here we only check for @JsonDeserialize to detect both Model and Builder
+        //       classes to support @JsonPojoBuilder. The @JsonDeserialize annotiona can


There is a typo in annotationa. Also the sentence could possible be expressed in a more clear manner, but it's definitely not something to hold up the PR for :)

gwenneg · 2019-08-31T15:47:31Z

Fixes #3652

stuartwdouglas · 2019-09-03T03:24:20Z

For some reason this PR has increased the build time of the kubernetes client native by around 40 minutes.

geoand · 2019-09-03T04:33:19Z

I'll check it out

geoand · 2019-09-03T04:46:58Z

Although I need to dig through it, my guess is that now the entire Kubernetes Client model is being registered for reflection, as opposed to being "weakly" registered.

geoand · 2019-09-03T07:03:06Z

I tweaked the Kubernetes extension in this PR.

gsmet changed the title ~~Fixed issue #3652~~ Add support for Jackson's JsonPOJOBuilder Aug 26, 2019

geoand added the coding-in-progress label Aug 26, 2019

geoand removed the coding-in-progress label Aug 30, 2019

geoand requested changes Aug 30, 2019

View reviewed changes

geoand requested changes Aug 31, 2019

View reviewed changes

extensions/jackson/deployment/src/main/java/io/quarkus/jackson/deployment/JacksonProcessor.java Outdated Show resolved Hide resolved

geoand added this to the 0.22.0 milestone Aug 31, 2019

geoand approved these changes Aug 31, 2019

View reviewed changes

geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 31, 2019

geoand reviewed Aug 31, 2019

View reviewed changes

geoand merged commit 79edb31 into quarkusio:master Aug 31, 2019

geoand mentioned this pull request Sep 3, 2019

Ensure the Kubernetes extension doesn't register too much reflection for Jackson #3814

Merged

misl mentioned this pull request Dec 6, 2019

integration-tests/jackson fails in native mode #5993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Jackson's JsonPOJOBuilder #3688

Add support for Jackson's JsonPOJOBuilder #3688

misl commented Aug 25, 2019 •

edited

Loading

geoand commented Aug 26, 2019 •

edited

Loading

geoand commented Aug 27, 2019

geoand commented Aug 27, 2019

misl commented Aug 27, 2019

geoand commented Aug 27, 2019

misl commented Aug 27, 2019

misl commented Aug 27, 2019

geoand commented Aug 27, 2019

geoand commented Aug 28, 2019

misl commented Aug 28, 2019 •

edited

Loading

geoand commented Aug 28, 2019 •

edited

Loading

misl commented Aug 28, 2019

geoand commented Aug 29, 2019

misl commented Aug 29, 2019

misl commented Aug 30, 2019

geoand commented Aug 30, 2019

geoand commented Aug 30, 2019

FroMage commented Aug 30, 2019

geoand commented Aug 30, 2019

geoand left a comment

geoand commented Aug 31, 2019

misl commented Aug 31, 2019 •

edited

Loading

geoand commented Aug 31, 2019

geoand commented Aug 31, 2019 •

edited

Loading

geoand left a comment

geoand Aug 31, 2019

gwenneg commented Aug 31, 2019

stuartwdouglas commented Sep 3, 2019

geoand commented Sep 3, 2019

geoand commented Sep 3, 2019

geoand commented Sep 3, 2019

Add support for Jackson's JsonPOJOBuilder #3688

Add support for Jackson's JsonPOJOBuilder #3688

Conversation

misl commented Aug 25, 2019 • edited Loading

geoand commented Aug 26, 2019 • edited Loading

geoand commented Aug 27, 2019

geoand commented Aug 27, 2019

misl commented Aug 27, 2019

geoand commented Aug 27, 2019

misl commented Aug 27, 2019

misl commented Aug 27, 2019

geoand commented Aug 27, 2019

geoand commented Aug 28, 2019

misl commented Aug 28, 2019 • edited Loading

geoand commented Aug 28, 2019 • edited Loading

misl commented Aug 28, 2019

geoand commented Aug 29, 2019

misl commented Aug 29, 2019

misl commented Aug 30, 2019

geoand commented Aug 30, 2019

geoand commented Aug 30, 2019

FroMage commented Aug 30, 2019

geoand commented Aug 30, 2019

geoand left a comment

Choose a reason for hiding this comment

geoand commented Aug 31, 2019

misl commented Aug 31, 2019 • edited Loading

geoand commented Aug 31, 2019

geoand commented Aug 31, 2019 • edited Loading

geoand left a comment

Choose a reason for hiding this comment

geoand Aug 31, 2019

Choose a reason for hiding this comment

gwenneg commented Aug 31, 2019

stuartwdouglas commented Sep 3, 2019

geoand commented Sep 3, 2019

geoand commented Sep 3, 2019

geoand commented Sep 3, 2019

misl commented Aug 25, 2019 •

edited

Loading

geoand commented Aug 26, 2019 •

edited

Loading

misl commented Aug 28, 2019 •

edited

Loading

geoand commented Aug 28, 2019 •

edited

Loading

misl commented Aug 31, 2019 •

edited

Loading

geoand commented Aug 31, 2019 •

edited

Loading