Add log for mismatching content-type #2264
Conversation
WalkthroughAdds a centralized pre-evaluation guard in AbstractExchangeExpression to detect empty bodies or unacceptable Content-Type and return type-appropriate defaults; XPath and JsonPath expression classes now call this guard and return early when applicable; JsonPath's prior local guard/helper was removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (Expression.evaluate)
participant Expr as Expression (XPath/JsonPath)
participant Abstract as AbstractExchangeExpression
Caller->>Expr: evaluate(exchange, type)
Expr->>Abstract: checkContentTypeAndBody(msg, type, contentTypeOk, label, log)
alt empty body or wrong Content-Type
Abstract-->>Expr: default result (String="", Boolean=false, or null)
Expr-->>Caller: early return default
else valid body and Content-Type
Expr->>Expr: perform XPath/JsonPath evaluation
Expr-->>Caller: evaluation result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-06-18T12:03:09.931ZApplied to files:
⏰ 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)
🔇 Additional comments (2)
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. |
![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 (1)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
94-102: Consider returningnullinstead ofnew Object()for Object type.Returning
new Object()creates a fresh instance each time, which is unconventional for a "no evaluation" scenario. This could lead to unexpected behavior if callers check for identity, equality, or null.Consider using
nullto indicate no result was produced, which is more idiomatic and aligns with typical Java conventions for absent values.Apply this diff:
private <T> T resultForNoEvaluation(Class<T> type) { if (String.class.isAssignableFrom(type)) { return type.cast(""); } if (Boolean.class.isAssignableFrom(type)) { return type.cast(FALSE); } - return type.cast(new Object()); + return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
core/src/main/java/com/predic8/membrane/core/util/XMLUtil.java (1)
XMLUtil(30-85)
⏰ 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/lang/xpath/XPathExchangeExpression.java (1)
28-28: LGTM: Imports are appropriately added.The
IOExceptionandFALSEimports are correctly added to support the new guard logic and default value handling.Also applies to: 31-31
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
48-57: Past review feedback not fully addressed.The past review comment suggested splitting the two conditions (empty body vs. non-XML content) into separate checks with distinct log messages. While the log level was changed to INFO as requested, the conditions remain combined in a single check.
Additionally, the same concern from JsonpathExchangeExpression applies here: INFO level may be too verbose if empty bodies or non-XML requests are common. Consider:
- Logging empty bodies at DEBUG level (common with health checks)
- Logging content-type mismatches at INFO level (potentially actionable)
Apply this diff to address the feedback:
- // Guard against empty body and other Content-Types - try { - if (msg.isBodyEmpty() || !msg.isXML()) { - log.info("Body is empty or Content-Type not XML. Nothing to evaluate for expression: {}", expression); - return resultForNoEvaluation(type); - } - } catch (IOException e) { - log.error("Error checking if body is empty", e); - return resultForNoEvaluation(type); - } + // Guard against empty body and other Content-Types + try { + if (msg.isBodyEmpty()) { + log.debug("Body is empty. Nothing to evaluate for expression: {}", expression); + return resultForNoEvaluation(type); + } + if (!msg.isXML()) { + log.info("Content-Type is not XML. Nothing to evaluate for expression: {}", expression); + return resultForNoEvaluation(type); + } + } catch (IOException e) { + log.error("Error checking if body is empty", e); + return resultForNoEvaluation(type); + }
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
94-102: Consider extracting duplicate logic to a shared utility.The
resultForNoEvaluationmethod is identical in bothXPathExchangeExpressionandJsonpathExchangeExpression. This duplication could be eliminated by extracting the method to a shared utility class or abstract base class.For example, move this to
AbstractExchangeExpression:protected <T> T resultForNoEvaluation(Class<T> type) { if (String.class.isAssignableFrom(type)) { return type.cast(""); } if (Boolean.class.isAssignableFrom(type)) { return type.cast(FALSE); } return type.cast(new Object()); }core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java (1)
69-69: Change log level from INFO to DEBUG for routine guard clause.Treat empty request bodies or Content-Type mismatches from clients as validation issues — log at DEBUG by default; reserve INFO for unexpected or anomalous cases. The inline comment "// Normal" confirms this is expected behavior. Additionally, use DEBUG/TRACE to record message content during development or investigation — avoid INFO/WARN in normal production for volume reasons.
The identical guard clause in
XPathExchangeExpression.java(line 51) uses the same INFO level; consider aligning both to DEBUG for consistency with this pattern and logging best practices.log.debug("Body is empty or Content-Type not JSON. Nothing to evaluate for expression: {}", expression); // Normal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
core/src/main/java/com/predic8/membrane/core/util/XMLUtil.java (1)
XMLUtil(30-85)
⏰ 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/lang/xpath/XPathExchangeExpression.java (1)
28-31: LGTM!The new imports are necessary for the guard block's IOException handling and the FALSE constant in
resultForNoEvaluation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
48-49: Consider reusing the message variable.Line 46 retrieves the message into
msg, but line 48 callsexchange.getMessage(flow)again. You could passmsgto avoid the duplicate retrieval.Apply this diff:
Message msg = exchange.getMessage(flow); - T check = checkContentTypeAndBody(exchange.getMessage(flow), type, Message::isXML, "XML", log); + T check = checkContentTypeAndBody(msg, type, Message::isXML, "XML", log); if (check != null) return check;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/lang/AbstractExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
core/src/main/java/com/predic8/membrane/core/util/XMLUtil.java (1)
XMLUtil(30-85)
⏰ 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 (6)
core/src/main/java/com/predic8/membrane/core/lang/AbstractExchangeExpression.java (3)
16-22: LGTM! Imports support the new guard method.All added imports are necessary for the centralized content-type and body checking logic.
65-81: LGTM! Centralized guard aligns with PR objectives.The method correctly logs at
infolevel when body is empty or content-type mismatches, fulfilling the PR's stated objective. The null-return pattern to signal "continue with evaluation" is clear in context.
83-91: LGTM! Type-appropriate defaults are sensible.The helper returns appropriate fallback values for each type: empty string for String, FALSE for Boolean, and null otherwise.
core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java (2)
21-21: LGTM! Import needed for method reference.The
Messageimport is required for theMessage::isJSONmethod reference in the guard check.
67-68: LGTM! Consistent with XPath implementation.The centralized guard correctly checks for JSON content-type and empty bodies before evaluation, maintaining consistency with the XPath expression handling.
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
28-31: Remove unused imports from XPathExchangeExpression.javaBoth the
IOExceptionandFALSEimports are unused in this file. While the parent classAbstractExchangeExpressionuses both, each class imports only what it directly uses. XPathExchangeExpression does not call methods that throwIOException, nor does it directly referenceFALSE.Remove lines 28 and 31:
- Line 28:
import java.io.IOException;- Line 31:
import static java.lang.Boolean.FALSE;Likely an incorrect or invalid review comment.
|
/ok-to-test |
Change log level from debug to info?
Summary by CodeRabbit
Bug Fixes
Refactor
Style