setBody#2462
Conversation
- Introduced `SetBodyInterceptor` to set HTTP message body content using static strings or dynamic expressions. - Added `DollarTemplateParserContext` for SpEL-based template parsing. - Refactored `TemplateExchangeExpression` and related classes to use the centralized `DollarTemplateParserContext`. - Improved error handling in `AbstractSetterInterceptor` with clearer logging and internal responses.
…d `AbstractSetterInterceptor`
WalkthroughAdds a new SetBodyInterceptor that evaluates expressions to set HTTP message bodies, extracts a shared DollarTemplateParserContext for SpEL templates, updates TemplateExchangeExpression imports/usage, and renames an error detail key from "value" to "expression" in AbstractSetterInterceptor. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Exchange
participant SetBodyInterceptor
participant ExpressionEngine
participant Message
participant ProblemDetails
Note over Client,SetBodyInterceptor: SetBodyInterceptor evaluation and apply flow
Client->>Exchange: send request / receive response
Exchange->>SetBodyInterceptor: handleRequest(handleResponse)(exc)
SetBodyInterceptor->>ExpressionEngine: evaluate(expression, exchange)
alt evaluation succeeds
ExpressionEngine-->>SetBodyInterceptor: evaluatedString
SetBodyInterceptor->>Message: setBody(evaluatedString as UTF-8)
SetBodyInterceptor->>Exchange: continue()
else evaluation fails
ExpressionEngine-->>SetBodyInterceptor: exception
SetBodyInterceptor->>ProblemDetails: build(detailKey="expression", cause)
SetBodyInterceptor->>Exchange: abortWith(ProblemDetails)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java (2)
37-54: Consider tightening response characteristics and log level for evaluation failuresThe core path is correct and the ProblemDetails usage is consistent, but two small improvements might help:
- Since you always serialize the evaluated value as UTF‑8, consider explicitly aligning or setting the
Content-Type/charsetof the message body when appropriate so downstream components and clients don’t see a header/encoding mismatch.- The log entry for failed evaluations is currently at
infolevel; depending on expected frequency, this might be better suited towarn(for unexpected failures) ordebug(if failures can be user‑driven and noisy).
These are behavioral/operational tweaks only; the current logic is otherwise sound.
56-79: Config API and descriptions are clear; optionally guard for verbose or null expressionsThe
@MCAttributesetter andgetDisplayNameare straightforward and consistent. One optional tweak: if expressions can be very long or unset, you might wantgetShortDescription()to either truncate the expression or fall back to a static description to avoid surfacing"null"or overly verbose text in UIs/logs.core/src/main/java/com/predic8/membrane/core/lang/spel/DollarTemplateParserContext.java (1)
3-3: Consider using a specific import instead of wildcard.Wildcard imports can obscure which classes are actually used and may lead to naming conflicts. Consider importing only the specific class needed.
Apply this diff:
-import org.springframework.expression.common.*; +import org.springframework.expression.common.TemplateParserContext;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractSetterInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/DollarTemplateParserContext.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java(0 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java (1)
core/src/main/java/com/predic8/membrane/core/lang/spel/DollarTemplateParserContext.java (1)
DollarTemplateParserContext(5-13)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java (2)
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (1)
ProblemDetails(36-414)core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java (1)
ExceptionUtil(16-39)
⏰ 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 (3)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractSetterInterceptor.java (1)
62-62: LGTM! Improved error detail naming.The change from "value" to "expression" makes the error detail key more descriptive and consistent with the variable name and surrounding context (e.g., line 55's error message also references "expression").
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java (1)
22-35: Request/response wiring and reuse viahandleInternallook solidUsing
handleRequest/handleResponseas thin wrappers around a sharedhandleInternalkeeps behavior consistent across flows and avoids duplication. No issues from a correctness or design perspective here.core/src/main/java/com/predic8/membrane/core/lang/spel/DollarTemplateParserContext.java (1)
5-11: Refactoring verified and complete.The singleton implementation is correct and thread-safe. All references to the old
DollarBracketTemplateParserContextinner class have been successfully removed, and the newDOLLAR_TEMPLATE_PARSER_CONTEXTinstance is properly imported and used inTemplateExchangeExpression.newInstance(). The refactoring improves code organization by centralizing parser context configuration. TheExchangeExpression.expression()factory method intentionally passesnullto support plain SpEL expressions without templating, serving a different use case thanTemplateExchangeExpression.
…ngeExpression` - Clarified UTF-8 encoding in `SetBodyInterceptor` Javadoc and added corresponding comment in method logic. - Added context on null `parserContext` in `SpELExchangeExpression` construction.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java (2)
47-47: Consider logging exceptions at WARN or ERROR level instead of INFO.Logging exceptions at INFO level may cause them to be missed in production environments where INFO logging is often disabled. Since this represents an error condition that aborts the exchange, WARN or ERROR would be more appropriate.
- log.info(message); + log.error(message);
65-68: Consider adding validation for the expression value.The
setValuemethod does not validate whether the provided expression is null or empty. While this might be intentional or handled by the parent class, consider adding validation to fail fast during configuration rather than at runtime.@MCAttribute public void setValue(String value) { + if (value == null || value.trim().isEmpty()) { + throw new IllegalArgumentException("Expression cannot be null or empty"); + } this.expression = value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java (2)
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (1)
ProblemDetails(36-414)core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java (1)
ExceptionUtil(16-39)
⏰ 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)
|
This pull request needs "/ok-to-test" from an authorized committer. |
…tests - Updated `SetBodyInterceptor` to handle cases where expression evaluation returns null, setting the body to "null" instead. - Introduced `SetBodyInterceptorTest` to verify null handling, expression evaluation, and response body modifications.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java (1)
48-59: Consider allowing stacktraces for easier debugging in non‑production.Currently, the ProblemDetails builder unconditionally calls
.stacktrace(false), which suppresses stacktraces even whengetRouter().isProduction()is false. This can make expression failures harder to debug during development, given thatProblemDetailsalready supports conditional exposure of details based on theproductionflag (seeProblemDetailsin core). Based on relevant code snippets, this suppression may be redundant in non‑production.You might consider either:
- Removing
.stacktrace(false)and relying onProblemDetails’ default behavior, or- Making stacktrace inclusion conditional on a debug/dev flag.
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptorTest.java (1)
22-52: Good coverage of primary behaviors; consider optional extra cases.The tests nicely cover literal values, simple path templating, and status-derived output on the request flow. If you want to harden this further, you could optionally add:
- A
handleResponse-based test to ensure the interceptor behaves identically on the response path.- A failure-case test (e.g., invalid expression) asserting that a ProblemDetails response is produced and the exchange is aborted.
These are non-blocking and can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptorTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java (2)
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (1)
ProblemDetails(36-414)core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java (1)
ExceptionUtil(16-39)
⏰ 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 (1)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptor.java (1)
39-48: Core interception logic and null handling look solid.Delegating both flows to
handleInternalkeeps behavior consistent, and the explicitresultvariable plus null-to-"null" mapping avoids NPEs when evaluating the expression. UTF‑8 encoding is appropriate given the configuration story.No changes needed here.
…ogic - Updated test method to use `handleResponse` instead of `handleRequest` in `SetBodyInterceptorTest`. - Adjusted assertions to verify response body content instead of request.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptorTest.java(1 hunks)
⏰ 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 (5)
core/src/test/java/com/predic8/membrane/core/interceptor/lang/SetBodyInterceptorTest.java (5)
1-11: LGTM!Imports are appropriate and well-organized for testing the SetBodyInterceptor with JUnit 5.
19-28: LGTM!The setup method correctly initializes test fixtures with appropriate test data. The Exchange is properly constructed with both request and response for comprehensive testing.
30-36: LGTM!The test correctly verifies that a literal "null" string value is set as the request body. The initialization, invocation, and assertion sequence is appropriate.
38-44: LGTM!The test correctly verifies that the expression
${path}is evaluated and resolved to the request path "/foo". Expression evaluation logic is properly exercised.
46-52: LGTM!The test correctly verifies that response bodies can be set using expressions. The
${statusCode}expression properly resolves to "501" from the exchange response.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.