fixed NPE in OpenAPI validation, added test to run official JSON Schema test suite (needs to be downloaded seperately) #1906
Conversation
…ma test suite (needs to be downloaded seperately)
WalkthroughThe changes extend nullability detection in Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as JsonSchemaTestSuiteTests
participant FileSystem as TestSuite Directory
participant Validator as OpenAPIValidator
participant Logger as Logger
TestRunner->>FileSystem: Read JSON test files
loop For each test file
TestRunner->>FileSystem: Parse schemas and test cases
loop For each test case
TestRunner->>Validator: Validate test data against schema
Validator-->>TestRunner: Validation result (valid/invalid)
TestRunner->>Logger: Log result and details
end
end
TestRunner->>Logger: Print summary statistics
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
/ok-to-test |
![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
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java (1)
110-112: Nullable check looks correct – consider minor readability tweakThe added third condition correctly recognises a schema where the sole
"type"is"null". That closes the NPE gap described in the PR title.
You could slightly improve readability/defensiveness withObjects.equals(schema.getType(), "null"), but functionally the change is solid.core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (1)
62-69: Raw-typeMapusage triggers unchecked castsThe numerous casts to raw
Mapproduce unchecked warnings and hide type errors.
Introduce type parameters, e.g.Map<String, Object>, to improve safety and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (2)
110-126: Result evaluation logic can dereferenceerrorswhen null
errorsis only initialised inside thetryblock; if validation throws, the subsequent check dereferenceserrors == null.
Return early or initialiseerrors = new ValidationErrors()before the try.ValidationErrors errors = new ValidationErrors(); try { errors = validator.validate(post.body(body));Likely an incorrect or invalid review comment.
35-35: ```shell
#!/bin/bashShow the locateTests method implementation for context
rg 'private void locateTests' -n -A 10 -B 5 core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (2)
111-118:computeIgnoredReasonheuristic is brittleMatching on
openapi.contains("http://")ignores legitimatehttps://external refs, and substring matches in example strings.
Consider parsing the schema and checking$refvalues explicitly instead of string contains.
43-54: Minor: counters should belongto avoid overflowThe full draft-4 suite easily runs >10 000 cases. Using
intrisks overflow if you later enable multiple drafts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (2)
Learnt from: rrayst
PR: membrane/api-gateway#1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.873Z
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.
Learnt from: rrayst
PR: membrane/api-gateway#1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.873Z
Learning: The mediaType method in the Message class (which Request extends) throws jakarta.mail.internet.ParseException because it creates a ContentType object for parsing MIME media types. The jakarta.mail.internet.ParseException import is correct and necessary in test files that call mediaType().
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (1)
95-109:schemais blindly cast toMap– add defensive check
generateOpenAPIForSchemaassumes every schema node is a JSON object.
A handful of draft-4 tests contain boolean schemas (true/false), which will throwClassCastExceptionhere.Either:
- Short-circuit boolean schemas (they are valid in draft-4), or
- Fail early with a clear message.
- if (((Map) schema).containsKey("definitions")) { + if (schema instanceof Map && ((Map<?, ?>) schema).containsKey("definitions")) { oa.put("components", of("schemas", ((Map) schema).get("definitions"))); System.out.println(" warning: The schema contains definitions. They have been moved from #/definitions/ to #/components/schemas/ ."); + } else if (!(schema instanceof Map)) { + return yamlMapper.writeValueAsString(oa); // boolean schema – nothing to relocate }
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Outdated
Show resolved
Hide resolved
- fixed NPE - support "type":"null" in schema - do not accept "123.45" as a valid number, when this appears in a JSON - JSON Schema 'pattern' is not anchored
…mbrane/service-proxy into fix-npe-in-openapi-validation
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (3)
41-41: Use OS-independent path separators.The hard-coded Windows backslashes will cause issues on Unix-based systems.
-public final String TEST_SUITE_BASE_PATH = "git\\JSON-Schema-Test-Suite\\tests\\draft6"; +public final String TEST_SUITE_BASE_PATH = "git/JSON-Schema-Test-Suite/tests/draft6";Alternatively, use
Path.of("git", "JSON-Schema-Test-Suite", "tests", "draft6").toString()for full OS independence.
64-64: Guard against NPE from listFiles().
File.listFiles()returnsnullwhen the path doesn't exist or isn't a directory, causing NPE in the enhanced for-loop.- for (File file : base.listFiles()) { + File[] files = base.listFiles(); + if (files == null) { + throw new RuntimeException("Unable to list test files in directory: " + baseDir); + } + for (File file : files) {
159-161: Avoid silently swallowing validation exceptions.The current exception handling only prints the stack trace but doesn't fail the test, making debugging difficult when validators throw unexpected exceptions.
- } catch (Exception e) { - e.printStackTrace(); + } catch (Exception e) { + throw new RuntimeException("Validator threw exception during test: " + description2, e); }
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/openapi/validators/NullValidator.java (1)
22-24: Remove unused imports.The imports for
BigDecimalandparseDoubleare not used in this class and should be removed.-import java.math.BigDecimal; - -import static java.lang.Double.parseDouble;core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (2)
110-110: Use consistent logging instead of System.out.println.Replace
System.out.printlnwith the logger to maintain consistent output handling.- System.out.println(" warning: The schema contains definitions. They have been moved from #/$defs/ to #/components/schemas/ ."); + LOG.warn(" The schema contains definitions. They have been moved from #/$defs/ to #/components/schemas/ .");
167-167: Use consistent logging instead of System.out.println.Replace
System.out.printlnwith the logger for consistent output handling.- System.out.println(" Test: NOT OK!"); + LOG.error(" Test: NOT OK!");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractBodyValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/ArrayValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/IJSONSchemaValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/NullValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberValidator.java(3 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java(3 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/StringValidator.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/SchemaValidatorTest.java(1 hunks)core/src/test/resources/openapi/specs/header-params.yml(1 hunks)pom.xml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/IJSONSchemaValidator.java
- pom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java
🧰 Additional context used
🧠 Learnings (1)
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (2)
Learnt from: rrayst
PR: membrane/api-gateway#1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.873Z
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.
Learnt from: rrayst
PR: membrane/api-gateway#1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.873Z
Learning: The mediaType method in the Message class (which Request extends) throws jakarta.mail.internet.ParseException because it creates a ContentType object for parsing MIME media types. The jakarta.mail.internet.ParseException import is correct and necessary in test files that call mediaType().
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (12)
core/src/main/java/com/predic8/membrane/core/openapi/validators/NullValidator.java (1)
28-37: LGTM! Correct null value detection.The method properly handles both Java
nullvalues and JacksonNullNodeinstances, returning the appropriateNULLconstant.core/src/test/java/com/predic8/membrane/core/openapi/validators/SchemaValidatorTest.java (1)
89-89: LGTM! Stricter number validation is appropriate.The test update correctly reflects that
TextNodeinstances should not be validated as numbers, even if they contain numeric strings. This stricter type checking aligns with proper JSON Schema validation semantics.core/src/main/java/com/predic8/membrane/core/openapi/validators/ArrayValidator.java (1)
50-50: LGTM! Proper context handling.The assignment ensures that any modifications to the validation context are properly captured. This pattern suggests
schemaType()returns a new or modified context instance rather than mutating in place.core/src/test/resources/openapi/specs/header-params.yml (1)
93-93: LGTM! Pattern anchoring ensures complete string validation.Adding
^and$anchors ensures the entire string consists only of word characters, providing stricter validation semantics. This change complements the regex matching adjustments inStringValidator.java.core/src/main/java/com/predic8/membrane/core/openapi/validators/StringValidator.java (1)
207-207: Verify the regex matching behavior change is intentional.Changing from
matches()tofind()alters the validation semantics from full-string matching to substring matching. This could allow patterns to match partially where they previously required complete string matches.Ensure this change is intentional and consistent with the pattern anchoring seen in
header-params.yml(line 93).#!/bin/bash # Description: Search for regex patterns in OpenAPI specs to verify anchoring consistency # Expected: Most patterns should be anchored with ^ and $ if substring matching is used echo "Searching for regex patterns in OpenAPI specs..." fd -e yml -e yaml | xargs rg -n "pattern:" -A 1 -B 1 echo -e "\nSearching for unanchored patterns that might be affected..." fd -e yml -e yaml | xargs rg -n 'pattern:.*[^$]"$'core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractBodyValidator.java (1)
46-47: Good defensive programming improvements.The null check prevents potential NPE when accessing the schema's
$refproperty, and the assignment pattern correctly handles the immutable ValidationContext design whereschemaType()returns a new instance.core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberValidator.java (4)
20-20: Import addition supports stricter type validation.The TextNode import enables explicit handling of JSON string nodes in number validation.
26-30: Clear documentation of validation logic.The comments effectively explain the distinction between valid numbers (in parameters as strings) and invalid numbers (JSON string nodes containing numeric text), aligning with JSON Schema specifications.
39-41: Correct JSON Schema compliance for TextNode handling.Returning
nullfor TextNode instances correctly indicates that JSON strings containing numbers should not be validated as numbers according to JSON Schema spec.
61-63: Consistent validation error for JSON string numbers.The immediate validation error for TextNode instances aligns with the
canValidatelogic and enforces proper JSON Schema type validation.core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (2)
13-13: Import is correct for mediaType() method usage.Based on retrieved learnings, the
jakarta.mail.internet.ParseExceptionimport is necessary because themediaType()method in the Message class throws this exception when parsing MIME types.
34-38: Excellent addition of comprehensive JSON Schema compliance testing.This test class provides valuable integration testing against the official JSON Schema Test Suite, which will help ensure the OpenAPI validators correctly implement JSON Schema specifications.
core/src/main/java/com/predic8/membrane/core/openapi/validators/NullValidator.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Outdated
Show resolved
Hide resolved
|
This pull request needs "/ok-to-test" from an authorized committer. |
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (5)
59-59: Method name doesn't match its behavior.The method
runTestsFoundInDirectorysuggests it only finds tests, but it actually runs them. Consider renaming torunTestsInDirectoryfor clarity.
1-172: Consider extracting utility methods and improving logging.The class has grown quite complex. Consider extracting utility methods for:
- OpenAPI spec generation (lines 98-118)
- Schema transformation logic (definitions/$defs handling)
- Test result evaluation
Also, replace direct
System.out.printlnusage with proper logging throughout the class.
41-41: Use OS-independent path separators.Based on past review feedback, the hard-coded Windows backslashes should be replaced with OS-independent paths:
- public final String TEST_SUITE_BASE_PATH = "git\\JSON-Schema-Test-Suite\\tests\\draft6"; + public final String TEST_SUITE_BASE_PATH = "git/JSON-Schema-Test-Suite/tests/draft6";Forward slashes work on all operating systems, including Windows.
64-64: Potential NPE when listFiles() returns null.The
listFiles()method can returnnullif the directory doesn't exist or isn't readable:- for (File file : base.listFiles()) { + File[] files = base.listFiles(); + if (files == null) { + throw new RuntimeException("Unable to list files in directory: " + baseDir); + } + for (File file : files) {
159-161: Exception handling may hide validation failures.The current exception handling only prints the stack trace but doesn't fail the test, which could hide real validation issues:
} catch (Exception e) { - e.printStackTrace(); + log.error("Validation failed for test: " + description2, e); + throw new RuntimeException("Validator threw exception during test: " + description2, e); }This ensures tests fail loudly when unexpected exceptions occur, making debugging easier.
🧹 Nitpick comments (1)
core/src/test/resources/openapi/specs/integer.yml (1)
59-60: LGTM - Clean addition for testing validation without explicit type.The new
maximum-without-typeproperty is well-designed to test validation behavior when only constraints are specified without explicit type declarations.However, fix the missing newline at the end of the file:
maximum-without-type: - maximum: 5 + maximum: 5 +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/openapi/validators/NullValidator.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/IntegerTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/NullTest.java(1 hunks)core/src/test/resources/openapi/specs/integer.yml(1 hunks)core/src/test/resources/openapi/specs/null.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/NullValidator.java
🧰 Additional context used
🧠 Learnings (1)
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (2)
Learnt from: rrayst
PR: membrane/api-gateway#1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.873Z
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.
Learnt from: rrayst
PR: membrane/api-gateway#1906
File: core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java:0-0
Timestamp: 2025-06-18T12:03:09.873Z
Learning: The mediaType method in the Message class (which Request extends) throws jakarta.mail.internet.ParseException because it creates a ContentType object for parsing MIME media types. The jakarta.mail.internet.ParseException import is correct and necessary in test files that call mediaType().
🪛 YAMLlint (1.37.1)
core/src/test/resources/openapi/specs/integer.yml
[error] 60-60: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/null.yml
[HIGH] 1-33: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[HIGH] 1-33: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
core/src/test/resources/openapi/specs/null.yml (1)
1-32: Excellent OpenAPI 3.1.0 spec for null type testing.This spec effectively covers the key null validation scenarios:
- Standalone
nulltype validation- Array items with union types (
integerornull)The OpenAPI 3.1.0 version is correct since it properly supports
nullas a standalone type, which is essential for these test cases.core/src/test/java/com/predic8/membrane/core/openapi/validators/IntegerTest.java (1)
109-121: Well-implemented test methods for maximum-without-type validation.Both test methods follow the established pattern and properly test the boundary conditions:
invalidMaximumWithoutTypeInBody()correctly validates that 13 > 5 failsvalidMaximumWithoutTypeInBody()correctly validates that 5 = 5 passesThe tests complement the new schema property and ensure validation works correctly even without explicit type declarations.
core/src/test/java/com/predic8/membrane/core/openapi/validators/NullTest.java (1)
36-63: Comprehensive null validation test coverage.The test class provides excellent coverage for null type validation scenarios:
- Standalone null validation - Tests both valid null and invalid string inputs
- Array with union types - Tests arrays containing null/integer combinations and validates proper error handling for invalid types
The tests are well-structured, follow established patterns, and properly validate error messages. The use of
mapToJson(null)correctly handles null serialization for testing.
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
Outdated
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores