-
Notifications
You must be signed in to change notification settings - Fork 151
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
Support Spring Boot 3 and jakarta
#882
base: master
Are you sure you want to change the base?
Support Spring Boot 3 and jakarta
#882
Conversation
jakarta
b1b9e32
to
42e3839
Compare
@@ -21,7 +21,7 @@ dependencies { | |||
providedCompile 'org.eclipse.microprofile:microprofile:4.0.1' | |||
|
|||
implementation project(':crnk-setup:crnk-setup-rs') | |||
implementation project(':crnk-setup:crnk-setup-cdi') | |||
// implementation project(':crnk-setup:crnk-setup-cdi') |
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.
This, and other commented out cdi-related things is temporary - I was having issues with CDI and decided to focus on other parts. Welcome to any suggestions.
Getting this error from many tests - I tried adding |
5aebf74
to
35889f3
Compare
To be compatible with java version, bump gradle version. Disable some plugins for now to get building
35889f3
to
e419642
Compare
Attempt to remove incompatible SB setup modules and update the rest of the framework to use the SB3 module
At least now more tests are running
e419642
to
c954d0e
Compare
Uncomment CDI dependencies, declare explicit dependency on crnk-setup-cdi in the modules that use it
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.
looks good.. Will check and build and if possible will raise a PR on your branch.
api 'net.jodah:typetools:0.6.3' | ||
api 'org.slf4j:slf4j-api:2.0.9' | ||
api 'com.fasterxml.jackson.module:jackson-module-jakarta-xmlbind-annotations:2.15.2' | ||
api 'jakarta.validation:jakarta.validation-api:3.0.2' |
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.
Would it be worth either adding variables for common dependencies or moving them to the root project.
plugins { | ||
id 'java' | ||
id 'java-library' | ||
} |
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.
Might be worth adding back the
sourceCompatibility = 1.17
targetCompatibility = 1.17
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 and fixed in another location where I simply had 17
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.
I would suggest that you use this notation:
sourceCompatibility = JavaVersion.VERSION_17
targetCompatibility = JavaVersion.VERSION_17
compile project(':crnk-gen:crnk-gen-typescript') | ||
compile project(':crnk-gen:crnk-gen-asciidoc') | ||
compile project(':crnk-gen:crnk-gen-openapi') | ||
implementation 'com.fasterxml.jackson.module:jackson-module-jakarta-xmlbind-annotations' |
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.
does the version number need to be included here?
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.
This was actually unneeded as it gets this dependency from other modules. Removed.
implementation 'io.swagger.core.v3:swagger-integration:2.0.8' | ||
implementation 'io.swagger.core.v3:swagger-core:2.0.8' | ||
implementation 'io.swagger.core.v3:swagger-models:2.0.8' | ||
implementation 'io.swagger.parser.v3:swagger-parser:2.0.8' |
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.
won't these need to be updated to latest versions, for java 17 compactability ?
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.
Updated. parser
doesn't seem to have a -jakarta
variant
compile 'io.swagger.core.v3:swagger-core:2.0.8' | ||
compile 'io.swagger.core.v3:swagger-models:2.0.8' | ||
compile 'io.swagger.parser.v3:swagger-parser:2.0.8' | ||
implementation 'guru.nidi:graphviz-java:0.8.3' |
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.
does this need upgrading to the latest version ?
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.
Updated
testCompile project(':crnk-meta') | ||
testCompile project(':crnk-test') | ||
testCompile project(':crnk-data:crnk-data-facet') | ||
testImplementation 'commons-io:commons-io:2.5' |
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.
seeing different version between builds, should we update all the latest and/or but the version in a variable/property
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.
Updated to latest throughout
crnk-home/build.gradle
Outdated
testCompile group: 'net.javacrumbs.json-unit', name: 'json-unit-fluent', version: '1.5.3' | ||
testCompile group: 'nl.jqno.equalsverifier', name: 'equalsverifier', version: '1.7.2' | ||
testCompile group: 'com.jayway.jsonpath', name: 'json-path', version: '2.2.0' | ||
api group: 'junit', name: 'junit', version: '4.12' |
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.
Should remove the junit4 dependency and update tests to junit 5 ?
|
||
compile group: 'io.dropwizard', name: 'dropwizard-core', version: '1.0.0' | ||
implementation group: 'io.dropwizard', name: 'dropwizard-core', version: '1.0.0' |
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.
Shouldn't this be updated.
@@ -78,7 +82,7 @@ private synchronized void initImpl() { | |||
} else if (requestFactory instanceof HttpComponentsClientHttpRequestFactory) { | |||
HttpComponentsClientHttpRequestFactory apacheRequestFactory = | |||
(HttpComponentsClientHttpRequestFactory) impl.getRequestFactory(); | |||
apacheRequestFactory.setReadTimeout(networkTimeout.intValue()); | |||
// apacheRequestFactory.setReadTimeout(networkTimeout.intValue()); |
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.
Any reason for removing?
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.
Ah forgot about this one. The method/property were deprecated:
As of version 6.0, setting this property has no effect.
To change the socket read timeout, use SocketConfig.Builder.setSoTimeout(Timeout), supply the resulting SocketConfig to org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManagerBuilder.setDefaultSocketConfig(SocketConfig), use the resulting connection manager for org.apache.hc.client5.http.impl.classic.HttpClientBuilder.setConnectionManager(HttpClientConnectionManager), and supply the built HttpClient to HttpComponentsClientHttpRequestFactory(HttpClient).
Deprecated
as of 6.0, in favor of SocketConfig.Builder.setSoTimeout(Timeout), see above.
compile 'org.assertj:assertj-core:3.9.1' | ||
compile 'org.apache.httpcomponents:httpclient:4.5.2' | ||
implementation 'org.assertj:assertj-core:3.9.1' | ||
implementation 'org.apache.httpcomponents:httpclient:4.5.2' |
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.
Should we move upto client5.httpclient or at least the latest version of 4.5.x
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.
This is wrapped up in the same changes as #882 (comment). I think bumping up the the latest might be the right move, but haven't gotten too deep into that one yet.
@jsteelepfpt Have started some more changes which once I get the Junit 4 -> 5 changes done I'll raise a PR into your branch.. |
Next focus is Spring MVC changes. From what I can tell, Crnk only overrode the DefaultWebMvcTagsProvider to "enhance" the URL in MVC metrics. We may be able to do the same with |
307ccb2
to
fee8476
Compare
crnk-core/build.gradle
Outdated
@@ -8,15 +8,11 @@ dependencies { | |||
api 'org.slf4j:slf4j-api:2.0.9' | |||
api 'com.fasterxml.jackson.module:jackson-module-jakarta-xmlbind-annotations:2.15.2' | |||
api 'jakarta.validation:jakarta.validation-api:3.0.2' | |||
api 'javax.xml.bind:jaxb-api:2.4.0-b180830.0359' |
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.
This may be a problem, we need jackson to use the jakarta version of this
If excluding crnk-setup-cdi, this allows more tests to pass without javax errors
I've been excluding Edit: see https://stackoverflow.com/questions/77319334/java-lang-noclassdeffounderror-javax-enterprise-context-spi-contextual-when-swi/77512767#77512767, this may be because an external dependency isn't using jakarta yet. For example:
|
…guration with CrnkServerRequestObservationConvention and CrnkMvcObservationAutoConfiguration; Add tests
Hello! is this still being worked on? im open to help finish this but i dont want to duplicate work if youre working your way through this already |
@byrondaniels Hi! I haven't been able to work on this recently, so feel free to help out if you'd like! |
Branch and PR inspired by Issue 869: #869
The goal is to support Spring Boot 3.
javax
tojakarta
dependencies