Validator: Support for JSON Schema versions 2019-09 and 2020-12#2225
Conversation
WalkthroughAdds networknt JSON Schema validator dependency; implements schema-version-aware JSON/YAML JSON Schema validator, version parser, and Membrane schema loader; exposes configurable Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ValidatorInterceptor
participant JSONYAMLSchemaValidator
participant MembraneSchemaLoader
participant JsonSchemaFactory
Client->>ValidatorInterceptor: incoming request
activate ValidatorInterceptor
ValidatorInterceptor->>ValidatorInterceptor: getMessageValidator(schemaPath, schemaVersion)
ValidatorInterceptor->>JSONYAMLSchemaValidator: new(resolver, schemaPath, failureHandler, schemaVersion)
ValidatorInterceptor->>JSONYAMLSchemaValidator: init()
activate JSONYAMLSchemaValidator
JSONYAMLSchemaValidator->>MembraneSchemaLoader: load schema URI
MembraneSchemaLoader-->>JsonSchemaFactory: provide InputStreamSource
JsonSchemaFactory->>JSONYAMLSchemaValidator: JsonSchema ready
JSONYAMLSchemaValidator-->>ValidatorInterceptor: initialized
deactivate JSONYAMLSchemaValidator
ValidatorInterceptor->>JSONYAMLSchemaValidator: validateMessage(exchange)
activate JSONYAMLSchemaValidator
alt validation passes
JSONYAMLSchemaValidator-->>ValidatorInterceptor: CONTINUE (increment valid)
else validation fails
JSONYAMLSchemaValidator-->>ValidatorInterceptor: ABORT + set Problem Details response (increment invalid)
end
deactivate JSONYAMLSchemaValidator
ValidatorInterceptor-->>Client: response
deactivate ValidatorInterceptor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (1)
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: 4
🧹 Nitpick comments (5)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
12-14: Handle the Optional.get() call more gracefully.The
.get()call on the Optional returned byfromId()could throwNoSuchElementExceptionif the SchemaId is not recognized. WhilealiasToSpecIdshould prevent invalid IDs, adding a null check on the input parameter and handling the empty Optional case would make this more robust.Apply this diff to add null validation and better Optional handling:
public static SpecVersion.VersionFlag parse(String version) { + if (version == null) { + throw new ConfigurationException("JSON Schema version cannot be null"); + } - return SpecVersion.VersionFlag.fromId(aliasToSpecId(version)).get(); + return SpecVersion.VersionFlag.fromId(aliasToSpecId(version)) + .orElseThrow(() -> new ConfigurationException("Unsupported JSON Schema version ID: " + version)); }distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java (1)
89-89: Remove .log().all() from test.The
.log().all()statement should be removed to keep test output clean and focused on failures.Apply this diff:
.when() .post("http://localhost:2001") .then() - .log().all() .statusCode(400)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONSchemaValidationTest.java (1)
82-82: Remove or replace debug println with proper logging.If this logging is needed for test diagnostics, consider using a test logger instead of
System.out.println, or remove it entirely if it's only for temporary debugging.Apply this diff to remove the debug statement:
assertEquals(1, jn.get("errors").size()); - - System.out.println("exc.getResponse().getBodyAsStringDecoded() = " + exc.getResponse().getBodyAsStringDecoded()); }core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java (1)
12-22: Add test coverage for error cases.The test currently only covers valid version aliases. Consider adding tests for:
- Invalid/unknown version strings to verify
ConfigurationExceptionis thrown- Null input handling
- The "draft-2019-09" alias (currently only "2019-09" is tested)
Add these test cases:
@Test void parseUnknownVersion() { assertThrows(ConfigurationException.class, () -> parse("invalid-version")); } @Test void parseNullVersion() { assertThrows(ConfigurationException.class, () -> parse(null)); } @Test void parseDraft2019() { assertEquals(V201909, parse("draft-2019-09")); }core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONYAMLSchemaValidatorTest.java (1)
66-66: Remove or replace debug println statements.Debug print statements on lines 66 and 77 should be removed or replaced with proper test logging for cleaner test output.
Apply this diff to remove the debug statements:
validator.validateMessage( exc, REQUEST); assertEquals(1, validator.getInvalid()); assertEquals(0, validator.getValid()); - System.out.println("exc.getResponse().getBodyAsStringDecoded() = " + exc.getResponse().getBodyAsStringDecoded()); }And for line 77:
validator.validateMessage( exc, REQUEST); - System.out.println("exc.getResponse().getBodyAsStringDecoded() = " + exc.getResponse().getBodyAsStringDecoded()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
core/pom.xml(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java(6 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/MembraneSchemaLoader.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONSchemaValidationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONYAMLSchemaValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java(1 hunks)core/src/test/resources/validation/json-schema/simple-schema.json(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
JSONSchemaVersionParser(10-26)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
TextUtil(33-340)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java (2)
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (1)
ProblemDetails(36-414)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
JSONSchemaVersionParser(10-26)
⏰ 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)
core/pom.xml (1)
172-176: LGTM! Dependency addition is appropriate.The addition of
json-schema-validatorversion 1.5.9 is the latest stable release and provides the necessary support for JSON Schema versions 2019-09 and 2020-12 as indicated in the PR objectives.Based on learnings
core/src/test/resources/validation/json-schema/simple-schema.json (1)
1-18: LGTM! Test schema is well-defined.The schema provides good test coverage with various constraint types (minLength, minimum, pattern) and
additionalProperties: falseto test validation of unknown properties.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @predic8. * #2225 (comment) The following files were modified: * `core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java` * `core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java` * `core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java` * `core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/MembraneSchemaLoader.java`
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
docs/ROADMAP.md (2)
15-17: Clarify intent and add migration guidance for FailureHandler removalRecommend tightening wording and adding a migration note. This avoids ambiguity and helps users upgrade smoothly. As per coding guidelines.
- - ValidatorInterceptor: remove FailureHandler - - It is often called and only used for logging. - - Put logging in validators + - ValidatorInterceptor: remove FailureHandler + - Predominantly used for logging; move logging into validators. + - Migration: replace FailureHandler usages with validator-level logging; ensure correlation IDs/Exchange context remain available for logs.
48-50: Expand release notes with library + config pointers; verify correct version section
- Suggest explicitly naming the validator library and pointing users to how to choose schema version and handle format assertions. Also confirm that this belongs under 6.4.0 (and not 6.5.0/7.0.0). Based on learnings.
-Release Notes: -- JSON Schema validation support for schema versions 2019-09 and 2020-12 +Release Notes: +- JSON Schema validation support for JSON Schema 2019-09 and 2020-12 (via networknt json-schema-validator). +- Document how to select the schema version (e.g., schemaVersion attribute) and the "format" behavior (annotation vs assertion), with a link to usage docs/examples.Please verify:
- Is 6.4.0 the correct section for this feature, or should it be listed under a later version targeted by this PR?
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
21-23: Inconsistent static imports for SchemaId constants.Only
V4andV6are explicitly imported, whileV7,V201909, andV202012use qualified names. For consistency, either import all SchemaId constants used in the switch expression or use qualified names throughout.Apply this diff to import all used constants:
import static com.networknt.schema.SchemaId.*; -import static com.networknt.schema.SchemaId.V4; -import static com.networknt.schema.SchemaId.V6;Or alternatively, remove the wildcard and list all explicitly:
-import static com.networknt.schema.SchemaId.*; -import static com.networknt.schema.SchemaId.V4; -import static com.networknt.schema.SchemaId.V6; +import static com.networknt.schema.SchemaId.V4; +import static com.networknt.schema.SchemaId.V6; +import static com.networknt.schema.SchemaId.V7; +import static com.networknt.schema.SchemaId.V201909; +import static com.networknt.schema.SchemaId.V202012;core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java (1)
24-52: Consider adding a test for unsupported SchemaId constants.The test suite covers unknown aliases and null values, but doesn't test the scenario where
aliasToSpecId()returns a valid SchemaId constant thatSpecVersion.VersionFlag.fromId()doesn't recognize (which would cause the.get()call to fail). While this may be unlikely with the current networknt library version, adding such a test would ensure robustness.However, this suggestion depends on whether the parser is modified to handle the empty Optional case (as recommended in the previous file's review).
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java (4)
55-55: Make Content-Type assertion robust against charset parametersServers often append charset (e.g., application/problem+json; charset=utf-8). Assert header startsWith to avoid brittle equality.
- .contentType(APPLICATION_PROBLEM_JSON) + .header("Content-Type", Matchers.startsWith(APPLICATION_PROBLEM_JSON))Apply the same change at both occurrences (Line 55 and Line 88).
Also applies to: 88-88
91-97: Remove duplicate assertion: '/price' is checked twiceLine 92 and Line 96 both assert containsString("/price"). Drop one.
.body(Matchers.containsString("diesel")) - .body(Matchers.containsString("/price")); + // '/price' already asserted above
58-60: Prefer structured JSONPath checks over free-text message fragmentsFree-text messages like "number expected", "array expected" can vary between JSON Schema drafts/validator versions. If the problem payload includes structured fields (e.g., violations[].instancePath, violations[].keyword, violations[].schemaPath), assert those instead of or in addition to containsString.
Example (adjust to actual payload shape):
- .body("violations.find { it.instancePath == '/price' }.keyword", Matchers.equalTo("type"))
- .body("violations.find { it.instancePath == '/properties/tags/type' }.keyword", Matchers.equalTo("type"))
Based on learningsAlso applies to: 91-95
33-65: Reduce duplication: parameterize or extract a helperBoth tests share the same request/response structure with port- and file-specific parameters. Consider a @ParameterizedTest with Arguments(port, goodFile, badFile, expectedMatchers) or extract a small helper to perform the roundtrip for a port.
Optional sketch:
- @ParameterizedTest
- Arguments.of(2000, "good2000.json", "bad2000.json", List.of(/* matchers */))
- Arguments.of(2001, "good2001.json", "bad2001.json", List.of(/* matchers */))
- In test body, loop expected matchers and apply .body(m) on the response spec.
Also applies to: 66-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java(7 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/MembraneSchemaLoader.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONSchemaValidationTest.java(0 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONYAMLSchemaValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java(2 hunks)docs/ROADMAP.md(2 hunks)
💤 Files with no reviewable changes (1)
- core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONSchemaValidationTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/JSONYAMLSchemaValidatorTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/MembraneSchemaLoader.java
- core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONYAMLSchemaValidator.java
🧰 Additional context used
🧬 Code graph analysis (3)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.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/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
JSONSchemaVersionParser(25-43)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/ValidatorInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/TextUtil.java (1)
TextUtil(33-340)
🪛 LanguageTool
docs/ROADMAP.md
[grammar] ~16-~16: There might be a mistake here.
Context: ... often called and only used for logging. - Put logging in validators # 6.5.0 - Da...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...ckURLs) throws Exception Release Notes: - JSON Schema validation support for schem...
(QB_NEW_EN)
⏰ 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/interceptor/schemavalidation/ValidatorInterceptor.java (4)
55-60: LGTM! Schema version field and accessors are well-implemented.The new
schemaVersionfield with default value "2020-12" (latest JSON Schema version) and the corresponding getter/setter methods are properly documented and follow the existing patterns in the class. The@MCAttributeannotation ensures proper configuration binding.Also applies to: 218-230
20-20: LGTM! Clean import reorganization.The addition of the json validation package import and the static import of
linkURLimproves code readability by reducing verbosity in thegetLongDescription()method.Also applies to: 36-36, 289-289, 293-293, 297-297, 301-301
314-320: LGTM! Safer FailureHandler implementation.Returning a no-op lambda instead of
nullfor the default failure handler is a defensive programming practice that prevents potentialNullPointerExceptionwhen the handler is invoked. This aligns with theVOIDconstant pattern already defined in theFailureHandlerinterface.
100-101: No action required: constructor signature matches invocation
TheJSONYAMLSchemaValidatorconstructor accepts(Resolver, String, FailureHandler, String), which aligns with the current call.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
25-27: The unsafe.get()call on Optional has already been flagged.This concern was raised in a previous review. The suggestion to use
.orElseThrow()with a descriptiveConfigurationExceptionremains valid and should be applied.
🧹 Nitpick comments (4)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java (3)
18-18: Remove the wildcard Hamcrest import.The wildcard import
org.hamcrest.*;is redundant since specific matchers are explicitly imported on lines 25-26. Wildcard imports can lead to namespace pollution and make dependencies less clear.Apply this diff to remove the wildcard import:
-import org.hamcrest.*;
58-63: Consolidate redundant body assertions.Lines 60-62 use general
containsStringchecks while line 63 performs a more specific GPath assertion for the same validation error. The general checks provide limited additional value and reduce clarity.Consider following the pattern used in the
port2001()test (lines 95-98) which uses only specific GPath expressions. Apply this diff:.statusCode(400) .contentType(APPLICATION_PROBLEM_JSON) .body("title", equalTo("JSON validation failed")) .body("type", equalTo("https://membrane-api.io/problems/user/validation")) - .body(containsString("not found")) - .body(containsString("/required")) - .body(containsString("p1")) .body("errors.find { it.pointer == '/required' }.message", containsString("not found"));
37-37: Consider adding service startup verification.Both tests start the service proxy and immediately make requests without verifying the service is ready. If the service takes time to initialize or fails to start, tests may fail with unclear connection errors.
Consider adding a brief readiness check or wait after starting the service, for example:
try(Process2 ignored = startServiceProxyScript()) { // Wait for service to be ready await().atMost(10, TimeUnit.SECONDS) .pollInterval(100, TimeUnit.MILLISECONDS) .until(() -> { try { given().get("http://localhost:2000").then().statusCode(anyOf(200, 404)); return true; } catch (Exception e) { return false; } }); // existing test code... }Alternatively, if the service consistently starts quickly in your environment, this may not be necessary.
Also applies to: 71-71
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
23-40: Consider adding documentation for the public API.Adding Javadoc to the
parse()method and class-level documentation would help users understand the supported version formats without reading the implementation. For example, documenting that valid inputs include "04", "draft-04", "2019-09", "draft-2019-09", etc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java(2 hunks)docs/ROADMAP.md(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
🪛 LanguageTool
docs/ROADMAP.md
[grammar] ~16-~16: There might be a mistake here.
Context: ...r logging; move logging into validators. - Migration: replace FailureHandler usages...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...hange context remain available for logs. - Check if it is used by customer installa...
(QB_NEW_EN)
⏰ 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)
docs/ROADMAP.md (2)
15-18: Verify formatting and wording of ValidatorInterceptor deprecation guidance.The content describes removal and migration, but LanguageTool flagged potential grammar or alignment issues. Review lines 16-17 for clarity—ensure the migration guidance is precise and actionable for teams using FailureHandler.
48-51: This reads like a TODO rather than completed release-note documentation.Line 51 states "Document how to select the schema version..." which implies the documentation task is not yet complete. If this section is meant to describe functionality that will be available in 6.4.0, clarify whether:
- The documentation (e.g., schemaVersion attribute usage, format annotation vs. assertion behavior) already exists elsewhere and should be linked here, or
- This is a forward-looking note and the actual docs are still to be written.
Also consider whether "Release Notes:" is the right label—this ROADMAP typically describes planned work, not release summaries.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (1)
25-27: Handle empty Optional fromfromId()gracefully.This issue was previously flagged but remains unaddressed. Calling
.get()on the Optional returned bySpecVersion.VersionFlag.fromId()will throwNoSuchElementExceptionif the SchemaId is not recognized. UseorElseThrow()with a descriptiveConfigurationExceptioninstead.Apply this diff:
public static SpecVersion.VersionFlag parse(String version) { - return SpecVersion.VersionFlag.fromId(aliasToSpecId(version)).get(); + return SpecVersion.VersionFlag.fromId(aliasToSpecId(version)) + .orElseThrow(() -> new ConfigurationException("JSON Schema version not supported by validator library: " + version)); }
🧹 Nitpick comments (3)
core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java (1)
36-46: Comprehensive alias coverage.All supported version aliases are correctly tested. The test could optionally be refactored as a
@ParameterizedTestfor better organization, but the current approach is clear and functional.Optional refactor using parameterized tests:
+ @ParameterizedTest + @CsvSource({ + "04, V4", + "06, V6", + "07, V7", + "draft-04, V4", + "draft-06, V6", + "draft-07, V7", + "2019-09, V201909", + "2020-12, V202012" + }) + void parseFromAlias(String alias, SpecVersion.VersionFlag expected) { + assertEquals(expected, parse(alias)); + } - @Test - void parseFromAlias() { - assertEquals(V4, parse("04")); - assertEquals(V6, parse("06")); - assertEquals(V7, parse("07")); - assertEquals(V4, parse("draft-04")); - assertEquals(V6, parse("draft-06")); - assertEquals(V7, parse("draft-07")); - assertEquals(V201909, parse("2019-09")); - assertEquals(V202012, parse("2020-12")); - }distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java (2)
33-97: Consider documenting schema versions per port.Given the PR adds support for JSON Schema versions 2019-09 and 2020-12, consider adding brief comments explaining which schema version each port validates against. This will help future maintainers understand the test coverage.
Example:
+ // Port 2000 validates against JSON Schema draft 2019-09 @Test void port2000() throws Exception {
33-97: Optional: Reduce duplication with parameterized test.Both test methods share identical structure. Consider refactoring to a
@ParameterizedTestto reduce duplication while maintaining clarity.Example approach:
@ParameterizedTest @CsvSource({ "2000, good2000.json, bad2000.json, '/required', 'not found'", "2001, good2001.json, bad2001.json, '/properties/id/type', 'integer expected'" }) void validateJsonSchema(int port, String goodFile, String badFile, String errorPointer, String errorMessage) throws Exception { // shared test logic }However, keeping separate methods is also acceptable for integration test clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.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/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.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). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (7)
core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParserTest.java (2)
1-23: LGTM!The license header, package declaration, and imports are correctly structured. The static imports improve test readability.
26-34: LGTM!The error case tests correctly verify that both unknown and null version strings throw
ConfigurationException.core/src/main/java/com/predic8/membrane/core/interceptor/schemavalidation/json/JSONSchemaVersionParser.java (2)
1-22: LGTM!The license header, package declaration, and imports are correctly structured. The static import of
SchemaId.*improves readability of the version mapping logic.
29-40: LGTM! Correct alias mappings.The version aliases correctly follow JSON Schema conventions:
- Draft-prefixed versions for older drafts (04, 06, 07)
- Date-based identifiers for newer versions (2019-09, 2020-12) without the "draft-" prefix
The null and unknown version handling with
ConfigurationExceptionprovides clear error messages.distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/validation/JSONSchemaValidationExampleTest.java (3)
20-24: LGTM: Idiomatic REST-assured imports.The static imports improve test readability and follow REST-assured best practices.
65-97: Excellent detailed error assertions.The port2001 test demonstrates comprehensive validation with precise JSON path assertions that verify both error pointers and messages. This is a strong example of thorough integration testing.
41-41: All test data files referenced in the test are present and accessible indistribution/examples/validation/json-schema/. No issues found.
|
This pull request needs "/ok-to-test" from an authorized committer. |
|
/ok-to-test |
2 participants
Summary by CodeRabbit
New Features
Chores
Tests
Documentation