Post empty body#2558
Conversation
- Simplified creation of HTTP requests in tests with utility methods (`HttpTestUtil`). - Consolidated HTTP test resources into reusable format using `convertMessage`. - Refactored `Request` logic for better readability and modularity. - Enhanced test coverage and code quality across various HTTP-related test classes (`RequestTest`, `MessageAnalyserTest`, etc.). - Removed redundant `create` method from `Request` class in favor of streamlined implementations.
…n test classes - Enhanced return type descriptions in `HttpTestUtil` Javadoc. - Cleaned up unused `@throws` annotations in `RequestTest`. - Removed redundant `@SuppressWarnings` in `HttpUtilTest`.
📝 WalkthroughWalkthroughThe PR removes a public create(...) initializer from Request, adds METHOD_POST to the set of methods that may have an optional body, refactors multiple tests to use a fluent builder API and an HTTP message test utility, and deletes legacy test resource fixtures. Changes
Sequence Diagram(s)(omitted — changes are focused on API/test refactor and do not introduce new multi-component control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@core/src/test/java/com/predic8/membrane/core/transport/http/HttpServerHandlerTest.java:
- Around line 7-8: Fix the typos in the TODO comment inside the
HttpServerHandlerTest class: replace "emty" with "empty" and correct "body.k" to
a proper phrase such as "body." so the comment reads clearly (e.g., "TODO: Start
Router. RESTAssured post empty body."). Update the comment text near the
class/method where the TODO appears so it accurately documents the test intent.
In @core/src/test/java/com/predic8/membrane/core/util/HttpTestUtil.java:
- Around line 14-16: convertMessage currently calls getBytes() which uses the
platform default charset; update convertMessage to call getBytes with an
explicit charset to ensure consistent test behavior (e.g., use
StandardCharsets.ISO_8859_1 for HTTP message bytes) by replacing getBytes() with
getBytes(StandardCharsets.ISO_8859_1) in the convertMessage method and add the
necessary import or fully-qualified reference to StandardCharsets.
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/http/RequestTest.java (1)
119-149: Consider simplifying tearDown with a helper method.The manual null checks and close calls are correct but verbose. Consider extracting a helper method to reduce duplication.
♻️ Proposed refactor
Add a helper method and simplify tearDown:
+ private void closeQuietly(AutoCloseable closeable) { + if (closeable != null) { + try { + closeable.close(); + } catch (Exception e) { + // Ignore cleanup exceptions in tests + } + } + } + @AfterEach public void tearDown() throws Exception { - if (getReq != null) { - getReq.close(); - } - if (inPost != null) { - inPost.close(); - } - if (inEmptyBody != null) { - inEmptyBody.close(); - } - if (inEmptyBodyContentLength != null) { - inEmptyBodyContentLength.close(); - } - if (inEmptyBodyContentType != null) { - inEmptyBodyContentType.close(); - } - if (inNoChunks != null) { - inNoChunks.close(); - } - if (inChunked != null) { - inChunked.close(); - } - if (tempIn != null) { - tempIn.close(); - } - if (tempOut != null) { - tempOut.close(); - } - + closeQuietly(getReq); + closeQuietly(inPost); + closeQuietly(inEmptyBody); + closeQuietly(inEmptyBodyContentLength); + closeQuietly(inEmptyBodyContentType); + closeQuietly(inNoChunks); + closeQuietly(inChunked); + closeQuietly(tempIn); + closeQuietly(tempOut); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/src/main/java/com/predic8/membrane/core/http/Request.javacore/src/test/java/com/predic8/membrane/core/exchangestore/LimitedMemoryExchangeStoreTest.javacore/src/test/java/com/predic8/membrane/core/http/RequestTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/MessageAnalyserTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/soap/SoapOperationExtractorTest.javacore/src/test/java/com/predic8/membrane/core/transport/http/HttpServerHandlerTest.javacore/src/test/java/com/predic8/membrane/core/util/HttpTestUtil.javacore/src/test/java/com/predic8/membrane/core/util/HttpUtilTest.javacore/src/test/resources/request-chunked-soap.msgcore/src/test/resources/request-post.msg
💤 Files with no reviewable changes (2)
- core/src/test/resources/request-post.msg
- core/src/test/resources/request-chunked-soap.msg
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-18T12:03:09.931Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.931Z
Learning: The mediaType method in the Message class (which Request extends) throws jakarta.mail.internet.ParseException because it creates a ContentType object for parsing MIME media types. The jakarta.mail.internet.ParseException import is correct and necessary in test files that call mediaType().
Applied to files:
core/src/test/java/com/predic8/membrane/core/interceptor/MessageAnalyserTest.java
📚 Learning: 2025-06-18T12:03:09.931Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.931Z
Learning: The mediaType method in the Request class throws jakarta.mail.internet.ParseException, not java.text.ParseException, making the jakarta.mail.internet.ParseException import necessary and correct in JsonSchemaTestSuiteTests.java.
Applied to files:
core/src/test/java/com/predic8/membrane/core/interceptor/MessageAnalyserTest.javacore/src/test/java/com/predic8/membrane/core/http/RequestTest.javacore/src/test/java/com/predic8/membrane/core/util/HttpUtilTest.java
📚 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:
core/src/test/java/com/predic8/membrane/core/interceptor/MessageAnalyserTest.javacore/src/test/java/com/predic8/membrane/core/http/RequestTest.javacore/src/main/java/com/predic8/membrane/core/http/Request.java
🧬 Code graph analysis (4)
core/src/test/java/com/predic8/membrane/core/exchangestore/LimitedMemoryExchangeStoreTest.java (2)
core/src/main/java/com/predic8/membrane/core/http/Response.java (1)
Response(38-500)core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-357)
core/src/test/java/com/predic8/membrane/core/interceptor/MessageAnalyserTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-357)
core/src/test/java/com/predic8/membrane/core/interceptor/soap/SoapOperationExtractorTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-357)
core/src/test/java/com/predic8/membrane/core/util/HttpUtilTest.java (1)
core/src/test/java/com/predic8/membrane/core/util/HttpTestUtil.java (1)
HttpTestUtil(7-17)
⏰ 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). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (12)
core/src/test/java/com/predic8/membrane/core/exchangestore/LimitedMemoryExchangeStoreTest.java (2)
17-23: LGTM!The migration to wildcard imports and static imports for
Response.ok()improves test code readability and aligns with the broader test suite migration to fluent builder patterns.
61-66: LGTM!The migration to the fluent builder pattern (
Request.get("/test").buildExchange()andok().body(...).build()) simplifies test setup and aligns with the new API design. The broadened exception signature is acceptable for test code.core/src/test/java/com/predic8/membrane/core/interceptor/soap/SoapOperationExtractorTest.java (2)
16-22: LGTM!The migration to wildcard imports and static imports for
Request.post()andSoapOperationExtractorconstants improves test code readability and consistency with the broader test suite refactoring.
69-71: LGTM!The migration to the fluent builder pattern (
post("/test").body(...).buildExchange()) simplifies test setup and correctly passes the resource stream to the body builder. The broadened exception signature is acceptable for test code.core/src/test/java/com/predic8/membrane/core/interceptor/MessageAnalyserTest.java (3)
23-24: LGTM!The addition of static imports for
Request.postandResponse.okimproves test code readability and aligns with the broader test suite refactoring to use fluent builder patterns.
65-69: LGTM!The migration to the static import
ok()improves readability. Note thatgetResponse()manually creates anExchangewhilegetRequest()usesbuildExchange()— this difference appears intentional since response-only exchanges don't need the full exchange setup with destinations and original URI.
71-73: LGTM!The migration to the fluent builder pattern (
post("/test").body(...).buildExchange()) simplifies test setup and aligns with the new API design. The broadened exception signature is acceptable for test code.core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
49-59: POST requests can now have optional bodies.The implementation is correct:
METHOD_POSTjoinsMETHOD_DELETEand WebDAV methods inmethodsWithOptionalBody, allowing POST requests to omit bodies whenContent-Length: 0or when neitherContent-LengthnorTransfer-Encodingheaders are present. TheshouldNotContainBody()logic properly handles all cases, and existing tests comprehensively cover empty POST body scenarios (includingContent-Length: 0,Transfer-Encodingpresence, and implicit empty body). No validation logic in the codebase requires POST to have a body, and this aligns with HTTP specifications.core/src/test/java/com/predic8/membrane/core/util/HttpUtilTest.java (1)
28-28: LGTM: Clean migration to in-test HTTP construction.The refactoring to use
HttpTestUtil.convertMessage()with an inlinePOST_REQUESTconstant is cleaner and more maintainable than loading from external resource files. The test logic remains correct.Also applies to: 37-62
core/src/test/java/com/predic8/membrane/core/http/RequestTest.java (3)
48-105: LGTM: Well-structured test fixtures.The inline HTTP message constants with text blocks are clear and maintainable. The
@SuppressWarningsannotation for trailing whitespace inNO_CHUNKSis appropriately documented and necessary for chunked parsing.
151-369: LGTM: Comprehensive test coverage with clean implementation.The test methods effectively cover various HTTP request scenarios (GET, POST, chunked encoding, empty bodies, OPTIONS, CONNECT). The refactoring to use the new fixture-based approach maintains correctness while improving maintainability.
203-207: LGTM: Well-designed test helper methods.The helper methods effectively reduce duplication and improve test readability. The
create()method provides a test-specific alternative to the removed production initializer and is appropriately scoped for test usage.Also applies to: 336-340, 365-369, 371-382
|
This pull request needs "/ok-to-test" from an authorized committer. |
…ests - Updated `convertMessage` in `HttpTestUtil` to use UTF-8 encoding explicitly. - Simplified resource cleanup in `RequestTest` with `closeQuietly`. - Refactored `shouldBodyBeRead` to clarify logic and improve readability.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @core/src/test/java/com/predic8/membrane/core/http/RequestTest.java:
- Around line 204-222: The tests isHTTP11Chunked and isKeepAliveChunked
currently assert on the default request (variable request) but their names imply
they should use chunked input; fix by either (A) creating a chunked request
instance and asserting against it (e.g., build a chunkedRequest with
Transfer-Encoding: chunked or use the existing helper that produces chunked
input, then call chunkedRequest.isHTTP11() and chunkedRequest.isKeepAlive()), or
(B) if the intent was to test default behavior, rename the test methods to
remove the "Chunked" suffix (rename isHTTP11Chunked -> isHTTP11 and
isKeepAliveChunked -> isKeepAlive) and update any duplicated test names to avoid
conflicts; ensure you update assertions to reference the correct variable
(request vs chunkedRequest) and keep method names consistent with the input
used.
- Around line 341-352: The test's create(...) helper directly mutates Request's
internal fields (method, uri, version, header) and calls createBody(in); replace
this with Request's public construction API (e.g., a builder, factory or public
constructor) or add a package-visible factory on Request that accepts method,
uri, protocol and InputStream and performs initialization/validation internally;
update the create(...) method to call that public/factory method instead of
setting fields and invoking createBody(in) so the test relies only on Request's
public behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/test/java/com/predic8/membrane/core/http/RequestTest.javacore/src/test/java/com/predic8/membrane/core/util/HttpTestUtil.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/com/predic8/membrane/core/util/HttpTestUtil.java
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-18T12:03:09.931Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.931Z
Learning: The mediaType method in the Request class throws jakarta.mail.internet.ParseException, not java.text.ParseException, making the jakarta.mail.internet.ParseException import necessary and correct in JsonSchemaTestSuiteTests.java.
Applied to files:
core/src/test/java/com/predic8/membrane/core/http/RequestTest.java
📚 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:
core/src/test/java/com/predic8/membrane/core/http/RequestTest.java
📚 Learning: 2025-06-18T12:03:09.931Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.931Z
Learning: The mediaType method in the Message class (which Request extends) throws jakarta.mail.internet.ParseException because it creates a ContentType object for parsing MIME media types. The jakarta.mail.internet.ParseException import is correct and necessary in test files that call mediaType().
Applied to files:
core/src/test/java/com/predic8/membrane/core/http/RequestTest.java
🧬 Code graph analysis (1)
core/src/test/java/com/predic8/membrane/core/http/RequestTest.java (4)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-357)core/src/test/java/com/predic8/membrane/core/util/HttpTestUtil.java (1)
HttpTestUtil(10-20)core/src/test/java/com/predic8/membrane/core/util/StringTestUtil.java (1)
StringTestUtil(19-24)
⏰ 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). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (13)
core/src/test/java/com/predic8/membrane/core/http/RequestTest.java (13)
24-27: LGTM: Test import reorganization.The shift to wildcard static imports for
MimeTypeandHttpTestUtilis appropriate for test files and improves readability by reducing import clutter.
34-85: LGTM: Comprehensive fixture-based test data.The refactoring from file-based fixtures to embedded string literals using text blocks significantly improves test maintainability. The variety of test cases (GET, POST, chunked encoding, empty body variants) provides good coverage of different HTTP message formats.
86-126: LGTM: Proper resource management in test lifecycle.The setUp/tearDown pattern ensures all InputStreams are properly initialized and closed, preventing resource leaks in tests.
97-101: LGTM: Clean helper method for body reading verification.The helper method appropriately encapsulates the logic for verifying whether a request body should be read based on HTTP message structure.
128-146: LGTM: Start line and chunked encoding tests.These tests appropriately verify request parsing for GET requests and chunked transfer encoding scenarios.
148-162: LGTM: POST and empty chunked body tests.Good coverage of POST requests with body content and the edge case of chunked encoding with zero-length body.
164-183: LGTM: Comprehensive empty body test coverage.The three test variants (no body headers, explicit Content-Length: 0, Content-Type only) provide thorough coverage of empty POST body scenarios.
185-202: LGTM: Write/read roundtrip test.This test appropriately verifies that Request serialization and deserialization preserves both metadata and body content.
224-234: LGTM: Body emptiness checks.These tests appropriately verify the
isBodyEmpty()method using various construction approaches (empty string, empty bytes, GET request, POST with body).
257-333: LGTM: Comprehensive HTTP semantics tests.These tests provide good coverage of header manipulation, body replacement behavior, OPTIONS/CONNECT method handling, and content type detection. The tests correctly verify that replacing a body triggers reading of the original body to prevent keep-alive session corruption.
335-339: LGTM: Helper method for body retrieval.Clean extraction of common test logic for reading a message and obtaining its unread body.
354-361: LGTM: Standard resource cleanup utility.The
closeQuietlyhelper is a standard pattern for test tearDown methods, ensuring resources are closed without propagating exceptions.
237-254: No action needed. The resource file/getBank.xmlexists atcore/src/test/resources/getBank.xmland is properly accessible to the tests.
3 participants
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.