-
Notifications
You must be signed in to change notification settings - Fork 357
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
JSON Jackson does not handle resource returning CompletionStage<X> #3672 #3978
base: 2.x
Are you sure you want to change the base?
Conversation
…d its sub-classes Signed-off-by: mageshwaranr <mageshwaran.r@gmail.com>
…d its sub-classes Signed-off-by: mageshwaranr <mageshwaran.r@gmail.com>
There is a valid ECA on file for mageshwaran.r@gmail.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR, it looks good. However, any PR that modifies the code should have a test. Jackson seems to have tests under examples/json-jackson.
Sorry for the delay. Added test cases. Is there a way to re-trigger build ? |
Attempting to re-trigger the build |
@jansupol Anything expected from me to take this forward ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added required test cases
@jansupol Can we please have this in 2.29? This has been hurting for a while now 😅 |
Hm,...I was about to approve this...but then I realized (late, sorry)... org/glassfish/jersey/jackson/internal/jackson/jaxrs/base/ProviderBase.java is actually repackaged from Jackson 2.8.10. Should the PR be made against Jackson rather than against Jersey? If a new version gets repackaged, the code could be lost...The best way is to create a PR against Jackson (2.9.x) and then a PR/issue to Jersey to repackage the latest version Sorry for that coming late, but it still is doable before 2.29. |
Hm... If it gets merged to Jackson, we can merge this to Jersey, too. But since this is 3rd party code, we need to ask for a CQ, first. |
The last commit is missing sign-off-by (-s)... |
@@ -0,0 +1,44 @@ | |||
package org.glassfish.jersey.examples.jackson; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright header is missing
@mageshwaranr The code in Jackson seems to be stable at least from 2.8.4. I think we could add this to Jersey. Can you please fix the reported issues?
|
We have adopted Jackson 2.9.9 |
@mageshwaranr could you please add the last request from @jansupol so we can have it. |
There's one more thing missing to complete this for me, which is to unwrap CompletionException properly so appropriate javax.ws.rs.ext.ExceptionMapper are called down the line. If it helps anyone else, I am modifying the mapException method of org.glassfish.jersey.server.ServerRuntime using this code to workaround it, but unsure if there's a better way it can be integrated:
|
A resource method like below fails with java.lang.IllegalArgumentException: Class * not subtype of *.
This pull request attempts to fix this. Let me know whether the fix is appropriate.