PATCH Method with no Body#2770
Conversation
… body handling tests - Changed `@BeforeAll` and `@AfterAll` annotations to non-static for consistency. - Reorganized test setup and teardown logic to ensure proper resource management. - Added test cases for HTTP/1.0 requests with and without bodies. - Introduced `PATCH_EMPTY_BODY` and improved input stream naming for clarity. - Updated `shouldNotContainBody` logic to handle HTTP/1.0 requests correctly.
…variable declarations
📝 WalkthroughWalkthroughRequest API visibility and internals were reorganized: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issuesNone 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
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/src/test/java/com/predic8/membrane/core/http/RequestTest.java`:
- Around line 140-153: The tearDown method omits closing the inPatchEmptyBody
InputStream causing a resource leak; update the tearDown method (which currently
calls closeQuietly for getReq, inPost, inPostEmptyBody, etc.) to also call
closeQuietly(inPatchEmptyBody) so the stream initialized in setUp is properly
closed; ensure the reference uses the exact field name inPatchEmptyBody and keep
it alongside the other closeQuietly(...) calls in tearDown.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/http/Request.java (2)
48-51: Fieldsmethodandurichanged fromprivateto package-private.This relaxes encapsulation to allow direct field access from test code in the same package (see
RequestTest.create()at line 396). This is a reasonable trade-off for testability, but worth noting that any class incom.predic8.membrane.core.httpcan now mutate these fields directly, bypassing setters.
178-200: Static factory methods create a newURIFactoryon every call.Each
get(String),post(String),delete(String), etc. instantiates anew URIFactory()internally. IfURIFactoryconstruction is non-trivial, this could become a concern on hot paths. For now this is fine, but consider documenting that theURIFactory-accepting overloads should be preferred for performance-sensitive code.
…est` - Replaced direct field access with `setMethod` and `setUri` to improve encapsulation. - Adjusted `RequestTest` to use setters for `method` and `uri`.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/src/main/java/com/predic8/membrane/core/http/Request.java`:
- Around line 259-262: The Builder.contentType(String) method currently calls
req.getHeader().add(CONTENT_TYPE, value) which can create duplicate Content-Type
headers; change it to use the same header replacement logic as json()/xml() by
calling the header's setContentType(...) or equivalent replace method on
req.getHeader() so the Content-Type is replaced rather than appended (i.e.,
update Builder.contentType to use req.getHeader().setContentType(...) or the
header object used by json()/xml()).
- Around line 215-226: The buildExchange() path currently always adds the field
fullURL to exc.getDestinations(), which can be null when Request was built via
uri(String) or connect(String); update
Request.buildExchange(AbstractHttpHandler) (and the no-arg buildExchange()) to
avoid adding null: if fullURL is non-null add it as now, otherwise add a safe
alternative such as req.getUri().toString() (or simply skip adding) so no null
is inserted into Exchange.getDestinations(); locate the code in the
buildExchange methods and replace the unconditional
exc.getDestinations().add(fullURL) with a null-check and the fallback.
…er` for header consistency
…ch-with-no-body-fix
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/src/main/java/com/predic8/membrane/core/http/Request.java`:
- Around line 350-354: The connect() Builder helper directly calls
req.setMethod(METHOD_CONNECT) and req.setUri(...) and therefore bypasses the
method(...) and url(...) helpers so fullURL is never initialized, causing
buildExchange() to add a null destination; fix by ensuring
Builder.connect(String url) sets fullURL (e.g., fullURL = url) and then
delegates to method(METHOD_CONNECT) and url(url) or at minimum assign fullURL
before calling req.setUri/getAuthority; update the connect() implementation
(refer to Builder.connect, method(...), url(...), fullURL, req.setMethod,
req.setUri, buildExchange()) to match the other verb helpers' behavior.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/http/Request.java (2)
56-68: STOMP fallback silently swallows malformed HTTP request lines.Since
stompPattern(^(.+?)$) matches any non-empty string, a malformed HTTP request line like"GET /foo"(missing version) will be silently parsed as a STOMP frame instead of raisingEOFWhileReadingFirstLineException. This could mask real protocol errors in non-STOMP contexts.Consider adding a stricter STOMP check (e.g., matching known STOMP commands like
CONNECT,SEND,SUBSCRIBE, etc.) or using an external flag to indicate whether STOMP is expected on this connection.
178-200: Nopatch()convenience method on the Builder, despiteMETHOD_PATCHbeing defined.Static entry points and instance methods exist for GET, POST, PUT, DELETE, OPTIONS, and CONNECT, but PATCH is missing — which is ironic given the PR title. Consider adding
patch(String)andpatch(URIFactory, String)for consistency.Proposed addition
public static Builder connect(String url) throws URISyntaxException { return new Builder().connect(url); } + +public static Builder patch(String url) throws URISyntaxException { + return new Builder().patch(url); +}And in the
Builderclass:public Builder patch(URIFactory uriFactory, String url) throws URISyntaxException { return method(Request.METHOD_PATCH).url(uriFactory, url); } public Builder patch(String url) throws URISyntaxException { return patch(new URIFactory(), url); }
|
/ok-to-test |
|
This pull request needs "/ok-to-test" from an authorized committer. |
3 participants
Summary by CodeRabbit
New Features
Improvements
Tests