Fix: HttpServletHandler: Request.create() caused build failure#2604
Conversation
…and caused build failure
📝 WalkthroughWalkthroughAdded two fluent-builder methods ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request needs "/ok-to-test" from an authorized committer. |
|
/ok-to-test |
…tHandler` to improve clarity and retain functionality.
christiangoerdes
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
war/src/main/java/com/predic8/membrane/servlet/embedded/HttpServletHandler.java (1)
134-145: Consider reducing visibility and avoiding parameter shadowing.The new helper method correctly constructs a
Requestfrom the given parameters. Two minor observations:
Visibility: The method is
publicbut only called internally. If external access isn't needed, consider reducing to package-private to minimize API surface.Parameter naming: The local
requestparameter shadows the class fieldthis.request(of typeHttpServletRequest). While the types differ and won't cause compilation issues, renaming toreqorresultcould improve clarity.♻️ Optional refactor
- public Request createRequest(String method, String uri, String protocol, Header header, InputStream in) throws IOException { - Request request = new Request(); - request.setMethod(method); - request.setUri(uri); + Request createRequest(String method, String uri, String protocol, Header header, InputStream in) throws IOException { + Request req = new Request(); + req.setMethod(method); + req.setUri(uri); if (!protocol.startsWith("HTTP/")) throw new RuntimeException("Unknown protocol '" + protocol + "'"); - request.setVersion(protocol.substring(5)); - request.setHeader(header); + req.setVersion(protocol.substring(5)); + req.setHeader(header); - request.createBody(in); - return request; + req.createBody(in); + return req; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/http/Message.javacore/src/main/java/com/predic8/membrane/core/http/Response.javawar/src/main/java/com/predic8/membrane/servlet/embedded/HttpServletHandler.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.787Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.787Z
Learning: Request.get().path() in com.predic8.membrane.core.openapi.model.Request preserves the query string portion of the path, so URIFactory.createWithoutException(request.getPath()).getQuery() correctly extracts query parameters.
Applied to files:
war/src/main/java/com/predic8/membrane/servlet/embedded/HttpServletHandler.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
core/src/main/java/com/predic8/membrane/core/http/Message.java (1)
167-206: LGTM - visibility change is appropriate.Changing
createBody(InputStream in)fromprotectedtopublicenables external callers (likeHttpServletHandlerin thewarmodule) to construct request/response bodies. This aligns with the PR objective of movingRequest.create()logic intoHttpServletHandler.core/src/main/java/com/predic8/membrane/core/http/Response.java (1)
424-430: LGTM - visibility change maintains override contract.The
createBody(InputStream in)override must match or exceed the visibility of the parentMessage.createBody(). Since the parent is nowpublic, this override correctly follows suit.war/src/main/java/com/predic8/membrane/servlet/embedded/HttpServletHandler.java (2)
32-32: LGTM - necessary import for new method signature.
126-132: LGTM - clean delegation to the new helper method.The refactored
createRequest()correctly extracts all necessary parameters fromHttpServletRequestand delegates to the new helper.
…HttpServletHandler.createRequest()` to use Builder for improved readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
war/src/main/java/com/predic8/membrane/servlet/embedded/HttpServletHandler.java (1)
134-145: Well-structured builder-based request construction.The protocol validation at line 135-136 correctly guards the
substring(5)call. The builder chain clearly expresses the request assembly.Two minor observations:
Visibility: The method is
publicbut the enclosing class is package-private. If this is solely for testability, consider keeping it package-private to match the class scope.Exception type: Consider using
IllegalArgumentExceptioninstead ofRuntimeExceptionfor the invalid protocol case, as it better communicates that the input is invalid.Optional: Use IllegalArgumentException for clearer semantics
public Request createRequest(String method, String uri, String protocol, Header header, InputStream in) { if (!protocol.startsWith("HTTP/")) - throw new RuntimeException("Unknown protocol '" + protocol + "'"); + throw new IllegalArgumentException("Unknown protocol '" + protocol + "'"); return new Request.Builder() .method(method) .uri(uri) .header(header) .body(in) .version(protocol.substring(5)) .build(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/http/Request.javawar/src/main/java/com/predic8/membrane/servlet/embedded/HttpServletHandler.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.787Z
Learning: Request.get().path() in com.predic8.membrane.core.openapi.model.Request preserves the query string portion of the path, so URIFactory.createWithoutException(request.getPath()).getQuery() correctly extracts query parameters.
📚 Learning: 2025-09-19T11:12:26.787Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.787Z
Learning: Request.get().path() in com.predic8.membrane.core.openapi.model.Request preserves the query string portion of the path, so URIFactory.createWithoutException(request.getPath()).getQuery() correctly extracts query parameters.
Applied to files:
war/src/main/java/com/predic8/membrane/servlet/embedded/HttpServletHandler.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
252-260: LGTM! Clean fluent builder additions.Both methods follow the established pattern of other builder methods in this class (e.g.,
method(),header()). Theuri()method provides a simpler alternative tourl(URIFactory, String)when the path/query is already extracted, andversion()allows overriding the default "1.1" set in the constructor.war/src/main/java/com/predic8/membrane/servlet/embedded/HttpServletHandler.java (1)
126-132: Clean delegation to the new builder-based method.The refactoring properly extracts the request construction logic while maintaining the existing behavior for path/query assembly and context root handling.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.