Add SpEL support for yaml configuration#2640
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds restricted SpEL evaluation for YAML scalars (env accessor, parser context, evaluation context and engine), integrates SpEL-aware scalar conversion into YAML coercion paths, and updates JSON schema generation to emit multi-type Changes
Sequence Diagram(s)sequenceDiagram
participant YAML as "YAML Source"
participant Generic as "GenericYamlParser"
participant SpELEngine as "SpELEngine"
participant SPEL_CTX as "SPEL_CTX"
participant Method as "MethodSetter"
participant Schema as "JsonSchemaGenerator"
YAML->>Generic: supply scalar node (e.g. "#{env.PORT}" or "pre-#{env.PORT}-suf")
Generic->>Generic: convertScalarOrSpel(node, targetType)
alt node contains SpEL template
Generic->>SpELEngine: eval(template, SPEL_CTX)
SpELEngine->>SPEL_CTX: request env('PORT')
SPEL_CTX-->>SpELEngine: resolvedValue
SpELEngine-->>Generic: resolvedValue
else no template
Generic->>Generic: SCALAR_MAPPER.convertValue(node, targetType)
end
Generic-->>Method: pass resolved value for coercion
Method->>Method: coerceTextual / coerceNonTextual (numbers, booleans, refs, maps)
Method-->>Generic: coercedValue
Generic->>Schema: createProperty(...).typeAllowingString(type)
Schema-->>Schema: emit `type` or `type` array (e.g., ["integer","string"])
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
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AbstractSchema.java`:
- Around line 110-116: The method typeAllowingString currently sets typeList
when STRING_ALLOWED_TYPES contains the input and sets type otherwise but does
not clear the alternate field, which can leave stale values and make json() emit
incorrect types; update typeAllowingString in AbstractSchema so that when
assigning this.typeList = List.of(type, "string") you also clear this.type (set
to null/empty), and when assigning this.type = type you clear this.typeList (set
to null/empty), ensuring only one of type or typeList is populated before json()
is called.
|
This pull request needs "/ok-to-test" from an authorized committer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/YamlParsingUtils.java`:
- Around line 123-139: resolveSpelValue currently returns null when a SpEL
expression yields null, which later causes unclear errors if the targetType is a
primitive; change the null-handling so that after evaluating value (in
resolveSpelValue) you check if value == null and targetType.isPrimitive(), and
if so throw a new ParsingException with a clear message like "SpEL expression
evaluated to null for primitive target type: <type>" including the existing
node; otherwise keep returning null for non-primitive targets. Use the existing
ParsingException and node variable so callers get a helpful error.
- Around line 53-59: The current newSpelContext() uses StandardEvaluationContext
which still allows reflective method/property access even with a blocked type
locator; change it to use SimpleEvaluationContext (or, if keeping
StandardEvaluationContext, install a locked-down MethodResolver and
PropertyAccessor) so only the intended env(...) functions on SpelRoot are
available; update newSpelContext() to create a SimpleEvaluationContext rooted
with new SpelRoot and explicitly register only the allowed functions/properties,
or add a custom MethodResolver/PropertyAccessor that rejects all calls except
the env handler to strictly limit evaluation surface.
In `@annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java`:
- Around line 597-623: Test spelEnvInYamlScalar is flaky because it relies on a
fixed environment variable MEMBRANE_SPEL_TEST; change the test to generate a
unique env key (e.g. MEMBRANE_SPEL_TEST_<randomSuffix>), programmatically set
that env var to a numeric value before calling parseYAML and use the same
generated key inside the YAML string, then clean up the env var after the
assertion; update spelEnvInYamlScalar to build the YAML with the generated key
and ensure parseYAML is invoked with that deterministic numeric value.
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
122-131: Replaceassertwith explicit runtime validation.
Line 130 usesassert, which is disabled in production; ifnodeis ever null, this will later surface as a less actionable NPE. Consider throwing aParsingException(orObjects.requireNonNull) with a clear message instead.
…SpEL context handling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java (1)
123-147: SpEL short‑circuit breaks non‑String setters and references.
Returning early for any textual node bypasses enum/reference/primitive coercion and feeds aStringinto non‑String setters, which will fail for references or quoted numeric/boolean values.🐛 Proposed fix: only evaluate SpEL then keep existing coercion
@@ -import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.TextNode; @@ - if (node != null && node.isTextual()) { - return resolveSpelValue(node.asText(), String.class, node); - } + if (node != null && node.isTextual()) { + String raw = node.asText(); + if (raw.contains("#{")) { + Object resolved = resolveSpelValue(raw, String.class, node); + if (resolved == null) return null; + node = TextNode.valueOf(String.valueOf(resolved)); + } + }
♻️ Duplicate comments (2)
annot/src/main/java/com/predic8/membrane/annot/yaml/YamlParsingUtils.java (1)
118-131: Fail fast when SpEL resolves to null for primitives.
Returning null here still yields a confusing reflective setter error for primitive targets.🐛 Proposed guard for null → primitive
- if (value == null) return null; + if (value == null) { + if (targetType.isPrimitive()) { + throw new ParsingException( + "SpEL expression evaluated to null for primitive %s; provide a default value." + .formatted(targetType.getSimpleName()), + node); + } + return null; + }annot/src/test/java/com/predic8/membrane/annot/YAMLParsingTest.java (1)
597-651: Avoid env‑dependent test flakiness.
These tests depend on a fixed env key; if CI sets it, defaults won’t be used.🔧 Suggested adjustment (randomized env key)
@@ -import java.util.List; +import java.util.List; +import java.util.UUID; @@ - BeanRegistry br = parseYAML(result, """ + String envKey = "MEMBRANE_SPEL_TEST_" + UUID.randomUUID().toString().replace("-", ""); + BeanRegistry br = parseYAML(result, """ root: - timeout: "#{env('MEMBRANE_SPEL_TEST','42')}" - """); + timeout: "#{env('%s','42')}" + """.formatted(envKey)); @@ - BeanRegistry br = parseYAML(result, """ + String envKey = "MEMBRANE_SPEL_TEST_" + UUID.randomUUID().toString().replace("-", ""); + BeanRegistry br = parseYAML(result, """ root: - msg: "prefix-#{env('MEMBRANE_SPEL_TEST','X')}-suffix" - """); + msg: "prefix-#{env('%s','X')}-suffix" + """.formatted(envKey));
…textFactory`, and `SpELEngine`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java`:
- Around line 204-206: The conditional block in MethodSetter that checks "if
(node.isTextual() && isBeanReference(wanted)) { return resolveReference(ctx,
node, key, wanted); }" is unreachable because MethodSetter only calls this
method when !node.isTextual(); remove this dead code to avoid confusion and
duplicated logic; ensure no other callers depend on this branch and keep textual
bean-reference handling in coerceTextual (lines around coerceTextual and
resolveReference) unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@annot/src/main/java/com/predic8/membrane/annot/yaml/MethodSetter.java`:
- Around line 167-168: The bean-reference path in MethodSetter is re-reading the
raw node text so SpEL-evaluated values are ignored; update the logic in
MethodSetter (around coerceTextual and the branch that calls
isBeanReference/resolveReference) to pass the evaluated string (the result
produced by coerceTextual, e.g. variable v) into resolveReference instead of
calling node.asText() again; do the same fix for the other occurrence around
lines where resolveReference is called (the 227-233 block) so both
bean-reference branches use the evaluated value rather than the raw "#{...}"
text.
…and add specific handling for String targets.
predic8
previously approved these changes
Jan 20, 2026
…for cleaner YAML parsing logic.
…dling in `MethodSetter`.
…lt values in SpEL templates.
Collaborator
Author
|
/ok-to-test |
…ble access and update tutorial accordingly.
predic8
previously approved these changes
Jan 21, 2026
predic8
approved these changes
Jan 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
New Utilities
Behavior
Refactor
Tests & Tutorials
✏️ Tip: You can customize this high-level summary in your review settings.