YAML Tutorial#2285
Conversation
WalkthroughAdds explicit TemplateExecutionException propagation and handling in templating, extends scripting bindings with request method/path, refactors Kubernetes schema models to use generic AbstractSchema with enum support, enhances YAML enum validation, adjusts logging/error paths, and adds many tutorial configs and startup scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TemplateInterceptor
participant AbstractTemplateInterceptor
participant TemplatingEngine
Client->>TemplateInterceptor: request -> handleInternal(exchange, flow)
TemplateInterceptor->>AbstractTemplateInterceptor: delegate handling
AbstractTemplateInterceptor->>TemplatingEngine: process(exchange, flow)
alt TemplateExecutionException thrown
TemplatingEngine-->>TemplateInterceptor: TemplateExecutionException (with root cause)
TemplateInterceptor->>TemplateInterceptor: inspect root cause
alt MissingPropertyException
TemplateInterceptor-->>Client: ABORT + ProblemDetails (property)
else MissingMethodException
TemplateInterceptor-->>Client: ABORT + ProblemDetails (method)
else other
TemplateInterceptor-->>Client: ABORT + ProblemDetails (exception attached)
end
else other Exception
TemplatingEngine-->>TemplateInterceptor: Exception
TemplateInterceptor-->>Client: ABORT (generic)
end
sequenceDiagram
participant Script
participant ScriptingUtils
participant BindingContext
Script->>ScriptingUtils: setupBindings(exchange, flow)
ScriptingUtils->>BindingContext: put "exc"/"exchange", "flow"
alt exchange.request != null
ScriptingUtils->>BindingContext: put "method" (request.method)
ScriptingUtils->>BindingContext: put "path" (request.uri)
end
BindingContext-->>Script: bindings ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
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: 5
🧹 Nitpick comments (2)
distribution/tutorials/getting-started/data/person.json (1)
2-3: Use camelCase for property names to follow JSON conventions.The property names "firstname" and "lastname" use all lowercase, which deviates from standard JSON naming conventions. Consider using "firstName" and "lastName" instead for consistency with typical JavaScript/JSON practices.
Apply this diff to fix the property names:
{ - "firstname": "Olivia", - "lastname": "Garcia", + "firstName": "Olivia", + "lastName": "Garcia", "address":{ "street": "Main Street 217", "city": "Boston" } }distribution/tutorials/getting-started/40-Template.yaml (1)
1-21: Potential duplication with 50-Jsonpath.yaml.This file and
50-Jsonpath.yamlappear to demonstrate nearly identical functionality—both render plain text templates with method, path, and date on port 2000. Consider either:
- Making them demonstrate different aspects of templating (e.g., one for plain text, another for different template features)
- Removing the duplicate if both serve the same purpose
- If 50-Jsonpath.yaml is meant to demonstrate JSONPath, update its content accordingly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
-
core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java(2 hunks) -
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java(2 hunks) -
core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java(1 hunks) -
distribution/tutorials/getting-started/00-First-API.yaml(1 hunks) -
distribution/tutorials/getting-started/05-Logging.yaml(1 hunks) -
distribution/tutorials/getting-started/07-Message-Flow.yaml(1 hunks) -
distribution/tutorials/getting-started/10-Basic-Path-Routing.yaml(1 hunks) -
distribution/tutorials/getting-started/20-Short-Circuit.yaml(1 hunks) -
distribution/tutorials/getting-started/30-SetHeader.yaml(1 hunks) -
distribution/tutorials/getting-started/40-Template.yaml(1 hunks) -
distribution/tutorials/getting-started/50-Jsonpath.yaml(1 hunks) -
distribution/tutorials/getting-started/60-Json-Template.yaml(1 hunks) -
distribution/tutorials/getting-started/README.md(1 hunks) -
distribution/tutorials/getting-started/data/order.json(1 hunks) -
distribution/tutorials/getting-started/data/person.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java (1)
ExceptionUtil(16-39)
🪛 LanguageTool
distribution/tutorials/getting-started/README.md
[locale-violation] ~7-~7: In American English, “take a look” is more commonly used.
Context: ... # Basic # To get a basic understanding have a look at the following tutorials: # # Advance...
(HAVE_A_LOOK)
[grammar] ~14-~14: Ensure spelling is correct
Context: ... tutorials: # # Advanced: # # - Header - Espression Language SPEL - Properties - Templating...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~20-~20: Ensure spelling is correct
Context: ...- JSONPath - Advanced Routing - Routing Rewritting - Routing Redirect - CORS XML Tutorial S...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
distribution/tutorials/getting-started/README.md
3-3: Multiple headings with the same content
(MD024, no-duplicate-heading)
4-4: Multiple headings with the same content
(MD024, no-duplicate-heading)
5-5: Multiple headings with the same content
(MD024, no-duplicate-heading)
8-8: Multiple headings with the same content
(MD024, no-duplicate-heading)
10-10: Multiple headings with the same content
(MD024, no-duplicate-heading)
11-11: Multiple headings with the same content
(MD024, no-duplicate-heading)
34-34: Bare URL used
(MD034, no-bare-urls)
36-36: Bare URL used
(MD034, no-bare-urls)
38-38: Bare URL used
(MD034, no-bare-urls)
40-40: Bare URL used
(MD034, no-bare-urls)
42-42: Bare URL used
(MD034, no-bare-urls)
44-44: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (5)
distribution/tutorials/getting-started/data/order.json (1)
1-20: LGTM!The JSON structure is well-formed with clear data for tutorial purposes.
core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java (1)
56-57: LGTM!The new "method" and "path" bindings provide useful context for template-based responses and follow the existing binding pattern.
distribution/tutorials/getting-started/30-SetHeader.yaml (1)
1-20: LGTM!The YAML configuration correctly demonstrates header manipulation with both static and dynamic values using the new ${method} binding.
distribution/tutorials/getting-started/20-Short-Circuit.yaml (1)
1-21: LGTM!The configuration clearly demonstrates the short-circuit pattern with appropriate status code (503) and maintenance message.
distribution/tutorials/getting-started/07-Message-Flow.yaml (1)
1-61: Well-structured tutorial demonstrating message flow and script bindings.This tutorial effectively demonstrates:
- Multi-document YAML configuration with request and response flow separation
- Template variable bindings (${path}, ${statusCode}) that integrate with the scripting context enhancements
- Clear documentation and usage instructions for users
The configuration is properly formatted and provides an excellent complement to the other tutorials.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
distribution/tutorials/getting-started/README.md (2)
34-44: Wrap bare URLs in Markdown link syntax.Bare URLs should be formatted as Markdown links for better presentation and accessibility:
-https://api.ipify.org/?format=json +[https://api.ipify.org/?format=json](https://api.ipify.org/?format=json) -https://dummyjson.com/quotes +[https://dummyjson.com/quotes](https://dummyjson.com/quotes) -https://api.agify.io/?name=thomas +[https://api.agify.io/?name=thomas](https://api.agify.io/?name=thomas) -https://httpbin.org/status/500 +[https://httpbin.org/status/500](https://httpbin.org/status/500) -https://httpbin.org/delay/5 +[https://httpbin.org/delay/5](https://httpbin.org/delay/5) -https://httpbin.org/xml +[https://httpbin.org/xml](https://httpbin.org/xml)
7-7: Minor phrasing improvement for American English.Change "have a look" to "take a look" for more natural American English phrasing:
-# To get a basic understanding have a look at the following tutorials: +# To get a basic understanding take a look at the following tutorials:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
distribution/tutorials/getting-started/00-First-API.yaml(1 hunks)distribution/tutorials/getting-started/60-Json-Template.yaml(1 hunks)distribution/tutorials/getting-started/README.md(1 hunks)distribution/tutorials/getting-started/data/person.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- distribution/tutorials/getting-started/00-First-API.yaml
🧰 Additional context used
🪛 LanguageTool
distribution/tutorials/getting-started/README.md
[locale-violation] ~7-~7: In American English, “take a look” is more commonly used.
Context: ... # Basic # To get a basic understanding have a look at the following tutorials: # # Advance...
(HAVE_A_LOOK)
🪛 markdownlint-cli2 (0.18.1)
distribution/tutorials/getting-started/README.md
3-3: Multiple headings with the same content
(MD024, no-duplicate-heading)
4-4: Multiple headings with the same content
(MD024, no-duplicate-heading)
5-5: Multiple headings with the same content
(MD024, no-duplicate-heading)
8-8: Multiple headings with the same content
(MD024, no-duplicate-heading)
10-10: Multiple headings with the same content
(MD024, no-duplicate-heading)
11-11: Multiple headings with the same content
(MD024, no-duplicate-heading)
34-34: Bare URL used
(MD034, no-bare-urls)
36-36: Bare URL used
(MD034, no-bare-urls)
38-38: Bare URL used
(MD034, no-bare-urls)
40-40: Bare URL used
(MD034, no-bare-urls)
42-42: Bare URL used
(MD034, no-bare-urls)
44-44: Bare URL used
(MD034, no-bare-urls)
⏰ 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)
distribution/tutorials/getting-started/data/person.json (1)
1-8: LGTM!Valid JSON data file with appropriate example data for the templating tutorials.
There was a problem hiding this comment.
Actionable comments posted: 8
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/model/AttributeInfo.java (1)
89-108: MissingFLOAThandling regresses existing setters.The new switch omits
TypeKind.FLOAT, so any setter with afloatparameter now falls into the default branch and triggers the “Not implemented”ProcessingException. We already map"xsd:float"ingetSchemaType, so please restore explicit handling forFLOAT(mirroring the previous behavior) to avoid breaking annotation processing on float attributes.Suggested fix:
- switch (ve.asType().getKind()) { - case INT, LONG: + switch (ve.asType().getKind()) { + case INT, LONG: xsdType = "spel_number"; return; case DOUBLE: xsdType = "xsd:double"; return; + case FLOAT: + xsdType = "xsd:float"; + return;
🧹 Nitpick comments (4)
distribution/tutorials/getting-started/README.md (2)
24-26: Convert unformatted section titles to Markdown headings.Lines 24 and 26 ("XML Tutorial" and "SOAP Tutorial") should use proper Markdown heading syntax for consistency with the rest of the document.
-XML Tutorial +## XML Tutorial-SOAP Tutorial +## SOAP Tutorial
34-44: Wrap bare URLs in Markdown link syntax.Lines 34, 36, 38, 40, 42, and 44 contain bare URLs. Per Markdown best practices (MD034), these should be wrapped in Markdown link syntax for proper formatting and accessibility.
-https://api.ipify.org/?format=json +- [https://api.ipify.org/?format=json](https://api.ipify.org/?format=json)-https://dummyjson.com/quotes +- [https://dummyjson.com/quotes](https://dummyjson.com/quotes)-https://api.agify.io/?name=thomas +- [https://api.agify.io/?name=thomas](https://api.agify.io/?name=thomas)-https://httpbin.org/status/500 +- [https://httpbin.org/status/500](https://httpbin.org/status/500)-https://httpbin.org/delay/5 +- [https://httpbin.org/delay/5](https://httpbin.org/delay/5)-https://httpbin.org/xml +- [https://httpbin.org/xml](https://httpbin.org/xml)core/src/main/java/com/predic8/membrane/core/interceptor/rewrite/RewriteInterceptor.java (2)
71-82: Consider documenting the "redirect" alias.The method accepts "redirect" as an alias for "redirect-temporary" (line 78), but this is not documented in the JavaDoc for
setDo()at lines 126-130. If this alias is intentional for backward compatibility or convenience, please update the documentation to reflect this accepted value.Apply this diff to update the documentation:
/** * @description What to do: "rewrite", "redirect-temporary" or "redirect-permanent". + * Note: "redirect" is accepted as an alias for "redirect-temporary". * @default rewrite (default) or redirect (if "to" contains "://") * @example redirect-temporary */
80-80: Consider using original parameter value in error message.The error message uses
what(the lowercased version) instead of the originaldo_parameter. Using the original value would provide clearer feedback to users about what they actually provided.Apply this diff:
- default -> throw new IllegalArgumentException("Unknown value '%s' for rewriter/@do.".formatted(what)); + default -> throw new IllegalArgumentException("Unknown value '%s' for rewriter/@do.".formatted(do_));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java(8 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sJsonSchemaGenerator.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AbstractSchema.java(3 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/BasicSchema.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaArray.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaString.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/model/AttributeInfo.java(6 hunks)annot/src/main/java/com/predic8/membrane/annot/model/ElementInfo.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/rewrite/RewriteInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/GenericYamlParser.java(6 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/WrongEnumConstantException.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java(1 hunks)distribution/tutorials/advanced/30-PathParameters.yaml(1 hunks)distribution/tutorials/advanced/40-Path-Parameter-Routing.yaml(1 hunks)distribution/tutorials/advanced/50-Path-Rewriting.yaml(1 hunks)distribution/tutorials/advanced/60-Redirects.yaml(1 hunks)distribution/tutorials/advanced/README.md(1 hunks)distribution/tutorials/getting-started/07-Message-Flow.yaml(1 hunks)distribution/tutorials/getting-started/08-Message-Flow2.yaml(1 hunks)distribution/tutorials/getting-started/60-Json-Template.yaml(1 hunks)distribution/tutorials/getting-started/80-OpenAPI.yaml(1 hunks)distribution/tutorials/getting-started/README.md(1 hunks)docs/ROADMAP.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- distribution/tutorials/advanced/README.md
- distribution/tutorials/advanced/60-Redirects.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- distribution/tutorials/getting-started/07-Message-Flow.yaml
- distribution/tutorials/getting-started/60-Json-Template.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-19T11:12:26.787Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.787Z
Learning: Request.get().path() in com.predic8.membrane.core.openapi.model.Request preserves the query string portion of the path, so URIFactory.createWithoutException(request.getPath()).getQuery() correctly extracts query parameters.
Applied to files:
core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java
📚 Learning: 2025-10-23T13:04:11.388Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2240
File: annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java:210-215
Timestamp: 2025-10-23T13:04:11.388Z
Learning: In JsonSchemaGenerator.java, when allowForeign is true for non-list child elements, use a property named "ref" (not "$ref") with type "string" to provide IDE completion hints for Spring bean references while avoiding JSON Schema keyword collision.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.javaannot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sJsonSchemaGenerator.javaannot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java
🧬 Code graph analysis (3)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java (1)
SchemaFactory(5-60)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sJsonSchemaGenerator.java (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java (1)
SchemaFactory(5-60)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java (1)
SchemaFactory(5-60)
🪛 markdownlint-cli2 (0.18.1)
distribution/tutorials/getting-started/README.md
3-3: Multiple headings with the same content
(MD024, no-duplicate-heading)
4-4: Multiple headings with the same content
(MD024, no-duplicate-heading)
5-5: Multiple headings with the same content
(MD024, no-duplicate-heading)
8-8: Multiple headings with the same content
(MD024, no-duplicate-heading)
10-10: Multiple headings with the same content
(MD024, no-duplicate-heading)
11-11: Multiple headings with the same content
(MD024, no-duplicate-heading)
34-34: Bare URL used
(MD034, no-bare-urls)
36-36: Bare URL used
(MD034, no-bare-urls)
38-38: Bare URL used
(MD034, no-bare-urls)
40-40: Bare URL used
(MD034, no-bare-urls)
42-42: Bare URL used
(MD034, no-bare-urls)
44-44: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (18)
distribution/tutorials/getting-started/08-Message-Flow2.yaml (3)
1-1: Verify schema path for IDE validation outside build process.The schema reference path includes
target/classes/, which is a generated directory typically not committed to the repository. This may prevent YAML language servers from validating the file correctly outside the build environment.Can you verify that this schema path works for IDE validation and, if not, confirm whether there's an alternative schema reference (like a distribution artifact or published schema) that should be used for tutorial files?
35-40: Clarify response flow execution order in comments.The comment states the response flow is "executed...from bottom to top", but the YAML entries are listed in order (6, 5, 4). Confirm whether the actual execution reverses the YAML order or if the comment should clarify that the response phase itself occurs bottom-up in the gateway's request/response lifecycle (while YAML order remains top-to-bottom).
28-43: YAML structure and syntax look good.The tutorial is well-structured with clear comments, proper variable interpolation (
${path},${statusCode}), and good pedagogical flow comparing two message-flow implementations.distribution/tutorials/getting-started/80-OpenAPI.yaml (1)
16-16: Address the TODO comment.The TODO marker on the
locationfield suggests the configuration may not be finalized. Verify that the path../../distribution/conf/fruitshop-api.ymlis correct and then remove the TODO comment.distribution/tutorials/getting-started/README.md (1)
1-12: Fix malformed Markdown heading structure.Lines 3–5 and 8, 10–11 contain empty or duplicate heading markers, violating Markdown best practices. The heading hierarchy should be consolidated with distinct content for each heading, as flagged in previous reviews.
Consider restructuring as follows:
-# -# Membrane API Gateway Tutorials -# -# -# -# Basic -# To get a basic understanding take a look at the following tutorials: -# -# Advanced: -# -# +# Membrane API Gateway Tutorials + +## Basic + +To get a basic understanding take a look at the following tutorials: + +## Advancedcore/src/main/java/com/predic8/membrane/core/interceptor/rewrite/RewriteInterceptor.java (2)
34-34: LGTM: Proper locale-aware import.The static import of
Locale.USis appropriate for the case-insensitive string comparison inparseDo, ensuring consistent behavior across different system locales.
67-67: LGTM: Clean refactoring.Delegating to
parseDo(do_)improves code organization by extracting the parsing logic into a dedicated helper method.docs/ROADMAP.md (1)
33-35: LGTM! Clear documentation of breaking change.The documentation clearly describes the breaking change in ScriptingUtils variable bindings for Groovy scripting. The note about the difference from SpEL is helpful for users migrating or comparing the two approaches.
distribution/tutorials/advanced/30-PathParameters.yaml (1)
1-18: LGTM! Well-structured path parameters tutorial.The tutorial clearly demonstrates path parameter extraction and usage. The example includes:
- Clear path template with named parameters
- Template interpolation showing how to access the parameters
- Helpful curl command for testing
distribution/tutorials/advanced/40-Path-Parameter-Routing.yaml (1)
8-16: Good demonstration of dynamic routing with path parameters.The tutorial effectively shows how to use path parameters in both logging and target URL construction, demonstrating a common routing pattern.
distribution/tutorials/advanced/50-Path-Rewriting.yaml (1)
8-21: Nice demonstration of path rewriting with before/after logging.The tutorial effectively shows the path transformation by logging both the original and rewritten paths, making it easy to understand the rewriting behavior.
core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java (1)
57-61: Good addition of method and path bindings with appropriate null safety.The null check is sensible given test scenarios. The implementation enables the
${method}and${path}template variables used in the tutorials.Note: The "path" binding uses
getUri()which, based on the codebase patterns, includes the query string portion. This is likely intentional for template simplicity, but consider documenting this behavior if users might expect only the path component without query parameters.annot/src/main/java/com/predic8/membrane/annot/model/ElementInfo.java (1)
94-96: ****The original review conflates unrelated concerns and mischaracterizes the code.
The method is correctly named
getXSDTypeNamebecause it genuinely serves both purposes: generating XSD type names (used inSchemas.javaforxsd:complexTypeandxsd:elementdeclarations) and JSON Schema definition names (used inJsonSchemaGenerator.javaandK8sJsonSchemaGenerator.javafor#/$defs/references). The comment explaining the dot-to-underscore transformation is necessary context, not evidence of inconsistency.The naming collision concern is theoretical. The change replaces dots with underscores in fully-qualified class names. For collisions to occur, the codebase would need two different class names differing only in dot placement—e.g.,
com.predic8.interceptor_ws.addressing.Parserversuscom.predic8.interceptor.ws_addressing.Parser. The codebase standardizes on package names likews_addressing(underscores already present) with no evidence of such pathological overlaps. Given Java naming conventions, this is not a practical risk.The change is a legitimate fix: it makes generated schemas compatible with JSON Schema parsers that reject dots in identifiers, which is the stated justification in the code comment.
Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/kubernetes/GenericYamlParser.java (2)
137-148: Excellent enum parsing with proper error handling.The changes improve enum parsing robustness:
- Uses
Locale.ROOTfor locale-independent case conversion- Wraps
IllegalArgumentExceptionin the customWrongEnumConstantExceptionfor clearer error messages- Properly declares checked exception in method signature
127-127: No issues found - Java 21 is confirmed for the core module.The parent pom.xml specifies
javac.sourceandjavac.targetas Java 21, which the core module inherits. The use ofList.getFirst()on line 127 is fully compatible with the project's Java 21 target version.annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (3)
42-42: LGTM! Well-implemented duplicate prevention.The
topLevelAddedtracking mechanism correctly prevents duplicate top-level parser definitions. The map is properly cleared inassemble()(line 78) for reusability, and the check at line 120 ensures parsers are only created once per child name.Also applies to: 120-120, 124-124
166-180: Good refactoring to centralize property creation.Extracting
createPropertyimproves maintainability and makes the enum handling logic more explicit.
90-90: Reference format is correct and consistent with codebase naming conventions.The underscore format in
com_predic8_membrane_core_config_spring_ApiParsermatches the output ofgetXSDTypeName(), which explicitly converts package dots to underscores. This is intentional—JSON Schema parsers do not accept names like a.b.c. The hardcoded reference aligns with how all other references are dynamically generated throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactory.java (1)
152-161: Verify the necessity of reading the resource twice.The refactored
getOpenAPImethod reads the OpenAPI resource twice:
- Line 160:
parseFromLocationcallsOpenAPIParser.readLocation()to parse the OpenAPI- Line 154:
getInputStreamForLocationretrieves the content again to read it as a JsonNodeWhile functionally correct, this double I/O could impact performance, especially for remote resources.
Consider whether both reads are necessary, or if the
JsonNodecould be obtained from theOpenAPIobject or cached from a single read. If both are required, consider adding a comment explaining why.If this pattern is acceptable for your use case, this is just a minor optimization suggestion and can be safely ignored.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sJsonSchemaGenerator.java(4 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/BeanCache.java(5 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactory.java(7 hunks)distribution/tutorials/getting-started/80-OpenAPI.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- distribution/tutorials/getting-started/80-OpenAPI.yaml
- annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sJsonSchemaGenerator.java
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/kubernetes/BeanCache.java (1)
core/src/main/java/com/predic8/membrane/core/exceptions/SpringConfigurationErrorHandler.java (1)
SpringConfigurationErrorHandler(30-197)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactory.java (1)
core/src/main/java/com/predic8/membrane/core/util/URIUtil.java (1)
URIUtil(28-146)
🔇 Additional comments (5)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactory.java (4)
40-40: LGTM! Necessary import for URI utility functions.The static import is required for the new
convertPath2FilePathStringusage at line 166.
89-89: LGTM! Log level adjustment reduces noise.Changing from
infotodebugfor individual spec parsing events is appropriate, as this can generate excessive logs when processing multiple specs.
163-169: LGTM! Correct URI conversion logic.The method correctly identifies when a path needs conversion to a file URI by checking for existing protocol prefixes. The implementation aligns with the
URIUtilutility functions.
140-140: No action required - exception handling verified.The verification confirms that the IOException propagation is correctly handled. The only call site at line 90 is wrapped in a try-catch block that catches
Exception, which properly handlesIOExceptionas a subclass. Since this is a private method and the caller correctly handles the exception, the code changes are appropriate.core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptor.java (1)
189-189: LGTM! Appropriate log level adjustment.Changing from
infotodebugfor this trace-level logging reduces noise for routine Swagger UI requests. All existing instantiations ofOpenAPIPublisherInterceptoralready use the parameterized constructor accepting aMap<String, OpenAPIRecord>, so the removal of the no-arg constructor does not break any code in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
distribution/tutorials/json/60-Json-Template.yaml (1)
30-36: Remove trailing comma from array items to ensure valid JSON output.The template generates invalid JSON due to the trailing comma after the closing brace on line 35. When the template renders, the last object in the positions array will have a trailing comma, which is not valid in strict JSON parsers.
Apply this diff to fix the trailing comma issue:
<% for (item in property.items) { %> { "product": "<%= item.article %>", "quantity": <%= item.quantity %>, "amount": <%= item.quantity * item.price %> - }, + }<%= item != property.items[-1] ? ',' : '' %> <% } %>Or alternatively, restructure to avoid the issue:
"positions": [ - <% for (item in property.items) { %> + <% + def first = true + for (item in property.items) { + if (!first) { %>,<% } + first = false + %> { "product": "<%= item.article %>", "quantity": <%= item.quantity %>, "amount": <%= item.quantity * item.price %> - }, + } <% } %>
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpressionTest.java (1)
187-198: Consider additional test cases for edge cases.The current test coverage is good for basic scenarios. Consider adding tests for:
- Case variations:
"False","NO","TRUE"(verify case-insensitive handling)- Whitespace edge cases:
" false "," 0 "," "(whitespace not trimmed in implementation)- Numeric variations:
"0.0","00","-1","2"(only"0"is explicitly FALSE)- Boolean objects:
Boolean.TRUE,Boolean.FALSE(line 145-146 in implementation)- Other types:
Integer.valueOf(0),Integer.valueOf(1), emptyList(default case returns FALSE)Particularly important: The implementation doesn't trim whitespace, so
" false "would evaluate to TRUE (not FALSE), which might be unexpected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptor.java(2 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/flow/IfInterceptorSpELTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpressionTest.java(2 hunks)distribution/tutorials/README.md(1 hunks)distribution/tutorials/advanced/65-if.yaml(1 hunks)distribution/tutorials/advanced/70-Scripting-Groovy.yaml(1 hunks)distribution/tutorials/getting-started/00-First-API.yaml(1 hunks)distribution/tutorials/getting-started/80-OpenAPI.yaml(1 hunks)distribution/tutorials/getting-started/90-OpenAPI-Validation.yaml(1 hunks)distribution/tutorials/getting-started/README.md(1 hunks)distribution/tutorials/getting-started/membrane.cmd(1 hunks)distribution/tutorials/getting-started/membrane.sh(1 hunks)distribution/tutorials/json/60-Json-Template.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- distribution/tutorials/getting-started/README.md
- distribution/tutorials/getting-started/00-First-API.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-02T18:05:47.059Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1947
File: distribution/tutorials/soap/membrane.sh:17-22
Timestamp: 2025-07-02T18:05:47.059Z
Learning: When using environment variable shortcuts like `$MEMBRANE_HOME` in shell scripts that look for installation directories, validate that required files (like `starter.jar` for Membrane) exist in the candidate directory before returning it. This prevents hard-to-diagnose runtime errors when the environment variable points to an invalid or stale location.
Applied to files:
distribution/tutorials/getting-started/membrane.cmddistribution/tutorials/getting-started/membrane.sh
🧬 Code graph analysis (1)
core/src/test/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpressionTest.java (1)
core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java (1)
SpELExchangeExpression(34-160)
🪛 markdownlint-cli2 (0.18.1)
distribution/tutorials/README.md
38-38: Bare URL used
(MD034, no-bare-urls)
🪛 Shellcheck (0.11.0)
distribution/tutorials/getting-started/membrane.sh
[warning] 8-8: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
🔇 Additional comments (9)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptor.java (1)
194-194: LGTM: Appropriate log level adjustment.Changing from info to debug for API documentation requests reduces log noise while maintaining observability for troubleshooting.
core/src/test/java/com/predic8/membrane/core/interceptor/flow/IfInterceptorSpELTest.java (2)
47-49: LGTM: Clean test method rename.The rename from
simpleRequestTruetorequestTrueimproves conciseness while maintaining clarity.
120-129: LGTM: Exception contract correctly narrowed.Removing
throws Exceptionfrom the method signature is appropriate and improves the method contract by making it more precise. Since the code compiles, the methods called internally (init()andhandleRequest()) either don't throw checked exceptions or only throw runtime exceptions, which aligns with the exception handling refactoring described in the PR context.core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java (1)
130-130: LGTM - Visibility widened for testability.The visibility change from
private statictostatic(package-private) enables direct testing, which is appropriate given the new test coverage.core/src/test/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpressionTest.java (1)
27-28: LGTM - Clean use of Boolean constants.Using Boolean constants improves test readability compared to primitive boolean literals.
distribution/tutorials/advanced/70-Scripting-Groovy.yaml (1)
1-51: LGTM! Clear demonstration of Groovy scripting capabilities.The tutorial effectively demonstrates three different Groovy scripting patterns:
- Variable interpolation and flow context
- Dynamic destination selection with Collections API
- Response builder pattern with fluent API
The YAML structure and Groovy syntax are correct.
distribution/tutorials/advanced/65-if.yaml (1)
1-39: LGTM! Effective demonstration of conditional flow control.The tutorial clearly shows:
- Default SpEL language for conditions
- Language switching to Groovy where needed
- Request and response phase conditional processing
- Proper error handling with appropriate status codes
The configuration is well-structured and the examples are instructive.
distribution/tutorials/getting-started/membrane.sh (1)
1-21: LGTM! Shell script correctly locates Membrane root with proper validation.The script implementation is sound:
- Uses CDPATH-aware directory resolution (line 8)
- Validates both
starter.jarandrun-membrane.shexist before using a directory (lines 12-15)- Properly exports environment variables and delegates to the launcher
- Provides clear error message on failure
Note: The shellcheck warning on line 8 about "space after =" is a false positive. The syntax
CDPATH= cdintentionally sets CDPATH to empty for that command only, preventing CDPATH from affecting directory resolution.Based on learnings
distribution/tutorials/getting-started/membrane.cmd (1)
1-24: LGTM! Windows batch script correctly mirrors the Unix shell script logic.The implementation is correct:
- Properly resolves script directory with trailing backslash handling (lines 4-5)
- Validates both
starter.jarandrun-membrane.cmdexist before using a directory (line 10)- Correctly ascends directory hierarchy until root
- Properly sets environment variables and delegates execution
- Preserves exit code from the launched process
- Provides clear error message on failure
Based on learnings
|
/ok-to-test |
|
@coderabbitai plz review |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
174-181: Keep enum casing stable.Lowercasing the enum literals (Line 175 onward) rewrites every generated schema to use lowercase strings, breaking consumers that rely on the existing uppercase values (e.g.,
EUR,USD,VALID). This regression was already flagged and remains unresolved—please preserve the original casing.- s.enumValues(enumsAsLowerCaseList(ai)); + s.enumValues(ai.getEnumValues());- private static List<String> enumsAsLowerCaseList(AttributeInfo ai) { - return ai.getEnumValues().stream().map(String::toLowerCase).toList(); - }
🧹 Nitpick comments (3)
core/src/test/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpressionTest.java (1)
188-204: Excellent test coverage for boolean conversion logic!The test comprehensively validates the
toBooleanmethod with various input types: string literals (case variations), edge cases (null, empty), numeric strings, affirmative/negative terms, and Boolean wrappers. All assertions correctly align with the implementation behavior.Optionally, consider adding test cases for these edge scenarios:
- Whitespace-only strings (e.g.,
" "," ")- Numeric types beyond strings (e.g.,
Integer.valueOf(0),Integer.valueOf(1))+ assertEquals(TRUE, toBoolean(" ")); // whitespace → TRUE (not in FALSE list) + assertEquals(FALSE, toBoolean(0)); // numeric zero → FALSE (default case) + assertEquals(FALSE, toBoolean(1)); // numeric one → FALSE (default case)Note: The current coverage is already strong; these additions would only marginally improve edge-case documentation.
distribution/tutorials/advanced/README.md (1)
1-10: Consider adding explicit references to tutorial files.The README introduces the advanced topics well but doesn't explicitly list or link to the individual tutorial YAML files (10-PathParameters.yaml, 20-Path-Parameter-Routing.yaml, etc.). Adding file references or links would help users navigate the tutorials more easily.
Example enhancement:
# Advanced Membrane Tutorial In this tutorial you will learn advanced features of Membrane, including: +- [Path Parameters](10-PathParameters.yaml) +- [Path Parameter Routing](20-Path-Parameter-Routing.yaml) +- [Path Rewriting](20-Path-Rewriting.yaml) +- [Redirects](30-Redirects.yaml)Before starting, we recommend completing the Getting Started tutorial.
</blockquote></details> <details> <summary>distribution/tutorials/getting-started/70-Template.yaml (1)</summary><blockquote> `9-13`: **Enhance tutorial with expected output.** The test instruction "Observe the response" is somewhat vague. Consider adding a brief note about what output to expect (e.g., "Method: GET, Path: /foo, Date: ...") to make the tutorial more self-contained and help users verify correctness quickly. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5b6360c0d671e72eb96213ceee552797bd05890a and cba8408b03163716bb3b67a4d78c22e7f23c28f0. </details> <details> <summary>📒 Files selected for processing (20)</summary> * `annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` (9 hunks) * `annot/src/main/java/com/predic8/membrane/annot/model/AttributeInfo.java` (1 hunks) * `core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptor.java` (2 hunks) * `core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPISpec.java` (0 hunks) * `core/src/test/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpressionTest.java` (2 hunks) * `distribution/tutorials/advanced/10-PathParameters.yaml` (1 hunks) * `distribution/tutorials/advanced/20-Path-Parameter-Routing.yaml` (1 hunks) * `distribution/tutorials/advanced/20-Path-Rewriting.yaml` (1 hunks) * `distribution/tutorials/advanced/30-Redirects.yaml` (1 hunks) * `distribution/tutorials/advanced/README.md` (1 hunks) * `distribution/tutorials/getting-started/00-First-API.yaml` (1 hunks) * `distribution/tutorials/getting-started/10-Logging.yaml` (1 hunks) * `distribution/tutorials/getting-started/20-Message-Flow.yaml` (1 hunks) * `distribution/tutorials/getting-started/30-Message-Flow2.yaml` (1 hunks) * `distribution/tutorials/getting-started/40-Basic-Path-Routing.yaml` (1 hunks) * `distribution/tutorials/getting-started/50-Short-Circuit.yaml` (1 hunks) * `distribution/tutorials/getting-started/60-SetHeader.yaml` (1 hunks) * `distribution/tutorials/getting-started/70-Template.yaml` (1 hunks) * `distribution/tutorials/getting-started/80-OpenAPI.yaml` (1 hunks) * `distribution/tutorials/getting-started/90-OpenAPI-Validation.yaml` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPISpec.java </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptor.java * distribution/tutorials/getting-started/90-OpenAPI-Validation.yaml * distribution/tutorials/getting-started/00-First-API.yaml </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (1)</summary> <details> <summary>📚 Learning: 2025-10-23T13:04:11.388Z</summary>Learnt from: predic8
Repo: membrane/api-gateway PR: 2240
File: annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java:210-215
Timestamp: 2025-10-23T13:04:11.388Z
Learning: In JsonSchemaGenerator.java, when allowForeign is true for non-list child elements, use a property named "ref" (not "$ref") with type "string" to provide IDE completion hints for Spring bean references while avoiding JSON Schema keyword collision.**Applied to files:** - `annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java` </details> </details><details> <summary>🧬 Code graph analysis (3)</summary> <details> <summary>core/src/test/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpressionTest.java (1)</summary><blockquote> <details> <summary>core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java (1)</summary> * `SpELExchangeExpression` (34-160) </details> </blockquote></details> <details> <summary>annot/src/main/java/com/predic8/membrane/annot/model/AttributeInfo.java (1)</summary><blockquote> <details> <summary>annot/src/main/java/com/predic8/membrane/annot/AnnotUtils.java (1)</summary> * `AnnotUtils` (22-99) </details> </blockquote></details> <details> <summary>annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)</summary><blockquote> <details> <summary>annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java (1)</summary> * `SchemaFactory` (5-60) </details> </blockquote></details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: Automated tests * GitHub Check: Analyze (java) </details> <details> <summary>🔇 Additional comments (13)</summary><blockquote> <details> <summary>core/src/test/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpressionTest.java (1)</summary><blockquote> `27-29`: **LGTM! Appropriate static imports for the new test.** The static imports support the new `booleanConversion()` test method and align with the widened visibility of the `toBoolean` method. </blockquote></details> <details> <summary>distribution/tutorials/advanced/30-Redirects.yaml (1)</summary><blockquote> `1-28`: **Well-structured redirect tutorial with clear guidance.** The YAML configuration demonstrates a clean redirect pattern with helpful inline instructions. The log messages and curl examples provide good guidance for users following the tutorial. </blockquote></details> <details> <summary>distribution/tutorials/advanced/20-Path-Parameter-Routing.yaml (1)</summary><blockquote> `1-14`: **Clean and focused path parameter routing tutorial.** The configuration clearly demonstrates path parameter syntax and template substitution. The example is straightforward and easy to follow. </blockquote></details> <details> <summary>distribution/tutorials/advanced/20-Path-Rewriting.yaml (1)</summary><blockquote> `1-25`: **Effective tutorial demonstrating path rewriting with logging.** The before/after logging pattern is pedagogically valuable for learners to observe the rewriting transformation. The inline comment about quoting special characters is helpful contextual guidance. </blockquote></details> <details> <summary>distribution/tutorials/advanced/10-PathParameters.yaml (1)</summary><blockquote> `15-25`: **Tutorial content is well-structured.** The path parameter definition and template rendering demonstrate the concept clearly. The setup instructions with curl example are helpful for users. </blockquote></details> <details> <summary>distribution/tutorials/getting-started/50-Short-Circuit.yaml (1)</summary><blockquote> `1-26`: **LGTM!** Well-structured tutorial with clear documentation, correct configuration syntax, and practical curl examples. </blockquote></details> <details> <summary>distribution/tutorials/getting-started/20-Message-Flow.yaml (1)</summary><blockquote> `1-28`: **LGTM!** Clear tutorial structure with platform-specific instructions. Variable bindings `${path}` and `${statusCode}` are used as per the core changes supporting enhanced scripting context. </blockquote></details> <details> <summary>distribution/tutorials/getting-started/30-Message-Flow2.yaml (1)</summary><blockquote> `1-53`: **LGTM!** Excellent tutorial progression showing request/response flow control. Metadata naming and Admin Console integration provide good learning value. Structured logging clearly demonstrates execution order. </blockquote></details> <details> <summary>distribution/tutorials/getting-started/10-Logging.yaml (1)</summary><blockquote> `1-30`: **Configuration and documentation are sound.** The logging tutorial effectively demonstrates the logging plugin with clear cross-platform instructions and expected output. </blockquote></details> <details> <summary>distribution/tutorials/getting-started/40-Basic-Path-Routing.yaml (1)</summary><blockquote> `1-40`: **Configuration and documentation are well-structured.** Path routing with multiple backend targets is clearly demonstrated. The use of `---` separators and ordering rules is well-documented with practical curl examples. </blockquote></details> <details> <summary>distribution/tutorials/getting-started/60-SetHeader.yaml (1)</summary><blockquote> `1-24`: **LGTM!** Concise tutorial demonstrating both static and dynamic header values. The `${method}` binding is supported by the core scripting context enhancements introduced in this PR. </blockquote></details> <details> <summary>distribution/tutorials/getting-started/80-OpenAPI.yaml (1)</summary><blockquote> `1-26`: **LGTM!** Past review feedback has been successfully addressed. The typo correction and filename fix are in place. Configuration correctly demonstrates loading both remote and local OpenAPI specifications. Schema path is consistent with other tutorials. </blockquote></details> <details> <summary>distribution/tutorials/getting-started/70-Template.yaml (1)</summary><blockquote> `1-27`: **No issues found with template binding availability in response phase.** The `method` and `path` bindings are extracted from the Exchange request object at the parameter binding stage (ScriptingUtils lines 59-60), before flow-specific logic. These bindings are therefore available in both request and response phases, making the tutorial configuration correct and functional. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@predic8: I'll review the changes in this PR right away! ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
annot/src/main/java/com/predic8/membrane/annot/model/AttributeInfo.java (1)
140-156: InitializeenumValuesfor enum types withjava.lang.Enumsuperclass.When the setter parameter's declared type has
java.lang.Enumas its superclass, the code setsisEnum = truebut never initializes theenumValuesfield. This will cause aNullPointerExceptionwhenisEnumBoolean()orgetEnumValues()is subsequently called, since those methods dereferenceenumValues.Apply this diff to populate
enumValuesbefore returning:if (superClass.getQualifiedName().toString().equals("java.lang.Enum")) { isEnum = true; + enumValues = getEnumValues(e); xsdType = "xsd:string"; // TODO: restriction, but be carefull about Spring EL usage, for example "#{config.XXX}"annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
174-176: Fix enum case handling inconsistency and potential breaking change.Converting non-boolean enum values to lowercase (line 175) creates inconsistent schema generation and may break existing consumers:
Inconsistency: Boolean enums get type "boolean" with no enum values, while other enums get lowercase string values. This means enum constants like
VALID,INVALIDappear as"valid","invalid"in schemas.Breaking change: Existing validators and configurations expecting uppercase enum values (e.g.,
EUR,USD,TRUE,FALSEas seen in test data) will fail validation.Consider preserving the original enum constant casing to maintain compatibility and consistency across all enum types.
Apply this diff to preserve original casing:
- if (ai.isEnum(processingEnv.getTypeUtils()) && !"boolean".equals(type)) { - s.enumValues(ai.enumsAsLowerCaseList(processingEnv.getTypeUtils())); - } + if (ai.isEnum(processingEnv.getTypeUtils()) && !"boolean".equals(type)) { + s.enumValues(ai.getEnumValues(processingEnv.getTypeUtils())); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java(9 hunks)annot/src/main/java/com/predic8/membrane/annot/model/AttributeInfo.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPISpec.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T13:04:11.388Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2240
File: annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java:210-215
Timestamp: 2025-10-23T13:04:11.388Z
Learning: In JsonSchemaGenerator.java, when allowForeign is true for non-list child elements, use a property named "ref" (not "$ref") with type "string" to provide IDE completion hints for Spring bean references while avoiding JSON Schema keyword collision.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java
🧬 Code graph analysis (2)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java (1)
SchemaFactory(5-60)
annot/src/main/java/com/predic8/membrane/annot/model/AttributeInfo.java (1)
annot/src/main/java/com/predic8/membrane/annot/AnnotUtils.java (1)
AnnotUtils(22-99)
🔇 Additional comments (3)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPISpec.java (1)
151-151: LGTM!Removing the trailing semicolon is correct Java style for enums without additional members after the constants.
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (2)
166-178: Good refactoring to extract property creation logic.The new
createPropertyhelper method improves code organization by consolidating the schema property construction logic that was previously inline. The use ofSchemaFactory.from(type)and the clear handling of enum vs. non-enum cases makes the code more maintainable.
197-268: Approve use of generic wildcards for schema flexibility.The refactoring to use
AbstractSchema<?>in method signatures (lines 197, 200, 235, 257) improves type safety and flexibility, allowing methods to work with any schema type (SchemaObject, SchemaArray, etc.) without unnecessary type constraints.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
annot/src/main/java/com/predic8/membrane/annot/model/AttributeInfo.java (1)
140-156: InitializeenumValueswhen detecting enum via superclass.This issue was flagged in a previous review but remains unaddressed. When a declared type's superclass is
java.lang.Enum, the code setsisEnum = true(line 143) but never initializes theenumValuesfield. Subsequently, calls togetEnumValues(),isEnumBoolean(), orenumsAsLowerCaseList()will return or dereferencenull, causingNullPointerException.Apply this diff to initialize
enumValues:if (superClass.getQualifiedName().toString().equals("java.lang.Enum")) { isEnum = true; + enumValues = getEnumValues(e); xsdType = "xsd:string"; // TODO: restriction, but be carefull about Spring EL usage, for example "#{config.XXX}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
annot/src/main/java/com/predic8/membrane/annot/model/AttributeInfo.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
annot/src/main/java/com/predic8/membrane/annot/model/AttributeInfo.java (1)
annot/src/main/java/com/predic8/membrane/annot/AnnotUtils.java (1)
AnnotUtils(22-99)
⏰ 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)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation