-
Notifications
You must be signed in to change notification settings - Fork 126
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
added spring boot 3.2 support #675
Conversation
Great work. Will review and release it soon |
public boolean handle(Request request, Response response, Callback callback) throws Exception { | ||
Content.Chunk chunk = request.read(); | ||
chunk.skip(Integer.MAX_VALUE); | ||
if (request.getHttpURI().getPath().startsWith(PATH_WITH_PAYLOAD)) { |
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 did you changed equals to startsWith?
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've reverted changes
@@ -36,7 +37,7 @@ public void shouldCallNotDefaultMethodOnActualImplementation() throws Throwable | |||
DefaultMethodHandler defaultMethodHandler | |||
= new DefaultMethodHandler(TestInterface.class.getMethod("defaultMethod")); | |||
|
|||
TestInterface mockImplementation = mock(TestInterface.class); | |||
TestInterface mockImplementation = spy(TestInterface.class); |
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 spy?
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've reverted changes to use mock
@@ -321,7 +332,7 @@ public void shouldReturnFirstResultBeforeSecondSent() throws InterruptedExceptio | |||
AtomicInteger sentCount = new AtomicInteger(); | |||
AtomicInteger receivedCount = new AtomicInteger(); | |||
|
|||
CompletableFuture<AllFeaturesApi.TestObject> firstReceived = new CompletableFuture<>(); | |||
CompletableFuture<AllFeaturesApi.TestObject> firstReceived = CompletableFuture.completedFuture(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.
I don't want completed future here.
I'm going to complete it later
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
ConcurrentLinkedQueue<byte[]> receivedAll = new ConcurrentLinkedQueue<>(); | ||
|
||
CompletableFuture<ByteBuffer> firstReceived = new CompletableFuture<>(); | ||
CompletableFuture<ByteBuffer> firstReceived = CompletableFuture.completedFuture(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.
I don't want completed future here.
I'm going to complete it later
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
@@ -480,7 +490,7 @@ public void shouldEncodePathParam() { | |||
|
|||
@Test | |||
public void shouldEncodePathParamWithReservedChars() { | |||
String PATH_PARAM = "workers?in=(\"123/321\")"; | |||
String PATH_PARAM = "workers?in=(\"123321\")"; |
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 did you change this test string?
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.
jetty12 (as Servlet 6) no longer supports / as part of the path section of the URI
<artifactId>jetty-webapp</artifactId> | ||
<scope>test</scope> | ||
<groupId>org.eclipse.jetty.ee10</groupId> | ||
<artifactId>jetty-ee10-webapp</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.
this dependency introduces licensing issue:
Dual license: EPL-2.0, GPL-2.0-with-classpath-exception
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 have no idea what to do with this. Any suggestions? :)
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.
try to add test scope
<groupId>org.eclipse.jetty</groupId> | ||
<artifactId>jetty-servlet</artifactId> | ||
<groupId>org.eclipse.jetty.ee10</groupId> | ||
<artifactId>jetty-ee10-servlets</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.
this dependency introduces licensing issue:
Dual license: EPL-2.0, GPL-2.0-with-classpath-exception
<scope>test</scope> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>org.eclipse.jetty</groupId> | ||
<artifactId>jetty-webapp</artifactId> | ||
<groupId>org.eclipse.jetty.ee10</groupId> |
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 dependency introduces licensing issue:
Dual license: EPL-2.0, GPL-2.0-with-classpath-exception
feign-reactor-parent/pom.xml
Outdated
@@ -74,6 +75,59 @@ | |||
<scope>import</scope> | |||
</dependency> | |||
|
|||
<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.
why did you add all this dependency here?
it should come from spring-boot-dependencies
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 are right, I've reverted changes here
Hi @kptfh @Donquih0te , Thank you for working on this PR to support Spring Boot 3.2.x! This feature is critical for my project, and I'm eagerly awaiting its release. Could you please provide an estimated timeline for when this might be merged and released? It would help me plan my upgrades accordingly. Thanks in advance. |
Support for spring boot 3.2.x version.
Support for Jetty 12.