Query Params as Lists in SpEL, Groovy#2789
Conversation
…rategy, update logic to support lists, and adjust affectd components and tests
…tegy` import, remove unused `ParamBuilder`, and clean up `TemplateInterceptorTest`.
📝 WalkthroughWalkthroughThis pull request extends Membrane's query parameter handling from single-value assumptions to multi-valued support across the codebase. URLParamUtil is refactored to return Changes
Sequence DiagramsequenceDiagram
participant Client
participant Request Handler
participant URLParamUtil
participant Expression Evaluator
participant Template Engine
participant Response
Client->>Request Handler: HTTP request (query: a=1&a=2)
Request Handler->>URLParamUtil: parseQueryString(queryStr)
URLParamUtil->>URLParamUtil: Extract key-value pairs
URLParamUtil-->>Request Handler: Map<String, List<String>><br/>{a: ["1", "2"]}
Request Handler->>Expression Evaluator: evaluate("params.a?.[0]")<br/>with params map
Expression Evaluator->>Expression Evaluator: Extract first element<br/>from list via indexing
Expression Evaluator-->>Request Handler: "1" (String result)
Request Handler->>Template Engine: render(template,<br/>params with lists)
Template Engine->>Template Engine: Access params.a[0]<br/>via template syntax
Template Engine-->>Request Handler: Rendered output<br/>with first values
Request Handler-->>Response: HTTP response
Response-->>Client: Response with data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
distribution/examples/templating/text/apis.yaml (1)
42-44:⚠️ Potential issue | 🟡 MinorParam values are now lists — iteration output will change.
Since params are now
Map<String, List<String>>,p.valuein this template loop will render as[value](list toString) rather than the bare value. If this example is meant to demonstrate readable param output, you may want to handle the list (e.g., join or iterate the values).core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java (1)
67-74:⚠️ Potential issue | 🟠 MajorNew
getParams(URIFactory, Exchange)is missing form-encoded body parameter fallback — breaking change for Groovy scripts and templates.The multi-value
getParams()method at URLParamUtil lines 37-44 only extracts parameters from the URI query string (line 43). It lacks the form body fallback present in the oldgetParams(URIFactory, Exchange, Strategy)method (lines 54-58), which checks forapplication/x-www-form-urlencodedbody when the query string is absent.Groovy scripts or templates that access form-encoded POST parameters via
paramsorparambindings will now receive an empty map instead of the form data. ScriptingUtils line 69 uses this new variant.
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java`:
- Around line 82-85: The code in SpELExchangeExpression handling List-to-String
conversion must guard against empty lists and null first elements; in the block
that checks "if (o instanceof List<?> l)" add checks like "if (l.isEmpty())
return null;" and "var first = l.getFirst(); if (first == null) return null;"
then convert with "type.cast(first.toString())" (or use Objects.toString(first,
null)) so you don't throw NoSuchElementException or NPE when a List is empty or
its first element is null.
In `@core/src/main/java/com/predic8/membrane/core/util/URLParamUtil.java`:
- Around line 138-153: parseQueryString currently treats an empty string query
"" as a segment and, because paramsPat matches it, adds a {"": [""]} entry; fix
this by short-circuiting empty queries (similar to the existing null check) or
by skipping empty segments before applying paramsPat: update parseQueryString to
return the empty params map when query.isEmpty() or to continue the loop when
p.isEmpty() so paramsPat.matcher(p) is not invoked and no empty key/value is
inserted.
- Around line 37-44: The multi-value getParams(URIFactory, Exchange) in
URLParamUtil currently returns parseQueryString(getUri(...).getRawQuery()) and
omits the form-encoded body fallback present in the single-value overload;
update this multi-value overload to mirror the single-value logic by checking if
getRawQuery() is null/empty and, if so and the request Content-Type is
"application/x-www-form-urlencoded", read the request body (via the Exchange
request body accessor used by the single-value method), call parseQueryString on
that body and return the resulting Map<String,List<String>>; ensure you use the
same helpers (getUri, parseQueryString) and preserve existing exception
handling/return of emptyMap() when appropriate.
In `@distribution/examples/scripting/javascript/apis-preview.yaml`:
- Line 22: The example currently calls params.get('id').get(0) and
params.get('city').get(0) with no null guard; update the example to either add a
null check/optional chaining around params.get('id') and params.get('city')
before calling .get(0) (e.g., guard or use ?.get(0)) or add a short comment
above the expression stating that id and city are required query parameters so
readers know the happy-path assumption; target the expression using the symbols
params.get('id'), params.get('city') and the .get(0) calls to locate and fix the
code.
In `@distribution/examples/templating/text/apis.yaml`:
- Line 11: The template uses a non-null-safe access params.name.get(0) which
will throw if the name query param is missing; replace it with the null-safe
pattern used elsewhere (e.g., params?.name?.[0] or params.name?[0]) so the
template reads something like "src: Hello ${params?.name?.[0]}!" to avoid an NPE
when the param is absent.
In `@README.md`:
- Line 350: Update the SpEL optional chaining used in the JSON snippet by
replacing the incorrect `${params.answer?[0]}` token with the correct
`${params.answer?.[0]}` form so safe index access uses the dot-before-bracket
syntax; locate the occurrence of `${params.answer?[0]}` in the README and change
it to `${params.answer?.[0]}` to match the working example `params.city?.[0]`.
🧹 Nitpick comments (4)
core/src/test/java/com/predic8/membrane/core/lang/groovy/GroovyExchangeExpressionTest.java (2)
104-115: Consider usingassertInstanceOffor clearer failure messages.The
if/else fail()pattern loses type information in failure output. Also note the existing tests (line 72) use the inverse check with early return — this test uses a different pattern for the same purpose.Minor: a more idiomatic JUnit 5 approach:
♻️ Suggested refactor
- var o = expression(new InterceptorAdapter(router), getLanguage(), "params.a").evaluate(exc, REQUEST, Object.class); - if (o instanceof List l) { - assertEquals(2, l.size()); - assertEquals("1", l.get(0)); - assertEquals("2", l.get(1)); - } else fail(); + var o = expression(new InterceptorAdapter(router), getLanguage(), "params.a").evaluate(exc, REQUEST, Object.class); + assertInstanceOf(List.class, o); + List l = (List) o; + assertEquals(2, l.size()); + assertEquals("1", l.get(0)); + assertEquals("2", l.get(1));
117-119: Minor inconsistency: hardcodedGROOVYvsgetLanguage().Line 107 uses
getLanguage()while this helper hardcodesGROOVY. They're equivalent here, but usinggetLanguage()would be consistent. Also, extra whitespace before)on line 117.♻️ Suggested fix
- private String evaluateQueryParam(Exchange exc, String expr ) { - return expression(new InterceptorAdapter(router), GROOVY, expr).evaluate(exc, REQUEST, String.class); + private String evaluateQueryParam(Exchange exc, String expr) { + return expression(new InterceptorAdapter(router), getLanguage(), expr).evaluate(exc, REQUEST, String.class); }distribution/examples/templating/json/apis.yaml (1)
13-13: Potential null risk ifanswerparam is missing.Other examples in this PR use double optional chaining (e.g.,
params?.answer?.[0]), but hereparams.answer?[0]only null-guards the index access. If theanswerkey is absent andparams.answerreturnsnull, this should still be safe due to?[0]. However, the inconsistency with other examples (which also guard the property access) is worth noting — consider aligning toparams.answer?.[0]orparams?.answer?.[0]for consistency.README.md (1)
679-679: Clarify syntax inconsistency and edge case behavior.This line uses
params.answer?[0](without a dot), while line 350 usesparams.city?.[0](with a dot). If both are valid syntax for their respective contexts, this should be documented to avoid user confusion. If one is incorrect, it should be corrected.Additionally, consider documenting the behavior when:
- The
answerparameter is not provided in the query string- The parameter is provided but has an empty value (e.g.,
?answer=)- Multiple values are provided (e.g.,
?answer=42&answer=43)📝 Consider adding a comment or note in the example
+ # Query parameters are arrays - use ?[0] to safely access the first value api: port: 2000 flow: - request: - template: contentType: application/json pretty: true src: | { "answer": ${params.answer?[0]} }
…move unused tests and imports, add null checks, and improve YAML and JSON parsing.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Documentation
params.city?.[0])Tests