-
Notifications
You must be signed in to change notification settings - Fork 559
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
feat: Provide Spring Boot 3 support #510
Conversation
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 your contribution. I made some comments, mostly related to the dependencies used.
Please make sure to update the pet-store samples in the samples
folder as well.
@@ -17,9 +17,24 @@ | |||
|
|||
<properties> | |||
<jaxrs.version>2.1.1</jaxrs.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.
Update to 3.1.0 or remove property if no longer in use
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.
Done, thanks.
@@ -17,9 +17,24 @@ | |||
|
|||
<properties> | |||
<jaxrs.version>2.1.1</jaxrs.version> | |||
<servlet.version>3.1.0</servlet.version> | |||
<servlet.version>5.0.0</servlet.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.
Any reason why we are not going with 6.0.0? Spring Boot also uses that 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.
AwsServletContext.SERVLET_API_MAJOR_VERSION
and AwsServletContext.SERVLET_API_MINOR_VERSION
need to be changed along with 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.
Updating to 6.0.0 gave me a bunch of 'class not found" issues. That's why I went with 5.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.
Well they removed a bunch of already deprecated stuff in 6.0.0 where we most likely return UnsupportedOperationException
anyway (e.g. for HttpSessionContext
related things as we don't support sessions). Let's go for 6.0.0 to be aligned with Spring.
<version>${servlet.version}</version> | ||
<groupId>jakarta.servlet</groupId> | ||
<artifactId>jakarta.servlet-api</artifactId> | ||
<version>5.0.0</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.
Either stick with ${servlet.version}
or remove the property above if it's no longer in use.
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.
Done.
<version>${jaxrs.version}</version> | ||
<groupId>jakarta.ws.rs</groupId> | ||
<artifactId>jakarta.ws.rs-api</artifactId> | ||
<version>3.0.0-M1</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.
Either stick with ${jaxrs.version}
and use 3.1.0 instead of the old milestone release or remove the property above if it's no longer in use.
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.
Done, thanks.
</dependency> | ||
<dependency> | ||
<groupId>org.eclipse.angus</groupId> | ||
<artifactId>angus-mail</artifactId> |
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.
Why is it necessary to add this dependency?
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.
After doin the namespace change I got this error:
[ERROR] getMimeType_mimeTypeOfCorrectFile_expectMime Time elapsed: 0.001 s <<< ERROR!
java.lang.RuntimeException: Provider for jakarta.activation.spi.MimeTypeRegistryProvider cannot be found
at com.amazonaws.serverless.proxy.internal.servlet.AwsServletContextTest.getMimeType_mimeTypeOfCorrectFile_expectMime(AwsServletContextTest.java:62)
After some research I found this on stackoverflow. After I added the dependency, the error disappeared.
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.
Ok I see, that's related to #504. Need to revisit that, angus-mail should not be required just for mimetype resolution.
<groupId>javax.activation</groupId> | ||
<artifactId>activation</artifactId> | ||
<version>1.1.1</version> | ||
<scope>test</scope> |
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.
why no longer scope test?
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 missed that. Updated.
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 be 2.1.1
<artifactId>javax.el</artifactId> | ||
<version>3.0.0</version> | ||
<artifactId>jakarta.el</artifactId> | ||
<version>5.0.0-M1</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.
Probably should be org.glassfish.expressly:expressly:5.0.0
instead of this old milestone release.
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.
Thanks, updated.
<dependency> | ||
<groupId>jakarta.activation</groupId> | ||
<artifactId>jakarta.activation-api</artifactId> | ||
<version>2.1.0</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.
2.1.1? Why is it needed to have this with compile scope?
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.
It's used in the AwsServletContext class. With a test scope, it gives the following errors:
package jakarta.activation.spi does not exist
cannot find symbol
[ERROR] symbol: class MimetypesFileTypeMap
[ERROR] location: class com.amazonaws.serverless.proxy.internal.servlet.AwsServletContext
Updated to 2.1.1
<maven.compiler.source>1.8</maven.compiler.source> | ||
<maven.compiler.target>1.8</maven.compiler.target> | ||
<spring.version>6.0.7</spring.version> | ||
<springboot.version>3.0.4</springboot.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.
Should be 3.0.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.
Updated.
@@ -16,7 +16,7 @@ | |||
<parent> | |||
<groupId>org.springframework.boot</groupId> | |||
<artifactId>spring-boot-starter-parent</artifactId> | |||
<version>2.7.10</version> | |||
<version>3.0.4</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.
3.0.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.
Updated
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.
Let's move all poms to 2.0-SNAPSHOT along with that change. Please note the samples also contain build.gradle files as an alternative that need to be aligned to the Maven poms.
@@ -17,9 +17,24 @@ | |||
|
|||
<properties> | |||
<jaxrs.version>2.1.1</jaxrs.version> | |||
<servlet.version>3.1.0</servlet.version> | |||
<servlet.version>5.0.0</servlet.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.
Well they removed a bunch of already deprecated stuff in 6.0.0 where we most likely return UnsupportedOperationException
anyway (e.g. for HttpSessionContext
related things as we don't support sessions). Let's go for 6.0.0 to be aligned with Spring.
|
||
@Override | ||
public int getSessionTimeout() { | ||
return 15; |
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.
let's use 0, wdyt?
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.
Makes sense.
</dependency> | ||
<dependency> | ||
<groupId>org.eclipse.angus</groupId> | ||
<artifactId>angus-mail</artifactId> |
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.
Ok I see, that's related to #504. Need to revisit that, angus-mail should not be required just for mimetype resolution.
samples/spring/pet-store/pom.xml
Outdated
<maven.compiler.source>1.8</maven.compiler.source> | ||
<maven.compiler.target>1.8</maven.compiler.target> | ||
<spring.version>5.3.26</spring.version> | ||
<java.version>17</java.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.
java.version
is a Spring Boot specific property, isn't it? Afaik it only has an effect if the pom extends spring-boot-starter-parent
.
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.
You're right. Thanks for pointing that out.
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.
Let's remove the java9-plus
profile along with the old com.sun.activation:jakarta.activation
dependency.
|
||
@Override | ||
public String getProtocolRequestId() { | ||
return null; |
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.
return empty string, implement in AwsHttpServletRequest
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
@@ -498,6 +492,21 @@ public AsyncContext getAsyncContext() { | |||
return asyncContext; | |||
} | |||
|
|||
@Override | |||
public String getRequestId() { | |||
return null; |
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.
we have a requestId, let's return it. implement in AwsHttpServletRequest
.
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.
Done
// public void setStatus(int i, String s) { | ||
// if (!canSetHeader()) return; | ||
// statusCode = i; | ||
// statusMessage = s; |
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 like statusMessage
variable can be remove completely
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.
Done
@@ -80,7 +80,7 @@ public void forward(ServletRequest servletRequest, ServletResponse servletRespon | |||
} | |||
|
|||
if (isNamedDispatcher) { | |||
lambdaContainerHandler.doFilter((HttpServletRequest) servletRequest, (HttpServletResponse) servletResponse, getServlet(dispatchTo)); |
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 got explicitly added in rev. f1755de
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
@@ -44,8 +46,8 @@ | |||
//------------------------------------------------------------- | |||
// Constants - Public | |||
// ------------------------------------------------------------- | |||
public static final int SERVLET_API_MAJOR_VERSION = 3; | |||
public static final int SERVLET_API_MINOR_VERSION = 1; | |||
public static final int SERVLET_API_MAJOR_VERSION = 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.
should be 6 now
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.
Thanks. Updated
<groupId>javax.activation</groupId> | ||
<artifactId>activation</artifactId> | ||
<version>1.1.1</version> | ||
<scope>test</scope> |
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 be 2.1.1
<spring.version>6.0.8</spring.version> | ||
<springboot.version>3.0.5</springboot.version> | ||
<springsecurity.version>6.0.2</springsecurity.version> | ||
<java.version>17</java.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.
java.version is a Spring Boot specific property. It only has an effect if the pom extends spring-boot-starter-parent.
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.
Right. Updated.
<scope>test</scope> | ||
</dependency> | ||
<dependency> | ||
<groupId>jakarta.activation</groupId> | ||
<artifactId>jakarta.activation-api</artifactId> |
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.
is it needed here? seems it also works without 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.
Updated.
…ty-web, servlet API, commons-fileupload.
- Fixed 2 failing tests in AwsServletContextTest by adding version 2.0.0 of the angus-mail dependency
…ata (cannot find symbol @notblank, @Email etc...)
…izerError in SecurityAppTest - Updated hibernate-validator version to 8.0.0.FINAL to fix failing test in ServletAppTest (jakarta.validation not working) - Updated Spring-core version to 6.0.7 to fix CVSS error
…assfish:jakarta.el 5.0.0-M1 in Spring component's pom.xml -Updated the CommonsMultipartResolver to StandardServletMultipartResolver
… tests failing in JerseyAwsProxyTest class with error (java.lang.NoClassDefFoundError: jakarta/ws/rs/core/EntityPart)
- updated dependencies versions in Jersey, Spring and SpringBoot archetypes
- Updated some dependencies versions
- updated build.gradle files to be aligned to the Maven poms - moved all poms to 2.0-SNAPSHOT
….activation dependency.
9023174
to
980ebfa
Compare
Issue #, if available: #487
Description of changes:
By submitting this pull request