You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2021/10/26 13:14:20 UTC
[GitHub] [fineract] ptuomola opened a new pull request #1934: FINERACT-1418: Upgrade to Jersey 2
ptuomola opened a new pull request #1934:
URL: https://github.com/apache/fineract/pull/1934
## Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update unit or integration tests for verifying the changes made.
- [ ] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-951944167
Found the missing line - now single calls work...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-958775144
@ptuomola what was the problem? Anything I can help with?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-952038044
Ok - hopefully have now fixed the classpath issues. Let's see if this works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-958775144
@ptuomola what was the problem? Anything I can help with?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-952461338
@ptuomola ... please check this for the Jersey configuration: https://github.com/FITER1/fineract-ng/blob/feature/01-migration-eclipse-link/fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/boot/JerseyConfiguration.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#discussion_r742561273
##########
File path: build.gradle
##########
@@ -131,7 +131,6 @@ allprojects {
dependency 'jakarta.jms:jakarta.jms-api:2.0.3'
dependency 'jakarta.xml.bind:jakarta.xml.bind-api:2.3.3'
dependency 'org.glassfish.jaxb:jaxb-runtime:2.3.5'
- dependency 'jakarta.validation:jakarta.validation-api:3.0.0'
Review comment:
I tried first retaining the validation-api 3.0.0. The problem is that it is incompatible with about half a dozen libraries included in spring-boot-starter-jersey, and for many of which no version exists that is compatible for 3.0.0. So I had to backtrack and downgrade validation API to 2.x.
I think overall it would make sense for us to try to remove as many of the specific library dependencies and instead use the Spring Boot bundles wherever they are available - that way we don't have to worry about compatibility of every individual library with everything else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-960483711
@ptuomola ... only those 2 small comments above to resolve... I saw that you also replaced the ancient "ord.codehouse.jackson.*" references with the latest Jackson packages... great! The rest of your changes look good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola closed pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola closed pull request #1934:
URL: https://github.com/apache/fineract/pull/1934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#discussion_r742566262
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/data/ApiGlobalErrorResponse.java
##########
@@ -194,8 +194,7 @@ public ApiGlobalErrorResponse(final List<ApiParameterError> errors) {
this.errors = errors;
}
- @XmlElementWrapper(name = "errors")
- @XmlElement(name = "errorResponse")
+ @JsonProperty("errors")
Review comment:
This code is actually used to serialise to JSON, not to XML. And with the new version of Jackson, it seems the behaviour was changed. In old Jackson, this would serialise to:
errors: [ "error1", "error2" ]
ie take the name from the @XmlElementWrapper
In the new version of Jackson, the annotation @XmlElementWrapper is ignored, and instead the existing code serialises to:
errorResponse: [ "error1", "error2 ]
Which resulted in a lot of integration tests failing
So to restore the existing behaviour I changed the annotation to @JsonProperty (as this is anyway used for Json, not XML) - and for JSON there is no concept of a wrapper anyway.
You can find discussion about this issue here:
https://github.com/FasterXML/jackson-modules-base/issues/45
Now if the same class is somewhere serialised to XML then I fully agree that this will introduce a regression. But I wasn't able to find any such cases - just the serialisation for JSON, for which this change actually retains the current behaviour...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-960496999
@ptuomola ... thanks for the clarifications... I agree with not micro managing the dependencies... let Jersey fix the dependencies, eventually they'll trickle down to us. Concerning the JAXB annotations: actually I'm glad that they disappeared and you used JsonProperty... and if there's no other references to that then the small structure change should be OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-958958883
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-958775144
@ptuomola what was the problem? Anything I can help with?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-958958883
It all works for me locally - needed to rewrite the error handling for Interop as new Jersey did not serialise the HttpClientErrors correctly. Now test is failing with the Docker step - I'll try to see what's wrong with that tonight...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on a change in pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic commented on a change in pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#discussion_r742557612
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/data/ApiGlobalErrorResponse.java
##########
@@ -194,8 +194,7 @@ public ApiGlobalErrorResponse(final List<ApiParameterError> errors) {
this.errors = errors;
}
- @XmlElementWrapper(name = "errors")
- @XmlElement(name = "errorResponse")
+ @JsonProperty("errors")
Review comment:
@ptuomola isn't this changing the structure of the error response? As I see it from the changes here:
Before:
```
<errors>
<errorResponse>
...
</errorResponse>
<errorResponse>
...
</errorResponse>
</errors>
```
After:
```
<errors>
<apiParameterError>
...
</apiParameterError>
<apiParameterError>
...
</apiParameterError>
</errors>
```
... if I'm missing something here and the errors still serialize to the same structure then please ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola closed pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola closed pull request #1934:
URL: https://github.com/apache/fineract/pull/1934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola closed pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola closed pull request #1934:
URL: https://github.com/apache/fineract/pull/1934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-952486107
Thanks! I think I actually found a simpler solution: if you just do package() with the root package then all the classes get automatically found.
Also fixed the classpath issues. Now just need to debug the failing integration tests...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola closed pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola closed pull request #1934:
URL: https://github.com/apache/fineract/pull/1934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-951982212
Now trying to find some way to fix this classpath nightmare
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-960468576
Build finally passes! @vidakovic can you review and merge once ok? After that, I'll take a look at rewriting the Spring config
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic edited a comment on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic edited a comment on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-952462634
I'll have also a look at the classpath issues... I remember that I fixed something similar by cleaning up all the dependencies (that was before the major improvements happened after version 1.3)... so, the Gradle file in that branch is very different from what we have now upstream, but maybe it contains part of the solution... just as a caveat.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-955615255
This now works for me locally - just need to see if the tests pass here as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola closed pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola closed pull request #1934:
URL: https://github.com/apache/fineract/pull/1934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on a change in pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic commented on a change in pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#discussion_r742559252
##########
File path: build.gradle
##########
@@ -131,7 +131,6 @@ allprojects {
dependency 'jakarta.jms:jakarta.jms-api:2.0.3'
dependency 'jakarta.xml.bind:jakarta.xml.bind-api:2.3.3'
dependency 'org.glassfish.jaxb:jaxb-runtime:2.3.5'
- dependency 'jakarta.validation:jakarta.validation-api:3.0.0'
Review comment:
@ptuomola Any reason why we would replace the newer Jakarta validation lib with the older javax.* stuff? This dependency results in a couple of import statement changes further down... unless there is no strict requirement by Jersey 2.x I would revert this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-952462634
I'll have also a look at the classpath issues... I remember that I fixed something similar by cleaning up all the dependencies (that was before the major improvements happened after version 1.3)... so, the Gradle file in that branch is very different from what we have now upstream... just as a caveat.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-958775144
@ptuomola what was the problem? Anything I can help with?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#issuecomment-958958883
It all works for me locally - needed to rewrite the error handling for Interop as new Jersey did not serialise the HttpClientErrors correctly. Now test is failing with the Docker step - I'll try to see what's wrong with that tonight...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1934:
URL: https://github.com/apache/fineract/pull/1934#discussion_r742566262
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/data/ApiGlobalErrorResponse.java
##########
@@ -194,8 +194,7 @@ public ApiGlobalErrorResponse(final List<ApiParameterError> errors) {
this.errors = errors;
}
- @XmlElementWrapper(name = "errors")
- @XmlElement(name = "errorResponse")
+ @JsonProperty("errors")
Review comment:
This code is actually used to serialise to JSON, not to XML. And with Jersey 2.x, the behaviour was changed: in Jersey 1.x, this would serialise to:
errors: [ "error1", "error2" ]
ie take the name from the @XmlElementWrapper
In Jersey 2.x, the annotation @XmlElementWrapper is ignored, and instead the existing code serialises to:
errorResponse: [ "error1", "error2 ]
Which resulted in a lot of integration tests failing
So to restore the existing behaviour I changed the annotation to @JsonProperty (as this is anyway used for Json, not XML) - and for JSON there is no concept of a wrapper anyway.
You can find discussion about this issue here:
https://github.com/FasterXML/jackson-modules-base/issues/45
Now if the same class is somewhere serialised to XML then I fully agree that this will introduce a regression. But I wasn't able to find any such cases - just the serialisation for JSON, for which this change actually retains the current behaviour...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic merged pull request #1934: FINERACT-1418: Upgrade to Jersey 2
Posted by GitBox <gi...@apache.org>.
vidakovic merged pull request #1934:
URL: https://github.com/apache/fineract/pull/1934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org