fix JSON escaping #2743
fix JSON escaping #2743
Conversation
|
/ok-to-test |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTemplates across examples were changed to emit raw JSON values (unquoted expressions); core templating was updated with JSON-aware parameter bindings and escaping. A new StreamingTemplateEngine implementation and changes to GroovyBuiltInFunctions and related signatures enable JSON-aware template rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TemplateInterceptor
participant ScriptingUtils
participant GroovyBuiltInFunctions
participant StreamingTemplateEngine
participant Router
Client->>TemplateInterceptor: incoming request
TemplateInterceptor->>ScriptingUtils: createParameterBindings(..., producesJSON)
ScriptingUtils->>GroovyBuiltInFunctions: construct binding (params map, escape hook)
GroovyBuiltInFunctions-->>ScriptingUtils: binding map returned
TemplateInterceptor->>StreamingTemplateEngine: render template with bindings
StreamingTemplateEngine->>GroovyBuiltInFunctions: evaluate expressions (calls escape as needed)
StreamingTemplateEngine-->>TemplateInterceptor: rendered output (JSON-aware)
TemplateInterceptor-->>Client: send response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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: 2
🤖 Fix all issues with AI agents
In `@distribution/examples/web-services-soap/rest2soap-template/proxies.xml`:
- Around line 23-24: The population property is being extracted as a string and
then serialized with fn.toJSON, producing a JSON string; update the extraction
to use numeric XPath so the JSON output is a number: change the setProperty that
defines "population" to use fn.xpath('number(//population)') (language="xpath")
so property.population is a numeric value and fn.toJSON(property.population)
yields a JSON number; locate the setProperty/usage around property.population
and replace the current string XPath extraction with
fn.xpath('number(//population)').
In `@distribution/tutorials/orchestration/30-Orchestration.yaml`:
- Around line 32-34: The JSON template emits a stray trailing quote because
fn.toJSON(property.title) and fn.toJSON(fn.jsonPath('$.name')) already return
quoted strings, but the template left an extra literal " after the author
expression; remove the extra double-quote following
fn.toJSON(fn.jsonPath('$.name')) so the "author" line reads with only the
fn.toJSON(...) expression (refer to fn.toJSON, fn.jsonPath('$.name'), and
property.title to locate the affected template).
🧹 Nitpick comments (2)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/ApiKeyToJwtTest.java (1)
32-32: Stale hardcoded TOKEN still encodes scope as a string.The
TOKENconstant on line 32 was generated with the old format where scope is"[accounting, finance]"(a string). ThevalidJwttest on line 88 expectsJWT Scopes: [accounting, finance], which works because the old token's string value renders that way.However, this creates a discrepancy:
validKeyvalidates the new array format whilevalidJwtimplicitly relies on the old string format. If the TOKEN is ever regenerated to match the new scope format, the assertion on line 88 would likely need updating too (an array might render as["accounting", "finance"]rather than[accounting, finance]).Consider regenerating the TOKEN with the new scope format and updating line 88 accordingly, so both tests exercise the same contract.
Also applies to: 80-89
distribution/examples/orchestration/for-loop/apis.yaml (1)
33-34: Inconsistency:p.pricenot wrapped withfn.toJSONunlike the tutorial counterpart.In
distribution/tutorials/orchestration/50-Loop-get-Details.yaml(line 49),p.priceis wrapped withfn.toJSON(p.price). Here it remains as raw${p.price}. For consistency and safety (e.g., if the value isnullor an unexpected type), consider wrapping it as well.Suggested fix
- "name": ${fn.toJSON(p.name)}, - "price": ${p.price} + "name": ${fn.toJSON(p.name)}, + "price": ${fn.toJSON(p.price)}
distribution/examples/web-services-soap/rest2soap-template/proxies.xml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java`:
- Around line 131-133: The producesJSON method in TemplateInterceptor currently
does a case-sensitive check (contentType.contains("json") ) which misses
uppercase MIME variants; update TemplateInterceptor.producesJSON to normalize
contentType (e.g., call contentType.toLowerCase(Locale.ROOT) or use
equalsIgnoreCase on the subtype) before searching for "json", and guard against
null by keeping the existing null check so the method returns true for any
case-insensitive occurrence of "json".
In
`@core/src/main/java/com/predic8/membrane/core/lang/groovy/adapted/StreamingTemplateEngine.java`:
- Around line 86-93: The template engine sets "delegate = _b.get('fn')" which
throws NPE when make(null) passes a null binding; in StreamingTemplateEngine
(the make()/make(null) path) guard the binding lookup by checking _b for null
(and that _b.get('fn') is non-null) before assigning delegate, or default to a
safe no-op delegate when _b is null so Template.make() and make(null) do not
NPE; update the code around the "delegate = _b.get('fn')" expression to use a
null-check on _b and only call get('fn') when _b != null.
In `@distribution/tutorials/transformation/60-SOAP-Array-to-JSON.yaml`:
- Around line 56-57: Remove the explicit double quotes around the name
expression in the JSON template so the <%= xpath('string(./f:name)', fruit) %>
output isn't double-quoted; replace the quoted pattern for the "name" field with
the unquoted expression (same style as the numeric "price" field) so
StreamingTemplateEngine's escape()/toJSON() returns a correctly-serialized JSON
string without extra quotes.
🧹 Nitpick comments (2)
core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (1)
247-257: Test correctly validates the new unquoted JSON template syntax.The switch from
"${fn.user()}"to${user()}relies on JSON-aware escaping to produce properly quoted string values. The assertion (contains("alice")) confirms the value is rendered. Consider also asserting that the output is valid JSON to ensure the escaping produces correct structure.Optional: assert valid JSON output
assertTrue(exc.getRequest().getBodyAsStringDecoded().contains("alice")); + // Verify the output is valid JSON + assertDoesNotThrow(() -> om.readTree(exc.getRequest().getBodyAsStringDecoded()));core/src/test/java/com/predic8/membrane/core/interceptor/templating/GroovyTemplateEscapingTest.java (1)
39-47: Consider adding a test for values containing special JSON characters.To fully validate the escaping fix (per the PR title "fix JSON escaping"), it would be valuable to test values that contain characters requiring JSON escaping (e.g., quotes, backslashes, newlines). This would confirm the fix handles the edge cases that likely motivated this PR.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/lang/groovy/adapted/StreamingTemplateEngine.java
Outdated
Show resolved
Hide resolved
distribution/tutorials/transformation/60-SOAP-Array-to-JSON.yaml
Outdated
Show resolved
Hide resolved
rrayst
force-pushed
the
fix-scopes
branch
2 times, most recently
from
86ea70f to
b404087
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
distribution/examples/security/jwt/apikey-to-jwt-conversion/README.md (1)
22-24:⚠️ Potential issue | 🟡 MinorProse still references the removed
setPropertystep.Lines 22–24 mention "store the API key's scopes in a property named
scopes" and the property being "used in a template." SincesetPropertywas removed andscopes()is now called directly in the template, this text is stale and should be updated to reflect the new approach.
🤖 Fix all issues with AI agents
In `@distribution/examples/security/jwt/apikey-to-jwt-conversion/README.md`:
- Line 36: The README snippet's <template> tag is missing the contentType
attribute; update the example so the <template> element includes
contentType="application/json" to match the actual proxies.xml (ensure the
README's <template> tag mirrors the one at line 16 in the real proxies.xml).
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/lang/groovy/adapted/StreamingTemplateEngine.java (1)
479-482:AccessController.doPrivilegedis deprecated for removal.The
@SuppressWarnings("removal")suppression and TODO at line 479 acknowledge this. Java 17+ deprecatedAccessControllerand it's slated for removal. This is inherited from the original Groovy code, so no action needed now, but worth tracking.
distribution/examples/security/jwt/apikey-to-jwt-conversion/README.md
Outdated
Show resolved
Hide resolved
rrayst
force-pushed
the
fix-scopes
branch
4 times, most recently
from
21efaab to
ff5bc45
Compare
core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java
Outdated
Show resolved
Hide resolved
...c/test/java/com/predic8/membrane/core/interceptor/templating/GroovyTemplateEscapingTest.java
Outdated
Show resolved
Hide resolved
.../src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java
Show resolved
Hide resolved
rrayst
force-pushed
the
fix-scopes
branch
2 times, most recently
from
1a11aa8 to
9a55318
Compare
…ome modifications
…ON content-type; removed 'fn.' prefix from Groovy function calls
2 participants
Summary by CodeRabbit
Documentation
Templating
Tests