Skip to content

Fix Jetty CORS simple request TCK handling - #1060

Open
graemerocher wants to merge 5 commits into
6.0.xfrom
issue-929-jetty-cors-simple-request
Open

Fix Jetty CORS simple request TCK handling#1060
graemerocher wants to merge 5 commits into
6.0.xfrom
issue-929-jetty-cors-simple-request

Conversation

@graemerocher

Copy link
Copy Markdown
Contributor

Summary

  • re-enable CorsSimpleRequestTest in the Jetty TCK suite
  • split Jetty-specific unread form-body handling into dedicated Jetty servlet request and handler classes
  • keep the generic servlet engine hook overridable instead of using reflection for Jetty request access

Verification

  • ./gradlew :micronaut-http-server-jetty:compileJava
  • ./gradlew :test-suite-http-server-tck-jetty:test --tests '*FormUrlEncodedBodyInRequestFilterTest.bodyParsingInFilter' --tests '*CorsSimpleRequestTest.corsSimpleRequestAllowedForLocalhostAndOriginLocalhost'

Resolves #929

@graemerocher graemerocher added the type: bug Something isn't working label Apr 29, 2026
@graemerocher graemerocher self-assigned this Apr 29, 2026
@graemerocher
graemerocher requested a review from Copilot April 29, 2026 16:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR fixes Jetty-specific handling around unread form/multipart request bodies so Jetty can reliably commit responses (including CORS), and re-enables the Jetty TCK case that was previously excluded.

Changes:

  • Re-enable CorsSimpleRequestTest for the Jetty HTTP server TCK suite.
  • Add a prepareForResponse() hook to allow servlet engines to consume unread request bodies before committing responses.
  • Split Jetty-specific unread-form-body consumption into JettyServletHttpRequest and a JettyServletHttpHandler that replaces the default handler.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test-suite-http-server-tck-jetty/src/test/java/io/micronaut/http/server/tck/jetty/tests/JettyHttpServerTestSuite.java Re-enables the Jetty CORS simple request TCK test by removing an exclusion.
servlet-engine/src/main/java/io/micronaut/servlet/engine/DefaultServletHttpRequest.java Makes request body lazy + adds response-prep hook and overridable container-specific form-body preparation.
servlet-engine/src/main/java/io/micronaut/servlet/engine/DefaultServletHttpHandler.java Exposes protected accessors needed by a Jetty-specific handler replacement.
servlet-core/src/main/java/io/micronaut/servlet/http/ServletHttpRequest.java Introduces prepareForResponse() extension point for servlet-based requests.
servlet-core/src/main/java/io/micronaut/servlet/http/ServletHttpHandler.java Invokes prepareForResponse() prior to response transfer/commit.
http-server-jetty/src/main/java/io/micronaut/servlet/jetty/JettyServletHttpRequest.java Implements Jetty-native unread-body consumption for form/multipart before response commit.
http-server-jetty/src/main/java/io/micronaut/servlet/jetty/JettyServletHttpHandler.java Replaces the default handler to create the Jetty-specific request implementation.

@graemerocher

Copy link
Copy Markdown
Contributor Author

@copilot apply changes based on the comments in this thread

Copilot AI commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

@copilot apply changes based on the comments in this thread

Applied all four changes from the review thread in commit b6f91e8:

  1. close() – now reads byteBody into a local, closes it if non-null, and calls runDisposalResources() in the finally block.
  2. createByteBody() async path – restored the ByteBufferBodyAdapter.adapt(new ServletStreamPublisher(...), length) branch for async-supported containers to avoid blocking I/O regression.
  3. createByteBody() uses delegate() – replaced the stored delegate field with (HttpServletRequest) delegate() so async-dispatched requests use the correct wrapped request object.
  4. getRawFormFields() lazy bodyAccessed – moved bodyAccessed = true inside both Flux.defer lambdas so it is only set when the publisher is actually subscribed, not when the publisher is merely returned.

Also added Javadoc to prepareUnusedFormBodyForResponse() to fix the pre-existing checkstyle DesignForExtension error introduced in the PR.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • checkstyle.org
    • Triggering command: /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/xml/tools/xml-extractor.jar --fileList=/tmp/codeql-scratch-cb5f4094b95c6d98/dbs/java/working/files-to-index8566596677725783892.list --sourceArchiveDir=/tmp/codeql-scratch-cb5f4094b95c6d98/dbs/java/src --outputDir=/tmp/codeql-scratch-cb5f4094b95c6d98/dbs/java/trap/java (dns block)
  • ge.micronaut.io
    • Triggering command: /usr/lib/jvm/temurin-21-jdk-amd64/bin/java /usr/lib/jvm/temurin-21-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED --add-opens=java.base/java.time=ALL-UNNAMED -Xmx1g -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@graemerocher
graemerocher requested a review from yawkat April 30, 2026 12:38
@graemerocher
graemerocher marked this pull request as ready for review April 30, 2026 15:22
graemerocher and others added 5 commits May 4, 2026 08:36
Co-Authored-By: Codex with GPT-5 <codex@openai.com>
…azy bodyAccessed, restore async path, use delegate()

Agent-Logs-Url: https://github.com/micronaut-projects/micronaut-servlet/sessions/9c56aab2-dd08-417b-9188-0fe3875fbfa4

Co-authored-by: graemerocher <66626+graemerocher@users.noreply.github.com>
Co-Authored-By: Codex with GPT-5 <codex@openai.com>
Co-Authored-By: Codex with GPT-5 <codex@openai.com>
Co-Authored-By: Codex with GPT-5 <codex@openai.com>
@graemerocher
graemerocher force-pushed the issue-929-jetty-cors-simple-request branch from 80c521d to f571be5 Compare May 4, 2026 08:41
@sonarqubecloud

sonarqubecloud Bot commented May 4, 2026

Copy link
Copy Markdown

all {
resources.autodetect()
buildArgs.add("-H:+SharedArenaSupport")
if (JavaVersion.current().majorVersion.toInteger() >= 25) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're java 25+ only, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Java 25 baseline.

this.messageBodyHandlerRegistry = messageBodyHandlerRegistry;
this.ioExecutor = ioExecutor;
this.sslSessionProvider = sslSessionProvider;
long contentLengthLong = delegate.getContentLengthLong();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change? if it's just to close the source stream, this seems like the wrong layer to make the change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it was to make the bytebody initialization lazy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug Something isn't working

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

CorsSimpleRequestTest Jetty TCK fails for 4.9

5 participants