Add support for reference schemas in JSON/YAML schema validation (7.X) #2477
Conversation
* feat: json2xml support for arrays * refactor: minor * refactor: minor * refactor: minor * refactor: minor * refactor: minor * refactor: minor * refactor: minor * refactor: minor * refactor: minor --------- Co-authored-by: Thomas Bayer <bayer@predic8.de>
…tart_router.sh (for 6.X) (#2381)
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: github-actions <github-actions@github.com>
…th reference mappings
WalkthroughAdds schemaMappings support for JSON/YAML validation: new SchemaMappings model, ValidatorInterceptor getters/setters, JSONYAMLSchemaValidator wiring to a SchemaRegistry/SchemaLoader that uses mappings, example configs/scripts/schemas/payloads, and an integration test exercising mapped reference resolution. Changes
Sequence Diagram(s)sequenceDiagram participant Client participant ValidatorInterceptor participant JSONYAMLSchemaValidator participant SchemaRegistry participant Backend Client->>ValidatorInterceptor: send request (JSON/YAML) ValidatorInterceptor->>JSONYAMLSchemaValidator: validate payload (passes schema reference) alt schemaMappings present JSONYAMLSchemaValidator->>SchemaRegistry: create with SchemaLoader + schemaMappings SchemaRegistry->>JSONYAMLSchemaValidator: resolved schema (URN refs mapped to local files) else no mappings JSONYAMLSchemaValidator->>SchemaRegistry: create default registry end JSONYAMLSchemaValidator-->>ValidatorInterceptor: validation result (OK / errors) alt OK ValidatorInterceptor->>Backend: forward request Backend-->>Client: response else Errors ValidatorInterceptor-->>Client: 400 Problem+details end Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (1)
61-61: Consider null safety in setSchemaMappings.The
schemaMappingsfield is initialized to an emptyHashMapon line 61, but the setter on lines 222-224 could acceptnull, potentially causing an NPE whenmappings(schemaMappings)is called on line 105. Consider either:
- Adding a null check in the setter and defaulting to an empty map
- Documenting that null is not allowed
- Handling null explicitly in
init()🔎 Suggested null-safe setter
public void setSchemaMappings(Map<String, String> schemaMappings) { - this.schemaMappings = schemaMappings; + this.schemaMappings = schemaMappings != null ? schemaMappings : new HashMap<>(); }Also applies to: 222-224
core/src/main/java/com/predic8/membrane/core/util/json/JsonToXml.java (1)
37-68: Consider documenting the three conversion cases.The three-case logic (single-property object, top-level array, normal case) is clever and handles different JSON structures appropriately. However, the behavior could be surprising to users unfamiliar with the implementation.
Consider adding JavaDoc comments explaining:
- When case 1 applies and why the single property name becomes the root
- When case 2 applies and why arrayName is used directly without extra nesting
- The default case 3 behavior
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/SchemaMappings.java (1)
18-22: Consider adding validation in getSchemaMap().The current implementation doesn't validate that schema IDs and locations are non-null or that IDs are unique. While this may work if downstream code is defensive, consider adding validation to fail fast with clear error messages:
🔎 Suggested validation
public Map<String, String> getSchemaMap() { Map<String, String > referenceSchemas = new HashMap<>(); - schemas.forEach(schema -> referenceSchemas.put(schema.getId(), schema.getLocation())); + schemas.forEach(schema -> { + if (schema.getId() == null || schema.getLocation() == null) { + throw new IllegalStateException("Schema mapping must have both id and location"); + } + if (referenceSchemas.containsKey(schema.getId())) { + throw new IllegalStateException("Duplicate schema id: " + schema.getId()); + } + referenceSchemas.put(schema.getId(), schema.getLocation()); + }); return referenceSchemas; }core/src/main/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptor.java (2)
52-59: Consider validating configuration values in init().The initialization is clean and follows the proper lifecycle pattern. Consider adding validation for the
arrayanditemconfiguration values to catch invalid XML element names early (e.g., null, empty, or names containing invalid characters) rather than during conversion.Optional: Add configuration validation
@Override public void init() { super.init(); + if (array != null && array.trim().isEmpty()) { + throw new IllegalArgumentException("array name cannot be empty"); + } + if (item != null && item.trim().isEmpty()) { + throw new IllegalArgumentException("item name cannot be empty"); + } converter = new JsonToXml().arrayName(array).itemName(item).rootName(root); // root gets handled dynamically at runtime because it depends on json document type // unless explicitly set via @MCAttribute }
125-149: Minor documentation inconsistency.The getter/setter implementations are correct and the
@MCAttributeannotations properly expose these as configuration options. However, there's a small documentation inconsistency:
- Line 131:
setArray()uses@default "array"tag- Line 144:
setItem()uses plain text "Default is 'item'."Consider using the
@defaulttag consistently for both methods.Optional: Standardize documentation format
/** * Sets the XML tag name used for array items. - * Default is "item". + * @default "item" */ @MCAttribute public void setItem(String item) { this.item = item; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
annot/pom.xml(1 hunks)core/.factorypath(1 hunks)core/pom.xml(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java(4 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java(4 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/SchemaMappings.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptor.java(4 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameterParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/json/JsonToXml.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptorTest.java(5 hunks)core/src/test/java/com/predic8/membrane/core/util/JsonUtilTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/json/JsonToXmlTest.java(1 hunks)distribution/examples/extending-membrane/custom-interceptor/pom.xml(1 hunks)distribution/examples/extending-membrane/embedding-java/pom.xml(1 hunks)distribution/examples/validation/json-schema/schema-mappings/README.md(1 hunks)distribution/examples/validation/json-schema/schema-mappings/bad2000.json(1 hunks)distribution/examples/validation/json-schema/schema-mappings/bad2001.json(1 hunks)distribution/examples/validation/json-schema/schema-mappings/good2000.json(1 hunks)distribution/examples/validation/json-schema/schema-mappings/good2001.json(1 hunks)distribution/examples/validation/json-schema/schema-mappings/membrane.cmd(1 hunks)distribution/examples/validation/json-schema/schema-mappings/membrane.sh(1 hunks)distribution/examples/validation/json-schema/schema-mappings/proxies.xml(1 hunks)distribution/examples/validation/json-schema/schema-mappings/schemas/base-param.json(1 hunks)distribution/examples/validation/json-schema/schema-mappings/schemas/meta.json(1 hunks)distribution/examples/validation/json-schema/schema-mappings/schemas/schema2000.json(1 hunks)distribution/examples/validation/json-schema/schema-mappings/schemas/schema2001.json(1 hunks)distribution/examples/web-services-soap/add-soap-header/pom.xml(1 hunks)distribution/examples/xml/basic-xml-interceptor/pom.xml(1 hunks)distribution/examples/xml/stax-interceptor/pom.xml(1 hunks)distribution/pom.xml(1 hunks)distribution/scripts/start_router.sh(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaMappingsExampleTest.java(1 hunks)maven-plugin/pom.xml(1 hunks)membrane.spec(1 hunks)pom.xml(1 hunks)test/pom.xml(1 hunks)war/.factorypath(1 hunks)war/pom.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common 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. 📚 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/test/java/com/predic8/membrane/core/util/JsonUtilTest.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameterParser.javacore/src/test/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptorTest.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java
📚 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:
distribution/examples/xml/stax-interceptor/pom.xmlcore/.factorypathdistribution/examples/web-services-soap/add-soap-header/pom.xmldistribution/examples/extending-membrane/embedding-java/pom.xmlwar/.factorypathdistribution/examples/extending-membrane/custom-interceptor/pom.xml
📚 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/examples/validation/json-schema/schema-mappings/membrane.cmddistribution/examples/validation/json-schema/schema-mappings/membrane.sh
📚 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:
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/SchemaMappings.javacore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java
📚 Learning: 2025-09-19T11:12:26.787Z
Learnt from: predic8 Repo: membrane/api-gateway PR: 2156 File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131 Timestamp: 2025-09-19T11:12:26.787Z Learning: Request.get().path() in com.predic8.membrane.core.openapi.model.Request preserves the query string portion of the path, so URIFactory.createWithoutException(request.getPath()).getQuery() correctly extracts query parameters. Applied to files:
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java
📚 Learning: 2025-12-19T09:01:40.311Z
Learnt from: rrayst Repo: membrane/api-gateway PR: 2441 File: annot/pom.xml:0-0 Timestamp: 2025-12-19T09:01:40.311Z Learning: In annot/pom.xml, ensure log4j dependencies log4j-slf4j2-impl and log4j-core have <scope>test</scope>. In the distribution module (separate pom.xml), these dependencies should use the normal compile-scope. This guidance applies to all pom.xml files under the annot and distribution module structure where these dependencies appear; verify both modules are configured accordingly to reflect testing vs. runtime usage. Applied to files:
annot/pom.xml
🧬 Code graph analysis (9)
core/src/test/java/com/predic8/membrane/core/util/JsonUtilTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java (1)
JsonUtil(22-78)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/AbstractMessageValidator.java (1)
AbstractMessageValidator(18-40)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameterParser.java (1)
core/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java (1)
JsonUtil(22-78)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaMappingsExampleTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/test/java/com/predic8/membrane/core/util/json/JsonToXmlTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/json/JsonToXml.java (1)
JsonToXml(9-193)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java (1)
core/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java (1)
JsonUtil(22-78)
core/src/main/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/StringUtil.java (1)
StringUtil(19-52)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java (1)
core/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java (1)
JsonUtil(22-78)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java (1)
core/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java (1)
JsonUtil(22-78)
🪛 LanguageTool
distribution/examples/validation/json-schema/schema-mappings/README.md
[locale-violation] ~21-~21: In American English, “take a look” is more commonly used.
Context: ...p with multiple referenced schemas. 1. Have a look at schemas/schema2001.json and note t...
(HAVE_A_LOOK)
🪛 Shellcheck (0.11.0)
distribution/examples/validation/json-schema/schema-mappings/membrane.sh
[warning] 8-8: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
🔇 Additional comments (50)
war/.factorypath (1)
2-2: LGTM! Version bump is correct.The factorypath configuration has been updated to reference the correct 6.3.12-SNAPSHOT version of service-proxy-annot, aligning with the project-wide version bump.
distribution/examples/extending-membrane/custom-interceptor/pom.xml (1)
28-28: LGTM! Dependency version updated correctly.The service-proxy-core dependency has been updated to 6.3.12-SNAPSHOT, consistent with the project-wide version bump.
membrane.spec (1)
7-7: LGTM! Spec version updated correctly.The RPM package version has been updated to 6.3.12-SNAPSHOT, aligning with the project-wide version bump.
core/.factorypath (1)
2-2: LGTM! Factorypath updated correctly.The factorypath configuration has been updated to reference 6.3.12-SNAPSHOT service-proxy-annot, consistent with the version bump.
distribution/examples/extending-membrane/embedding-java/pom.xml (1)
32-32: LGTM! Dependency version updated correctly.The service-proxy-core dependency has been updated to 6.3.12-SNAPSHOT.
distribution/examples/web-services-soap/add-soap-header/pom.xml (1)
21-21: LGTM! Dependency version updated correctly.The service-proxy-core dependency has been updated to 6.3.12-SNAPSHOT.
distribution/examples/xml/basic-xml-interceptor/pom.xml (1)
21-21: LGTM! Dependency version updated correctly.The service-proxy-core dependency has been updated to 6.3.12-SNAPSHOT.
distribution/examples/xml/stax-interceptor/pom.xml (1)
22-22: LGTM! Dependency version updated correctly.The service-proxy-core dependency has been updated to 6.3.12-SNAPSHOT.
core/pom.xml (1)
26-26: LGTM! Version bump is consistent.The parent version update to 6.3.12-SNAPSHOT aligns with the project-wide version increment.
test/pom.xml (1)
26-26: LGTM! Version bump is consistent.The parent version update to 6.3.12-SNAPSHOT aligns with the project-wide version increment.
distribution/pom.xml (1)
30-30: LGTM! Version bump is consistent.The parent version update to 6.3.12-SNAPSHOT aligns with the project-wide version increment.
annot/pom.xml (1)
26-26: LGTM! Version bump is consistent.The parent version update to 6.3.12-SNAPSHOT aligns with the project-wide version increment.
maven-plugin/pom.xml (1)
28-28: LGTM! Version bump is consistent.The parent version update to 6.3.12-SNAPSHOT aligns with the project-wide version increment.
pom.xml (1)
20-20: LGTM! Version bump is consistent.The project version update to 6.3.12-SNAPSHOT is correctly applied at the parent level and consistently referenced by all child modules.
war/pom.xml (1)
26-26: LGTM! Version bump is consistent.The parent version update to 6.3.12-SNAPSHOT aligns with the project-wide version increment.
distribution/scripts/start_router.sh (1)
24-31: This is a new startup script file; the normalization scope is intentionally limited to log4j configuration paths.The script correctly normalizes only
-Dlog4j.configurationFilebecause path normalization applies only to configuration file paths that can be relative. Other Java options (like-Xmx,-Xms,-XX:*flags, and unrelated-Dproperties) don't require path normalization. The comment on line 24 documents this intent. Since this is a new file being added, there is no impact on existing deployments.core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java (1)
27-27: LGTM! Import path updated for JsonUtil package refactoring.The import correctly reflects the relocation of
JsonUtilto thecom.predic8.membrane.core.util.jsonpackage.core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java (1)
19-19: LGTM! Import path updated for JsonUtil package refactoring.The import correctly reflects the relocation of
JsonUtilto thecom.predic8.membrane.core.util.jsonpackage.core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java (1)
26-26: LGTM! Import path updated for JsonUtil package refactoring.The import correctly reflects the relocation of
JsonUtilto thecom.predic8.membrane.core.util.jsonpackage.distribution/examples/validation/json-schema/schema-mappings/good2000.json (1)
1-7: LGTM! Valid test example.The JSON structure and ISO 8601 timestamp format are correct for a positive validation test case.
distribution/examples/validation/json-schema/schema-mappings/schemas/schema2001.json (1)
1-22: LGTM! Well-structured schema with URN-based references.The schema correctly uses JSON Schema draft 2020-12 with URN-based
$refreferences for composing reusable schema definitions (BaseParameter and Meta). The strict validation withadditionalProperties: falseand array constraintminItems: 1appropriately enforce data quality.distribution/examples/validation/json-schema/schema-mappings/bad2001.json (1)
1-12: LGTM! Appropriate invalid test example.The payload correctly includes validation violations for negative testing: the "unexpected" property should trigger schema validation errors, likely against the BaseParameter schema or additionalProperties constraints.
distribution/examples/validation/json-schema/schema-mappings/bad2000.json (1)
1-7: LGTM! Appropriate invalid test example.The payload correctly includes multiple validation violations for negative testing: invalid timestamp format ("not-a-date"), unexpected "extra" property, and potentially incomplete param object structure.
core/src/test/java/com/predic8/membrane/core/util/JsonUtilTest.java (1)
20-20: LGTM! Test import updated for JsonUtil package refactoring.The import correctly reflects the relocation of
JsonUtilto thecom.predic8.membrane.core.util.jsonpackage. Test coverage remains unchanged.distribution/examples/validation/json-schema/schema-mappings/schemas/base-param.json (1)
1-21: LGTM! Well-formed JSON Schema for reusable parameter definition.The schema correctly defines a reusable BaseParameter under
$defswith draft-2020-12. The flexiblevalueproperty (empty schema allowing any type) provides good reusability across different validation scenarios.core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameterParser.java (1)
22-22: LGTM! Import path correctly updated for JsonUtil package relocation.The static import path change aligns with the package relocation of JsonUtil from
com.predic8.membrane.core.utiltocom.predic8.membrane.core.util.json.distribution/examples/validation/json-schema/schema-mappings/schemas/schema2000.json (1)
1-19: LGTM! Schema correctly demonstrates external reference mapping.The schema is well-formed and demonstrates the new schema mappings feature by referencing
urn:app:base_parameter_def#/$defs/BaseParameter, which requires the schema mapping infrastructure introduced in this PR.distribution/examples/validation/json-schema/schema-mappings/membrane.sh (2)
8-8: LGTM! CDPATH reset is intentional, not a syntax error.The
CDPATH=prefix correctly prevents CDPATH environment variable interference when resolving the script directory. The ShellCheck warning SC1007 is a false positive.
10-18: LGTM! Root location logic validates required files.The script correctly validates both
LICENSE.txtandscripts/run-membrane.shexist before settingMEMBRANE_HOME, preventing runtime errors from invalid directory locations.distribution/examples/validation/json-schema/schema-mappings/good2001.json (1)
1-16: LGTM! Well-formed test data for schema validation.The JSON structure is valid and provides appropriate test data for the schema mappings validation example.
distribution/examples/validation/json-schema/schema-mappings/schemas/meta.json (1)
1-24: LGTM! Well-formed schema for metadata validation.The schema correctly defines a reusable Meta object under
$defswith appropriate constraints for source and requestId fields.distribution/examples/validation/json-schema/schema-mappings/membrane.cmd (2)
9-14: LGTM! Root location logic validates required files.The script correctly validates both
LICENSE.txtandscripts\run-membrane.cmdexist before settingMEMBRANE_HOME, mirroring the validation pattern in the Unix counterpartmembrane.sh.
16-20: LGTM! Proper delegation to run-membrane.cmd.The script correctly sets environment variables and delegates to
run-membrane.cmdwith all original arguments, preserving the error level on exit.core/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java (1)
15-15: Package relocation successfully completed.The relocation from
com.predic8.membrane.core.utiltocom.predic8.membrane.core.util.jsonis correct. Verification confirms no remaining references to the old package path exist in the codebase—all dependent imports have been updated.distribution/examples/validation/json-schema/schema-mappings/proxies.xml (1)
9-37: LGTM! Configuration is well-structured.The validator configuration with schema mappings is correctly defined:
- Port 2000 demonstrates single schema reference mapping
- Port 2001 demonstrates multiple schema reference mappings
- Backend properly uses XML entity encoding in the Groovy response string
The progressive complexity from simple to advanced examples aids learning.
core/src/test/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptorTest.java (2)
99-125: LGTM! Comprehensive nested array validation.The nested array test provides excellent coverage of complex JSON structures, including:
- Single-nested arrays
[1]- Double-nested arrays
[[2]]- Triple-nested arrays
[[[3]]]The XPath assertions correctly validate the expected XML structure at each nesting level.
186-193: LGTM! Proper error handling validation.The invalid test now correctly validates both:
- HTTP 500 status code
- Error response structure using JsonPath to parse the problem details
This ensures error responses follow the expected format.
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java (1)
123-125: LGTM! Clear warning for unsupported feature.The helper method appropriately warns users when schema mappings are configured but not supported by the selected validator type (WSDL, XML Schema, or Schematron).
core/src/test/java/com/predic8/membrane/core/util/json/JsonToXmlTest.java (2)
199-227: LGTM! Comprehensive literal parsing tests.The
testParseLiteraltest provides excellent coverage of edge cases:
- Integer and floating-point parsing
- Scientific notation
- Overflow handling (line 212-213)
- Quoted vs unquoted strings
- Non-numeric edge cases like "-" and "." (lines 224-226)
This defensive testing ensures robust handling of various JSON value formats.
37-89: LGTM! Thorough nested structure validation.The
ObjectsAndArraystest class validates complex scenarios:
- Single-nested arrays
- Multiply-nested arrays
- Mixed structures (objects in arrays, arrays in arrays)
The assertions correctly verify the XML path structure at each nesting level, ensuring the conversion handles arbitrary complexity.
core/src/main/java/com/predic8/membrane/core/util/json/JsonToXml.java (2)
78-107: LGTM! Robust literal parsing with overflow handling.The
parseLiteralmethod includes several defensive programming practices:
- Attempts Double parsing first to avoid Long overflow (line 83)
- Falls back to Long only if the value fits (lines 86-92)
- Handles malformed numbers gracefully by treating them as strings (lines 96-98)
- Correctly strips quotes from string literals (lines 103-104)
This prevents data loss from integer overflow while maintaining type precision where possible.
156-167: LGTM! Proper XML name sanitization.The
sanitizeXmlNamemethod correctly:
- Replaces illegal XML characters with underscores (line 159)
- Ensures element names start with a valid character by prefixing underscore if needed (lines 162-164)
This prevents XML parsing errors from unusual JSON keys.
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/SchemaMappings.java (1)
34-57: LGTM! Clean data holder class.The
Schemainner class is properly annotated for Spring configuration with:
@MCElementfor XML element mapping@MCAttributefor XML attribute mappingThe simple getter/setter structure is appropriate for a configuration data holder.
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaMappingsExampleTest.java (3)
26-31: LGTM!The test class setup is clean and correctly extends the distribution test base class. The example directory path construction properly uses the system separator.
78-126: Test data files exist; shell command in original comment is incorrect.The test files
good2001.jsonandbad2001.jsonare present indistribution/examples/validation/json-schema/. The assertions thoroughly validate the three different error scenarios and metadata fields correctly.
33-76: The test file references are properly configured and the required JSON files exist in the expected location. The service startup is protected by a 10-second timeout throughwaitForMembrane(), preventing indefinite hangs.Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptor.java (4)
31-40: LGTM!The updated documentation clearly explains the root element wrapping behavior and the conversion process. The description of defaults for objects vs. arrays is helpful.
71-92: LGTM! Error handling is well-designed.The refactored error handling properly uses
ProblemDetailsand includes several good practices:
- Disabling stacktrace for cleaner error responses
- Truncating the body excerpt to 200 characters to prevent leaking large payloads
- Including the flow context for debugging
The delegation pattern and early return for non-JSON messages are clean.
94-96: LGTM! Clean delegation to the converter.The simplified implementation properly delegates to the
JsonToXmlconverter and ensures UTF-8 encoding. Exceptions are appropriately handled by the caller.
108-123: LGTM! Excellent documentation.The enhanced documentation clearly explains the root element wrapping behavior and when defaults are applied. This will help users understand the configuration option.
...c/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java Show resolved Hide resolved
# Conflicts: # annot/pom.xml # core/.factorypath # core/pom.xml # core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java # core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java # core/src/main/java/com/predic8/membrane/core/interceptor/xml/Json2XmlInterceptor.java # core/src/main/java/com/predic8/membrane/core/util/json/JsonToXml.java # core/src/test/java/com/predic8/membrane/core/util/json/JsonToXmlTest.java # distribution/examples/extending-membrane/custom-interceptor/pom.xml # distribution/examples/extending-membrane/embedding-java/pom.xml # distribution/examples/web-services-soap/add-soap-header/pom.xml # distribution/examples/xml/basic-xml-interceptor/pom.xml # distribution/examples/xml/stax-interceptor/pom.xml # distribution/pom.xml # maven-plugin/pom.xml # membrane.spec # pom.xml # test/pom.xml # war/.factorypath # war/pom.xml
…PIs and improve error assertion handling in tests
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java(6 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaMappingsExampleTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common 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. 🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
JSONSchemaVersionParser(23-41)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaMappingsExampleTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
⏰ 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 (8)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (5)
23-23: LGTM: Import added for SchemaLoader functionality.The SchemaLoader import supports the new schema mapping configuration introduced in this PR.
64-64: LGTM: Schema mappings field added.The field is properly initialized and supports external configuration via the setter.
82-82: LGTM: Version parsing extracted to dedicated class.Good refactoring that improves maintainability and reusability of version parsing logic.
109-112: LGTM: Schema loading updated to support SchemaLocation and format detection.The format detection based on file extension is a reasonable heuristic that should work for most use cases.
103-106: No issues found. MembraneSchemaLoader is properly implemented and NetworkNT API usage is correct.The SchemaLoader configuration correctly uses the NetworkNT library APIs:
MembraneSchemaLoaderexists atcore/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/MembraneSchemaLoader.javaand properly implementsResourceLoaderSchemaRegistry.withDefaultDialect(),SchemaLoader.builder(), and the builder chain are used correctly per NetworkNT documentationaddFirst()appropriately prioritizes the custom loader in the resource loader chaindistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaMappingsExampleTest.java (3)
26-31: LGTM: Test class setup follows standard pattern.The test properly extends the distribution test base class and provides the example directory path in a cross-platform way.
33-78: LGTM: Comprehensive validation error testing.The test thoroughly validates both successful and failed validation scenarios with detailed assertions on error structure. The JSON path expressions, while complex, ensure that schema reference resolution works correctly.
80-130: LGTM: Test validates complex nested schema references.This test effectively validates:
- Array item validation with schema references
- Nested object validation with schema references
- Multiple validation errors from different referenced schemas
The error pointers containing
$refconfirm that schema reference mapping is working correctly.
...ava/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java Show resolved Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/SchemaMappings.java (3)
38-42: Consider defensive copying to prevent external mutation.The method directly assigns the input list without creating a defensive copy. This means the caller retains a reference and could modify the list after setting it, potentially leading to unexpected behavior.
🔎 Proposed change with defensive copy
@Required @MCChildElement public void setSchemas(List<Schema> schemas) { - this.schemas = schemas; + this.schemas = new ArrayList<>(schemas); }
44-46: Consider returning an unmodifiable view or defensive copy.The method returns a direct reference to the internal mutable list, allowing callers to modify the collection and potentially break encapsulation.
🔎 Proposed change with unmodifiable view
public List<Schema> getSchemas() { - return schemas; + return Collections.unmodifiableList(schemas); }Alternatively, return a defensive copy:
public List<Schema> getSchemas() { - return schemas; + return new ArrayList<>(schemas); }
48-71: Consider adding validation to Schema setters.The
idandlocationsetters don't validate their inputs, allowing null or empty values. Adding validation here would catch configuration errors earlier and provide clearer error messages.🔎 Proposed validation in setters
@MCAttribute public void setId(String id) { + if (id == null || id.trim().isEmpty()) { + throw new IllegalArgumentException("Schema id cannot be null or empty"); + } this.id = id; } @MCAttribute public void setLocation(String location) { + if (location == null || location.trim().isEmpty()) { + throw new IllegalArgumentException("Schema location cannot be null or empty"); + } this.location = location; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
annot/src/main/java/com/predic8/membrane/annot/generator/Scope.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/util/ReflectionUtil.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/ReflectionUtilTest.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java(4 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/SchemaMappings.java(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- annot/src/test/java/com/predic8/membrane/annot/util/ReflectionUtilTest.java
- annot/src/main/java/com/predic8/membrane/annot/generator/Scope.java
- annot/src/main/java/com/predic8/membrane/annot/util/ReflectionUtil.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common 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. 📚 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:
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/SchemaMappings.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)
...rc/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/SchemaMappings.java Show resolved Hide resolved
...ava/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java Outdated Show resolved Hide resolved
...ava/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java Outdated Show resolved Hide resolved
…h to use version 7.0.5-SNAPSHOT
…JsonSchema-Schema-mapping
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (1)
213-215: Previous review feedback still applies.The prior review comment on this setter regarding null validation and defensive copying remains unaddressed. Without these safeguards, passing
nullwill cause an NPE ingetJsonSchemaFactory(), and external modifications to the passed map could affect validation behavior unexpectedly.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (2)
105-107: Consider case-insensitive extension matching.The extension check is case-sensitive. While lowercase extensions are conventional, mixed-case extensions (
.YAML,.Yml) would incorrectly fall through to JSON format.🔎 Proposed fix for case-insensitive matching
private @NotNull InputFormat getSchemaFormat() { - return (jsonSchema.endsWith(".yaml") || jsonSchema.endsWith(".yml")) ? YAML : JSON; + String lowerSchema = jsonSchema.toLowerCase(java.util.Locale.ROOT); + return (lowerSchema.endsWith(".yaml") || lowerSchema.endsWith(".yml")) ? YAML : JSON; }
109-114: Method name should clarify that it returns a registry, not a factory.The method
getJsonSchemaFactory()uses the "get" prefix and contains "factory" in its name, but returns aSchemaRegistryinstance. Consider renaming tocreateSchemaRegistry(),getSchemaRegistry(), orschemaRegistry()to better reflect the actual return type.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common 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. 🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
JSONSchemaVersionParser(23-41)
⏰ 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)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (2)
63-63: LGTM!Good defensive initialization to an empty HashMap, ensuring
schemaMappingsis never null when used ingetJsonSchemaFactory().
76-76: LGTM!Good refactor to use the centralized
JSONSchemaVersionParserfor consistent version alias handling.
…nhance format detection for YAML edge cases
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (1)
213-215: Past review feedback still applies: add input validation and defensive copy.The previous review comment about adding null validation and a defensive copy remains valid. Direct assignment allows null to replace the safe default initialized at line 63, and permits external modification of the internal map state.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (1)
105-107: Consider usingLocale.ROOTfor case conversion.The extension check is correct. For consistency and to avoid potential locale-specific issues in edge cases, consider using
toLowerCase(Locale.ROOT).🔎 Proposed fix
private @NotNull InputFormat getSchemaFormat() { - return (jsonSchema.toLowerCase().endsWith(".yaml") || jsonSchema.toLowerCase().endsWith(".yml")) ? YAML : JSON; + return (jsonSchema.toLowerCase(Locale.ROOT).endsWith(".yaml") || jsonSchema.toLowerCase(Locale.ROOT).endsWith(".yml")) ? YAML : JSON; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common 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. 🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
JSONSchemaVersionParser(23-41)
⏰ 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 (4)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (4)
63-63: Good defensive initialization.Initializing with an empty
HashMapprevents potential NPE increateSchemaRegistry()ifsetSchemaMappingsis never called.
76-76: Good refactor to centralized version parsing.Using
JSONSchemaVersionParser.parse()provides consistent version alias handling and better error messages for invalid versions.
97-103: LGTM!The schema initialization properly uses the new registry-based approach with try-with-resources for the input stream.
109-114: Well-structured registry configuration.The
createSchemaRegistry()method properly wires the schema mappings and prioritizes theMembraneSchemaLoaderby adding it first, ensuring local resolution takes precedence.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.