Yaml Tutorial: JSON, Fixes#2306
Conversation
WalkthroughThis pull request modernizes configuration handling by transitioning from XML to YAML as the primary format, refactors generic types and boolean handling, expands JSONPath/SpEL capabilities, introduces comprehensive tutorial infrastructure with test coverage, and removes legacy SOAP/REST examples and outdated configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant start_router
participant RouterCLI
participant getDefaultConfig
participant FileSystem
User->>start_router: ./start_router.sh [-c config]
alt Config provided
start_router->>RouterCLI: init with provided config
else No config
start_router->>RouterCLI: init with no args
RouterCLI->>getDefaultConfig: resolve default config
getDefaultConfig->>FileSystem: check MEMBRANE_CALLER_DIR/apis.yaml
alt Found
FileSystem-->>getDefaultConfig: apis.yaml path
else Not found
getDefaultConfig->>FileSystem: check MEMBRANE_CALLER_DIR/apis.yml
alt Found
FileSystem-->>getDefaultConfig: apis.yml path
else Not found
getDefaultConfig->>FileSystem: check MEMBRANE_CALLER_DIR/proxies.xml
alt Found
FileSystem-->>getDefaultConfig: proxies.xml path
else Not found
getDefaultConfig->>FileSystem: check MEMBRANE_HOME/conf/apis.yaml
alt Found
FileSystem-->>getDefaultConfig: apis.yaml path
else Continue fallback...
getDefaultConfig-->>RouterCLI: Error - no config found
RouterCLI-->>User: Exit code 1
end
end
end
end
getDefaultConfig-->>RouterCLI: resolved config path
end
RouterCLI->>RouterCLI: Load config (YAML or XML)
RouterCLI-->>User: Router initialized
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/InMemoryURLStreamHandler.java (1)
55-68: Add null handling for base data path.Line 64 lacks null checks, creating inconsistent error handling compared to the overlay path (lines 58-63). If
datais null ordata.content.get(uri)returns null, this will throw a NullPointerException instead of a descriptive FileNotFoundException.Apply this diff to align error handling:
public InputStream getInputStream() throws IOException { try { URI uri = u.toURI(); if (overlay != null && overlay.content.containsKey(uri)) { byte[] buffer = overlay.content.get(uri); if (buffer == null) throw new FileNotFoundException("No in-memory resource for " + uri); return new ByteArrayInputStream(buffer); } + if (data == null || !data.content.containsKey(uri)) { + throw new FileNotFoundException("No in-memory resource for " + uri); + } + byte[] buffer = data.content.get(uri); + if (buffer == null) + throw new FileNotFoundException("No in-memory resource for " + uri); - return new ByteArrayInputStream(data.content.get(uri)); + return new ByteArrayInputStream(buffer); } catch (URISyntaxException e) { throw new RuntimeException(e); } }
🧹 Nitpick comments (9)
core/src/main/java/com/predic8/membrane/core/cli/util/JwkGenerator.java (1)
83-83: Consider improving error handling and resource usage in pre-existing code.While outside the scope of this header-addition PR, the following pre-existing improvements could enhance maintainability:
- Line 83: Use generic type
Map<String, Object>instead of rawMaptype.- Lines 76 & 87: Pass the full exception to the logger (
log.error("...", e)) instead of just the message to preserve stack traces for debugging.- Line 83: Consider making
ObjectMappera static final field to avoid repeated instantiation.These are optional improvements for a follow-up.
Also applies to: 76-76, 87-87
distribution/tutorials/data/animals.json (1)
1-15: Clarify the purpose of thenumberfield.The
numberfield set to 2 appears to represent the count of animals, but this is already implicit in the array length. Consider whether this field is intentional for the tutorial or if it could be removed to keep the sample data simpler.distribution/tutorials/data/README.md (2)
1-3: Add trailing newline to file.Most projects follow the convention of ending text files with a newline character.
# Sample Datasets -Here are some sample datasets for the tutorials. \ No newline at end of file +Here are some sample datasets for the tutorials.
1-3: Consider expanding documentation to guide tutorial readers.The file currently serves as a minimal placeholder. Consider adding descriptions of each available dataset, their structure, and which tutorials use them to provide more value to users.
annot/src/test/java/com/predic8/membrane/annot/util/ConcatenatingEnumeration.java (1)
28-28: Consider makingnextIndexprivate.The
nextIndexfield currently has package-private visibility. For better encapsulation, it should be declared asprivate.Apply this diff:
- int nextIndex = 0; + private int nextIndex = 0;annot/src/test/java/com/predic8/membrane/annot/util/CompositeClassLoader.java (1)
29-33: Consider adding null checks for the constructor parameters.While the current implementation works when valid loaders are provided, adding null validation would prevent potential NullPointerExceptions and make the class more robust.
Apply this diff to add defensive null checks:
public CompositeClassLoader(ClassLoader loaderA, ClassLoader loaderB) { super(null); + if (loaderA == null || loaderB == null) { + throw new IllegalArgumentException("ClassLoaders cannot be null"); + } this.loaderA = loaderA; this.loaderB = loaderB; }annot/src/test/java/com/predic8/membrane/annot/util/InMemoryURLStreamHandler.java (1)
50-52: Consider setting the connected flag.While the empty implementation is reasonable for in-memory streams, the URLConnection base class has a
protected boolean connectedfield that conventionally should be set totrueinconnect().Apply this diff if you want to follow the convention:
public void connect() { - + connected = true; }core/src/main/java/com/predic8/membrane/core/util/StringUtil.java (1)
51-63: Consider using\\Rfor cross-platform line ending handling.The method splits on
\nonly, which may not handle Windows line endings (\r\n) correctly. For consistency withYamlUtil.removeFirstYamlDocStartMarker(line 31) which uses\\R, consider using the same pattern here.Apply this diff:
- for (String line : s.split("\n")) { + for (String line : s.split("\\R")) { sb.append(String.format("%4d: %s\n", i++, line)); }distribution/tutorials/json/20-JSONPath.yaml (1)
17-18: Consider validating additional required fields.The test only checks for the presence of
$.animals, but the configuration also references$.number(line 23) and assumes$.animalsis a non-empty array (line 38). If these assumptions don't hold, the tutorial may produce unexpected results.Consider either:
- Documenting the expected JSON structure in the comments
- Adding more comprehensive validation in the test
Example validation for non-empty array:
- test: $.animals + test: $.animals[0]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
annot/src/main/java/com/predic8/membrane/annot/generator/Schemas.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AbstractSchema.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AnyOf.java(1 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(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaRef.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaString.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/util/SchemaGeneratorUtil.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/ParsingTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObjectTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompositeClassLoader.java(3 hunks)annot/src/test/java/com/predic8/membrane/annot/util/ConcatenatingEnumeration.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java(2 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryData.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryFileObject.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryJavaFileObject.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryURLStreamHandler.java(2 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InnerFileObject.java(2 hunks)annot/src/test/java/com/predic8/membrane/annot/util/LoggingFileObject.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/LoggingJavaFileObject.java(1 hunks)core/src/main/java/com/predic8/membrane/core/cli/util/JwkGenerator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/cli/util/YamlLoader.java(2 hunks)core/src/main/java/com/predic8/membrane/core/config/xml/XmlConfig.java(1 hunks)core/src/main/java/com/predic8/membrane/core/http/ReadingBodyException.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/RuleMatchingInterceptor.java(0 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/log/access/package-info.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java(4 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/kubernetes/WrongEnumConstantException.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java(3 hunks)core/src/main/java/com/predic8/membrane/core/util/StringUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/YamlUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/log/AccessLogCallTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CUnitTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptorTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/util/YamlUtilTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/IntegrationTests.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/InMemB2CResourceIntegrationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/JwtB2CResourceIntegrationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2ResourceB2CIntegrationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/SyncB2CResourceIntegrationTest.java(1 hunks)distribution/tutorials/advanced/50-Redirects.yaml(1 hunks)distribution/tutorials/advanced/60-if.yaml(2 hunks)distribution/tutorials/advanced/70-Scripting-Groovy.yaml(1 hunks)distribution/tutorials/advanced/membrane.cmd(1 hunks)distribution/tutorials/advanced/membrane.sh(1 hunks)distribution/tutorials/data/README.md(1 hunks)distribution/tutorials/data/animals.json(1 hunks)distribution/tutorials/getting-started/data/person.json(0 hunks)distribution/tutorials/json/20-JSONPath.yaml(1 hunks)distribution/tutorials/json/60-Json-Transformation.yaml(2 hunks)distribution/tutorials/json/README.md(1 hunks)distribution/tutorials/json/membrane.cmd(1 hunks)distribution/tutorials/json/membrane.sh(1 hunks)
💤 Files with no reviewable changes (2)
- core/src/main/java/com/predic8/membrane/core/interceptor/RuleMatchingInterceptor.java
- distribution/tutorials/getting-started/data/person.json
🧰 Additional context used
🧠 Learnings (4)
📚 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/advanced/membrane.shdistribution/tutorials/advanced/membrane.cmddistribution/tutorials/json/membrane.cmddistribution/tutorials/json/membrane.sh
📚 Learning: 2025-07-28T16:28:17.849Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1977
File: core/src/main/java/com/predic8/membrane/core/interceptor/opentelemetry/OpenTelemetryInterceptor.java:107-109
Timestamp: 2025-07-28T16:28:17.849Z
Learning: In Java exception handling for OpenTelemetry body logging, use log.debug() level with ExceptionUtil.concatMessageAndCauseMessages(e) to log body reading failures. This prevents cluttering info logs with potentially common stream reading issues while still providing comprehensive error information when debug logging is enabled.
Applied to files:
core/src/main/java/com/predic8/membrane/core/http/ReadingBodyException.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/Schemas.javaannot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaRef.javaannot/src/main/java/com/predic8/membrane/annot/generator/util/SchemaGeneratorUtil.java
📚 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/cli/util/YamlLoader.java
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (1)
core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java (1)
TemplateExchangeExpression(30-167)
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/StringUtil.java (1)
StringUtil(19-65)
🪛 Shellcheck (0.11.0)
distribution/tutorials/advanced/membrane.sh
[warning] 8-8: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
distribution/tutorials/json/membrane.sh
[warning] 8-8: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
⏰ 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 (81)
annot/src/test/java/com/predic8/membrane/annot/util/LoggingJavaFileObject.java (1)
1-13: License header addition looks good.The Apache 2.0 license header is properly formatted with the correct copyright year and entity. No functional changes to the file.
core/src/main/java/com/predic8/membrane/core/kubernetes/WrongEnumConstantException.java (1)
1-13: Apache License 2.0 header added.The license header is properly formatted and aligns with the licensing updates across the codebase as part of this PR.
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaString.java (1)
1-13: Apache 2.0 license header added.The license header has been added correctly with the proper formatting and copyright year. This aligns with the broader license header additions across multiple Kubernetes model classes in this PR.
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CUnitTest.java (1)
1-14: License header properly added.The Apache 2.0 license header is correctly formatted and positioned before the package declaration. This aligns with the PR objective of adding license headers across multiple files.
core/src/main/java/com/predic8/membrane/core/interceptor/log/access/package-info.java (1)
1-13: License header correctly added.The Apache 2.0 license header is properly formatted with accurate copyright attribution, complete license text, and appropriate disclaimers. This aligns with the PR's objective to add licensing headers across the codebase.
annot/src/test/java/com/predic8/membrane/annot/util/InMemoryJavaFileObject.java (2)
1-13: License header properly added.The Apache License 2.0 header is correctly formatted and includes the required copyright notice, license terms, and disclaimer.
24-54: Clean delegation wrapper with appropriate debug logging.The class properly implements the JavaFileObject interface through delegation to the parent's
innerfield, with consistent debug logging in all overridden methods. The logging includes the URI context, which is helpful for tracing compiler interactions. No behavioral changes detected.core/src/main/java/com/predic8/membrane/core/cli/util/JwkGenerator.java (1)
1-14: License header properly added.The Apache 2.0 license header is correctly formatted and positioned before the package declaration, following standard Java conventions.
annot/src/test/java/com/predic8/membrane/annot/util/InMemoryData.java (1)
1-14: License header addition is well-formed.The Apache 2.0 license header is correctly formatted and aligns with the PR's license compliance objectives.
annot/src/test/java/com/predic8/membrane/annot/util/InMemoryFileObject.java (2)
1-14: License header addition is well-formed.The Apache 2.0 license header is correctly formatted and consistent with the additions across the codebase.
31-37: Field removal refactoring is sound.The removal of the unused
InMemoryData datafield improves the design by eliminating unnecessary state storage. The data parameter is correctly threaded through to theInnerFileObjectconstructor, preserving all intended behavior without storing redundant references.Verify that no other methods in this class reference the removed
datafield and that theInMemoryFileObjectinstances work correctly in their call sites.distribution/tutorials/advanced/50-Redirects.yaml (1)
12-12: Minor tutorial documentation updates approved.Comment text refinements improve clarity for learners following the redirect tutorial. No functional or structural changes to the configuration.
Also applies to: 16-16
distribution/tutorials/advanced/70-Scripting-Groovy.yaml (2)
5-9: Tutorial URLs improved for clarity and learner experience.Converting relative curl commands to explicit absolute URLs (http://localhost:2000/groovy) is a solid improvement—learners following the tutorial can copy-paste without inferring context. Consistent with tutorial best practices and other files in this PR.
1-50: Summary inconsistency: /random spec present despite removal claim.The AI summary states the /random spec block was "removed," but lines 24–33 show it remains intact in the annotated code. Please verify whether this is an incomplete summary or if the code presentation does not reflect the final state.
distribution/tutorials/advanced/60-if.yaml (2)
5-6: Header simplified for consistency.Condensing tutorial guidance to "Try:" aligns with other advanced tutorial files and presents cleaner, more actionable instructions to learners.
16-17: Documentation of default SPEL language improves clarity.Repositioning the comment to explicitly document that SPEL is the default language for
ifblocks makes the tutorial more understandable. The removal of redundantlanguage: spelspecifications elsewhere in the config is appropriate given this clarification.annot/src/test/java/com/predic8/membrane/annot/util/ConcatenatingEnumeration.java (2)
1-14: LGTM! License header properly added.The Apache 2.0 license header is correctly formatted and aligns with the PR's objective of adding license headers across multiple files.
31-44: LGTM! Enumeration logic is correct.The
hasMoreElements()andnextElement()methods correctly implement the concatenation logic: iterating through the additional elements first, then delegating to the base enumeration. The implementation properly adheres to theEnumerationcontract.annot/src/test/java/com/predic8/membrane/annot/util/InMemoryClassLoader.java (2)
1-14: LGTM! License header properly added.The Apache 2.0 license header is correctly formatted and consistent with other files in this PR.
165-165: LGTM! Good use of the diamond operator.The update to use the diamond operator (
new ConcatenatingEnumeration<>()) is a clean modernization that maintains type safety through inference. The logic correctly constructs either an empty or single-element URL array based on whetheruri2is null.annot/src/test/java/com/predic8/membrane/annot/util/CompositeClassLoader.java (3)
1-13: LGTM! Standard license header.The Apache 2.0 license header is properly formatted and consistent with the PR's objective of adding license headers across files.
26-27: Good practice: immutable fields.Marking the loader fields as
finalensures immutability and thread-safety, which is appropriate for a ClassLoader implementation.
54-72: LGTM! Clean composite enumeration with diamond operator.The diamond operator on line 55 modernizes the code, and the enumeration logic correctly combines resources from both loaders in the proper order (loaderA first, then loaderB).
annot/src/test/java/com/predic8/membrane/annot/util/InMemoryURLStreamHandler.java (2)
1-13: LGTM! License header added.The Apache 2.0 license header is properly formatted and follows standard conventions.
46-46: Valid signature simplification.Removing the
throws IOExceptiondeclaration is valid Java—when overriding, you can omit checked exceptions declared by the parent if your implementation doesn't throw them. Since this method body doesn't directly throw IOException, the simplified signature is cleaner.annot/src/test/java/com/predic8/membrane/annot/util/InnerFileObject.java (3)
1-13: LGTM: License header added correctly.The Apache 2.0 license header is properly formatted with appropriate copyright attribution.
39-44: Valid exception specification narrowing.The removal of
throws IOExceptionfrom the method signature is correct—the implementation doesn't throw any checked exceptions. This covariant exception specification (narrowing) is permitted in Java and makes the signature more accurate.
47-63: Valid exception specification narrowing.Removing
throws IOExceptionfromopenOutputStream()is correct—the method body itself doesn't throw checked exceptions (it only returns a stream object). The returned stream's methods (flush(),close()) appropriately declarethrows IOException, which is separate from this method's contract.annot/src/test/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObjectTest.java (1)
1-14: LGTM!License header addition is appropriate and consistent with project-wide licensing updates.
annot/src/test/java/com/predic8/membrane/annot/ParsingTest.java (1)
1-14: LGTM!License header addition aligns with the project-wide licensing update.
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaArray.java (1)
1-14: LGTM!License header addition is consistent with the broader licensing update across the codebase.
core/src/main/java/com/predic8/membrane/core/config/xml/XmlConfig.java (1)
1-14: LGTM!License header addition aligns with the project-wide licensing update.
core/src/test/java/com/predic8/membrane/core/interceptor/log/AccessLogCallTest.java (1)
1-14: LGTM!License header addition is consistent with the project-wide licensing update.
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AnyOf.java (1)
1-14: LGTM!License header addition aligns with the project-wide licensing update.
core/src/test/java/com/predic8/membrane/core/util/YamlUtilTest.java (1)
1-14: LGTM!License header addition is consistent with the project-wide licensing update.
annot/src/main/java/com/predic8/membrane/annot/generator/util/SchemaGeneratorUtil.java (2)
1-13: License header looks good.The Apache 2.0 license header is properly formatted and complete.
19-41: JSON escaping implementation is correct.The
escapeJsonContentandescapemethods properly handle all standard JSON escape sequences and control characters according to the JSON specification.core/src/main/java/com/predic8/membrane/core/util/YamlUtil.java (2)
1-13: License header looks good.The Apache 2.0 license header is properly formatted and complete.
28-44: YAML marker removal implementation is correct.The method properly handles the removal of the first YAML document start marker, normalizes line endings, and handles null input gracefully.
annot/src/test/java/com/predic8/membrane/annot/util/LoggingFileObject.java (2)
1-13: License header looks good.The Apache 2.0 license header is properly formatted and complete.
24-86: Clean decorator pattern implementation.The logging wrapper correctly delegates all FileObject methods to the inner instance while adding debug logging. This is a standard and effective pattern for adding observability to file operations.
annot/src/test/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaTest.java (1)
1-13: License header looks good.The Apache 2.0 license header is properly formatted and complete.
core/src/main/java/com/predic8/membrane/core/cli/util/YamlLoader.java (2)
1-13: License header looks good.The Apache 2.0 license header is properly formatted and complete.
103-103: Lambda simplification is clean.The multi-line lambda body has been correctly collapsed to a single-line expression with no change in behavior.
core/src/main/java/com/predic8/membrane/core/http/ReadingBodyException.java (2)
1-13: License header looks good.The Apache 2.0 license header is properly formatted and complete.
18-20: Standard exception constructor pattern.The new constructor properly wraps an underlying exception, following common Java exception chaining patterns.
core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (3)
18-18: Import changes are style adjustments.The switch to wildcard imports is a stylistic choice with no functional impact.
Also applies to: 22-22, 24-24
115-115: Formatting improvement.Added space after comma improves code readability.
125-128: Control flow simplification is correct.Removing the unnecessary
elsebranch makes the code cleaner while maintaining the same behavior—whenexchangeExpressionis not null, the method returns the evaluated value; otherwise, it returnsurl.annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/BasicSchema.java (2)
1-13: LGTM: License header added.The Apache 2.0 license header is properly formatted and consistent with project standards.
19-25: LGTM: Public constructors extend API surface.The new constructors provide convenient initialization paths and correctly delegate to the parent class. This is a non-breaking API extension.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/AbstractTemplateInterceptor.java (3)
127-127: LGTM: Error message improved.The updated message "Could not read template from" is more accurate than "Could not create template from" for this context.
47-47: The review comment appears to be incorrect or based on outdated information.The current code already uses primitive
booleanconsistently: the field (line 47), getter (line 177), and setter (line 187) all use primitiveboolean. The setter is properly annotated with@MCAttribute, which handles XML configuration correctly for primitive types. Auto-unboxing in test code (e.g., passingBoolean.TRUEtosetPretty(boolean)) works without issues, and no actual breaking change exists in the codebase—all usages are type-compatible with the primitive boolean implementation.Likely an incorrect or invalid review comment.
177-189: Disregard this review comment; the setter signature change is not a breaking change.Spring's XML attribute parsing automatically converts string values from XML (e.g.,
pretty="true") to the target parameter type. The codebase already uses@MCAttributewith boolean parameters elsewhere (e.g.,FileExchangeStore.setRaw(boolean),ServletTransport.setRemoveContextRoot(boolean)), and XML configuration examples show theprettyattribute working with string values likepretty="yes"andpretty="true". The commit message indicates this was a deliberate fix to use the correct primitive boolean type rather than String.Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptor.java (2)
34-34: LGTM: Import added for enhanced error reporting.The static import of
addLineNumberssupports the improved error message below.
137-137: LGTM: Enhanced error message with line numbers.Adding line numbers to the template source in error messages significantly improves debugging experience when template syntax errors occur. This is a valuable enhancement.
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaRef.java (1)
1-14: LGTM: License header added.The Apache 2.0 license header is properly formatted and consistent with project standards.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/OAuth2ResourceB2CIntegrationTest.java (1)
1-14: LGTM: License header added.The Apache 2.0 license header is properly formatted and consistent with project standards.
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/AbstractSchema.java (1)
1-14: LGTM: License header added.The Apache 2.0 license header is properly formatted and consistent with project standards.
core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/SyncB2CResourceIntegrationTest.java (2)
1-14: LGTM: License header added.The Apache 2.0 license header is properly formatted and consistent with project standards.
20-25: LGTM: Test class properly configured.The test extends
OAuth2ResourceB2CIntegrationTestand overridescreateSessionManager()to provide aFakeSyncSessionStoreManager, which is appropriate for isolated testing. This follows the same pattern as other test implementations in the suite.core/src/test/java/com/predic8/membrane/core/interceptor/templating/StaticInterceptorTest.java (2)
26-26: LGTM: Clean import for Boolean constant.The static import of
TRUEis appropriate for the updated API usage.
50-51: LGTM: Test updated to match API change.The test correctly uses the Boolean constant with the updated
setPretty(boolean)signature.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/JwtB2CResourceIntegrationTest.java (2)
1-13: LGTM: Standard Apache 2.0 license header.
20-23: LGTM: Null session manager appropriate for JWT tests.Returning
nullfromcreateSessionManager()is appropriate for JWT-based authentication tests, as JWTs are typically stateless and don't require server-side session storage.core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java (2)
223-236: LGTM: Test correctly updated for API change.The
setPretty(TRUE)call now uses a Boolean constant instead of a String, matching the updated API signature.
238-249: LGTM: Consistent API usage.The second test also correctly uses the Boolean constant with
setPretty(TRUE).annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaFactory.java (2)
1-14: LGTM: Standard Apache 2.0 license header.
36-46: LGTM: Improved type safety with wildcard generic.Adding
<?>to the return typeAbstractSchema<?>makes the generic nature explicit and improves type safety without breaking existing callers.core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java (1)
53-61: LGTM: Test data enhanced with array fields.The addition of
numbersandobjectsarrays provides good coverage for testing JSONPath list handling and string conversion.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/InMemB2CResourceIntegrationTest.java (2)
1-13: LGTM: Standard Apache 2.0 license header.
20-24: LGTM: Clean test setup with in-memory session management.This test class appropriately uses
InMemorySessionManagerfor B2C OAuth2 resource tests that require session state, complementing the JWT variant that uses no session manager.distribution/tutorials/advanced/membrane.sh (2)
8-8: Shellcheck warning is a false positive.The syntax
CDPATH= cdis correct—it temporarily clears CDPATH for thecdcommand to prevent unexpected behavior. The warning can be safely ignored.
10-18: Root discovery logic looks good.The upward traversal correctly validates both
starter.jarandrun-membrane.shexist before setting MEMBRANE_HOME, preventing runtime errors from invalid installation directories.Based on learnings
distribution/tutorials/json/README.md (1)
1-3: LGTM!Clear and helpful guidance for users starting the JSON tutorial.
distribution/tutorials/json/membrane.sh (2)
8-8: Shellcheck warning is a false positive.The syntax
CDPATH= cdis correct—it temporarily clears CDPATH for thecdcommand. The warning can be safely ignored.
10-18: Root discovery implementation is correct.The script properly validates both required files exist before setting MEMBRANE_HOME, consistent with the pattern in
distribution/tutorials/advanced/membrane.sh.Based on learnings
distribution/tutorials/json/membrane.cmd (1)
9-14: Root discovery logic is sound.The upward traversal correctly validates both
starter.jarandrun-membrane.cmdexist before proceeding, and properly detects when the filesystem root is reached.Based on learnings
distribution/tutorials/advanced/membrane.cmd (1)
9-14: Root discovery implementation is correct.The logic mirrors
distribution/tutorials/json/membrane.cmdand properly validates required files before setting MEMBRANE_HOME.Based on learnings
distribution/tutorials/json/60-Json-Transformation.yaml (1)
38-49: Loop refactor improves clarity.The index-based loop with explicit trailing comma handling is cleaner and more maintainable than the previous implementation. The loop bounds and comma conditional logic are correct.
distribution/tutorials/json/20-JSONPath.yaml (1)
38-39: Add test coverage for empty arrays in JSONPath expressions.The framework handles array bounds via
PathNotFoundExceptioncatch (returningnull), but the tutorial and tests don't verify behavior when$.animalsis empty. Line 17 checks that the property exists but not whether it contains elements. If the array is empty,$.animals[0].namewould evaluate tonulland be substituted in the URL template as?animal=null, which may not be the intended behavior. Add a test case verifying this scenario or document the expected behavior explicitly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java (1)
112-118: Breaking change previously identified: verify alignment with JSONPath standards.This change was already flagged in a previous review as a breaking behavioral modification. The code now returns the entire list as a string representation (e.g.,
"[1, 2, 3]") instead of just the first element.The added comment claims this is "consistent with most JSONPath implementations," but this assertion should be verified against the JSONPath specification and popular implementations (e.g., Jayway JsonPath, json-path in other languages).
Additional considerations:
- Empty lists:
[].toString()produces"[]", which may be unexpected for users migrating from the first-element behavior- Null elements: Lists containing nulls will render them as the string
"null"in the output- Large lists: Converting large lists to strings may produce unwieldy output in logs or downstream systems
Please verify the JSONPath standard behavior:
How do JSONPath implementations handle conversion of list results to string? Does Jayway JsonPath convert lists to strings or extract first element?
🧹 Nitpick comments (2)
distribution/tutorials/getting-started/50-Short-Circuit.yaml (1)
27-30: Clarify the "reversed-flow" behavior in tutorial comments.The comment on lines 27–28 uses technical terminology ("reversed flow") that may not be immediately clear to tutorial readers. Consider making it more explicit about what the return action does.
The current comment:
# Reverses the flow. # If there is no response yet it will copy the body from the request.Consider something like:
# When no backend response exists, copy the request body to the response.This makes the behavior clearer without requiring familiarity with Membrane's flow reversal concept.
core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (1)
21-21: Consider replacing wildcard import with specific imports.Wildcard imports like
import javax.xml.*;can lead to ambiguity and make it harder to track dependencies. Consider importing only the specific classes needed (e.g.,XMLConstants).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaArray.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/soap/SampleSoapServiceInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/SOAPUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java(2 hunks)distribution/tutorials/data/animals.xml(1 hunks)distribution/tutorials/getting-started/45-Admin-Web-Console.yaml(1 hunks)distribution/tutorials/getting-started/50-Short-Circuit.yaml(2 hunks)distribution/tutorials/json/20-JSONPath.yaml(1 hunks)distribution/tutorials/json/README.md(1 hunks)distribution/tutorials/xml/10-JSON-to-XML.yaml(1 hunks)distribution/tutorials/xml/15-XML-to-JSON.yaml(1 hunks)distribution/tutorials/xml/20-XPath.yaml(1 hunks)distribution/tutorials/xml/README.md(1 hunks)distribution/tutorials/xml/membrane.cmd(1 hunks)distribution/tutorials/xml/membrane.sh(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- distribution/tutorials/xml/README.md
- distribution/tutorials/data/animals.xml
🚧 Files skipped from review as they are similar to previous changes (4)
- distribution/tutorials/json/20-JSONPath.yaml
- distribution/tutorials/json/README.md
- annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaArray.java
- core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java
🧰 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/xml/membrane.shdistribution/tutorials/xml/membrane.cmd
🪛 Shellcheck (0.11.0)
distribution/tutorials/xml/membrane.sh
[warning] 8-8: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
🔇 Additional comments (16)
core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java (1)
63-67: Improved error message clarity looks good.Calling out SpEL explicitly in the exception should make debugging easier for users working with mixed expression engines. Nicely targeted tweak.
distribution/tutorials/getting-started/50-Short-Circuit.yaml (1)
18-22: Verify the body-copying behavior works as expected.Step 3 instructs users to send a request with a body and observe the response body, implying that the request body will appear in the response. Ensure that the "return" action combined with the reversed-flow behavior actually echoes the request body as the response body.
distribution/tutorials/getting-started/45-Admin-Web-Console.yaml (3)
1-15: LGTM on the tutorial documentation and schema reference.The header comments clearly explain the tutorial's purpose and setup instructions. The schema reference should enable IDE validation support.
16-26: LGTM on the Admin Console spec configuration.The basicAuthentication with admin/admin credentials and readOnly mode are appropriate for a tutorial environment.
29-48: Routing precedence pattern is correctly implemented.Verification confirms the code is correct. The 45-Admin-Web-Console.yaml file follows the established routing pattern documented in 40-Basic-Path-Routing.yaml, which explicitly states: "Incoming requests are matched against the declared APIs from top to bottom." The path-specific specs (
/jokes,/api) are correctly ordered before the catch-all spec, matching the documented Membrane behavior across multiple tutorial files.core/src/main/java/com/predic8/membrane/core/util/SOAPUtil.java (1)
67-67: LGTM! Method rename aligns with XMLUtil API update.The change from
xml2stringtoxmlNode2Stringcorrectly reflects the updated XML utility API.core/src/main/java/com/predic8/membrane/core/interceptor/soap/SampleSoapServiceInterceptor.java (1)
210-210: LGTM! Method rename aligns with XMLUtil API update.The change from
xml2stringtoxmlNode2Stringcorrectly reflects the updated XML utility API.core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (3)
34-55: Excellent thread-safety and security improvements!The ThreadLocal pattern correctly addresses TransformerFactory's lack of thread-safety. The security settings (FEATURE_SECURE_PROCESSING, ACCESS_EXTERNAL_DTD, ACCESS_EXTERNAL_STYLESHEET) provide good protection against XXE attacks and entity expansion vulnerabilities.
59-61: Consider the implications of returning empty string for null input.Returning an empty string when
nodeis null could mask bugs where null is passed unexpectedly. Consider whether throwing an exception or logging a warning would be more appropriate for your use case.
58-71: All callers have been successfully updated to use the new method name.The comprehensive verification confirms:
- No remaining calls to the old
xml2stringmethod exist anywhere in the codebase- The new
xmlNode2Stringmethod is correctly being used in SOAPUtil and SampleSoapServiceInterceptor as expectedThe breaking API change has been properly addressed.
distribution/tutorials/xml/10-JSON-to-XML.yaml (1)
1-21: LGTM! Well-documented tutorial.The tutorial is clear, well-structured, and provides helpful guidance. The YAML configuration correctly demonstrates JSON to XML conversion with proper formatting.
distribution/tutorials/xml/15-XML-to-JSON.yaml (1)
1-20: LGTM! Consistent and clear tutorial.The tutorial follows the same well-structured pattern as the companion JSON-to-XML tutorial, making it easy for users to understand both transformation directions.
distribution/tutorials/xml/membrane.sh (1)
1-21: LGTM! Robust launcher script with proper validation.The script correctly:
- Validates the existence of both
starter.jarandrun-membrane.shbefore using a directory as Membrane root (addressing the learning from PR #1947)- Handles CDPATH to ensure reliable path resolution
- Provides clear error messaging if Membrane root cannot be found
Note: The Shellcheck warning at line 8 is a false positive. The syntax
CDPATH= cd ...correctly sets CDPATH to empty temporarily for that command only.Based on learnings
distribution/tutorials/xml/membrane.cmd (1)
1-24: LGTM! Proper Windows launcher with validation.The batch script correctly:
- Validates the existence of both
starter.jarandrun-membrane.cmdbefore using a directory (line 10), consistent with the learning from PR #1947- Handles Windows path resolution properly
- Propagates error levels correctly
- Mirrors the shell script's logic for cross-platform consistency
Based on learnings
distribution/tutorials/xml/20-XPath.yaml (2)
1-53: Comprehensive XPath tutorial with good examples.The tutorial effectively demonstrates various XPath capabilities including:
- Extracting NodeLists and individual values
- Using XPath functions like
count()- Filtering with predicates
- Conditional flow based on XPath results
The documentation is clear and provides good guidance for users.
44-47: XPath expression syntax is correct.The unwrapped expression
//animal[@species='dog']in thetestattribute (line 46) is the correct syntax for Membrane'sifinterceptor. Web search confirms thetestattribute expects a direct XPath expression without${}wrapping, while value/message attributes use wrapping. Your inline comment accurately documents this intentional difference.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/BooleanValidatorTest.java(1 hunks)docs/ROADMAP.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/ROADMAP.md
⏰ 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)
🔇 Additional comments (4)
core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java (1)
33-43: Validate method correctly handles case-insensitive boolean strings.The implementation properly lowercases the input before comparison, ensuring case-insensitive matching for "yes"/"no"/"true"/"false".
core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (3)
28-28: LGTM! Clean static import.The static import of
XMLConstantsimproves readability for the security-related constants used in theThreadLocalinitializer.
56-69: Thread-safe implementation is correct; null-handling change is defensible but verify intent.The implementation correctly uses
TF.get().newTransformer()to safely create freshTransformerinstances per call. Both identified callers (SampleSoapServiceInterceptor.java:210andSOAPUtil.java:67) pass non-null inputs from methods that cannot return null, so they are unaffected.However, the null-check represents a semantic change to the public API: previously,
nullinput would throwNullPointerException; now it returns"". While this is defensive and won't impact current usage, confirm whether this behavior was intentional for future external callers or if the null-check should be removed.
32-53: Code follows established codebase patterns and is correctly implemented.After verification of the codebase architecture, the implementation aligns with established design decisions:
ThreadLocal without cleanup is intentional: XMLProtector uses the identical pattern (line 62 in XMLProtector.java) with ThreadLocal for expensive factory reuse. No cleanup patterns exist anywhere in the codebase, indicating this is a deliberate performance optimization choice, not an oversight.
Silent exception handling is intentional: XMLProtector also uses silent catches for feature compatibility (line 162-163), and the comment explicitly states this is acceptable: "Log if you want, but do not fail hard because of missing feature." The exceptions are for optional features that may not be supported across JVM implementations.
Security measures are correctly applied: The implementation properly sets
FEATURE_SECURE_PROCESSINGand disables external DTD/stylesheet access, mirroring defensive security patterns used throughout the codebase.
|
This pull request needs "/ok-to-test" from an authorized committer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/TemplateTutorialTest.java (1)
8-8: Minor style: Add whitespace before opening brace.-public class TemplateTutorialTest extends AbstractGettingStartedTutorialTest{ +public class TemplateTutorialTest extends AbstractGettingStartedTutorialTest {distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiValidationTutorialTest.java (1)
10-10: Minor style: Add whitespace before opening brace.-public class OpenApiValidationTutorialTest extends AbstractGettingStartedTutorialTest{ +public class OpenApiValidationTutorialTest extends AbstractGettingStartedTutorialTest {distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/ShortCircuitTutorialTest.java (1)
8-8: Minor style: Add whitespace before opening brace.-public class ShortCircuitTutorialTest extends AbstractGettingStartedTutorialTest{ +public class ShortCircuitTutorialTest extends AbstractGettingStartedTutorialTest {distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/AdminWebConsoleTutorialTest.java (1)
8-8: Minor style: Add whitespace before opening brace.-public class AdminWebConsoleTutorialTest extends AbstractGettingStartedTutorialTest{ +public class AdminWebConsoleTutorialTest extends AbstractGettingStartedTutorialTest {distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiTutorialTest.java (1)
58-63: Remove trailing empty lines.Multiple trailing empty lines are present at the end of the file. Please remove them to maintain consistent code style.
Apply this diff to remove the trailing lines:
} - - - - - - }distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java (1)
45-45: Address the TODO comment.The TODO indicates that the example is not working and requires YAML fixes. Consider whether this test should be marked with
@Disabledor@Ignoreuntil the underlying issues are resolved to prevent test failures in CI.Do you want me to help generate an issue to track fixing the YAML configuration and test adjustments?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
distribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/AbstractAdvancedTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/AbstractGettingStartedTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/AdminWebConsoleTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/BasicPathRoutingTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/FirstApiTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/LoggingTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlowTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiValidationTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/SetHeaderTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/ShortCircuitTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/TemplateTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/json/AbstractJsonTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/xml/AbstractXmlTutorialTest.java(1 hunks)distribution/tutorials/getting-started/00-First-API.yaml(1 hunks)distribution/tutorials/getting-started/10-Logging.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- distribution/tutorials/getting-started/10-Logging.yaml
🧰 Additional context used
🧬 Code graph analysis (5)
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/AbstractGettingStartedTutorialTest.java (1)
distribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.java (1)
AbstractMembraneTutorialTest(9-27)
distribution/src/test/java/com/predic8/membrane/tutorials/xml/AbstractXmlTutorialTest.java (1)
distribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.java (1)
AbstractMembraneTutorialTest(9-27)
distribution/src/test/java/com/predic8/membrane/tutorials/json/AbstractJsonTutorialTest.java (1)
distribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.java (1)
AbstractMembraneTutorialTest(9-27)
distribution/src/test/java/com/predic8/membrane/tutorials/advanced/AbstractAdvancedTutorialTest.java (1)
distribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.java (1)
AbstractMembraneTutorialTest(9-27)
distribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.java (2)
distribution/src/test/java/com/predic8/membrane/examples/util/AbstractSampleMembraneStartStopTestcase.java (1)
AbstractSampleMembraneStartStopTestcase(21-35)distribution/src/test/java/com/predic8/membrane/examples/util/Process2.java (1)
Process2(38-256)
⏰ 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 (19)
distribution/tutorials/getting-started/00-First-API.yaml (1)
14-14: Documentation fix confirmed: Windows launcher command is correct.The update from
membrane.battomembrane.cmdis accurate. All launcher scripts across tutorial directories (advanced, getting-started, json, rest, soap, xml) consistently use the.cmdextension. The documentation now correctly reflects the actual launcher naming convention used in the repository.distribution/src/test/java/com/predic8/membrane/tutorials/xml/AbstractXmlTutorialTest.java (1)
1-12: LGTM!The abstract base class correctly extends the tutorial test framework and overrides
getTutorialDir()to return the XML tutorial directory. The implementation follows the established pattern used by other tutorial category bases.distribution/src/test/java/com/predic8/membrane/tutorials/advanced/AbstractAdvancedTutorialTest.java (1)
1-12: LGTM!The abstract base class correctly extends the tutorial test framework and overrides
getTutorialDir()to return the advanced tutorial directory. The implementation follows the established pattern consistently.distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/FirstApiTutorialTest.java (1)
1-27: LGTM!The test class correctly extends the getting-started tutorial base, specifies the appropriate YAML configuration, and performs a straightforward validation of the API showcase endpoint. The test expectations align with the tutorial objectives.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/AbstractGettingStartedTutorialTest.java (1)
1-12: LGTM!The abstract base class correctly extends the tutorial test framework and overrides
getTutorialDir()to return the getting-started tutorial directory. Provides a clean base for the multiple getting-started tutorial tests.distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/TemplateTutorialTest.java (1)
10-28: LGTM!The test correctly validates the template functionality by checking HTTP status, content type, and the presence of expected template variables (method, path, date) in the response.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiValidationTutorialTest.java (1)
12-33: LGTM!The test correctly validates OpenAPI validation behavior by posting a product with an invalid negative price and asserting that the response returns a 400 status with a structured validation error payload.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/ShortCircuitTutorialTest.java (1)
10-38: LGTM!The tests correctly validate the short-circuit functionality with two scenarios: basic status code verification and request body echo validation. Both tests appropriately verify the expected behavior.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/AdminWebConsoleTutorialTest.java (1)
10-26: LGTM!The test correctly validates admin console accessibility using basic authentication and verifies that HTML content is returned. The hardcoded credentials are appropriate for this test context.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/SetHeaderTutorialTest.java (1)
15-38: Nice coverage of both HTTP verbs.Validating both GET and POST ensures the tutorial stays honest about header handling. Nicely done.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/LoggingTutorialTest.java (1)
25-41: Solid log capture.The capture/restore pattern for System.out keeps the test self-contained while verifying the expected log markers. Looks good.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlowTutorialTest.java (1)
25-41: Log assertions line up.The flow log expectations match what the tutorial demonstrates and the capture logic mirrors the logging test safely. All good.
distribution/src/test/java/com/predic8/membrane/tutorials/json/AbstractJsonTutorialTest.java (1)
7-10: Consistent directory override.Matching the directory scheme used by the other tutorial bases keeps the hierarchy predictable. Looks good.
distribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.java (1)
20-24: Shared startup hook looks solid.Centralizing the script launch and passing
-c <yaml>here keeps the concrete tutorial tests lean while ensuring Membrane waits until it’s ready. Nicely structured.distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiTutorialTest.java (4)
1-15: LGTM!The test structure and imports are appropriate for an OpenAPI tutorial test.
17-28: LGTM!The test correctly verifies that the OpenAPI documentation endpoint returns the expected content.
30-42: LGTM!The test correctly validates the products endpoint response structure and content.
44-56: LGTM!The test correctly validates the DLP fields endpoint response structure.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java (1)
1-17: LGTM!The test structure and imports are appropriate, including the console capture utilities.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
distribution/tutorials/getting-started/10-Logging.yaml (1)
5-7: Suggest improved wording for documentation clarity.The phrasing "Engaged in the message flow" is slightly awkward. Consider rephrasing for better clarity:
-# Logging is helpful during API development, debugging and operation. -# Engaged in the message flow, the plugin logs the request and -# the response. +# Logging is helpful during API development, debugging and operation. +# Integrated into the message flow, the plugin logs the request and +# the response.Alternatively: "As part of the message flow" or "Operating within the message flow" would also read more naturally.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiTutorialTest.java (2)
24-24: Minor formatting: Add space before opening brace.There's a missing space between
AbstractGettingStartedTutorialTestand{.Apply this diff:
-public class OpenApiTutorialTest extends AbstractGettingStartedTutorialTest{ +public class OpenApiTutorialTest extends AbstractGettingStartedTutorialTest {
72-77: Clean up excessive trailing empty lines.Six trailing empty lines are more than typical Java conventions. Consider reducing to a single empty line at the end of the file.
distribution/router/conf/openapi/dlp-v1.0.0.oas.yml (2)
56-62: Consider adding array size limits for security.The response schema returns an array of FieldInfo objects without a maximum size limit. For a DLP (Data Loss Prevention) API, unbounded arrays could pose a security risk or enable DoS attacks if a client requests classification for an extremely large number of fields.
Consider adding a
maxItemsconstraint:"200": description: "Classification info for each requested field" content: application/json: schema: type: "array" + maxItems: 100 items: $ref: "#/components/schemas/FieldInfo"Based on static analysis hints.
69-76: Consider adding array size limits for input validation.The
fieldNamesarray in the request body lacks a maximum size constraint, which could allow clients to submit an unbounded number of field names for classification.Consider adding a
maxItemsconstraint:fieldNames: type: "array" + maxItems: 100 example: - "email" - "card_number" - "last_name" items: type: "string"distribution/router/conf/openapi/fruitshop-v2-2-0.oas.yml (1)
871-876: Consider adding a maximum item limit for orders.The
itemsarray in theOrderInputschema lacks amaxItemsconstraint. For an e-commerce API, orders should typically have a reasonable upper limit on the number of items to prevent abuse and ensure system stability.Consider adding a
maxItemsconstraint:items: type: "array" + maxItems: 100 items: $ref: "#/components/schemas/Item"Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/BooleanValidatorTest.java(1 hunks)distribution/router/README.txt(1 hunks)distribution/router/conf/README.md(1 hunks)distribution/router/conf/acl.xml(0 hunks)distribution/router/conf/apis.yaml(1 hunks)distribution/router/conf/fruitshop-api.yml(0 hunks)distribution/router/conf/generate-ssl-keys.bat(0 hunks)distribution/router/conf/generate-ssl-keys.sh(0 hunks)distribution/router/conf/openapi/dlp-v1.0.0.oas.yml(1 hunks)distribution/router/conf/openapi/fruitshop-v2-2-0.oas.yml(1 hunks)distribution/router/conf/proxies-full-sample.xml(0 hunks)distribution/router/conf/proxies-offline.xml(0 hunks)distribution/router/conf/proxies-security.xml(0 hunks)distribution/router/conf/proxies-soap.xml(0 hunks)distribution/router/conf/proxies.xml(6 hunks)distribution/router/conf/proxies.yaml(0 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/TutorialSoapExampleTest.java(0 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/config/GettingStartedExampleTest.java(0 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/config/ProxiesXMLExampleTest.java(2 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/config/ProxiesXMLFullExampleTest.java(0 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/config/ProxiesXMLSoapExampleTest.java(0 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/rest/TutorialRestInitialExampleTest.java(0 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/rest/TutorialRestStepsExampleTest.java(0 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/soap/TutorialSoapExampleTest.java(0 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/config/ProxiesXMLOfflineExampleTest.java(0 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/config/ProxiesXMLSecurityExampleTest.java(0 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/AbstractAdvancedTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/AbstractGettingStartedTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/AdminWebConsoleTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/BasicPathRoutingTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/FirstApiTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/LoggingTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlowTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiValidationTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/SetHeaderTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/ShortCircuitTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/TemplateTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/json/AbstractJsonTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/xml/AbstractXmlTutorialTest.java(1 hunks)distribution/tutorials/getting-started/10-Logging.yaml(1 hunks)distribution/tutorials/getting-started/20-Message-Flow.yaml(1 hunks)distribution/tutorials/rest/README.md(0 hunks)distribution/tutorials/rest/membrane.sh(0 hunks)distribution/tutorials/rest/proxies.xml(0 hunks)distribution/tutorials/soap/README.md(0 hunks)distribution/tutorials/soap/get2soap.xsl(0 hunks)distribution/tutorials/soap/membrane.sh(0 hunks)distribution/tutorials/soap/proxies.xml(0 hunks)distribution/tutorials/soap/strip-env.xsl(0 hunks)
💤 Files with no reviewable changes (26)
- distribution/tutorials/rest/membrane.sh
- distribution/router/conf/acl.xml
- distribution/router/conf/generate-ssl-keys.sh
- distribution/router/conf/proxies-security.xml
- distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/config/ProxiesXMLOfflineExampleTest.java
- distribution/src/test/java/com/predic8/membrane/examples/withinternet/rest/TutorialRestStepsExampleTest.java
- distribution/src/test/java/com/predic8/membrane/examples/withinternet/config/ProxiesXMLSoapExampleTest.java
- distribution/tutorials/soap/README.md
- distribution/router/conf/proxies.yaml
- distribution/src/test/java/com/predic8/membrane/examples/withinternet/rest/TutorialRestInitialExampleTest.java
- distribution/src/test/java/com/predic8/membrane/examples/withinternet/TutorialSoapExampleTest.java
- distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/soap/TutorialSoapExampleTest.java
- distribution/tutorials/soap/proxies.xml
- distribution/router/conf/fruitshop-api.yml
- distribution/router/conf/proxies-soap.xml
- distribution/tutorials/soap/membrane.sh
- distribution/tutorials/soap/strip-env.xsl
- distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/config/ProxiesXMLSecurityExampleTest.java
- distribution/router/conf/proxies-full-sample.xml
- distribution/router/conf/proxies-offline.xml
- distribution/tutorials/rest/proxies.xml
- distribution/tutorials/rest/README.md
- distribution/router/conf/generate-ssl-keys.bat
- distribution/src/test/java/com/predic8/membrane/examples/withinternet/config/GettingStartedExampleTest.java
- distribution/src/test/java/com/predic8/membrane/examples/withinternet/config/ProxiesXMLFullExampleTest.java
- distribution/tutorials/soap/get2soap.xsl
✅ Files skipped from review due to trivial changes (1)
- distribution/tutorials/getting-started/20-Message-Flow.yaml
🚧 Files skipped from review as they are similar to previous changes (13)
- distribution/src/test/java/com/predic8/membrane/tutorials/xml/AbstractXmlTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/SetHeaderTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiValidationTutorialTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/BooleanValidatorTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/AbstractGettingStartedTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/advanced/AbstractAdvancedTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/ShortCircuitTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/LoggingTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/AdminWebConsoleTutorialTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java
- distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/TemplateTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T13:18:09.463Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90
Timestamp: 2025-08-22T13:18:09.463Z
Learning: The Membrane API Gateway project has mixed Java version configurations in different modules, but the team uses Java 21 in practice. Modern Java 21 features like List.getFirst() are appropriate and should be used instead of older alternatives like List.get(0).
Applied to files:
distribution/router/README.txt
📚 Learning: 2025-08-22T13:18:09.463Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90
Timestamp: 2025-08-22T13:18:09.463Z
Learning: The Membrane API Gateway project uses Java 21, so modern Java 21 features like List.getFirst() are valid and should be used instead of the older get(0) approach.
Applied to files:
distribution/router/README.txt
🧬 Code graph analysis (1)
distribution/src/test/java/com/predic8/membrane/tutorials/json/AbstractJsonTutorialTest.java (1)
distribution/src/test/java/com/predic8/membrane/tutorials/AbstractMembraneTutorialTest.java (1)
AbstractMembraneTutorialTest(23-41)
🪛 Checkov (3.2.334)
distribution/router/conf/openapi/fruitshop-v2-2-0.oas.yml
[medium] 873-877: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
distribution/router/conf/openapi/dlp-v1.0.0.oas.yml
[medium] 56-60: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
⏰ 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 (15)
distribution/tutorials/getting-started/10-Logging.yaml (1)
14-14: Windows command change is accurate.The update from
membrane.battomembrane.cmdis correct;.cmdis the standard extension for Windows batch files and provides better clarity in the documentation.distribution/src/test/java/com/predic8/membrane/tutorials/json/AbstractJsonTutorialTest.java (1)
1-26: LGTM! Clean implementation following the established pattern.This abstract base class correctly implements the template method pattern by providing a JSON-specific tutorial directory while leaving
getTutorialYaml()for concrete subclasses. The implementation mirrors the existing XML tutorial test base and integrates cleanly with the tutorial test infrastructure.distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/OpenApiTutorialTest.java (1)
31-70: LGTM! Well-structured tutorial tests.All three test methods follow REST-assured best practices and include appropriate assertions for smoke testing the OpenAPI tutorial. The use of
containsString,notNullValue, andgreaterThanmatchers is appropriate for validating endpoint availability and response structure.distribution/src/test/java/com/predic8/membrane/examples/withinternet/config/ProxiesXMLExampleTest.java (2)
41-46: LGTM! Test expectations updated consistently.The test assertions have been updated to reflect the new API naming scheme (fruit-shop-api-v2-2-0, OpenAPI 3.0.3, version 2.2.0) and updated endpoint paths. The changes are consistent across all assertions.
55-55: LGTM! Endpoint URL updated correctly.The endpoint URL has been updated to match the new API path structure.
distribution/router/README.txt (2)
6-11: LGTM! Launcher instructions simplified.The unified launcher commands (membrane.sh and membrane.cmd) provide a cleaner user experience.
26-39: LGTM! Documentation and support sections improved.The updated documentation links and support information are clear and helpful.
distribution/router/conf/proxies.xml (2)
4-6: LGTM! Helpful guidance for YAML adoption.The comments provide clear guidance about when to use YAML vs XML configuration, which will help users make informed decisions.
30-30: LGTM! OpenAPI location updated correctly.The OpenAPI spec location has been updated to reference the new v2-2-0 specification file, consistent with other changes in this PR.
distribution/router/conf/openapi/fruitshop-v2-2-0.oas.yml (1)
1-1539: LGTM! Comprehensive and well-structured OpenAPI specification.This is a thorough OpenAPI 3.0.3 specification for the Fruit Shop API with:
- Clear resource organization (Products, Vendors, Orders, Customers)
- Proper CRUD operations with appropriate HTTP methods
- Well-defined schemas with validation constraints
- Comprehensive response definitions
- Reusable components (parameters, request bodies, responses)
- State machine support for order workflows
- HATEOAS-style links for resource navigation
The specification demonstrates best practices for REST API design.
distribution/router/conf/apis.yaml (1)
46-50: Note: Hardcoded credentials are present.The Admin Console uses hardcoded credentials (admin/admin) for Basic Authentication. While this is acceptable for a tutorial/sample configuration (as indicated by the comments), ensure these are changed in production deployments.
The configuration comments suggest this is for demonstration purposes, which is appropriate. Users should be reminded in documentation to secure the admin console properly for production use.
distribution/router/conf/README.md (1)
1-8: LGTM! Clear and concise configuration guidance.The README provides helpful information about:
- Configuration file locations and purpose
- Basic start commands for the gateway
- The expectation that users will customize the configuration
The guidance is clear and appropriate for users setting up Membrane.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/FirstApiTutorialTest.java (1)
1-41: LGTM! Clean and straightforward test implementation.The test follows the established tutorial testing pattern, with appropriate assertions for the tutorial's expected behavior.
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/BasicPathRoutingTutorialTest.java (2)
32-46: LGTM! Comprehensive product endpoint validation.The test properly validates the product API response structure and data constraints.
48-59: LGTM! Appropriate validation for the cat fact endpoint.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
distribution/src/test/java/com/predic8/membrane/tutorials/advanced/PathParametersTutorialTest.java (1)
8-8: Minor formatting: add space before opening brace.The class declaration is missing a space before the opening brace.
Apply this diff:
-public class PathParametersTutorialTest extends AbstractAdvancedTutorialTest{ +public class PathParametersTutorialTest extends AbstractAdvancedTutorialTest {distribution/src/test/java/com/predic8/membrane/tutorials/advanced/PathRewritingTutorialTest.java (1)
21-47: LGTM: Console output capture is well-handled.The test properly restores
System.outin the finally block and prints the console output for debugging. The dual validation of HTTP response and log messages ensures both functional and observability aspects work correctly.Optional: Consider extracting console capture pattern.
The console output capture pattern (lines 23-26, 38-40) is duplicated in
IfTutorialTest. For improved maintainability, consider extracting this into a helper method in the baseAbstractAdvancedTutorialTestclass, such as:protected String captureConsoleOutput(Runnable testAction) { ByteArrayOutputStream out = new ByteArrayOutputStream(); PrintStream original = System.out; System.setOut(new PrintStream(out)); try { testAction.run(); } finally { System.setOut(original); } String console = out.toString(); System.out.println(console); return console; }distribution/src/test/java/com/predic8/membrane/tutorials/advanced/IfTutorialTest.java (1)
32-55: LGTM: Console capture properly handled.The test correctly restores
System.outin the finally block and validates the log output.Optional: Add debug output for easier troubleshooting.
Consider printing the console output after line 52 (similar to
PathRewritingTutorialTestline 43) to aid debugging when assertions fail:String console = out.toString(); +System.out.println(console); assertTrue(console.contains("INFO LogInterceptor"));distribution/tutorials/xml/20-XPath.yaml (1)
1-53: Add trailing newline at end of file.The file is missing a trailing newline. This is a minor formatting convention that ensures consistency across the codebase.
- return: statusCode: 200 contentType: application/xml +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
distribution/src/test/java/com/predic8/membrane/tutorials/advanced/IfTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/PathParameterRoutingTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/PathParametersTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/PathRewritingTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/RedirectsTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/ScriptingGroovyTutorialTest.java(1 hunks)distribution/tutorials/advanced/10-PathParameters.yaml(1 hunks)distribution/tutorials/advanced/20-Path-Parameter-Routing.yaml(1 hunks)distribution/tutorials/advanced/30-Path-Rewriting.yaml(1 hunks)distribution/tutorials/advanced/50-Redirects.yaml(2 hunks)distribution/tutorials/advanced/60-if.yaml(2 hunks)distribution/tutorials/advanced/70-Scripting-Groovy.yaml(1 hunks)distribution/tutorials/getting-started/00-First-API.yaml(2 hunks)distribution/tutorials/getting-started/10-Logging.yaml(1 hunks)distribution/tutorials/getting-started/20-Message-Flow.yaml(2 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/45-Admin-Web-Console.yaml(1 hunks)distribution/tutorials/getting-started/50-Short-Circuit.yaml(2 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)distribution/tutorials/json/20-JSONPath.yaml(1 hunks)distribution/tutorials/json/60-Json-Transformation.yaml(1 hunks)distribution/tutorials/xml/10-JSON-to-XML.yaml(1 hunks)distribution/tutorials/xml/15-XML-to-JSON.yaml(1 hunks)distribution/tutorials/xml/20-XPath.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- distribution/tutorials/getting-started/70-Template.yaml
- distribution/tutorials/getting-started/30-Message-Flow2.yaml
- distribution/tutorials/advanced/20-Path-Parameter-Routing.yaml
- distribution/tutorials/advanced/10-PathParameters.yaml
- distribution/tutorials/getting-started/90-OpenAPI-Validation.yaml
- distribution/tutorials/getting-started/40-Basic-Path-Routing.yaml
- distribution/tutorials/getting-started/60-SetHeader.yaml
- distribution/tutorials/getting-started/80-OpenAPI.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- distribution/tutorials/json/60-Json-Transformation.yaml
- distribution/tutorials/xml/10-JSON-to-XML.yaml
- distribution/tutorials/getting-started/45-Admin-Web-Console.yaml
- distribution/tutorials/getting-started/10-Logging.yaml
- distribution/tutorials/xml/15-XML-to-JSON.yaml
- distribution/tutorials/advanced/50-Redirects.yaml
- distribution/tutorials/json/20-JSONPath.yaml
- distribution/tutorials/getting-started/20-Message-Flow.yaml
- distribution/tutorials/getting-started/00-First-API.yaml
- distribution/tutorials/advanced/60-if.yaml
- distribution/tutorials/advanced/70-Scripting-Groovy.yaml
⏰ 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 (17)
distribution/tutorials/getting-started/50-Short-Circuit.yaml (4)
1-1: Schema URL migrated to external Membrane API schema.The schema URL has been updated from a local path to the external endpoint
https://www.membrane-api.io/v6.3.11.json, consistent with the broader PR migration pattern documented in the AI summary and other tutorials in this PR.
6-6: Minor documentation improvements enhance clarity.The simplifications ("short-circuit and return a response immediately" and "Observe the status code") tighten the language and improve tutorial readability without altering meaning.
Also applies to: 16-16
18-22: New instructions guide practical testing of reversed-flow behavior.The addition of instructions to send a request with a body and observe the response body clarifies the tutorial's intent and helps users understand the reversed-flow feature in action. This improves the learning experience.
27-28: Clarify the semantics of the reversed-flow behavior and status code choice.Two related updates here: (1) added comments explaining that the
returnaction reverses flow and copies the request body if no response exists, and (2) the status code has changed from 503 to 200.Semantically, a 200 response is more appropriate than 503 (Service Unavailable) for a successful short-circuit operation. However, confirm that the reversed-flow behavior is correctly described and that the tutorial's intent is to demonstrate this feature rather than simulating a backend outage.
Also applies to: 30-30
distribution/src/test/java/com/predic8/membrane/tutorials/advanced/PathParametersTutorialTest.java (2)
10-13: LGTM!The method correctly overrides
getTutorialYaml()to specify the tutorial configuration file.
15-26: LGTM!The test method correctly validates path parameter extraction using RestAssured's fluent API. The assertions verify both the HTTP status and the presence of expected parameter values in the response body.
distribution/src/test/java/com/predic8/membrane/tutorials/advanced/ScriptingGroovyTutorialTest.java (3)
1-12: LGTM!The package declaration and imports are appropriate for a RestAssured-based test class.
14-19: LGTM!The class properly extends the abstract tutorial test and correctly specifies the tutorial YAML file.
55-67: LGTM!The test provides comprehensive assertions covering status code, content type, headers, and body properties. The endpoint is preserved in the tutorial YAML.
distribution/tutorials/advanced/30-Path-Rewriting.yaml (1)
1-1: LGTM: Schema reference updated for better IDE support.The change from a local schema path to an online schema URL improves IDE autocomplete and validation capabilities while maintaining version specificity.
distribution/src/test/java/com/predic8/membrane/tutorials/advanced/RedirectsTutorialTest.java (1)
9-27: LGTM: Well-structured redirect test.The test correctly disables redirect following and validates both the 301 status code and the Location header value. The implementation follows the established tutorial test pattern.
distribution/src/test/java/com/predic8/membrane/tutorials/advanced/IfTutorialTest.java (2)
20-30: LGTM: Clear error handling test.The test appropriately verifies that missing required headers result in a 400 status with a descriptive error message.
57-68: LGTM: Happy path test is well-structured.The test appropriately verifies successful routing behavior when the required header is present.
distribution/src/test/java/com/predic8/membrane/tutorials/advanced/PathParameterRoutingTutorialTest.java (1)
17-29: LGTM: Clean path parameter routing test.The test appropriately verifies that path parameters are correctly extracted and routed, with comprehensive assertions on the response structure.
distribution/tutorials/xml/20-XPath.yaml (3)
7-11: Data file verified—no action required.The verification confirms that
distribution/tutorials/data/animals.xmlexists and contains properly formatted XML content with valid animal data records. The referenced file is correctly placed and accessible for the tutorial.
1-1: Schema URL verified and file structure is correct.The schema URL
https://www.membrane-api.io/v6.3.11.jsonis accessible and returns HTTP 200. The flow structure correctly follows Membrane's execution semantics: request steps execute first (setting properties), then the response template uses those properties—YAML declaration order does not affect this. The referenced data fileanimals.xmlexists at the expected location.Minor note: The file is missing a trailing newline at the end, which is a formatting preference rather than a functional issue.
18-52: No changes needed—flow execution order is correct.The concern about property availability is based on a misunderstanding of Membrane's execution model. Membrane separates flow execution into REQUEST and RESPONSE phases: request handlers run sequentially until RETURN is encountered, then the interceptor stack is unwound calling response handlers in reverse order. Both phases operate on the same Exchange object, so properties set during the request phase (via
setPropertysteps) remain available when response handlers execute. The YAML declaration order is irrelevant—ResponseInterceptor executes during the response phase, and RequestInterceptor executes during the request phase, regardless of their order in the flow definition. The 20-XPath.yaml file follows correct Membrane patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java (1)
55-57: Correct the assertion logic to actually verify three occurrences.The repeated
assertTrue(console.contains("TODO"))assertions do not verify that "TODO" appears three times. Each assertion only checks if "TODO" exists at least once, so all three will pass even if "TODO" appears only once.This issue was previously identified in an earlier review.
🧹 Nitpick comments (1)
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java (1)
60-60: Address the TODO comment about YAML issues.The TODO comment indicates the example is not working due to YAML issues. This suggests the test may not be functioning correctly or is incomplete.
Please verify the current status of the YAML file (30-Message-Flow2.yaml) and whether the test is expected to pass. Would you like me to help identify potential issues in the YAML configuration or generate a script to validate the test behavior?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
distribution/src/test/java/com/predic8/membrane/examples/ExampleTests.java(2 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/IfTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/PathRewritingTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/RedirectsTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/advanced/ScriptingGroovyTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/LoggingTutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlowTutorialTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- distribution/src/test/java/com/predic8/membrane/tutorials/advanced/RedirectsTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/advanced/IfTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/advanced/ScriptingGroovyTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/advanced/PathRewritingTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlowTutorialTest.java
⏰ 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 (5)
distribution/src/test/java/com/predic8/membrane/examples/ExampleTests.java (2)
17-17: LGTM!The import is correctly added and properly used in the annotation below.
26-26: LGTM - verification confirms the package contains test classes.The
@SelectPackagesannotation correctly references the tutorials package, which exists and contains 22+ test classes across multiple subdirectories (advanced, getting_started, json, xml). Combining it with@SelectClassesis a valid JUnit 5 pattern.distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/LoggingTutorialTest.java (3)
26-31: LGTM!The test class structure is clean and follows the expected pattern for tutorial tests.
54-56: No issues found. The spacing is correct and intentional.The format string at line 97 in
LogInterceptor.javaexplicitly uses"==== %s %s ==="where the two%splaceholders are separated by a space, and a space precedes the closing===. Withflowset to REQUEST/RESPONSE andlabelas an empty string (the default), this produces exactly"==== REQUEST ==="and"==== RESPONSE ===", matching what the test assertions verify.
35-57: The parallel execution concern does not apply to this codebase.Parallel execution is explicitly disabled in
core/src/test/resources/junit-platform.properties—alljunit.jupiter.execution.parallel.*settings are commented out. Tests run sequentially by default, so the System.out capture approach does not have the thread-safety issues described in the review comment.While the
synchronized(System.out)pattern could be fragile if parallel execution were enabled in the future, it is currently safe and is used consistently across tutorial tests in this codebase as an established pattern for verifying log output.Likely an incorrect or invalid review comment.
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/tutorials/getting-started/30-Message-Flow2.yaml (1)
17-17: Update comment to reflect log message changes.The comment references "numbers 1 to 6 appear in the console," but the log messages now output descriptive steps ("Step 1" through "Step 6"). Update the documentation to be consistent with the new output.
Apply this diff:
- Observe in which order the numbers 1 to 6 appear in the console. + Observe in which order the steps 1 to 6 appear in the console.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
distribution/tutorials/getting-started/30-Message-Flow2.yaml(2 hunks)
⏰ 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 (2)
distribution/tutorials/getting-started/30-Message-Flow2.yaml (2)
1-1: Schema URL updated to online reference.The yaml-language-server schema has been switched from a local relative path to an absolute online URL. This aligns with the tutorial modernization goals.
32-43: Log messages updated to be more descriptive.The request and response flow log messages have been changed from numeric identifiers to descriptive step labels. This improves readability and makes the tutorial output more self-explanatory.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java (1)
53-67: Consider adding assertion messages for clearer failure diagnostics.The assertions correctly verify both the presence and order of step markers. The presence checks (lines 54-59) ensure that indexOf won't return -1 in the ordering checks (lines 61-65), which is good defensive practice.
However, adding descriptive messages to the ordering assertions would improve debugging when tests fail:
- assertTrue(console.indexOf("Step 1") < console.indexOf("Step 2 Path")); + assertTrue(console.indexOf("Step 1") < console.indexOf("Step 2 Path"), + "Step 1 should appear before Step 2 Path in console output");Apply similar messages to lines 62-65 for consistency.
Note: The past review comment about "TODO" assertions appears to reference an older version of this code and is no longer applicable to the current implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java(1 hunks)
⏰ 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 (3)
distribution/src/test/java/com/predic8/membrane/tutorials/getting_started/MessageFlow2TutorialTest.java (3)
1-13: LGTM: Proper Apache 2.0 license header.The license header is correctly formatted and consistent with the project's licensing requirements.
26-31: LGTM: Clean test structure following established pattern.The class correctly extends
AbstractGettingStartedTutorialTestand overridesgetTutorialYaml()to specify the tutorial configuration file.
33-51: LGTM: Proper stdout capture with thread safety.The test correctly:
- Synchronizes on
System.outto prevent concurrent access issues- Captures stdout for log verification
- Ensures cleanup via finally block
- Uses RestAssured appropriately for HTTP testing
Refactor initRouterByXml/initRouterByYAML to accept a config path. Add getDefaultConfig/tryConfig to locate apis.yaml | apis.yml (preferred) then proxies.xml in MEMBRANE_CALLER_DIR (or user dir) and MEMBRANE_HOME/conf. Print an error and exit if no config is found. Remove redundant default selection logic from start_router.sh and start_router.cmd so the CLI handles configuration discovery.
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)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
90-98: Fix dryRun() to handle YAML configs, remove unused ResolverMap parameter, and remove dead getErrorNotice() method.All three concerns are confirmed:
dryRun() behavioral issue (lines 88-98):
getRulesFile()now returns.yaml/.ymlfiles fromgetDefaultConfig()(prioritized first), butdryRun()always loads viaTrackingFileSystemXmlApplicationContext, which will fail on YAML. Implement the branching fix to detect and reject YAML or add proper YAML validation support.Unused ResolverMap parameter (line 225):
getDefaultConfig(ResolverMap rm)never uses thermparameter. Remove it and constructResolverMaponly where needed (ingetRulesFile()before thehasConfiguration()branch, or ingetRulesFileFromRelativeSpec()which actually uses it).Dead code (lines 277-285):
getErrorNotice()is never called.getRulesFileFromRelativeSpec()receives an empty string""at line 221, and the method definition is orphaned.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (2)
132-141: Extension-based dispatch for XML vs YAML looks good; consider normalizing the extension.Routing via
config.endsWith(".xml")vs.yaml/.ymlis clear and keeps concerns separated. If you expect configs with mixed‑case extensions (e.g.proxies.XML,apis.YAML), you might want to normalize once and check against the lower‑case form, and also include the filename in the error for easier debugging.Example refactor:
- private static Router initRouterByConfig(MembraneCommandLine commandLine) throws Exception { - String config = getRulesFile(commandLine); - if(config.endsWith(".xml")) { - return initRouterByXml(config); - } - if (config.endsWith(".yaml") || config.endsWith(".yml")) { - return initRouterByYAML(config); - } - throw new RuntimeException("Unsupported file extension."); - } + private static Router initRouterByConfig(MembraneCommandLine commandLine) throws Exception { + String config = getRulesFile(commandLine); + String lower = config.toLowerCase(Locale.ROOT); + if (lower.endsWith(".xml")) { + return initRouterByXml(config); + } + if (lower.endsWith(".yaml") || lower.endsWith(".yml")) { + return initRouterByYAML(config); + } + throw new RuntimeException("Unsupported config file extension: " + config); + }
150-166: New YAML initializer overload nicely centralizes logic; consider guardinglocation.The delegation from
initRouterByYAML(MembraneCommandLine, String)toinitRouterByYAML(String)reduces duplication and gives you a single YAML initialization path, which is good. To avoid surprising NPEs if the CLI option is missing or empty (or if a future caller passes an invalid value), you could add a simple validation before starting the router.For example:
private static Router initRouterByYAML(MembraneCommandLine commandLine, String option) throws Exception { String location = commandLine.getCommand().getOptionValue(option); - return initRouterByYAML(location); + if (location == null || location.isEmpty()) { + throw new IllegalArgumentException("YAML location (-" + option + ") must be provided."); + } + return initRouterByYAML(location); } private static Router initRouterByYAML(String location) throws Exception { + if (location == null || location.isEmpty()) { + throw new IllegalArgumentException("YAML location must not be null or empty."); + } var router = new HttpRouter(); router.setBaseLocation(location); router.setHotDeploy(false); router.setAsynchronousInitialization(true); router.start(); // ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java(4 hunks)distribution/scripts/start_router.cmd(1 hunks)distribution/scripts/start_router.sh(0 hunks)
💤 Files with no reviewable changes (1)
- distribution/scripts/start_router.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- distribution/scripts/start_router.cmd
⏰ 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/cli/RouterCLI.java (1)
186-193: XML init viaRouter.init(config)is clearer; behavior change seems intentional.Switching
initRouterByXmlto accept aconfigstring and delegate toRouter.init(config)simplifies the flow and keeps config resolution in one place. Exception handling remains consistent (specificXmlBeanDefinitionStoreExceptionmapped tohandleXmlBeanDefinitionStoreException, everything else bubbles up togetRouterandSpringConfigurationErrorHandler).As this changes the initialization entry point, it’s worth double‑checking that
Router.init(config)preserves any previous side effects (base location, hot-deploy behavior, async init) expected when starting from XML.
|
The new priority for |
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-22T19:49:29.473Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2156
File: pom.xml:0-0
Timestamp: 2025-09-22T19:49:29.473Z
Learning: In the Membrane API Gateway project, jackson-annotations is pinned to version "2.20" instead of using ${jackson.version} (2.20.0) because version 2.20.0 is not available in Maven repository yet for jackson-annotations, while it is available for jackson-core and jackson-databind. This version inconsistency is intentional and necessary due to different release schedules between Jackson modules.
Applied to files:
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java
🧬 Code graph analysis (3)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(16-27)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java (1)
Exchange(31-220)
core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java (1)
core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java (1)
CommonBuiltInFunctions(16-27)
⏰ 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)
🔇 Additional comments (4)
core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java (1)
13-23: LGTM! Clean delegation pattern.The Groovy wrapper correctly delegates to the shared CommonBuiltInFunctions implementation, providing a consistent interface for Groovy templates.
core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java (1)
107-109: LGTM! Groovy function binding added correctly.The new "fn" binding enables
${fn.functionname()}syntax in Groovy templates, consistent with the existing parameter binding pattern. The TODO comment appropriately flags the need for test coverage.core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java (2)
110-112: LGTM! Consistent delegation to shared implementation.The new
user()function correctly delegates toCommonBuiltInFunctions.user, maintaining consistency across SpEL and Groovy implementations.
119-131: LGTM! Good refactoring with proper null handling.The extraction of
getSecuritySchemes()improves code organization, and the use ofOptional.ofNullableon line 120 properly handles the case where the property returns null. This demonstrates the correct pattern for null safety.
Add a top-level "kind" discriminator enum (api, bean, serviceProxy, internal), construct separate api and bean SchemaObject variants referencing their respective parsers, and emit them via oneOf. Extend SchemaObject with a oneOf field and JSON serialization to support variant schemas.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java(3 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.javaannot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java
⏰ 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)
🔇 Additional comments (2)
annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.java (1)
29-29: LGTM! Clean implementation of oneOf support.The implementation correctly adds JSON Schema
oneOfcomposition support:
- Proper null-safe serialization at lines 55-61
- Fluent builder pattern at lines 106-109 matches existing code style
- Correctly serializes each schema variant via
schema.json(jnf.objectNode())The changes integrate well with the existing SchemaObject structure and are used appropriately in JsonSchemaGenerator.java to support the multi-variant top-level schema.
Also applies to: 55-61, 106-109
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java (1)
92-106: Add missing schema definitions for ApiParser and BeanParser or fix the references.The code at lines 97 and 105 references
#/$defs/com_predic8_membrane_core_config_spring_ApiParserand#/$defs/com_predic8_membrane_core_config_spring_BeanParser, but these definitions are never generated. TheaddParserDefinitionsmethod only generates definitions for classes annotated with@MCElement. Search of the codebase found no such classes for ApiParser or BeanParser, meaning consumers of this schema will encounter broken$refpointers when validating documents with"kind": "api"or"kind": "bean".Either add the corresponding
@MCElementclasses that should generate these definitions, or update the hard-coded schema references to point to correct (existing) definitions.⛔ Skipped due to learnings
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.
…nator" This reverts commit 54392ce.
# Conflicts: # core/src/main/java/com/predic8/membrane/core/lang/CommonBuiltInFunctions.java # core/src/main/java/com/predic8/membrane/core/lang/ScriptingUtils.java # core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyBuiltInFunctions.java # core/src/main/java/com/predic8/membrane/core/lang/spel/functions/BuiltInFunctions.java # distribution/tutorials/rest/membrane.sh # distribution/tutorials/soap/membrane.sh
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
distribution/src/test/java/com/predic8/membrane/examples/withinternet/config/ProxiesYAMLExampleTest.java (1)
50-56: Assertions for new Fruit Shop API key look correct; consider de-duplicating the ID literalThe expectations for
fruit-shop-api-v2-2-0(OpenAPI3.0.3, title, version, and both links) are consistent, and the size assertions (outer map = 1, inner map = 5) align with the asserted fields, so the behavior looks correct.To improve maintainability, you could assign
"fruit-shop-api-v2-2-0"to a local variable or constant and reuse it across the.body(...)calls to avoid copy‑paste errors if the ID changes again.
📜 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/spel/SpELExchangeExpression.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java(2 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/config/ProxiesYAMLExampleTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/lang/spel/SpELExchangeExpression.java
- core/src/test/java/com/predic8/membrane/core/interceptor/templating/TemplateInterceptorTest.java
⏰ 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)
2 participants
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Documentation