OpenAPIValidator: QueryString Support for Explode, Form, Arrays, Hashs #2156
Conversation
WalkthroughIntroduces a new query-parameter validation pipeline with dedicated parsers (array/object, exploded/non-exploded), expands OpenAPI utilities, standardizes ValidationErrors factory to error(...), renames IJSONSchemaValidator to public JsonSchemaValidator and updates validators, adds Request builder overloads, adjusts security/header/body validators, bumps swagger-parser, and adds extensive tests/spec resources. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Request as Request
participant OperationValidator
participant QueryParamVal as QueryParameterValidator
participant ParserFactory as AbstractParameterParser.instance
participant SchemaVal as SchemaValidator
participant OpenAPIUtil
Client->>OperationValidator: validate(request, operation)
OperationValidator->>QueryParamVal: validate(ctx, request, operation)
QueryParamVal->>QueryParamVal: parse raw query to map (decoded keys)
loop For each declared query parameter
QueryParamVal->>OpenAPIUtil: resolveSchema(api, parameter)
alt exploded array/object?
QueryParamVal->>ParserFactory: instance(api, type, parameter)
ParserFactory-->>QueryParamVal: ParameterParser
QueryParamVal->>ParserFactory: setValues(map)
ParserFactory-->>QueryParamVal: JsonNode getJson()
QueryParamVal->>SchemaVal: validate(ctx, schema, JsonNode)
SchemaVal-->>QueryParamVal: ValidationErrors/null
else scalar/simple
QueryParamVal->>SchemaVal: validate(ctx, schema, raw value)
SchemaVal-->>QueryParamVal: ValidationErrors/null
end
end
alt unknown params present
QueryParamVal->>OpenAPIUtil: isExplode + has additionalProperties?
alt allow additional
Note right of QueryParamVal: Unknowns ignored
else
QueryParamVal-->>OperationValidator: error("unknown query parameter")
end
end
QueryParamVal-->>OperationValidator: aggregated ValidationErrors/null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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)
✅ Files skipped from review due to trivial changes (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). (2)
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: 9
🧹 Nitpick comments (37)
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java (6)
21-27: Prefer explicit (static) imports over wildcards.Reduces namespace pollution and improves readability.
-import io.swagger.v3.oas.models.media.*; +import io.swagger.v3.oas.models.media.Schema; ... -import static com.predic8.membrane.core.openapi.util.Utils.*; +import static com.predic8.membrane.core.openapi.util.Utils.getComponentLocalNameFromRef;
35-38: Defensive null checks for constructor args.Avoids NPE later (e.g., pathItem.getParameters()).
- protected AbstractParameterValidator(OpenAPI api, PathItem pathItem) { - this.api = api; - this.pathItem = pathItem; + protected AbstractParameterValidator(OpenAPI api, PathItem pathItem) { + this.api = Objects.requireNonNull(api, "api must not be null"); + this.pathItem = Objects.requireNonNull(pathItem, "pathItem must not be null"); }
40-42: Guard against null Operation and tighten filtering type.If operation is ever null, this will NPE via getAllParameterSchemas(). Also, filtering relies on strict class equality in isTypeOf; see suggestion below.
- protected Stream<Parameter> getParametersOfType(Operation operation, Class<?> paramClazz) { - return getAllParameterSchemas(operation).stream().filter(p -> isTypeOf(p, paramClazz)); + protected Stream<Parameter> getParametersOfType(Operation operation, Class<?> paramClazz) { + Objects.requireNonNull(operation, "operation must not be null"); + return getAllParameterSchemas(operation).stream().filter(p -> isTypeOf(p, paramClazz)); }
48-50: Use assignability rather than strict class equality.More robust if subclasses/proxies appear.
- boolean isTypeOf(Parameter p, Class<?> clazz) { - return p.getClass().equals(clazz); + boolean isTypeOf(Parameter p, Class<?> clazz) { + return clazz.isInstance(p); }
58-67: Schema resolution: add generics and null-safety around Components.Prevents NPE when Components/Schemas are absent; clarifies types with Schema<?>.
- protected Schema getSchema(Parameter p) { - Schema schema = p.getSchema(); + protected Schema<?> getSchema(Parameter p) { + Schema<?> schema = p.getSchema(); if (schema == null) { return null; } - if(schema.get$ref() != null) { - String componentLocalNameFromRef = getComponentLocalNameFromRef(schema.get$ref()); - return api.getComponents().getSchemas().get(componentLocalNameFromRef); + if (schema.get$ref() != null) { + if (api.getComponents() == null || api.getComponents().getSchemas() == null) { + // Option A: return null and let callers decide. Option B: throw with context. + return null; + } + String componentLocalNameFromRef = getComponentLocalNameFromRef(schema.get$ref()); + return api.getComponents().getSchemas().get(componentLocalNameFromRef); } return schema; }Question: If a $ref cannot be resolved, should we return null, fall back to the original schema object, or emit a validation error? Please confirm desired behavior.
44-46: Naming nit: method returns Parameters, not Schemas.Consider renaming to getAllParameters(...) to avoid confusion.
core/src/test/resources/openapi/specs/customers.yml (1)
46-46: Fix wording of path-item description.Use singular for clarity.
- description: Single paths + description: Single pathcore/src/test/java/com/predic8/membrane/core/openapi/validators/exceptions/ExceptionInterceptorTest.java (1)
54-56: Strengthen assertion and align naming with scenario.Capture the exchange to assert no response was produced, and make the method name match the path hint.
- void paramWithNoSchema() throws Exception { - assertEquals(CONTINUE, interceptor.handleRequest(getExchange("/param-with-no-type?bar=7", null))); + void paramWithNoType() throws Exception { + var exc = getExchange("/param-with-no-type?bar=7", null); + assertEquals(CONTINUE, interceptor.handleRequest(exc)); + assertNull(exc.getResponse(), "Interceptor should not set a response on CONTINUE"); }core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
53-55: Deterministic join for Set to avoid flaky logs/tests.Joining a Set in iteration order is nondeterministic. Sort before joining.
- public static @NotNull String join(Set<String> s) { - return join(new ArrayList<>(s)); - } + public static @NotNull String join(Set<String> s) { + return join(s.stream().sorted().toList()); + }core/src/test/resources/junit-platform.properties (1)
1-6: Add trailing newline; keep as disabled template.Minor formatting fix to appease linters; no behavior change.
core/src/test/resources/openapi/specs/oas31/parameters/explode.yaml (2)
6-6: Fix comment: property name is “explode”, not “exploded”.-# query param array (default 'exploded=true') ?a=b&a=c +# query param array (default 'explode=true') ?a=b&a=c
27-27: Add newline at EOF to satisfy linters.core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml (2)
34-34: YAML style: add space after comma in type array.Improves readability and silences yamllint.
- type: [array,"null"] + type: [array, "null"]
40-40: Add newline at EOF to satisfy linters.core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
84-90: Resolve $refs and close streams; prefer English param name.Use ParseOptions(resolve=true) and try-with-resources; rename “pfad” → “path”.
- public static OpenAPI getApi(Object obj, String pfad) { - return new OpenAPIParser().readContents(readInputStream(getResourceAsStream(obj,pfad)), null, null).getOpenAPI(); - } + public static OpenAPI getApi(Object obj, String path) { + ParseOptions opts = new ParseOptions(); + opts.setResolve(true); + try (InputStream is = getResourceAsStream(obj, path)) { + return new OpenAPIParser().readContents(readInputStream(is), null, opts).getOpenAPI(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } @@ - public static InputStream getResourceAsStream(Object obj, String fileName) { - return obj.getClass().getResourceAsStream(fileName); - } + public static InputStream getResourceAsStream(Object obj, String fileName) { + return obj.getClass().getResourceAsStream(fileName); + }core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml (2)
34-53: Add minimal schemas to path/operation-level parameters for spec validity.Parameters foo/bar/baz/zap/pap/rap lack schema/content; OAS 3.1 requires one of them. Add a minimal schema (e.g., type: string) to keep the resource formally valid while still testing “required”.
/required: parameters: - name: foo in: query required: true + schema: + type: string - name: bar in: query required: false + schema: + type: string - name: baz in: query # optional by default + schema: + type: string get: parameters: - name: zap in: query required: true + schema: + type: string - name: pap in: query required: false + schema: + type: string - name: rap in: query + schema: + type: string
66-66: Add trailing newline to satisfy linters.YAMLlint flags missing newline at EOF.
- type: boolean + type: boolean +core/src/test/java/com/predic8/membrane/core/openapi/validators/BooleanTest.java (1)
55-55: Remove debug print from tests.System.out noise makes CI logs harder to read; rely on assertion messages when needed.
- System.out.println("errors = " + errors);core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
75-77: Harden getPath against nulls and document behavior.Avoid NPEs when api or api.getPaths() is null; return null and let callers decide.
- public static PathItem getPath(OpenAPI api, String path) { - return api.getPaths().get(path); - } + /** + * Returns the PathItem for the exact path key, or null if not present. + * Caller must handle templated paths and base paths if applicable. + */ + public static PathItem getPath(OpenAPI api, String path) { + if (api == null || api.getPaths() == null) return null; + return api.getPaths().get(path); + }core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java (1)
23-33: Add a negative case to guard against wrong-type/unknown params.Current tests only assert success. Add a failing case to ensure validator flags unexpected values (e.g., filter=abc when schema disallows, or unknown param).
Example additional test (place within class):
@Test void unknownParamIsRejected() { ValidationErrors err = validator.validate(get().path("/search?unknown=1")); assertTrue(err.size() > 0, "Unknown query parameter should be reported"); }core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterTest.java (1)
45-51: Expand coverage to null/empty/exponent forms.Add cases for "null", empty string, and scientific notation to match real query inputs.
Additional tests:
@Test void explicitNull() { JsonNode node = asJson("null"); assertTrue(node.isNull()); } @Test void emptyString() { JsonNode node = asJson(""); assertTrue(node.isTextual()); assertEquals("", node.textValue()); } @Test void scientific() { JsonNode node = asJson("3e4"); assertTrue(node.isDouble()); assertEquals(3e4, node.doubleValue(), 0.0001); }core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java (2)
31-33: Remove debug print from tests.Keep test output clean; failures will show diffs.
- System.out.println("err = " + err);
29-34: Add a negative case for empty value (not number or null).Schema permits number or null; an empty string should fail.
Example:
@Test void emptyValueIsInvalid() { ValidationErrors err = validator.validate(get().path("/array?number=")); assertTrue(err.size() > 0, "Empty value should not satisfy number|null"); }core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeFalseTest.java (2)
23-32: Drop console output from setupSystem.out in tests is noisy and hides failures in CI logs. Prefer a logger at DEBUG or remove.
- System.out.println(info.getDisplayName() + - " validator@" + System.identityHashCode(validator)); + // optional: log at debug if really needed
49-53: Remove debug prints inside testsPrinting ValidationErrors adds noise; assertions already capture failures.
- System.out.println("err = " + err); + // no-opAlso applies to: 62-65, 83-88
core/src/main/java/com/predic8/membrane/core/openapi/validators/OperationValidator.java (1)
52-52: Avoid passing PathItem twice to QueryParameterValidatorConstructor already receives pathItem; method also receives it. Pick one to avoid API duplication.
- errors.add(new QueryParameterValidator(api,pathItem).validateQueryParameters(ctx, req, pathItem, operation)); + errors.add(new QueryParameterValidator(api, pathItem).validateQueryParameters(ctx, req, operation));Note: adjust QueryParameterValidator.validateQueryParameters(...) accordingly.
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPIUtilTest.java (1)
71-74: Don’t commit empty testsEither implement or mark as @disabled to avoid false positives.
- @Test - void getOperation() { - - } + @Disabled("TODO: implement getOperation test") + @Test + void getOperation() {}core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (2)
52-52: Remove println from testsDrop System.out in tests to keep CI logs clean.
- System.out.println("errors = " + errors); + // no-op
82-85: Avoid double‑brace Map initialization in testsUse Map.of(...) for brevity and to avoid anonymous classes.
- new HashMap<>() {{ - put("api-key", new TextNode("234523")); - }}, + new HashMap<>(Map.of("api-key", new TextNode("234523"))), @@ - new HashMap<>() {{ - put("bar", new TextNode("2315124")); - }}, + new HashMap<>(Map.of("bar", new TextNode("2315124"))),Also applies to: 96-99
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleTest.java (3)
24-29: Don’t keep commented assertions—either implement or @disabledKeep the suite signal-to-noise high.
-// ValidationErrors errors = ... -// System.out.println(errors); -// assertEquals(0,errors.size()); +// @Disabled("TODO: add assertions for /simple case")
34-35: Remove debug print in testsAvoid console noise in passing tests.
- System.out.println("errors = " + errors); + // no-op
70-71: Placeholder testEmpty test method should be implemented or disabled.
- void array() {} + @Disabled("TODO: implement") + void array() {}core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (1)
51-73: Optional: numeric parsing order and BigDecimalCurrent int→long→double is fine; consider BigDecimal for preserving precision if needed by downstream validators.
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParamsTest.java (1)
50-57: Remove debug print from unknownQueryParam testKeep tests silent on pass.
- System.out.println("errors = " + errors); + // no-opcore/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (3)
147-151: Null-safe filter for API key query schemes.
scheme.getIn()can be null. Use constant-first equals.- return api.getComponents().getSecuritySchemes().values().stream() - .filter(scheme -> scheme != null && scheme.getType() != null && scheme.getType().equals(APIKEY) && scheme.getIn().equals(QUERY)) + return api.getComponents().getSecuritySchemes().values().stream() + .filter(scheme -> scheme != null && APIKEY.equals(scheme.getType()) && QUERY.equals(scheme.getIn())) .map(SecurityScheme::getName) .toList();
133-141: Don’t mutateqparams; report unsupported keys deterministically.Removing entries from the caller’s map is surprising. Build a derived set and use
join(...)for consistent messaging.- ValidationErrors validateAdditionalQueryParameters(ValidationContext ctx, Map<String, JsonNode> qparams, OpenAPI api) { - securitySchemeApiKeyQueryParamNames(api).forEach(qparams::remove); - - if (!qparams.isEmpty()) { - return ValidationErrors.create(ctx.entityType(QUERY_PARAMETER), "There are query parameters that are not supported by the API: " + qparams.keySet()); - } + ValidationErrors validateAdditionalQueryParameters(ValidationContext ctx, Map<String, JsonNode> qparams, OpenAPI api) { + Set<String> whitelist = new HashSet<>(securitySchemeApiKeyQueryParamNames(api)); + Set<String> unsupported = qparams.keySet().stream() + .filter(k -> !whitelist.contains(k)) + .collect(toCollection(LinkedHashSet::new)); + if (!unsupported.isEmpty()) { + return ValidationErrors.create(ctx.entityType(QUERY_PARAMETER), + "There are query parameters that are not supported by the API: " + join(unsupported)); + } return ValidationErrors.empty(); }
153-168: Optional: be tolerant to malformed percent-encoding and stray separators.The decoder throws
IllegalArgumentExceptionon bad%sequences; current code would abort parsing. The earlier diff adds a try/catch and skips empty keys—keep it to avoid noisy failures from malformed URLs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java(0 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractBodyValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/OperationValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeFalseTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPIUtilTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/BooleanTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(4 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParamsTest.java(4 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/exceptions/ExceptionInterceptorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterTest.java(1 hunks)core/src/test/resources/junit-platform.properties(1 hunks)core/src/test/resources/openapi/specs/customers.yml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/explode.yaml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/simple-null.yaml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml(1 hunks)
🔥 Files not summarized due to errors (1)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java: Error: Server error: no LLM provider could handle the message
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java
🧰 Additional context used
🧬 Code graph analysis (10)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterTest.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (1)
AbstractParameter(15-74)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleTest.java (2)
core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-91)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (1)
Nested(115-141)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-91)
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java (1)
Utils(43-307)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeFalseTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-91)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-91)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPIUtilTest.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(30-78)core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-91)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (3)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(30-78)core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-91)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(43-186)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParamsTest.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-132)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (3)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (1)
AbstractParameter(15-74)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml
[high] 1-67: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-67: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 20-24: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
core/src/test/resources/openapi/specs/oas31/parameters/simple-null.yaml
[high] 1-21: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-21: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 14-18: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml
[high] 1-41: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-41: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 19-23: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
core/src/test/resources/openapi/specs/oas31/parameters/explode.yaml
[high] 1-28: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-28: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
🪛 YAMLlint (1.37.1)
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml
[error] 66-66: no new line character at the end of file
(new-line-at-end-of-file)
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml
[warning] 34-34: too few spaces after comma
(commas)
[error] 40-40: no new line character at the end of file
(new-line-at-end-of-file)
core/src/test/resources/openapi/specs/oas31/parameters/explode.yaml
[error] 27-27: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (4)
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java (1)
31-31: Whitespace-only change.No action.
core/src/test/resources/openapi/specs/oas31/parameters/simple-null.yaml (1)
14-17: LGTM; union type for array items is correct for OAS 3.1.Spec accurately models number-or-null with JSON Schema’s type array.
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPIUtilTest.java (1)
31-33: LGTM: switched to TestUtils.getApi(...)The indirection via TestUtils simplifies resource loading.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameter.java (1)
24-42: JDK 21 List.getFirst() usage and empty-values handlingvalues.getFirst() requires JDK 21; likely incompatible if the project targets 17. Also guard for empty values and trim items.
- String[] items = values.getFirst().split(","); + if (values == null || values.isEmpty()) { + return Stream.empty(); + } + String[] items = values.get(0).split(","); @@ - return Arrays.stream(items); + return Arrays.stream(items).map(String::trim);Verification script (checks JDK baseline and getFirst usage):
Question: For foo=null you return MissingNode (treated as “absent”). Is this intended versus a JSON null array value? Please confirm desired semantics for OAS 3.1 type: [array|null].
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractBodyValidator.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Outdated
Show resolved
Hide resolved
|
/ok-to-test |
…into openapi-validator-explode-2155
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameter.java (1)
10-13: Empty-value guard: resolved.Thanks for adding the null-node handling when no value is present.
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (3)
60-62: LGTM: query extraction is correct.Confirmed by test and prior discussion; request.getPath() retains the query.
51-58: Null-safe check forrequired.Parameter.getRequired() is nullable; current filter can NPE.
- return parameters.stream() - .filter(Parameter::getRequired).map(Parameter::getName).collect(toSet()); + return parameters.stream() + .filter(p -> Boolean.TRUE.equals(p.getRequired())) + .map(Parameter::getName) + .collect(toSet());
176-178: Detect query params byin="query"(notinstanceof QueryParameter).Refs and generic Parameter instances would be missed otherwise.
- Set<Parameter> getQueryParameters (PathItem pathItem, Operation operation){ - return getAllParameter(operation).stream().filter(p -> p instanceof QueryParameter).collect(toSet()); - } + Set<Parameter> getQueryParameters (PathItem pathItem, Operation operation){ + return getAllParameter(operation).stream() + .filter(p -> p != null && "query".equalsIgnoreCase(p.getIn())) + .collect(toCollection(LinkedHashSet::new)); + }core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (1)
19-25: Fix NPE: parameter.getExplode() may be null; default per style/in before unboxing.OpenAPI’s explode defaults depend on style (form=true; others=false). Guard against null and compute the default; avoid unsafe auto-unboxing.
Apply:
- public static AbstractParameter instance(String type, Parameter parameter) { - AbstractParameter ap = getParameter(type); - ap.type = type; - ap.explode = parameter.getExplode(); - - return ap; - } + public static AbstractParameter instance(String type, Parameter parameter) { + AbstractParameter ap = getParameter(type); + ap.type = type; + Boolean explodeVal = parameter.getExplode(); + if (explodeVal == null) { + Parameter.StyleEnum style = parameter.getStyle(); + if (style == null) { + String in = parameter.getIn(); + style = ("query".equalsIgnoreCase(in) || "cookie".equalsIgnoreCase(in) || in == null) + ? Parameter.StyleEnum.FORM + : Parameter.StyleEnum.SIMPLE; + } + explodeVal = (style == Parameter.StyleEnum.FORM); + } + ap.explode = explodeVal; // safe unboxing + return ap; + }
🧹 Nitpick comments (15)
core/src/test/resources/openapi/specs/oas31/parameters/overwrite.yaml (1)
30-30: Add missing newline at EOF.Fix YAML lint error to keep CI happy.
- description: OK + description: OK +core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (2)
49-51: Close the InputStream to avoid a leak in tests.Wrap the resource stream in try-with-resources.
- public static JsonNode getYAMLResource(Object thisObj, String path) throws IOException { - return omYaml.readTree(getResourceAsStream(thisObj, path)); - } + public static JsonNode getYAMLResource(Object thisObj, String path) throws IOException { + try (InputStream is = getResourceAsStream(thisObj, path)) { + if (is == null) throw new FileNotFoundException("Resource not found: " + path); + return omYaml.readTree(is); + } + }
78-80: Avoid undefined behavior on empty map; use iterator and assert size.The current approach throws if the map is empty and relies on unspecified iteration order.
- public static OpenAPIRecord getSingleOpenAPIRecord(Map<String, OpenAPIRecord> m) { - return (OpenAPIRecord) m.values().toArray()[0]; - } + public static OpenAPIRecord getSingleOpenAPIRecord(Map<String, OpenAPIRecord> m) { + if (m.size() != 1) + throw new IllegalArgumentException("Expected exactly one OpenAPIRecord, found: " + m.size()); + return m.values().iterator().next(); + }core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/OverwriteParameterTest.java (1)
16-28: Make assertions less brittle to message wording.Checking for exact fragments like "of [number]" and "of [boolean]" is fragile. Prefer asserting on error type/code or at least contains "number"/"boolean" without brackets, if no structured code exists.
- assertTrue(err.get(0).getMessage().contains("of [number]")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("number")); ... - assertTrue(err.get(0).getMessage().contains("of [boolean]")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("boolean"));core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml (1)
80-80: Add missing newline at EOF.Silences YAML lint warning.
- type: boolean + type: boolean +core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleQueryParametersTest.java (4)
23-29: Remove commented code or mark test disabled.An empty test with commented logic reduces signal-to-noise.
- @Test - void string() { -// ValidationErrors errors = validator.validate(Request.get().path("/simple?empty=&string=blue&array=blue,black,brown&object=R=100,G=200,B=150")); - // ValidationErrors errors = validator.validate(Request.get().path("/simple?defau=")); -// System.out.println(errors); -// assertEquals(0,errors.size()); - } + @Disabled("TODO: add string param assertions") + @Test + void string() {}
31-36: Drop debug print from tests.Avoid stdout in unit tests unless diagnosing failures.
- ValidationErrors errors = validator.validate(get().path("/array?number=1&number=2")); - System.out.println("errors = " + errors); - assertEquals(0, errors.size()); + ValidationErrors errors = validator.validate(get().path("/array?number=1&number=2")); + assertEquals(0, errors.size());
38-41: Replace placeholder with @disabled or implement.A test that always passes on
assertTrue(true)is noise.- @Test - void parameterIsNotDescribed() { - assertTrue(true); //TODO - } + @Disabled("TODO: implement behavior when parameter is not described") + @Test + void parameterIsNotDescribed() {}
76-81: Consider making the message assertion less brittle.If messages change, keep intent by only checking presence of the expected type token.
- assertTrue(err.get(0).getMessage().contains("does not match any of [number]")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("number"));core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (1)
116-142: Useful utility coverage; consider one negative lookup test.Add a test asserting
getQueryParameter(..., "absent")returns null.@Test void get_QueryParameter_WithName() { PathItem pathItem = getPathItem("/array"); QueryParameterValidator qpv = new QueryParameterValidator(null, pathItem); assertEquals("String param", qpv.getQueryParameter(pathItem, pathItem.getGet(), "string").getDescription()); } + + @Test + void get_QueryParameter_Absent_ReturnsNull() { + PathItem pathItem = getPathItem("/array"); + QueryParameterValidator qpv = new QueryParameterValidator(null, pathItem); + assertNull(qpv.getQueryParameter(pathItem, pathItem.getGet(), "absent")); + }core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (2)
27-34: Remove redundant type assignment.getParameter() already sets the type; instance() sets it again.
- parameter.type = typeName; return parameter;
42-66: Prefer Long for 32–63 bit integers and avoid double parsing.Slightly cleaner and avoids parsing the same BigInteger twice.
- try { - // handles +/- and no decimals - if (!value.contains(".") && !value.contains("e") && !value.contains("E")) { - return new java.math.BigInteger(value).bitLength() <= 31 - ? FACTORY.numberNode(Integer.parseInt(value)) - : FACTORY.numberNode(new java.math.BigInteger(value)); - } - } catch (NumberFormatException ignore) { /* try decimal */ } + try { + if (!value.contains(".") && !value.contains("e") && !value.contains("E")) { + java.math.BigInteger bi = new java.math.BigInteger(value); + int bl = bi.bitLength(); + if (bl <= 31) return FACTORY.numberNode(bi.intValue()); + if (bl <= 63) return FACTORY.numberNode(bi.longValue()); + return FACTORY.numberNode(bi); + } + } catch (NumberFormatException ignore) { /* try decimal */ }core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java (1)
49-65: Good: de-dup by (name|in) with operation-level override.This matches OAS override rules and preserves order. Consider normalizing
into lowercase and guarding null.- Stream.concat(pathParams.stream(), opParams.stream()) - .filter(Objects::nonNull) - .forEach(p -> byKey.put(p.getName() + "|" + p.getIn(), p)); + Stream.concat(pathParams.stream(), opParams.stream()) + .filter(Objects::nonNull) + .forEach(p -> { + String in = p.getIn() == null ? "" : p.getIn().toLowerCase(Locale.ROOT); + byKey.put(p.getName() + "|" + in, p); + });core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (2)
154-169: Preserve order, tolerate bad percent-encoding, and ignore empty keys.Improves robustness for real-world query strings.
- private static @NotNull Map<String, List<String>> getParameterMapFromQuery (String query){ - Map<String, List<String>> parameterMap = new HashMap<>(); + private static @NotNull Map<String, List<String>> getParameterMapFromQuery (String query){ + Map<String, List<String>> parameterMap = new LinkedHashMap<>(); if (query == null) { return parameterMap; } for (String p : query.split("&")) { Matcher m = QUERY_PARAMS_PATTERN.matcher(p); if (m.matches()) { - String key = decode(m.group(1), UTF_8); - String value = decode(m.group(2), UTF_8); - List<String> ab = parameterMap.computeIfAbsent(key, k -> new ArrayList<>()); - ab.add(value); + String rawKey = m.group(1); + String rawValue = m.group(2); + String key, value; + try { + key = decode(rawKey, UTF_8); + value = decode(rawValue, UTF_8); + } catch (IllegalArgumentException iae) { + key = rawKey; + value = rawValue; + } + if (key.isEmpty()) continue; + parameterMap.computeIfAbsent(key, k -> new ArrayList<>()).add(value); } } return parameterMap; }
144-152: Null-safe filter for API key schemes’in.Avoid NPE when
inis missing.- return api.getComponents().getSecuritySchemes().values().stream() - .filter(scheme -> scheme != null && scheme.getType() != null && scheme.getType().equals(APIKEY) && scheme.getIn().equals(QUERY)) + return api.getComponents().getSecuritySchemes().values().stream() + .filter(scheme -> scheme != null + && scheme.getType() == APIKEY + && scheme.getIn() == QUERY) .map(SecurityScheme::getName) .toList();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleQueryParametersTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(4 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/OverwriteParameterTest.java(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/overwrite.yaml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameter.java
- core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
🧬 Code graph analysis (5)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleQueryParametersTest.java (3)
core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-95)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java (2)
Nested(23-41)Nested(26-40)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (1)
Nested(114-143)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-95)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(30-78)core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-95)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(43-180)core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-132)
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java (1)
Utils(43-307)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (1)
AbstractParameter(10-68)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/oas31/parameters/overwrite.yaml
[high] 1-31: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-31: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml
[high] 1-81: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-81: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 20-24: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 YAMLlint (1.37.1)
core/src/test/resources/openapi/specs/oas31/parameters/overwrite.yaml
[error] 30-30: no new line character at the end of file
(new-line-at-end-of-file)
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml
[error] 80-80: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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 (5)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java (1)
23-39: LGTM — good coverage of explode behavior and unknown param rejection.core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (2)
76-85: Good positive case covering security scheme allow‑list.Using TextNode matches production types and keeps the test realistic.
109-112: LGTM — query string extraction sanity check.Covers the utility seam used widely in validator code.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameter.java (1)
8-15: No change required — project targets Java 21; values.getFirst() is valid. Root pom defines <javac.source>21/<javac.target>21 and the maven‑compiler‑plugin uses those properties, so the getFirst() call compiles; changing to get(0) is unnecessary.core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
74-91: Nice: switched to multi-value List parsing and removed Set dedup.This preserves duplicates and order for array validation.
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
85-93: Null stream from classpath will NPE; validate before parsingMirror the guard you added in getYAMLResource().
public static OpenAPI getApi(Object obj, String path) { ParseOptions opts = new ParseOptions(); opts.setResolve(true); try (InputStream is = getResourceAsStream(obj, path)) { - return new OpenAPIParser().readContents(readInputStream(is), null, opts).getOpenAPI(); + if (is == null) { + throw new IllegalArgumentException("Resource not found on classpath: " + path); + } + return new OpenAPIParser().readContents(readInputStream(is), null, opts).getOpenAPI(); } catch (IOException e) { throw new UncheckedIOException(e); } }core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java (1)
81-91: Null-safe $ref resolution for schemas
api.getComponents()or.getSchemas()can be null; avoid NPE on valid specs lacking components.protected Schema<?> getSchema(Parameter p) { Schema<?> schema = p.getSchema(); if (schema == null) { return null; } if (schema.get$ref() != null) { - String componentLocalNameFromRef = getComponentLocalNameFromRef(schema.get$ref()); - return api.getComponents().getSchemas().get(componentLocalNameFromRef); + if (api == null || api.getComponents() == null || api.getComponents().getSchemas() == null) { + return null; + } + String componentLocalNameFromRef = getComponentLocalNameFromRef(schema.get$ref()); + return api.getComponents().getSchemas().get(componentLocalNameFromRef); } return schema; }
🧹 Nitpick comments (10)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterTest.java (1)
52-57: Prefer BigDecimal assertions over double tolerancesAvoid float rounding by asserting on
decimalValue()with BigDecimal.- assertTrue(node.isBigDecimal()); - assertEquals(3.14, node.doubleValue(), 0.0001); + assertTrue(node.isBigDecimal()); + assertEquals(0, node.decimalValue().compareTo(new java.math.BigDecimal("3.14"))); @@ - assertTrue(node.isBigDecimal()); - assertEquals(3e4, node.doubleValue(), 0.0001); + assertTrue(node.isBigDecimal()); + assertEquals(0, node.decimalValue().compareTo(new java.math.BigDecimal("3e4")));Also applies to: 59-64
core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
81-83: Avoid array allocation; use iterator for first valueSlightly cleaner and avoids creating an Object[].
- public static OpenAPIRecord getSingleOpenAPIRecord(Map<String, OpenAPIRecord> m) { - return (OpenAPIRecord) m.values().toArray()[0]; - } + public static OpenAPIRecord getSingleOpenAPIRecord(Map<String, OpenAPIRecord> m) { + return m.values().iterator().next(); + }core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml (1)
20-24: Optional: constrain array sizes if you want lints quietAdd
maxItemsin component array schemas to silence CKV_OPENAPI_21, if desirable for CI.core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleQueryParametersTest.java (4)
37-43: Remove dead/commented test or implement itKeep the suite signal-to-noise high: either flesh this out or drop it.
I can wire a concrete case for mixed types in one call if helpful.
51-54: Replace placeholder with a real assertionExercise the “unknown query param” path or required param validation; a no-op test obscures coverage.
- void parameterIsNotDescribed() { - assertTrue(true); //TODO - } + void parameterIsNotDescribed() { + var errors = validator.validate(get().path("/array?unknown=1")); + assertEquals(1, errors.size()); + assertTrue(errors.get(0).getMessage().toLowerCase().contains("not supported") + || errors.get(0).getMessage().toLowerCase().contains("invalid")); + }
83-85: Remove empty test method or add a caseEmpty @test blocks can mislead about coverage.
- @Test - void array() {} + @Test + void array() { + assertEquals(0, validator.validate(get().path("/array?string=a&string=b")).size()); + }
89-94: Assertion is brittle on exact message textPrefer checking structured fields or a stable substring/code to reduce flakiness across message changes.
- assertTrue(err.get(0).getMessage().contains("does not match any of [number]")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("number"), + "Expected type mismatch mentioning 'number', got: " + err.get(0).getMessage());core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (3)
35-37: Guard against null parameter lists on operationsSome operations may have no parameters; avoid NPE.
- private static boolean operationHasParamWithName(Operation get, String name) { - return get.getParameters().stream().anyMatch(param -> param.getName().equals(name)); - } + private static boolean operationHasParamWithName(Operation get, String name) { + return get.getParameters() != null + && get.getParameters().stream().anyMatch(param -> name.equals(param.getName())); + }
77-85: No need to copy Map.of(...) into HashMapMethod under test doesn’t mutate; pass the immutable map directly.
- assertTrue(queryParameterValidator.validateAdditionalQueryParameters( - new ValidationContext(), - new HashMap<>(Map.of("api-key", new TextNode("234523"))), + assertTrue(queryParameterValidator.validateAdditionalQueryParameters( + new ValidationContext(), + Map.of("api-key", new TextNode("234523")), new OpenAPI().components(new Components() {{ addSecuritySchemes("schemaA", new SecurityScheme().type(APIKEY).name("api-key").in(QUERY)); }}) ).isEmpty());
90-96: Same here—avoid unnecessary copy- new HashMap<>(Map.of("bar", new TextNode("2315124"))), + Map.of("bar", new TextNode("2315124")),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameter.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeFalseTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleQueryParametersTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(4 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/OverwriteParameterTest.java(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/overwrite.yaml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeFalseTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameter.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/OverwriteParameterTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameter.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java
🧬 Code graph analysis (4)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterTest.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (1)
AbstractParameter(24-89)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleQueryParametersTest.java (2)
core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-98)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (1)
Nested(114-150)
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java (1)
Utils(43-307)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(30-78)core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
TestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(43-180)core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-132)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/oas31/parameters/overwrite.yaml
[high] 1-32: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-32: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml
[high] 1-82: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-82: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 20-24: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (9)
core/src/test/resources/openapi/specs/oas31/parameters/overwrite.yaml (2)
18-27: Spec correctly models (name,in) overwrite semanticsOperation-level
a(query/number) overrides path-levela(query/string), andb(header) correctly does not overrideb(query). Looks good for the intended tests.
1-30: Ignore security lint for test specs or add a no-auth stubStatic checkers flag missing
securitysections. Since this is a focused unit-test spec, either ignore those findings or add a minimalcomponents/securitySchemesand top-level emptysecurity: []to placate scanners.
[raise_nitpick_refactor]core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterTest.java (1)
23-50: Solid coverage of booleans and integer/long parsingThese cases mirror AbstractParameter.asJson behavior well.
core/src/test/java/com/predic8/membrane/core/openapi/util/TestUtils.java (1)
49-53: Good: explicit null check for resource streamPrevents hidden NPEs and gives a clear error.
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml (1)
8-34: Spec is apt for explode array coverageDefinitions for number/string/bool arrays with explode=true suit the tests.
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (2)
109-112: LGTM: getQueryString covers the URI extraction pathGood minimal check for query extraction behavior.
114-150: Utility tests read well and match new APIsNice coverage for parameter lookup and required set computation.
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java (1)
52-67: Parameter merge with (name|in) override looks correctDedup + precedence logic aligns with OpenAPI rules.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameter.java (1)
11-14: Fail fast — getJson() must not return nullReturning null risks NPEs downstream; throw explicitly until object-parameter parsing is implemented.
@Override public JsonNode getJson() throws JsonProcessingException { - return null; + throw new UnsupportedOperationException("Object parameter type is not supported yet."); }If AbstractParameter.getParameter() can return ObjectParameter, ensure callers handle this exception or gate object-parameter support with tests.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptorTest.java (1)
84-87: Bug: assertNull used with arguments swapped; test always passes.Currently asserts null on literal null and treats the actual value as a message.
Apply:
- assertNull(null, interceptor3Server.getMatchingBasePath(exc)); + assertNull(interceptor3Server.getMatchingBasePath(exc));core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (3)
125-127: NPE: discriminator property may be absent.node.get(propertyName) can be null; asText() would NPE. Return null when missing.
- private String getBaseSchemaName(JsonNode node, String propertyName) { - return node.get(propertyName).asText(); - } + private String getBaseSchemaName(JsonNode node, String propertyName) { + JsonNode v = node.get(propertyName); + return v == null || v.isNull() ? null : v.asText(); + }
151-169: additionalProperties handling incorrect for boolean true.The else-branch currently errors for both true and false. Allow when true; error only when false.
- private ValidationErrors validateAddionalProperties(ValidationContext ctx, JsonNode node) { + private ValidationErrors validateAddionalProperties(ValidationContext ctx, JsonNode node) { if (schema.getAdditionalProperties() == null) return null; Map<String, JsonNode> additionalProperties = getAddionalProperties(node); if (additionalProperties.size() == 0) return null; ValidationErrors errors = new ValidationErrors(); - if (schema.getAdditionalProperties() instanceof Schema) { + if (schema.getAdditionalProperties() instanceof Schema) { additionalProperties.forEach((propName, value) -> errors.add(new SchemaValidator(api, (Schema) schema.getAdditionalProperties()).validate(ctx.addJSONpointerSegment(propName), value))); return errors; - } - - return errors.add(ctx.statusCode(400), format("The object has the additional %s: %s .But the schema does not allow additional properties.", getPropertyOrIes(additionalProperties.keySet()), joinByComma(additionalProperties.keySet()))); + } else if (schema.getAdditionalProperties() instanceof Boolean b) { + if (b) { + return null; // allowed + } + return errors.add(ctx.statusCode(400), + format("The object has the additional %s: %s. But the schema does not allow additional properties.", + getPropertyOrIes(additionalProperties.keySet()), + joinByComma(additionalProperties.keySet()))); + } + // Unknown type – be safe and return an error + return errors.add(ctx.statusCode(400), + format("Unsupported additionalProperties type: %s", schema.getAdditionalProperties().getClass().getSimpleName())); }
179-188: Null-safe properties lookup for additionalProperties check.schema.getProperties() may be null; treat as empty.
- private Map<String, JsonNode> getAddionalProperties(JsonNode node) { - Map<String, JsonNode> addionalProperties = new HashMap<>(); + private Map<String, JsonNode> getAddionalProperties(JsonNode node) { + Map<String, JsonNode> addionalProperties = new HashMap<>(); for (Iterator<String> it = node.fieldNames(); it.hasNext(); ) { String propName = it.next(); - if (!schema.getProperties().containsKey(propName)) { + Map<String, Schema<?>> props = schema.getProperties(); + boolean defined = props != null && props.containsKey(propName); + if (!defined) { addionalProperties.put(propName, node.get(propName)); } } return addionalProperties; }core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java (2)
26-43: canValidate rejects valid integral numbers (Long/BigInteger/JsonNode integral)Using only Integer/parseLong misses integral JsonNodes and long-range values.
Apply this diff:
+import java.math.BigInteger; + @Override public String canValidate(Object obj) { - if (obj instanceof JsonNode j) { - obj = j.numberValue(); - } - - try { - if (obj instanceof String s) { - parseLong(s); - return INTEGER; - } - if (obj instanceof Integer) - return INTEGER; - return null; - } catch (Exception e) { - return null; - } + if (obj instanceof JsonNode j) { + return j.isIntegralNumber() ? INTEGER : null; + } + if (obj instanceof String s) { + try { + new BigInteger(s); + return INTEGER; + } catch (NumberFormatException ex) { + return null; + } + } + if (obj instanceof Byte || obj instanceof Short || obj instanceof Integer || obj instanceof Long || obj instanceof BigInteger) { + return INTEGER; + } + return null; }
52-76: validate throws at runtime for valid integral types (e.g., Long); must accept all integral NumbersCurrent code throws RuntimeException for Long/BigInteger. Return success for integral types; error for non-integral.
Apply this diff:
- public ValidationErrors validate(ValidationContext ctx, Object value) { - - if (value instanceof JsonNode) { - value = ((JsonNode) value).numberValue(); - } - - try { - if (value instanceof String) { - parseLong((String) value); - return null; - } - if (value instanceof Integer) { - return null; - } - if (value instanceof Body) - return null; - - if (value != null) - throw new RuntimeException("Do not know type " + value.getClass()); - throw new RuntimeException("Value is null!"); - - } catch (NumberFormatException e) { - return ValidationErrors.create(ctx.schemaType("integer"), String.format("%s is not an integer.", value)); - } + public ValidationErrors validate(ValidationContext ctx, Object value) { + if (value instanceof JsonNode j) { + return j.isIntegralNumber() ? null + : ValidationErrors.create(ctx.schemaType("integer"), + String.format("%s is not an integer.", j.asText())); + } + if (value instanceof String s) { + try { + new BigInteger(s); + return null; + } catch (NumberFormatException e) { + return ValidationErrors.create(ctx.schemaType("integer"), + String.format("%s is not an integer.", s)); + } + } + if (value instanceof Byte || value instanceof Short || value instanceof Integer || value instanceof Long || value instanceof java.math.BigInteger) { + return null; + } + if (value instanceof Body) { + return null; + } + if (value == null) { + return ValidationErrors.create(ctx.schemaType("integer"), "null is not an integer."); + } + return ValidationErrors.create(ctx.schemaType("integer"), + String.format("Do not know type %s for integer validation.", value.getClass().getName())); }core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (1)
81-83: Assertion checks the wrong direction; test isn’t validating requirementYou need to assert the API description does NOT contain the conversion notice, not the other way around.
Apply this diff:
- assertFalse("OpenAPI description was converted to OAS 3 from Swagger 2 by Membrane API Gateway.".contains( - rec.api.getInfo().getDescription() - )); + assertFalse(rec.api.getInfo().getDescription().contains( + "OpenAPI description was converted to OAS 3 from Swagger 2 by Membrane API Gateway." + ));core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberValidator.java (1)
31-77: Bug: validate() throws on numeric values (Number not handled)validate() lacks a branch for Number and will throw “Should never happen!” for numeric inputs. Also, using Double.parseDouble() accepts “NaN”/“Infinity” which should not be valid JSON numbers for parameter coercion; prefer BigDecimal for strictness and precision.
public class NumberValidator implements JsonSchemaValidator { @@ @Override public String canValidate(Object value) { try { if (value instanceof Number) { return NUMBER; } if (value instanceof TextNode) { return null; } if (value instanceof JsonNode jn) { new BigDecimal((jn).asText()); return NUMBER; } if (value instanceof String s) { - parseDouble(s); + new BigDecimal(s); return NUMBER; } } catch (NumberFormatException ignored) { } return null; } @@ @Override public ValidationErrors validate(ValidationContext ctx, Object value) { try { + if (value instanceof Number) { + return null; + } if (value instanceof TextNode) { return ValidationErrors.create(ctx.schemaType(NUMBER), String.format("%s is not a number.", value)); } if (value instanceof JsonNode jn) { // Not using double prevents from losing fractions new BigDecimal(jn.asText()); return null; } if (value instanceof String s) { - parseDouble(s); + new BigDecimal(s); return null; } } catch (NumberFormatException ignored) { return ValidationErrors.create(ctx.schemaType(NUMBER), String.format("%s is not a number.", value)); } throw new RuntimeException("Should never happen!"); } }core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java (1)
95-101: Nullable with absent type should not produce an errorWhen schema has no type(s) and is nullable, a null value should be accepted; returning an error here contradicts the behavior in the else-branch and JSON Schema semantics where no type constraint means any type.
- if (schemaHasNoTypeAndTypes(schema.getType())) { - if ((value == null || value instanceof NullNode) && isNullable()) - return ValidationErrors.create(ctx, "Value is null and no type is set."); - } else { - if ((value == null || value instanceof NullNode) && isNullable()) - return errors; - } + if ((value == null || value instanceof NullNode) && isNullable()) + return errors;
🧹 Nitpick comments (49)
core/src/test/java/com/predic8/membrane/test/TestUtil.java (1)
58-60: Optional: Rename test method to avoid name clash with utility.Overloaded name
getPathFromResource()for both the util and the test is a small readability hit. Consider renaming the test togetPathFromResource_resolvesLogConfig().core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptorTest.java (1)
100-101: Replace hardcoded '27' with a dynamic size checkReplace the magic number with interceptor.apis.size(); also update the constructor's assertion in the same file.
File: core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptorTest.java — constructor (line ~72) and getApiDirectory (line ~100)
Apply this diff:
- assertTrue(OpenAPITestUtils.getMapFromResponse(get).size() >= 27); + assertTrue(OpenAPITestUtils.getMapFromResponse(get).size() >= interceptor.apis.size());core/src/main/java/com/predic8/membrane/core/openapi/validators/NullValidator.java (1)
38-46: Contract clarity: returning null vs empty ValidationErrors.This validate() returns null on success, while other top-level validators return an empty ValidationErrors. Please confirm callers tolerate null here; otherwise consider returning an empty container for consistency.
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptorTest.java (1)
116-116: Remove noisy stdout in tests.Drop println to keep CI logs clean.
- System.out.println("exc = " + exc.getRequest().getHeader()); + // no-opcore/src/test/java/com/predic8/membrane/core/openapi/util/JsonTestUtil.java (1)
26-30: Visibility of FACTORY.FACTORY is protected and unused in this class. If other tests need it, make it public; otherwise remove to avoid dead code.
- protected static final JsonNodeFactory FACTORY = JsonNodeFactory.instance; + // Make public if consumed by other tests; otherwise remove. + public static final JsonNodeFactory FACTORY = JsonNodeFactory.instance;core/src/main/java/com/predic8/membrane/core/openapi/validators/JsonSchemaValidator.java (1)
19-48: Public interface promotion LGTM.Consider tightening the contract in Javadoc: explicitly state whether a null return from validate() denotes “no errors” or if an empty ValidationErrors is expected, for consistency across validators.
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml (3)
57-57: YAML style lint: space after comma.Fix inline array spacing to satisfy yamllint.
- type: [array,"null"] + type: [array, "null"]
61-63: Add trailing newline.Satisfy “no-newline-at-EOF” linter rule.
6-49: Security and array bounds warnings (tests).Checkov flags missing security and maxItems. Since this is a test fixture, it may be intentional; if not, consider adding a minimal security: [] and maxItems to arrays to silence scans.
Also applies to: 50-63
core/src/main/java/com/predic8/membrane/core/openapi/validators/StringValidator.java (1)
201-205: Avoid ClassCastException when formatting enum values.If the enum list contains non-strings, String.join may throw. Map to String defensively.
- private String getEnumValues() { - //noinspection unchecked - return String.join(",",schema.getEnum()); - } + private String getEnumValues() { + //noinspection unchecked + return ((java.util.List<Object>) schema.getEnum()) + .stream() + .map(String::valueOf) + .collect(java.util.stream.Collectors.joining(",")); + }core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (2)
257-276: Preserve JSON pointer when validating patternProperties.Include the matched field name in the context for accurate error locations.
- errors.add(new SchemaValidator(api, propSchema) - .validate(ctx, childNode)); + errors.add(new SchemaValidator(api, propSchema) + .validate(ctx.addJSONpointerSegment(fieldName), childNode));
56-67: Stateful ‘node’ field risks concurrency issues.Validator keeps mutable state set in canValidate(); prefer passing JsonNode locally through validate() to make the class stateless/thread-safe.
core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesComplexTest.java (1)
114-117: Make assertions order-agnostic to avoid flaky testsError ordering is not guaranteed; compare as a set.
Apply this diff:
- if(errors.hasErrors()) { - assertEquals(errMsg1, errors.get(0).getMessage()); - assertEquals(errMsg2, errors.get(1).getMessage()); - } + if (errors.hasErrors()) { + List<String> actual = Arrays.asList(errors.get(0).getMessage(), errors.get(1).getMessage()); + assertTrue(actual.contains(errMsg1)); + assertTrue(actual.contains(errMsg2)); + }core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java (3)
24-29: Also accept native Java Boolean in canValidateCurrently only TextNode/String/BooleanNode pass; plain Boolean should too.
Apply this diff:
- public String canValidate(Object obj) { + public String canValidate(Object obj) { String str = getStringValue(obj); - if(obj instanceof BooleanNode || str.equals("true") || str.equals("false")) + if (obj instanceof BooleanNode || obj instanceof Boolean || "true".equals(str) || "false".equals(str)) return BOOLEAN; return null; }
31-46: Validate should accept Boolean instancesPlain Boolean currently falls through to error. Accept it early.
Apply this diff:
- public ValidationErrors validate(ValidationContext ctx, Object value) { + public ValidationErrors validate(ValidationContext ctx, Object value) { ValidationErrors errors = new ValidationErrors(); - if (value instanceof BooleanNode) + if (value instanceof BooleanNode || value instanceof Boolean) return errors;
31-31: Optional: add @OverRide on validate for consistencyMatches other validators; improves tooling.
Apply this diff:
- public ValidationErrors validate(ValidationContext ctx, Object value) { + @Override + public ValidationErrors validate(ValidationContext ctx, Object value) {core/src/test/java/com/predic8/membrane/core/openapi/validators/AbstractValidatorTest.java (2)
38-38: Unnecessary public visibility on setUp()JUnit 5 doesn’t require public; keep package‑private to reduce surface.
Apply this diff:
- public void setUp() throws Exception { + void setUp() throws Exception {
50-52: Name clash potential with getResourceAsStreamThis instance method name shadows the statically imported OpenAPITestUtils.getResourceAsStream; consider renaming for clarity.
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (2)
64-76: Minor: Method name vs. asserted SpecVersion mismatchreadAndParseOpenAPI31() asserts V30. Consider renaming for clarity or asserting V31 if appropriate.
110-118: Duplicate test methods with identical assertionsreferencesRelativeFilesInSameDirectory and referencesRelativeFilesInSameDirectory2 are identical; deduplicate or parameterize.
core/src/main/java/com/predic8/membrane/core/openapi/validators/ArrayValidator.java (2)
63-69: Fix user-facing grammar in error messagesUse “than” instead of “then” in minItems/maxItems messages.
- errors.add(ctx, format("Array has %d items. This is less then minItems of %d.", node.size(), schema.getMinItems())); + errors.add(ctx, format("Array has %d items. This is less than minItems of %d.", node.size(), schema.getMinItems())); ... - errors.add(ctx, format("Array has %d items. This is more then maxItems of %d.", node.size(), schema.getMaxItems())); + errors.add(ctx, format("Array has %d items. This is more than maxItems of %d.", node.size(), schema.getMaxItems()));
88-90: Clarify uniqueItems violation text“has the not unique values” is awkward; prefer “contains non‑unique values”.
- return ValidationErrors.create(ctx, format("Array with restriction uniqueItems has the not unique values %s.", Utils.joinByComma(moreThanOnce))); + return ValidationErrors.create(ctx, format("Array with restriction uniqueItems contains non-unique values: %s.", Utils.joinByComma(moreThanOnce)));core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (3)
57-57: Make test-suite base path OS‑portableAvoid hardcoded backslashes to keep tests runnable on Linux/macOS.
- public final String TEST_SUITE_BASE_PATH = "git\\JSON-Schema-Test-Suite\\tests\\draft6"; + public final String TEST_SUITE_BASE_PATH = "git" + File.separator + + "JSON-Schema-Test-Suite" + File.separator + + "tests" + File.separator + "draft6";
170-174: Avoid shadowing static method name and improve readabilityRename local variable ‘post’ to avoid clashing with the statically imported post() factory.
- Request<Body> post = post().path("/test").mediaType("application/json"); + Request<Body> req = post().path("/test").mediaType("application/json"); ... - errors = validator.validate(post.body(body)); + errors = validator.validate(req.body(body));
176-178: Don’t print stack traces directly; use loggerUse SLF4J for consistent test output and CI logs.
- } catch (Exception e) { - e.printStackTrace(); - } + } catch (Exception e) { + log.error("Validation threw while executing JSON Schema test run", e); + }core/src/test/resources/openapi/specs/query-params.yml (3)
39-51: Be explicit about style/explode for object query paramsDefaults (style=form, explode=true) apply, but making them explicit improves readability and avoids ambiguity across tools.
/object: parameters: - in: query name: car + style: form + explode: true schema: type: object properties: color: type: string brand: type: string power: type: integer
52-62: Same for operation-level object query paramparameters: - in: query name: pet + style: form + explode: true schema: type: object properties: age: type: number kind: type: string
63-69: Header object param: declare style/explode explicitlyHeaders default to style=simple, explode=false; make it explicit so explode behavior is unambiguous in tests.
- in: header name: plant + style: simple + explode: false schema: type: object properties: size: type: numbercore/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedArrayParameter.java (1)
28-46: Remove stale commented codeDead/commented logic adds noise and confuses explode semantics.
-// if (explode) { -// return values.stream(); -// } -// String[] items = values.getFirst().split(","); -// if (items.length == 0) { -// return Stream.empty(); -// } -// if (items.length == 1) { -// if (items[0].equals("null")) { -// return null; -// } -// // foo= => foo: "" => Let assume an empty parameter is an empty array -// if (items[0].isEmpty()) { -// return Stream.empty(); -// } -// } -// return Arrays.stream(items);core/src/test/resources/openapi/specs/oas31/parameters/object.yaml (2)
48-48: Add missing newline at end of file.Static analysis correctly identified that the file is missing a newline character at the end, which violates YAML formatting standards.
maximum: 255 +
1-48: Consider security configuration for production use.The OpenAPI spec lacks security definitions. While this is acceptable for test specifications, consider adding security configurations if this spec template will be used for production APIs.
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java (1)
44-46: Incomplete test implementation with TODO.The
parameterIsNotDescribedtest contains only a TODO andassertTrue(true), making it ineffective. This test should either be implemented or removed.@Test void parameterIsNotDescribed() { - assertTrue(true); //TODO + ValidationErrors errors = validator.validate(get().path("/array?undefinedParam=value")); + assertTrue(errors.size() > 0, "Unknown parameters should generate validation errors"); }Would you like me to implement this test or create an issue to track this task?
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java (1)
58-61: Incomplete test with no assertions.The
explode()test in theObjectsclass creates validation errors but doesn't assert anything about the result. This makes the test ineffective.@Test void explode() { ValidationErrors err = validator.validate(get().path("/object?color=R,100,G,200,B,150")); + assertEquals(0, err.size(), "Object parameter should validate successfully"); }core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterTest.java (2)
38-38: Remove debug output from production test code.The
System.out.printlnstatement should be removed from the test as it clutters test output and serves no functional purpose in automated testing.-System.out.println("fields = " + fields);
45-69: Consider implementing commented test cases.The commented-out test methods (
one_too_much,empty,single_value) represent important edge cases for object parameter parsing. These should either be implemented or removed to avoid technical debt.Would you like me to help implement these test cases or create an issue to track their completion?
core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
18-18: Fix JavaDoc return tag.The JavaDoc is missing the return description.
- * @return + * @return JsonNode representation of the scalar valuecore/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java (2)
68-71: Avoid println in testsUse assertions only. Printing ValidationErrors increases noise in CI logs and can hide meaningful failures.
-ValidationErrors err = validator.validate(get().path("/array?number=1,2,3")); -System.out.println("err = " + err); -assertEquals(0, err.size()); +assertEquals(0, validator.validate(get().path("/array?number=1,2,3")).size());Also applies to: 75-81
49-60: Parametrize repeated “valid -> size()==0” casesThese are identical structure. Consider a @ParameterizedTest to reduce duplication and improve failure diagnostics.
Also applies to: 61-65, 66-71, 73-77, 78-81
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPIUtilTest.java (1)
71-74: Remove or implement empty testgetOperation()An empty test silently passes and provides no coverage. Either add a real assertion or drop it.
- @Test - void getOperation() { - - } + @Test + void getOperation() { + var api = OpenAPITestUtils.getApi(this,"/openapi/specs/customers.yml"); + var pi = OpenAPIUtil.getPath(api, "/customers/{cid}"); + assertNotNull(pi.getGet()); + }core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameter.java (1)
33-39: Null vs missing semanticsFor arrays you return MissingNode when items==null (comment “foo=null”). For objects (non-exploded) the suggested fix returns NullNode for “null”. Consider aligning semantics across parameter types to avoid validator surprises.
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (2)
82-84: Null‑safe parameter lookupoperation.getParameters() can be null.
- public static Parameter getParameter(Operation operation, String parameterName) { - return operation.getParameters().stream().filter(parameter -> parameter.getName().equals(parameterName)).findFirst().orElse(null); - } + public static Parameter getParameter(Operation operation, String parameterName) { + if (operation == null || operation.getParameters() == null) return null; + return operation.getParameters().stream() + .filter(Objects::nonNull) + .filter(p -> parameterName.equals(p.getName())) + .findFirst() + .orElse(null); + }
93-103: Guard$refresolution when components/schemas are absentAvoid NPE on APIs without components.
- if (schema.get$ref() != null) { - String componentLocalNameFromRef = getComponentLocalNameFromRef(schema.get$ref()); - return api.getComponents().getSchemas().get(componentLocalNameFromRef); - } + if (schema.get$ref() != null) { + if (api.getComponents() == null || api.getComponents().getSchemas() == null) return null; + String componentLocalNameFromRef = getComponentLocalNameFromRef(schema.get$ref()); + return api.getComponents().getSchemas().get(componentLocalNameFromRef); + }core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (2)
64-83: Single, correct “missing required” report; avoid per‑param duplicationCurrently you emit the same “required missing” list for each absent parameter and even when the current p is optional. Compute once based on the full set.
- Set<String> fields = new HashSet<>(parameterMap.keySet()); - - getAllQueryParameters(operation).forEach(p -> { - Schema schema = OpenAPIUtil.getSchema(api, p); - if (!parameterMap.containsKey(p.getName())) { - if (p.getRequired()) { - errors.add(ctx, "Required query parameter(s) '%s' missing.".formatted(join(required))); - } - return; - } - errors.add(validate(ctx, p.getName(), parameterMap, schema, p)); - fields.remove(p.getName()); - }); + Set<String> fields = new HashSet<>(parameterMap.keySet()); + + // report all missing required params once + var missingRequired = required.stream() + .filter(r -> !parameterMap.containsKey(r)) + .collect(toCollection(java.util.LinkedHashSet::new)); + if (!missingRequired.isEmpty()) { + errors.add(ctx, "Required query parameter(s) '%s' missing.".formatted(join(missingRequired))); + } + + getAllQueryParameters(operation).forEach(p -> { + Schema schema = OpenAPIUtil.getSchema(api, p); + if (!parameterMap.containsKey(p.getName())) + return; + errors.add(validate(ctx, p.getName(), parameterMap, schema, p)); + fields.remove(p.getName()); + });
149-152: Null‑safe filter on security schemeinAvoid NPE if
inis omitted.- .filter(scheme -> scheme != null && scheme.getType() != null && scheme.getType().equals(APIKEY) && scheme.getIn().equals(QUERY)) + .filter(scheme -> scheme != null && APIKEY.equals(scheme.getType()) && QUERY.equals(scheme.getIn()))core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (3)
25-26: Consider constants for type values.The string literals "ARRAY" and "OBJECT" used in the switch statement should be defined as constants to avoid typos and improve maintainability.
+public static final String ARRAY = "array"; +public static final String OBJECT = "object"; + public static AbstractParameter instance(OpenAPI api, String type, Parameter parameter) { AbstractParameter paramParser = switch (type) { - case ARRAY -> { + case ARRAY -> {
64-66: Add null safety for parameter name lookup.The
getValuesForParameter()method assumes thatparameter.getName()is not null and that the values map contains the parameter. Consider adding null checks for robustness.protected List<String> getValuesForParameter() { - return values.get(parameter.getName()); + String paramName = parameter.getName(); + if (paramName == null || values == null) { + return List.of(); + } + return values.getOrDefault(paramName, List.of()); }
31-32: Remove or document the commented code.Line 31 contains commented-out code that appears to be unused. Consider removing it if it's no longer needed, or add a comment explaining why it's kept.
-// protected List<String> values = new ArrayList<>(); protected Map<String, List<String>> values;core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParamsTest.java (2)
50-57: Debug output should be removed or made conditional.Line 52 contains
System.out.printlnfor debugging. Consider removing this or making it conditional based on a debug flag to keep test output clean.- System.out.println("errors = " + errors);
60-68: Multiple debug statements in test methods.Lines 62 should be cleaned up similar to the previous comment about debug output.
- System.out.println("err = " + err);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (76)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/xen/XenAuthenticationInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtSignInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.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/BooleanValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/JsonSchemaValidator.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(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java(2 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/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedArrayParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameter.java(1 hunks)core/src/main/java/com/predic8/membrane/core/transport/ssl/acme/AcmeClient.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/OpenAPIValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/model/RequestTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/ConstValueTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/NumberValidationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/RequestReferenceTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/SimpleReferenceTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesComplexTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesDocumentTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesNestedTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyOpenAPITest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/ApiDocsInterceptorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPI31Test.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/util/JsonTestUtil.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java(3 hunks)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPIUtilTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/AbstractValidatorTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/ArrayTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/CompositionTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/DiscriminatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/EnumTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/HeaderParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/InputStreamBodyTest.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/java/com/predic8/membrane/core/openapi/validators/NullableTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/ObjectTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(4 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParamsTest.java(4 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/ReadWriteOnlyTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/RequiredTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/SchemaValidatorTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/exceptions/ExceptionInterceptorTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/security/BasicAuthSecurityValidationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/security/OAuth2SecurityValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/JsonUtilTest.java(1 hunks)core/src/test/java/com/predic8/membrane/test/TestUtil.java(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/object.yaml(1 hunks)core/src/test/resources/openapi/specs/query-params.yml(1 hunks)
✅ Files skipped from review due to trivial changes (30)
- core/src/main/java/com/predic8/membrane/core/transport/ssl/acme/AcmeClient.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/SimpleReferenceTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/ConstValueTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/ObjectTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/NullTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/RequestReferenceTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/model/RequestTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/InputStreamBodyTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/EnumTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesDocumentTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/authentication/xen/XenAuthenticationInterceptor.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/ArrayTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/HeaderParameterTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/RequiredTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/CompositionTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/DiscriminatorTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtSignInterceptor.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/ReadWriteOnlyTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyOpenAPITest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.java
- core/src/test/java/com/predic8/membrane/core/proxies/SOAPProxyTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/OpenAPIValidatorTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/NumberValidationTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/security/OAuth2SecurityValidatorTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/security/BasicAuthSecurityValidationTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/ApiDocsInterceptorTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/NullableTest.java
- core/src/test/java/com/predic8/membrane/core/util/JsonUtilTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/test/java/com/predic8/membrane/core/openapi/validators/exceptions/ExceptionInterceptorTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameter.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/QueryParameterValidator.javacore/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParamsTest.java
🧬 Code graph analysis (21)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesComplexTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIPublisherInterceptorTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPI31Test.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/validators/AbstractValidatorTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameter.java (1)
core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
JsonUtil(6-54)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameter.java (1)
core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
JsonUtil(6-54)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterTest.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-104)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (1)
AbstractParameter(27-71)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterTest.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-104)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (1)
AbstractParameter(27-71)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameter.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-104)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptorTest.java (2)
core/src/test/java/com/predic8/membrane/core/openapi/util/JsonTestUtil.java (1)
JsonTestUtil(26-51)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesNestedTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterTest.java (3)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-104)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (1)
AbstractParameter(27-71)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterTest.java (1)
Nested(31-50)
core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (5)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-104)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameter.java (1)
AbstractParameter(27-71)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-104)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(44-229)core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-132)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParamsTest.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-132)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPIUtilTest.java (2)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-104)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/oas31/parameters/object.yaml
[high] 1-49: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-49: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml
[high] 1-64: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-64: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 19-23: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 YAMLlint (1.37.1)
core/src/test/resources/openapi/specs/oas31/parameters/object.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml
[warning] 57-57: too few spaces after comma
(commas)
[error] 63-63: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameter.java
Outdated
Show resolved
Hide resolved
...ain/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedArrayParameter.java
Outdated
Show resolved
Hide resolved
...in/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameter.java
Outdated
Show resolved
Hide resolved
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml
Outdated
Show resolved
Hide resolved
core/src/test/resources/openapi/specs/oas31/parameters/object.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (2)
122-124: NPE risk when discriminator property is absent.getBaseSchemaName dereferences node.get(propertyName) without null check; validateDiscriminator then assumes nullable. Fix to avoid NPE.
- private String getBaseSchemaName(JsonNode node, String propertyName) { - return node.get(propertyName).asText(); - } + private String getBaseSchemaName(JsonNode node, String propertyName) { + JsonNode v = node.get(propertyName); + return (v == null || v.isNull()) ? null : v.asText(); + }Also applies to: 89-92
185-193: NPE when schema.properties is null.getAddionalProperties assumes properties map is non-null.
private Map<String, JsonNode> getAddionalProperties(JsonNode node) { Map<String, JsonNode> addionalProperties = new HashMap<>(); for (Iterator<String> it = node.fieldNames(); it.hasNext(); ) { String propName = it.next(); - if (!schema.getProperties().containsKey(propName)) { + if (schema.getProperties() == null || !schema.getProperties().containsKey(propName)) { addionalProperties.put(propName, node.get(propName)); } } return addionalProperties; }
🧹 Nitpick comments (33)
core/src/test/java/com/predic8/membrane/core/openapi/validators/PatternPropertiesTest.java (2)
38-38: Drop println in tests; surface context via assertion messages.Console prints add noise to CI logs and are easy to forget. Prefer assertion messages or a message supplier.
Apply this diff:
- System.out.println("errors = " + errors);
39-39: Grammar fix LGTM; make the assertion less brittle and self-describing.Asserting on
errors.toString().contains(...)ties the test to a formatted aggregate string. Pass the errors as the failure message instead.Apply this diff:
- assertTrue(errors.toString().contains("Array has 0 items. This is less than minItems of 2.")); + assertTrue( + errors.toString().contains("Array has 0 items. This is less than minItems of 2."), + () -> "errors: " + errors + );If
ValidationErrorsexposes individual messages (e.g.,errors.get(0).getMessage()), prefer asserting on that for even stronger coupling to the actual signal.distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.java (2)
17-28: Prefer explicit imports for non-DSL packages; add only needed Hamcrest staticsWildcard imports for project/internal packages can hide symbol origins. Keep RestAssured/Hamcrest DSL imports focused and add only what’s used.
Apply:
-import com.predic8.membrane.examples.util.*; -import com.predic8.membrane.test.*; -import io.restassured.response.*; -import org.json.*; -import org.junit.jupiter.api.*; -import org.skyscreamer.jsonassert.*; +import com.predic8.membrane.examples.util.HttpAssertions; +import com.predic8.membrane.test.AbstractSampleMembraneStartStopTestcase; +import io.restassured.response.Response; +import org.json.JSONException; +import org.junit.jupiter.api.Test; +import org.skyscreamer.jsonassert.JSONAssert; import static com.predic8.membrane.core.http.MimeType.*; import static io.restassured.RestAssured.*; import static io.restassured.http.ContentType.*; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.hasEntry; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasItem;
321-326: Make the error assertion resilient to array ordering and wording changesAvoid
[0]index coupling and match the message loosely to survive library text tweaks.- .statusCode(400) - .body("validation.errors['REQUEST/QUERY_PARAMETER/limit'][0]", - allOf( - hasEntry("message", "200 is greater than the maximum of 100"), - hasEntry("schemaType", "integer") - )); + .statusCode(400) + .body("validation.errors['REQUEST/QUERY_PARAMETER/limit']", + hasItem(allOf( + hasEntry(equalTo("schemaType"), equalTo("integer")), + hasEntry(equalTo("message"), containsString("maximum of 100")) + )));Optional tidy-up: since this test no longer uses JSONAssert, you can drop
throws JSONExceptionfrom the method signature.core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (2)
24-33: Javadoc nits.Typo and grammar: “BooleNode” → “BooleanNode”; “String are not quoted!” → “Strings are not quoted.”
- * - true => BooleNode + * - true => BooleanNode - * into a JsonNode. String are not quoted! + * into a JsonNode. Strings are not quoted!
34-67: Integer sizing edge cases; prefer range checks over bitLength and trim input.
bitLengthpromotes -2^63 to BigInteger (not long), causing asymmetric typing at boundaries.- Consider trimming whitespace to be resilient to accidental spaces in query values.
Apply this diff:
public static JsonNode scalarAsJson(String value) { - if (value == null) return FACTORY.nullNode(); + if (value == null) return FACTORY.nullNode(); + // Be lenient towards accidental whitespace in query strings. + value = value.trim(); @@ - if (!value.contains(".") && !value.contains("e") && !value.contains("E")) { - java.math.BigInteger bi = new java.math.BigInteger(value); - int bl = bi.bitLength(); - if (bl <= 31) return FACTORY.numberNode(bi.intValue()); - if (bl <= 63) return FACTORY.numberNode(bi.longValue()); - return FACTORY.numberNode(bi); - } + if (!value.contains(".") && !value.contains("e") && !value.contains("E")) { + java.math.BigInteger bi = new java.math.BigInteger(value); + if (bi.compareTo(java.math.BigInteger.valueOf(Integer.MIN_VALUE)) >= 0 + && bi.compareTo(java.math.BigInteger.valueOf(Integer.MAX_VALUE)) <= 0) { + return FACTORY.numberNode(bi.intValue()); + } + if (bi.compareTo(java.math.BigInteger.valueOf(Long.MIN_VALUE)) >= 0 + && bi.compareTo(java.math.BigInteger.valueOf(Long.MAX_VALUE)) <= 0) { + return FACTORY.numberNode(bi.longValue()); + } + return FACTORY.numberNode(bi); + }Please confirm trimming won’t affect any tests expecting leading/trailing spaces to be preserved.
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml (1)
36-45: Optional: make RGB components required.If the intention is to require all color channels, add
required: [R, G, B]; otherwise ignore.schema: type: object additionalProperties: false + required: [R, G, B] properties: R: $ref: "#/components/schemas/Color" G: $ref: "#/components/schemas/Color" B: $ref: "#/components/schemas/Color"core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java (2)
72-77: Make the failure assertion less brittle.Hard-coding “does not match any of [number]” ties the test to a specific error phrasing. Assert on stable fragments instead.
- assertTrue(err.get(0).getMessage().contains("does not match any of [number]")); + assertAll( + () -> assertTrue(err.get(0).getMessage().contains("\"foo\"")), + () -> assertTrue(err.get(0).getMessage().toLowerCase().contains("number")) + );
37-41: Minor duplication withExplode.Array.number().
twoValuesoverlaps with the parametrized numeric case. Consider removing or merging into the nested group.core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java (2)
57-61: Case label mismatch: “with null” but path has nonull.Either remove the duplicate or actually include
nullin the path if meant to be valid.- arguments("multiple strings with null", "/array?string=blue,black,brown"), + // Duplicate removed (identical to previous case) + // If you intended to test null-in-list, move it to Invalid (already covered).
73-78: Reduce brittleness in message checks.Rely on stable fragments rather than the exact phrasing.
- assertTrue(err.get(0).getMessage().contains("\"foo\" is of type string")); + assertAll( + () -> assertTrue(err.get(0).getMessage().contains("\"foo\"")), + () -> assertTrue(err.get(0).getMessage().toLowerCase().contains("string")) + );core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyOpenAPITest.java (2)
118-125: Path typo: remove leading slash to be consistent.
getSpec("/validate-responses-extensions.yml")introduces a double slash in the resource path.- Map<String,Object> xValidation = getXValidation(getSpec("/validate-responses-extensions.yml")); + Map<String,Object> xValidation = getXValidation(getSpec("validate-responses-extensions.yml"));
219-221: Helper can be static (optional).
getXValidation(OpenAPISpec)does not use instance state; making itstaticclarifies intent.- private Map<String, Object> getXValidation(OpenAPISpec spec) { + private static Map<String, Object> getXValidation(OpenAPISpec spec) {core/src/test/java/com/predic8/membrane/core/openapi/validators/SchemaValidatorTest.java (1)
95-97: Comment accuracy.
10_000_000_000Lis a long literal, not a double; the3.142literal is a double, not a float.- Arguments.of(numberValidator, 3.142, NUMBER), // Float - Arguments.of(numberValidator, 382147189247.141592653589793, NUMBER), // Double - Arguments.of(numberValidator, 10_000_000_000L, NUMBER), // Double + Arguments.of(numberValidator, 3.142, NUMBER), // Double + Arguments.of(numberValidator, 382147189247.141592653589793, NUMBER), // Double + Arguments.of(numberValidator, 10_000_000_000L, NUMBER), // Longcore/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java (1)
49-57: Support java.lang.Boolean inputs as wellIf callers pass plain Boolean (not a Jackson node/string), it’s currently rejected. Treat Boolean consistently.
Apply:
private static String getStringValue(Object value) { if (value instanceof TextNode tn) { return tn.asText(); } if (value instanceof String s) { return s; } + if (value instanceof Boolean b) { + return b ? "true" : "false"; + } return ""; }core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedArrayParameterParser.java (1)
21-26: Align null/empty semantics with non‑exploded arrays and use helperMirror ArrayParameterParser behavior: single value "null" → whole array is null; single empty → empty array. Also use getValuesForParameter() to avoid NPEs on missing names.
Apply:
- protected Stream<String> getItems() { - var parameterValues = values.get(parameter.getName()); - if (parameterValues == null) - return Stream.empty(); - return parameterValues.stream(); - } + protected Stream<String> getItems() { + var vs = getValuesForParameter(); + if (vs.isEmpty()) return Stream.empty(); + if (vs.size() == 1) { + var v = vs.get(0); + if ("null".equals(v)) return null; + if (v.isEmpty()) return Stream.empty(); + } + return vs.stream(); + }core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java (1)
37-40: Close the InputStream (use utility that does)parseOpenAPI(InputStream) doesn’t close the stream; swap to OpenAPITestUtils.getApi(...), which does.
Apply:
- void setUp() { - OpenAPIRecord apiRecord = new OpenAPIRecord(parseOpenAPI(getResourceAsStream(this, "/openapi/specs/oas31/exclusive-min-max.yaml")), new OpenAPISpec()); + void setUp() { + OpenAPIRecord apiRecord = new OpenAPIRecord(getApi(this, "/openapi/specs/oas31/exclusive-min-max.yaml"), new OpenAPISpec());core/src/test/resources/openapi/specs/oas31/parameters/object-additional.yaml (1)
55-55: Add trailing newlineYAMLlint flags missing newline at EOF. Add one to satisfy linters.
Apply:
- description: Ok + description: Ok +core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterParser.java (1)
42-42: Optionally trim split itemsHelps with inputs like number=1, 2, 3.
Apply:
- return Arrays.stream(items); + return Arrays.stream(items).map(String::trim);core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterTest.java (1)
45-54: Add edge‑case tests: null and emptyConsider adding:
- single “null” → parsed as JSON null
- single empty “”: foo= → empty array
I can draft these tests if helpful.
core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberValidator.java (1)
41-47: Avoid double parsing for performance.You parse the same numeric string twice (in canValidate and validate). Cache or factor a small parse helper to reduce work; not critical.
- if (value instanceof JsonNode jn) { - new BigDecimal((jn).asText()); + if (value instanceof JsonNode jn) { + parseNumber(jn.asText()); return NUMBER; } - if (value instanceof String s) { - new BigDecimal(s); + if (value instanceof String s) { + parseNumber(s); return NUMBER; } ... - if (value instanceof JsonNode jn) { - // Not using double prevents from losing fractions - new BigDecimal(jn.asText()); + if (value instanceof JsonNode jn) { + parseNumber(jn.asText()); return null; } - if (value instanceof String s) { - new BigDecimal(s); + if (value instanceof String s) { + parseNumber(s); return null; }Also applies to: 66-73
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectAdditionalParameterTest.java (1)
58-59: Remove debug prints from tests.System.out output is noisy in CI. Prefer assertions only.
- System.out.println("fields = " + fields); + // no-op ... - System.out.println("fields = " + fields); + // no-opAlso applies to: 70-71
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java (1)
26-26: Remove stdout logging.Avoid System.out in library code. Use a logger if needed.
- System.out.println("values = " + values); + // no logging herecore/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (3)
172-175: Misleading error message for unsupported additionalProperties type.Message prints property names instead of the actual type.
- return errors.add(ctx.statusCode(400), - format("Unsupported additionalProperties type: %s", joinByComma(additionalProperties.keySet()))); + return errors.add(ctx.statusCode(400), + format("Unsupported additionalProperties type: %s", + schema.getAdditionalProperties().getClass().getName()));
134-136: Microcopy: use “than” not “then”.Minor grammar tweak in validation messages.
- return ValidationErrors.create(ctx, String.format("Object has %d properties. This is smaller then minProperties of %d.", node.size(), schema.getMinProperties())); + return ValidationErrors.create(ctx, String.format("Object has %d properties. This is smaller than minProperties of %d.", node.size(), schema.getMinProperties())); ... - return ValidationErrors.create(ctx, String.format("Object has %d properties. This is more then maxProperties of %d.", node.size(), schema.getMaxProperties())); + return ValidationErrors.create(ctx, String.format("Object has %d properties. This is more than maxProperties of %d.", node.size(), schema.getMaxProperties()));Also applies to: 141-143
148-175: Typo in method/variable names (“Addional”).Consider renaming to “Additional” for clarity; optional, non-breaking now, or deprecate old names later.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
62-68: Guard against unset values map.getValuesForParameter() should not NPE if setValues() wasn’t called.
protected List<String> getValuesForParameter() { String paramName = parameter.getName(); if (paramName == null) { return List.of(); } - return values.getOrDefault(paramName, List.of()); + if (values == null) return List.of(); + return values.getOrDefault(paramName, List.of()); }core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java (1)
52-56: Comment mismatch (“empty parameter is an empty array”).Code returns nullNode; adjust comment or behavior.
- // foo= => foo: "" => Let assume an empty parameter is an empty array + // foo= => interpret empty parameter as JSON null for object typecore/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java (1)
83-96: additionalBoolean: return value conflates behavior.Method returns true for both true/false cases, forcing caller to return early. It works but is hard to follow. Consider clearer flow or rename.
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (2)
47-52: Avoid double‑brace initialization.Use straightforward list construction to prevent anonymous classes and potential leaks.
- return factory.create(new ArrayList<>() {{ - add(new OpenAPISpec() {{ - setLocation(fileName); - }}); - }}).get(id); + List<OpenAPISpec> specs = new ArrayList<>(); + OpenAPISpec spec = new OpenAPISpec(); + spec.setLocation(fileName); + specs.add(spec); + return factory.create(specs).get(id);
144-146: Reuse existing OpenAPITestUtils.getApi and drop duplicate helper.You already statically import OpenAPITestUtils.*. Remove the local getApi and use the shared util.
- private OpenAPI getApi(String pfad) { - return new OpenAPIParser().readContents(readInputStream(getResourceAsStream(this, pfad)), null, null).getOpenAPI(); - } + // remove local helper; use OpenAPITestUtils.getApi(this, path)And update usages:
- assertEquals("customers-api-v1-0", factory.getUniqueId(new HashMap<>(), new OpenAPIRecord(getApi("/openapi/specs/customers.yml"), null))); + assertEquals("customers-api-v1-0", factory.getUniqueId(new HashMap<>(), new OpenAPIRecord(getApi(this, "/openapi/specs/customers.yml")), null))); ... - assertEquals("customers-api-v1-0-0", factory.getUniqueId(recs, new OpenAPIRecord(getApi("/openapi/specs/customers.yml"), null))); + assertEquals("customers-api-v1-0-0", factory.getUniqueId(recs, new OpenAPIRecord(getApi(this, "/openapi/specs/customers.yml")), null)));Also applies to: 134-142
core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java (1)
60-65: Avoid redundant type checking for numeric types.Lines 60-62 handle Byte/Short/Integer/Long/BigInteger, but these were already validated by canValidate. Consider restructuring to avoid redundant checks.
public ValidationErrors validate(ValidationContext ctx, Object value) { if (value instanceof JsonNode j) { return j.isIntegralNumber() ? null : ValidationErrors.create(ctx.schemaType("integer"), String.format("%s is not an integer.", j.asText())); } if (value instanceof String s) { try { new BigInteger(s); return null; } catch (NumberFormatException e) { return ValidationErrors.create(ctx.schemaType("integer"), String.format("%s is not an integer.", s)); } } - if (value instanceof Byte || value instanceof Short || value instanceof Integer || value instanceof Long || value instanceof java.math.BigInteger) { - return null; - } if (value instanceof Body) { return null; } + // Numeric types (Byte/Short/Integer/Long/BigInteger) are already validated by canValidate() + if (canValidate(value) != null) { + return null; + } if (value == null) { return ValidationErrors.create(ctx.schemaType("integer"), "null is not an integer."); } return ValidationErrors.create(ctx.schemaType("integer"), String.format("Do not know type %s for integer validation.", value.getClass().getName()));core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
135-137: Exception handling is too broad and loses context.Catching all exceptions and wrapping them loses important debugging information. Be more specific about what could fail.
} catch (Exception e) { - throw new RuntimeException(e); // ToDO test foo=ff'ff + // JsonProcessingException from ap.getJson() or validation errors + throw new RuntimeException("Failed to parse/validate query parameter '" + parameterName + "': " + e.getMessage(), e); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/xen/XenAuthenticationInterceptor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/ArrayValidator.java(3 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/JsonSchemaValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberValidator.java(4 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java(10 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java(4 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/StringValidator.java(5 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/AbstractParameterParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedArrayParameterParser.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/ParameterParser.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/JsonUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/ConstValueTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/NumberValidationTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/SimpleReferenceTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesComplexTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesDocumentTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesNestedTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyOpenAPITest.java(10 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptorTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java(6 hunks)core/src/test/java/com/predic8/membrane/core/openapi/util/JsonTestUtil.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPIUtilTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/AbstractParameterParserValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/AbstractValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/IntegerValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java(4 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/NullTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/NullableTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/PatternPropertiesTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParamsTest.java(4 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/SchemaValidatorTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectAdditionalParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/JsonUtilTest.java(1 hunks)core/src/test/java/com/predic8/membrane/test/TestUtil.java(1 hunks)core/src/test/resources/openapi/specs/customers.yml(2 hunks)core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/object-additional.yaml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/object.yaml(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.java(2 hunks)pom.xml(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- core/src/test/java/com/predic8/membrane/core/openapi/validators/IntegerValidatorTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/AbstractParameterParserValidatorTest.java
🚧 Files skipped from review as they are similar to previous changes (24)
- core/src/test/java/com/predic8/membrane/core/openapi/validators/NullTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/SimpleReferenceTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/JsonSchemaValidator.java
- core/src/main/java/com/predic8/membrane/core/interceptor/authentication/xen/XenAuthenticationInterceptor.java
- core/src/test/resources/openapi/specs/customers.yml
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/NumberValidationTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParamsTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptorTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/ConstValueTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/ArrayValidator.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesDocumentTest.java
- core/src/test/java/com/predic8/membrane/test/TestUtil.java
- core/src/test/java/com/predic8/membrane/core/util/JsonUtilTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/JsonSchemaTestSuiteTests.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesComplexTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/multipleTypes/MultipleTypesNestedTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/StringValidator.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/AbstractValidatorTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodeTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/NullableTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/QueryParameterValidator.java
🧬 Code graph analysis (17)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameterParser.java (1)
core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
JsonUtil(20-68)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterTest.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(25-69)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java (1)
core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
JsonUtil(20-68)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(25-69)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java (2)
core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
JsonUtil(20-68)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
JsonUtil(20-68)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.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/openapi/util/OpenAPIUtilTest.java (2)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)
core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyOpenAPITest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java (3)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java (3)
Nested(45-88)Nested(48-87)Nested(70-86)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java (2)
Nested(37-55)Nested(40-54)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectAdditionalParameterTest.java (3)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java (1)
Nested(43-85)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(25-69)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (5)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(25-69)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/oas31/parameters/object-additional.yaml
[high] 1-56: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-56: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml
[high] 1-65: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-65: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 19-23: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
core/src/test/resources/openapi/specs/oas31/parameters/object.yaml
[high] 1-50: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-50: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
🪛 YAMLlint (1.37.1)
core/src/test/resources/openapi/specs/oas31/parameters/object-additional.yaml
[error] 55-55: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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 (20)
pom.xml (1)
334-337: Swagger Parser v3 bump — verify transitive swagger-core/model versions & Jackson convergence
- Repo evidence: pom.xml contains io.swagger:swagger-parser:1.0.75 and io.swagger.parser.v3:swagger-parser:2.1.34 (pom.xml ~lines 326–337); core/pom.xml references both (~277–292); pom.xml sets <jackson.version>2.20.0 (line ~80).
- Action: run and inspect resolved deps:
mvn -DskipTests dependency:tree -DoutputFile=dep.tree
grep -nE 'io.swagger(.parser.v3)?|swagger-(core|models|annotations)' dep.tree
grep -nE 'com.fasterxml.jackson.core:jackson-(databind|core|annotations|module-parameter-names|module-kotlin)' dep.tree- If you find conflicting swagger-core/models or mixed Jackson versions, enforce convergence (maven-enforcer dependencyConvergence) and/or import a Jackson BOM (2.20.0) in dependencyManagement to lock versions and avoid behavioral changes in the validator.
core/src/test/java/com/predic8/membrane/core/openapi/util/JsonTestUtil.java (1)
26-50: Rename looks good; helpers remain stable.No functional changes observed; tests depending on these helpers should continue to pass.
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml (1)
52-55: Min/max fix acknowledged.The previous “min/max” → “minimum/maximum” issue is corrected here; thanks for addressing it.
core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java (1)
21-31: Interface switch + overrides look goodImplementing JsonSchemaValidator and adding @OverRide is consistent with the refactor.
core/src/test/resources/openapi/specs/oas31/parameters/object.yaml (1)
13-13: Resolved: correctexplodekeyThe earlier “exploded” typo is fixed; both entries use explode: true/false correctly.
Also applies to: 25-25
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterTest.java (1)
28-57: LGTMTest exercises setValues→getJson path and validates numeric coercion as expected.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameterParser.java (1)
29-39: LGTMNull-as-whole-array semantics and scalarAsJson conversion are clear and consistent.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterParser.java (1)
29-31: Avoid List#getFirst (requires JDK 21); use index for wider compatibilityUsing getFirst() ties build to JDK 21’s SequencedCollection. Use get(0).
Apply:
- String[] items = value.getFirst().split(","); - if (items.length == 0) { - return Stream.empty(); - } + // Use index-based access for broader JDK compatibility. + String[] items = value.get(0).split(",");To confirm baseline JDK, run:
core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberValidator.java (1)
37-47: BigDecimal-based detection and TextNode rejection look correct.Switching to BigDecimal avoids precision loss; rejecting TextNode for JSON bodies aligns with JSON Schema. LGTM.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
39-55: Avoid NPE from boxedexplode; use a safe default and confirm where spec-defaults belong.parameter.getExplode() is nullable — use Boolean.TRUE.equals(...) for the conditionals and assignment; OpenAPI default: form => explode true, otherwise false. Confirm whether style-based defaults should be applied here (centralized) or in the concrete parsers.
- case ARRAY -> { - if (parameter.getExplode()) + case ARRAY -> { + if (Boolean.TRUE.equals(parameter.getExplode())) yield new ExplodedArrayParameterParser(); yield new ArrayParameterParser(); } case OBJECT -> { - if (parameter.getExplode()) + if (Boolean.TRUE.equals(parameter.getExplode())) yield new ExplodedObjectParameterParser(); yield new ObjectParameterParser(); } default -> new ScalarParameterParser(); }; paramParser.type = type; - paramParser.explode = parameter.getExplode(); + // TODO: apply spec default based on style if null (form=>true, otherwise false) + paramParser.explode = Boolean.TRUE.equals(parameter.getExplode());core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (2)
91-94: Good addition! The null checks prevent NPEs effectively.The method properly guards against null schema and properties, and the Schema<?> cast is safe since OpenAPI 3.x Map values are always Schema instances.
96-106: LGTM! Robust schema resolution with proper null handling.The method correctly handles both direct schemas and $ref schemas, with appropriate null checks for API components.
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPIUtilTest.java (1)
24-27: Clean refactor to use centralized test utilities.Good job consolidating API loading logic into OpenAPITestUtils, improving test maintainability.
core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java (1)
27-43: LGTM! Clean type checking logic.The method properly handles all integer types including BigInteger for large numbers, with a clear return pattern.
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (5)
67-68: Good! Now properly handles multi-valued query parameters.The switch from single values to List correctly supports repeated query parameters.
118-122: Handle missing types gracefully with a default.When types is null (OAS 3.0), the code returns early without validation. Consider defaulting to "string" to maintain backward compatibility.
Set<String> types = schema.getTypes(); // Try all e.g. type: [array, null] // Maybe there is no type if (types == null) { - return errors; + // OAS 3.0 compatibility: fall back to single type + String type = schema.getType(); + types = (type != null) ? Set.of(type) : Set.of("string"); }
206-207: instanceof check may miss generic Parameter with in="query".Filtering by QueryParameter subclass could miss generic Parameter instances with in="query" or $ref parameters.
Set<Parameter> getAllQueryParameters(Operation operation) { - return getAllParameter(operation).stream().filter(p -> p instanceof QueryParameter).collect(toSet()); + return getAllParameter(operation).stream() + .filter(p -> p != null && "query".equalsIgnoreCase(p.getIn())) + .collect(toSet()); }
231-238: NPE risk: getRequired() can return null.Parameter.getRequired() returns Boolean which can be null. The filter will throw NPE on auto-unboxing.
@NotNull Set<String> getRequiredQueryParameters(Operation operation) { Set<Parameter> parameters = getAllQueryParameters(operation); if (parameters == null) { return emptySet(); } return parameters.stream() - .filter(Parameter::getRequired).map(Parameter::getName).collect(toSet()); + .filter(p -> Boolean.TRUE.equals(p.getRequired())) + .map(Parameter::getName) + .collect(toSet()); }
171-186: Query parameter parsing needs better error handling.The decode() call can throw IllegalArgumentException for invalid percent-encoding, which would crash the validation.
private static @NotNull Map<String, List<String>> getParameterMapFromQuery(String query) { Map<String, List<String>> parameterMap = new HashMap<>(); if (query == null) { return parameterMap; } for (String p : query.split("&")) { Matcher m = QUERY_PARAMS_PATTERN.matcher(p); if (m.matches()) { - String key = decode(m.group(1), UTF_8); - String value = decode(m.group(2), UTF_8); + String key; + String value; + try { + key = decode(m.group(1), UTF_8); + value = decode(m.group(2), UTF_8); + } catch (IllegalArgumentException e) { + // Invalid percent-encoding - use raw values + key = m.group(1); + value = m.group(2); + } + if (key.isEmpty()) continue; // Skip empty keys from "&&" or "&=" List<String> ab = parameterMap.computeIfAbsent(key, k -> new ArrayList<>()); ab.add(value); } } return parameterMap; }core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java (1)
58-67: Consider validating that additionalProperties is actually enabled in the schema.The test assumes additional properties are allowed, but doesn't verify this against the schema configuration.
...a/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java
Show resolved
Hide resolved
...main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParser.java
Show resolved
Hide resolved
...main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Outdated
Show resolved
Hide resolved
...a/com/predic8/membrane/core/openapi/validators/parameters/ObjectAdditionalParameterTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (1)
40-64: canValidate should not throw for non‑Json inputs (except InputStream)Throwing for “unknown” non‑Json types in canValidate can bubble unexpected RuntimeExceptions during validator probing. Return null instead (keep InputStream exception per tests).
public String canValidate(Object obj) { if (obj instanceof JsonNode j) { return j instanceof ObjectNode ? OBJECT : null; } else if (obj instanceof InputStream) { throw new RuntimeException("InputStream should not happen!"); } else { - log.warn("This should not happen. Please check."); - throw new RuntimeException("Value cannot be read as object."); + log.warn("Value is not a JsonNode. Returning null from canValidate()."); + return null; } }
🧹 Nitpick comments (29)
core/src/test/resources/openapi/specs/oas31/parameters/object-additional.yaml (4)
1-6: Silence CI security linters for test specsCheckov flags CKV_OPENAPI_4/5 are noise for a test-only spec. Add skip pragmas to avoid red herrings in pipelines.
Apply this diff at the top:
+# checkov:skip=CKV_OPENAPI_4 reason: test spec; no global security on purpose +# checkov:skip=CKV_OPENAPI_5 reason: test spec; operations intentionally unauthenticated openapi: 3.1.0
9-20: Document serialization and optionality for foo (no additional props allowed)Make the parameter intent explicit and provide a concrete example for form + explode=false.
Apply this diff:
parameters: - name: foo in: query style: form explode: false + required: false + description: Object via form style + explode=false; only declared properties allowed. + example: a,hello schema: type: object additionalProperties: false properties: a: type: string
25-36: Add example showing additionalProperties: true behaviorProvide a multi-key example demonstrating acceptance of undeclared keys.
Apply this diff:
- name: foo in: query style: form explode: false + required: false + description: Object via form style + explode=false; undeclared properties allowed. + example: a,hello,b,2,c,3 schema: type: object additionalProperties: true properties: a: type: string
41-53: Example with numeric additional properties (coercion vs JSON parsing)Include an example that exercises numeric parsing for additional props; this helps validate the new scalar JSON parsing path.
Apply this diff:
- name: foo in: query style: form explode: false + required: false + description: Object via form style + explode=false; undeclared properties must be numbers. + example: a,hello,b,2.5,c,-1e3 schema: type: object additionalProperties: type: number properties: a: type: stringAlso confirm tests assert that:
- numeric additional props accept integers, decimals, and scientific notation,
- non-numeric values (e.g., "NaN", "inf", "2,3") are rejected,
- minus signs and leading zeros are handled as per JSON Schema 2020‑12.
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (3)
37-41: Type-safety: return Schema<?> instead of Object in getMail().Improves clarity and avoids raw typing in assertions.
- private static Object getMail(OpenAPIRecord rec) { + private static io.swagger.v3.oas.models.media.Schema<?> getMail(OpenAPIRecord rec) { return rec.api.getPaths().get("/users").getPost() .getRequestBody().getContent().get(APPLICATION_JSON) .getSchema().getProperties().get("email"); }Alternatively, add an import for Schema and use
Schema<?>.
43-49: Simplify helper and avoid intermediate mutable list.Use singletonList to reduce noise and allocation.
- private static OpenAPIRecord getOpenAPIRecord(String fileName, String id) { - List<OpenAPISpec> specs = new ArrayList<>(); - OpenAPISpec spec = new OpenAPISpec(); - spec.setLocation(fileName); - specs.add(spec); - return factory.create(specs).get(id); - } + private static OpenAPIRecord getOpenAPIRecord(String fileName, String id) { + OpenAPISpec spec = new OpenAPISpec(); + spec.setLocation(fileName); + return factory.create(Collections.singletonList(spec)).get(id); + }
89-94: NPE‑safe assertion when description might be absent.Guard against null descriptions and still test absence of the conversion note.
- assertFalse(rec.api.getInfo().getDescription().contains( - "OpenAPI description was converted to OAS 3 from Swagger 2 by Membrane API Gateway." - )); + String desc = Optional.ofNullable(rec.api.getInfo().getDescription()).orElse(""); + assertFalse(desc.contains( + "OpenAPI description was converted to OAS 3 from Swagger 2 by Membrane API Gateway." + ));distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.java (3)
17-31: Consolidate Hamcrest imports; avoid mixing CoreMatchers and Matchers.You already wildcard-import
org.hamcrest.Matchers.*, so the explicit imports andCoreMatchers.hasItemare redundant and risk confusion. Prefer a singleMatchers.*import.-import static org.hamcrest.CoreMatchers.hasItem; -import static org.hamcrest.Matchers.allOf; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.*; +import static org.hamcrest.Matchers.*;
316-329: Assertion rewrite looks good; minor matcher simplification possible.Nice switch to precise Hamcrest assertions over whole‑payload equality. You can simplify
hasEntry(equalTo(...), equalTo(...))to literalhasEntry(key, value)for readability.- hasItem(allOf( - hasEntry(equalTo("schemaType"), equalTo("integer")), - hasEntry(equalTo("message"), containsString("maximum of 100")) - ))); + hasItem(allOf( + hasEntry("schemaType", "integer"), + hasEntry("message", containsString("maximum of 100")) + )));
316-329: Add query style/explode coverage to align with PR goals.Given this PR targets query parsing (form/explode arrays and objects), consider adding e2e tests here for:
- form + explode=true arrays:
?color=blue&color=black&color=brown→ type=array- form + explode=false arrays:
?color=blue,black,brown→ type=array- form + explode=true objects:
?R=100&G=200&B=150→ type=object- form + explode=false objects:
?color=R,100,G,200,B,150→ type=object- deepObject (if supported):
?color[R]=100&color[G]=200Happy to draft concrete tests once you confirm the endpoint/parameter names in the sample API.
core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java (3)
34-34: Narrow constant visibilityUnless referenced from other classes, prefer not exposing test internals. Make the path constant package‑private or private.
- public static final String OPENAPI_PATH = "/openapi/specs/oas31/exclusive-min-max.yaml"; + private static final String OPENAPI_PATH = "/openapi/specs/oas31/exclusive-min-max.yaml";
38-39: Build validator once per class to speed up testsParsing the spec on every test can be slow. Consider per‑class setup.
+@TestInstance(TestInstance.Lifecycle.PER_CLASS) public class ExclusiveMinMaxTest { - @BeforeEach - void setUp() { + @BeforeAll + void setUp() { validator = new OpenAPIValidator(new URIFactory(), new OpenAPIRecord(getApi(this, OPENAPI_PATH), new OpenAPISpec())); }If
OpenAPIValidatorholds mutable state across validations, keep the current per‑test setup.
21-21: Remove unused jakarta.mail ParseExceptionThe test doesn’t use
ParseException; dropping the import andthrowsdecl trims noise.-import jakarta.mail.internet.*; @@ - void testExclusiveMinMax(String requestBody, int expectedErrorSize) throws ParseException { + void testExclusiveMinMax(String requestBody, int expectedErrorSize) {Also applies to: 55-55
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java (4)
31-35: Prefer OpenAPITestUtils.getApi(...) for clarity and better error surfacing.Using getApi(...) avoids a potential NPE from getResourceAsStream(...) and yields clearer failures if the resource is missing.
Apply:
- OpenAPIRecord apiRecord = new OpenAPIRecord(parseOpenAPI(getResourceAsStream(this, "/openapi/specs/oas31/parameters/simple.yaml")), new OpenAPISpec()); + OpenAPIRecord apiRecord = new OpenAPIRecord(getApi(this, "/openapi/specs/oas31/parameters/simple.yaml"), new OpenAPISpec());
43-61: Add edge cases: null/empty elements and invalid booleans.Exploded arrays often trip on null/empty tokens and non-boolean literals. Add a couple of targeted cases to lock behavior.
Apply:
@Test void number() { assertEquals(0, validator.validate(get().path("/array?number=1&number=2.2&number=3e4&number=-1&number=0")).size()); } + @Test + void numberAllowsNullAndEmpty() { + // Aligns with SimpleNullTest expectations for exploded arrays. + assertEquals(0, validator.validate(get().path("/array?number=null")).size()); + assertTrue(validator.validate(get().path("/array?number=")).isEmpty()); + } + @Test void string() { assertEquals(0, validator.validate(get().path("/array?string=a&string=bb&string=foo")).size()); } @Test void bool() { assertEquals(0, validator.validate(get().path("/array?bool=true&bool=false")).size()); } + + @Test + void boolInvalid() { + ValidationErrors err = validator.validate(get().path("/array?bool=foo")); + assertEquals(1, err.size()); + } @Test void noQueryString() { assertEquals(0, validator.validate(get().path("/array")).size()); }
43-56: Optional: Collapse valid cases into a parameterized test (less duplication).Mirror ArrayQueryParameterTest’s MethodSource pattern to reduce boilerplate.
Example:
+ static Stream<Arguments> valid() { + return Stream.of( + arguments("numbers", "/array?number=1&number=2.2&number=3e4&number=-1&number=0"), + arguments("strings", "/array?string=a&string=bb&string=foo"), + arguments("bools", "/array?bool=true&bool=false"), + arguments("no query", "/array") + ); + } + + @ParameterizedTest(name = "{index}: {0}") + @MethodSource("valid") + void zeroErrors(String name, String path) { + assertTrue(validator.validate(get().path(path)).isEmpty(), () -> name + " should have 0 errors"); + }
66-71: Make the error assertion resilient to validator wording.Validator messages vary across tests; assert only the essentials (parameter name "number" and offending value "foo") to avoid brittle checks.
Location: core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java:66-71Apply:
- assertTrue(err.get(0).getMessage().contains("does not match any of [number]")); + String msg = err.get(0).getMessage().toLowerCase(); + assertAll( + () -> assertTrue(msg.contains("number")), + () -> assertTrue(msg.contains("foo")) + );core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java (1)
60-65: Optional: reduce duplicate scans over values.explicitProperties, patternProperties, and additional* each traverse values. Consider a single pass accumulating matches to minimize overhead for large querysets.
Also applies to: 104-112, 91-96, 74-81
core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
24-24: Tighten visibility and minor import cleanup.FACTORY doesn’t need to be subclass‑visible; prefer private. Also you already import java.math.* — no need for FQN.
- protected static final JsonNodeFactory FACTORY = JsonNodeFactory.instance; + private static final JsonNodeFactory FACTORY = JsonNodeFactory.instance;- java.math.BigDecimal bd = new java.math.BigDecimal(value); + BigDecimal bd = new BigDecimal(value);Also applies to: 72-72
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (3)
69-80: Remove commented-out legacy block.Dead code adds noise and risks divergence from the active logic.
-// getAllQueryParameters(operation).forEach(p -> { -// Schema schema = OpenAPIUtil.getSchema(api, p); -// if (!parameterMap.containsKey(p.getName())) { -// if (p.getRequired()) { -// errors.add(ctx, "Required query parameter(s) '%s' missing.".formatted(join(required))); -// } -// return; -// } -// errors.add(validate(ctx, p.getName(), parameterMap, schema, p)); -// fields.remove(p.getName()); -// });
223-225: Null-safe object-property scan.Guard against null
typesandproperties.- if (schema.getTypes().contains("object") && schema.getProperties() != null) { + Set<String> types = schema.getTypes(); + boolean isObject = (types != null && types.contains("object")) || "object".equals(schema.getType()); + if (isObject && schema.getProperties() != null) { schema.getProperties().forEach((name, ignored) -> names.add(name)); }
171-186: Spec nuance: split BEFORE decoding to preserve encoded separators.Current flow decodes values before array-splitting (e.g., “%2C” becomes “,” and then splits incorrectly). Consider storing raw values here and deferring decoding to the parsers, which would split raw tokens then decode each item/value.
I can propose a small refactor across Array/Exploded parsers to decode per-item if you want to take this route.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterParser.java (1)
22-43: Optional: accept multiple occurrences for non‑exploded arrays.Spec expects a single occurrence for explode=false, but being lenient improves interop: flatten all values before splitting.
- String first = value.get(0); - String[] items = first.split(","); + String[] items = value.stream().flatMap(v -> Arrays.stream(v.split(","))).toArray(String[]::new);Would you like a follow‑up PR note documenting this interoperability choice?
core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java (2)
21-29: Boolean validation changes look good; minor consistency tweaks suggested
- Behavior (BooleanNode short‑circuit + "true"/"false" strings) aligns with query param needs. OK.
- Consider returning
nullon success (like IntegerValidator) to avoid allocating emptyValidationErrorsand keep validators consistent.Apply:
@@ - @Override - public ValidationErrors validate(ValidationContext ctx, Object value) { - - ValidationErrors errors = new ValidationErrors(); - - if (value instanceof BooleanNode) - return errors; - - String str = getStringValue(value); - - if (str.equals("true") || str.equals("false")) - return errors; - - errors.add(ctx.schemaType("boolean"), String.format("Value '%s' is not a boolean (true/false).", value)); - return errors; - } + @Override + public ValidationErrors validate(ValidationContext ctx, Object value) { + if (value instanceof BooleanNode) + return null; + String str = getStringValue(value); + if ("true".equals(str) || "false".equals(str)) + return null; + return ValidationErrors.create(ctx.schemaType("boolean"), + String.format("Value '%s' is not a boolean (true/false).", value)); + }Also applies to: 31-47, 49-60
49-60: Clarify case/whitespace policy for boolean stringsDo we intentionally require exact lowercase without surrounding spaces? If query parsing may yield "True"/" false ", consider normalizing; otherwise, keep strict.
Optional normalization:
- private static String getStringValue(Object value) { + private static String getStringValue(Object value) { if (value instanceof TextNode tn) { - return tn.asText(); + return tn.asText().trim(); } if (value instanceof String s) { - return s; + return s.trim(); } if (value instanceof Boolean b) { - return b.booleanValue() ? "true" : "false"; + return b ? "true" : "false"; } return ""; }And, if desired:
- if (str.equals("true") || str.equals("false")) + str = str.toLowerCase(Locale.ROOT); + if ("true".equals(str) || "false".equals(str)) return null;core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java (1)
19-25: Integer validator overhaul looks solid; improve message for non‑integral Number
- canValidate/validate logic is correct and handles JsonNode, String, and integer Number types well.
- For non‑integral Number inputs (e.g., Double), current fallback yields “Do not know type …”. Prefer a clearer “not an integer” error.
Apply:
@@ - // Numeric types (Byte/Short/Integer/Long/BigInteger) are already validated by canValidate() - if (canValidate(value) != null) { - return null; - } + // Numeric types (Byte/Short/Integer/Long/BigInteger) are already validated by canValidate() + if (canValidate(value) != null) { + return null; + } + // Provide a clearer message for numeric but non-integer types + if (value instanceof Number) { + return ValidationErrors.create(ctx.schemaType("integer"), + String.format("%s is not an integer.", value)); + } if (value == null) { return ValidationErrors.create(ctx.schemaType("integer"), "null is not an integer."); }Also applies to: 27-43, 45-73
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (3)
99-104: Variable shadowing and null‑safety on components
- Local
schemashadows the field; rename for clarity.- Guard against null
api/components/schemaswhen resolving base schema/ref.- Schema<?> schema = getBaseSchema(propertyValue); - if (schema == null) { + Schema<?> baseSchema = getBaseSchema(propertyValue); + if (baseSchema == null) { return ValidationErrors.create(ctx.statusCode(400), format("Discriminator value %s is not an valid type.", propertyValue)); } - return new SchemaValidator(api, schema).validate(ctx, node); + return new SchemaValidator(api, baseSchema).validate(ctx, node); @@ - private ValidationErrors validateRef(ValidationContext ctx, JsonNode node, String propertyValue) { - var schema2 = SchemaUtil.getSchemaFromRef(api, propertyValue); - if (schema2 == null) + private ValidationErrors validateRef(ValidationContext ctx, JsonNode node, String propertyValue) { + var refSchema = SchemaUtil.getSchemaFromRef(api, propertyValue); + if (refSchema == null) throw new RuntimeException("Should not happen!"); - return new SchemaValidator(api, schema2).validate(ctx, node); + return new SchemaValidator(api, refSchema).validate(ctx, node); @@ - private Schema getBaseSchema(String propertyValue) { - return api.getComponents().getSchemas().get(propertyValue); + private Schema getBaseSchema(String propertyValue) { + if (api == null || api.getComponents() == null || api.getComponents().getSchemas() == null) + return null; + return api.getComponents().getSchemas().get(propertyValue); }Also applies to: 106-111, 117-121
148-177: Spelling/clarity: “additional” helpers and wordingRename helpers and variables for readability; no behavior change.
- private ValidationErrors validateAdditionalProperties(ValidationContext ctx, JsonNode node) { + private ValidationErrors validateAdditionalProperties(ValidationContext ctx, JsonNode node) { if (schema.getAdditionalProperties() == null) return null; - Map<String, JsonNode> additionalProperties = getAddionalProperties(node); + Map<String, JsonNode> additionalProperties = getAdditionalProperties(node); @@ - return errors.add(ctx.statusCode(400), - format("The object has the additional %s: %s. But the schema does not allow additional properties.", - getPropertyOrIes(additionalProperties.keySet()), + return errors.add(ctx.statusCode(400), + format("The object has the additional %s: %s. But the schema does not allow additional properties.", + propertyOrProperties(additionalProperties.keySet()), joinByComma(additionalProperties.keySet()))); @@ - private String getPropertyOrIes(Set<String> addionalProperties) { - String propWord = "Property"; - if (addionalProperties.size() > 1) { - propWord = "Properties"; - } - return propWord; + private String propertyOrProperties(Set<String> additionalProperties) { + return additionalProperties.size() > 1 ? "Properties" : "Property"; } @@ - private Map<String, JsonNode> getAddionalProperties(JsonNode node) { - Map<String, JsonNode> addionalProperties = new HashMap<>(); + private Map<String, JsonNode> getAdditionalProperties(JsonNode node) { + Map<String, JsonNode> additionalProperties = new HashMap<>(); for (Iterator<String> it = node.fieldNames(); it.hasNext(); ) { String propName = it.next(); - if (schema.getProperties() == null || !schema.getProperties().containsKey(propName)) { - addionalProperties.put(propName, node.get(propName)); + if (schema.getProperties() == null || !schema.getProperties().containsKey(propName)) { + additionalProperties.put(propName, node.get(propName)); } } - return addionalProperties; + return additionalProperties; }Also applies to: 187-196
265-284: Pattern properties: optional micro‑perfPre‑compile pattern map once per instance if this becomes hot; current per‑validate compile is fine for now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java(10 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java(2 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/ArrayParameterParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedArrayParameterParser.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/ParameterParser.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/JsonUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyOpenAPITest.java(10 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java(6 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/PatternPropertiesTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/SchemaValidatorTest.java(3 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectAdditionalParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/object-additional.yaml(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameterParser.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParser.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectAdditionalParameterTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/PatternPropertiesTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyOpenAPITest.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/QueryParameterValidator.java
🧬 Code graph analysis (9)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
JsonUtil(22-79)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.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/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java (3)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java (3)
Nested(45-87)Nested(48-86)Nested(69-85)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/SimpleNullTest.java (2)
Nested(37-54)Nested(40-53)
core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (3)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)core/src/main/java/com/predic8/membrane/core/openapi/validators/ArrayValidator.java (1)
SuppressWarnings(30-94)core/src/main/java/com/predic8/membrane/core/openapi/util/SchemaUtil.java (1)
SuppressWarnings(27-53)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(25-69)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterTest.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(25-69)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/oas31/parameters/object-additional.yaml
[high] 1-57: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-57: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
⏰ 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: Automated tests
🔇 Additional comments (23)
core/src/test/resources/openapi/specs/oas31/parameters/object-additional.yaml (1)
47-49: Clarify number semantics in testsJSON Schema numbers exclude NaN/Infinity. Ensure validator does not coerce strings and that it treats values per JSON number grammar.
I can add negative/edge-case requests (e.g., foo=a,hi,b,NaN,c,Infinity,d,01) to the test suite if desired.
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (9)
52-63: LGTM: OAS 3.0 parse/read test.Clear assertions for title and SpecVersion.
66-71: LGTM: Swagger 2 → OAS 3 conversion path.Checks look appropriate; SpecVersion assertion validates conversion.
74-78: LGTM: Conversion notice presence.Sufficient signal that the converter touched description.
81-86: LGTM: Existing description preserved while adding notice.Covers the common “prepend notice” scenario.
110-117: LGTM: Relative reference resolution within same directory.Validates resolver behavior and email schema presence.
120-127: LGTM: Deep reference chain resolution.Good coverage for nested refs.
131-131: LGTM: Unique ID (no collision) check with getApi().Uses the shared utility; deterministic expectation looks correct.
137-139: LGTM: Unique ID collision handling.Seed map + expectation for “-0” suffix is accurate.
22-22: Approve — no lingering 'TestUtils' usages found.
rg output shows only OpenAPITestUtils imports across the test suite; no action required.distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.java (1)
41-41: JUnit 5 visibility: OK.Making the test method package‑private is valid in JUnit Jupiter. No action needed.
core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java (1)
29-29: Consolidated test util import: LGTMExclusiveMinMaxTest now uses the static import OpenAPITestUtils (line 29) and the test resource exists at core/src/test/resources/openapi/specs/oas31/exclusive-min-max.yaml. Repo-wide search for the old TestUtils did not run (ripgrep reported "No files were searched"); confirm there are no remaining imports/usages of com.predic8.membrane.core.openapi.util.TestUtils.
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java (1)
27-75: Solid coverage for exploded array params.Overall structure/readability are good; tests exercise number/string/bool plus no-query cases. See nits below.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java (2)
33-41: Defensive null-check for values to prevent NPE (repeat).values can be null if setValues() wasn’t called; explicitProperties/patternProperties/additional* call paths will NPE.
Apply this diff after resolving the schema:
var schema = resolveSchema(api, parameter); if (schema == null) { return obj; } + // Defensive: values may be null if setValues() was not provided. + if (values == null || values.isEmpty()) { + return obj; + }
31-31: FACTORY is declared in AbstractParameterParser — no change required.FACTORY is defined as
protected static final JsonNodeFactory FACTORY = JsonNodeFactory.instancein core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java, soObjectNode obj = FACTORY.objectNode();is valid.core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
37-78: Solid scalar parsing; covers JSON literals and numeric ranges.Behavior looks correct and side‑effect free. Nice use of BigInteger/BigDecimal with range checks.
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterTest.java (1)
45-54: Add edge-case tests for encoding and styles.Consider adding:
- form, explode=false with percent-encoded comma: number=1%2C2%2C3 (should be a single item, not split)
- multiple occurrences with explode=false: number=1,2&number=3
- values with leading/trailing spaces
Do you want me to add these as parameterized tests against explode.yaml/explode-false.yaml?
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (3)
118-126: Don’t skip validation whenschema.getTypes()is null (OAS 3.0).Fallback to
schema.getType()or a safe default to avoid silently ignoring parameters.- Set<String> types = schema.getTypes(); // Try all e.g. type: [array, null] - // Maybe there is no type - if (types == null) { - return errors; - } + Set<String> types = schema.getTypes(); // Try all e.g. type: [array, null] + if (types == null || types.isEmpty()) { + String t = schema.getType(); + types = (t != null && !t.isEmpty()) ? Set.of(t) : Set.of("string"); + }
236-238: Avoid NPE onParameter.getRequired().Use a Boolean‑safe predicate.
- return parameters.stream() - .filter(Parameter::getRequired).map(Parameter::getName).collect(toSet()); + return parameters.stream() + .filter(p -> Boolean.TRUE.equals(p.getRequired())) + .map(Parameter::getName) + .collect(toSet());
171-186: Harden percent-decoding and ignore empty keys.Invalid percent-encodings will throw and 500 the validation; also skip stray separators.
- private static @NotNull Map<String, List<String>> getParameterMapFromQuery(String query) { - Map<String, List<String>> parameterMap = new HashMap<>(); + private static @NotNull Map<String, List<String>> getParameterMapFromQuery(String query) { + Map<String, List<String>> parameterMap = new LinkedHashMap<>(); if (query == null) { return parameterMap; } for (String p : query.split("&")) { Matcher m = QUERY_PARAMS_PATTERN.matcher(p); if (m.matches()) { - String key = decode(m.group(1), UTF_8); - String value = decode(m.group(2), UTF_8); - List<String> ab = parameterMap.computeIfAbsent(key, k -> new ArrayList<>()); - ab.add(value); + String rawKey = m.group(1); + String rawValue = m.group(2); + String key; + String value; + try { + key = decode(rawKey, UTF_8); + value = decode(rawValue, UTF_8); + } catch (IllegalArgumentException iae) { + // fall back to raw tokens if percent-encoding is invalid + key = rawKey; + value = rawValue; + } + if (key.isEmpty()) continue; + parameterMap.computeIfAbsent(key, k -> new ArrayList<>()).add(value); } } return parameterMap; }core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ArrayParameterParser.java (1)
29-39: JDK compatibility: replaceList.getFirst(); unify NULL handling.
List#getFirst()requires Java 21+. Useget(0)for broader compatibility and use the shared NULL constant.- String[] items = value.getFirst().split(","); + String first = value.get(0); + String[] items = first.split(","); @@ - if (items.length == 1) { - if (items[0].equals("null")) { + if (items.length == 1) { + if (NULL.equals(items[0])) { return null; }Additional import (top of file):
+import static com.predic8.membrane.core.openapi.validators.JsonSchemaValidator.*;⛔ Skipped due to learnings
Learnt from: predic8 PR: membrane/api-gateway#2068 File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90 Timestamp: 2025-08-22T13:18:09.463Z Learning: The Membrane API Gateway project uses Java 21, so modern Java 21 features like List.getFirst() are valid and should be used instead of the older get(0) approach.Learnt from: predic8 PR: membrane/api-gateway#2068 File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90 Timestamp: 2025-08-22T13:18:09.463Z Learning: The Membrane API Gateway project has mixed Java version configurations in different modules, but the team uses Java 21 in practice. Modern Java 21 features like List.getFirst() are appropriate and should be used instead of older alternatives like List.get(0).core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedArrayParameterParser.java (1)
24-31: JDK compatibility: replaceList.getFirst()usage.Use
get(0)to avoid requiring Java 21+.- if (vs.size() == 1) { - var v = vs.getFirst(); + if (vs.size() == 1) { + var v = vs.get(0);⛔ Skipped due to learnings
Learnt from: predic8 PR: membrane/api-gateway#2068 File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90 Timestamp: 2025-08-22T13:18:09.463Z Learning: The Membrane API Gateway project uses Java 21, so modern Java 21 features like List.getFirst() are valid and should be used instead of the older get(0) approach.Learnt from: predic8 PR: membrane/api-gateway#2068 File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90 Timestamp: 2025-08-22T13:18:09.463Z Learning: The Membrane API Gateway project has mixed Java version configurations in different modules, but the team uses Java 21 in practice. Modern Java 21 features like List.getFirst() are appropriate and should be used instead of older alternatives like List.get(0).core/src/test/java/com/predic8/membrane/core/openapi/validators/SchemaValidatorTest.java (1)
26-26: Test refactor to JsonSchemaValidator: LGTM
- Imports, types, and assertions updated correctly.
- The InputStream exceptional path is explicitly covered.
Consider adding a few more cases to lock new behaviors:
- Boolean: "True"/" false " (expected null or BOOLEAN per chosen policy)
- Object: additionalProperties=false with an extra field (expect 400 via ObjectValidator)
- Discriminator: mapped ref and unmapped value
Also applies to: 33-39, 52-57, 95-97
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java
Show resolved
Hide resolved
...a/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java
Show resolved
Hide resolved
...a/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (30)
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml (5)
10-15: Explicitly set style: form for clarity.Query params default to style: form, but being explicit helps future readers/tests.
in: query + style: form
16-21: Add style: form explicitly; consider maxItems only if needed.
- Be explicit with style: form (matches PR’s focus).
- If you want to satisfy Checkov’s CKV_OPENAPI_21, add maxItems, but only if the tests require a bound.
in: query + style: form schema: type: array items: type: string
23-29: Reserved characters in enum: ensure percent‑encoding in tests; add style: form.
- Values contain '=' and '#'. Clients must percent‑encode them; do not enable allowReserved here.
- Consider adding a value with a comma to validate CSV encoding with explode: false (e.g., "a,b" => "a%2Cb").
in: query + style: form explode: false schema: type: array items: type: string enum: ["foo","bar","baz", "äöü","ä=#"]
31-35: Add style: form explicitly.Keeps parameter style obvious in a spec used for serializer tests.
in: query + style: form
41-45: Add style: form explicitly for object parameter.Clarifies form + explode: false object serialization (comma‑separated key,value pairs).
- name: color in: query + style: form explode: false schema:core/src/test/java/com/predic8/membrane/core/openapi/oas31/ConstValueTest.java (2)
36-38: Close the InputStream to avoid a resource leak (prefer getApi helper).getResourceAsStream() is never closed here. Use OpenAPITestUtils.getApi(..) which wraps try-with-resources, or explicitly close the stream.
Apply this diff:
- void setUp() { - validator = new OpenAPIValidator(new URIFactory(), new OpenAPIRecord(parseOpenAPI(getResourceAsStream(this, "/openapi/specs/oas31/const-value.yaml")), new OpenAPISpec())); - } + void setUp() { + validator = new OpenAPIValidator( + new URIFactory(), + new OpenAPIRecord(getApi(this, "/openapi/specs/oas31/const-value.yaml"), new OpenAPISpec()) + ); + }
19-19: Remove unnecessary checked exception and import.The test method doesn’t throw jakarta.mail.internet.ParseException. Drop the throws clause and its import.
-import jakarta.mail.internet.*; @@ - void testJsonConst(String requestBody, int expectedErrorSize) throws ParseException { + void testJsonConst(String requestBody, int expectedErrorSize) {Also applies to: 50-50
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (5)
38-42: Narrow the return type of getMail to Schema<?> for clarity.Improves type-safety and intent without changing behavior.
- private static Object getMail(OpenAPIRecord rec) { + private static io.swagger.v3.oas.models.media.Schema<?> getMail(OpenAPIRecord rec) { return rec.api.getPaths().get("/users").getPost() .getRequestBody().getContent().get(APPLICATION_JSON) .getSchema().getProperties().get("email"); }
44-48: Fail fast with a clear message when the expected record id is missing.Prevents NPEs later and yields actionable failures.
private static OpenAPIRecord getOpenAPIRecord(String fileName, String id) { OpenAPISpec spec = new OpenAPISpec(); spec.setLocation(fileName); - return factory.create(singletonList(spec)).get(id); + Map<String, OpenAPIRecord> recs = factory.create(singletonList(spec)); + OpenAPIRecord rec = recs.get(id); + assertNotNull(rec, "OpenAPIRecord with id '" + id + "' not found for file '" + fileName + "'"); + return rec; }
90-92: Avoid brittle duplication of the conversion notice literal. Extract a constant.Reduces duplication and future maintenance if the message changes once.
- assertFalse(rec.api.getInfo().getDescription().contains( - "OpenAPI description was converted to OAS 3 from Swagger 2 by Membrane API Gateway." - )); + assertFalse(rec.api.getInfo().getDescription().contains(CONVERSION_NOTICE));Add this class constant near the top of the class:
private static final String CONVERSION_NOTICE = "OpenAPI description was converted to OAS 3 from Swagger 2 by Membrane API Gateway.";
119-119: Rename test method for clarity.Make the intent obvious in reports.
- void deep() { + void referencesDeep() {
136-138: Consider adding a multi-collision case.Add a second preexisting key “customers-api-v1-0-0” to assert next id becomes “customers-api-v1-0-1”.
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml (3)
10-36: Be explicit about style=form for query params (avoid relying on defaults).Not required, but it makes intent clear and reduces ambiguity across OAS versions/tools.
- name: number in: query + style: form explode: true schema: $ref: "#/components/schemas/Numbers" - name: string description: String param in: query + style: form explode: true schema: type: array items: type: string - name: const in: query + style: form explode: true schema: type: array items: type: string enum: ["foo","bar","baz", "äöü","ä=#"] - name: bool in: query + style: form explode: true schema: $ref: "#/components/schemas/Booleans"
24-31: Optional: allow reserved characters unencoded if that’s what you want to test.Your enum includes “ä=#”. If you want to exercise unencoded reserved chars in the URI, set allowReserved. If you intend percent‑encoding instead, keep as-is.
- name: const in: query + allowReserved: true explode: true schema: type: array items: type: string enum: ["foo","bar","baz", "äöü","ä=#"]
51-55: Remove redundantrequired: false.Default is false; dropping it declutters.
- name: bar in: query - required: false schema: type: string ... - name: pap in: query - required: false schema: type: stringAlso applies to: 67-71
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (2)
95-100: Grammar nit in error message“an valid” → “a valid”.
Apply this diff:
- if (baseSchema == null) { - return ValidationErrors.error(ctx.statusCode(400), format("Discriminator value %s is not an valid type.", propertyValue)); - } + if (baseSchema == null) { + return ValidationErrors.error(ctx.statusCode(400), format("Discriminator value %s is not a valid type.", propertyValue)); + }
181-190: Exclude patternProperties from “additionalProperties” detectionadditionalProperties should not flag keys that match patternProperties.
Apply this diff:
- private Map<String, JsonNode> getAdditionalProperties(JsonNode node) { - Map<String, JsonNode> props = new HashMap<>(); - for (Iterator<String> it = node.fieldNames(); it.hasNext(); ) { - String propName = it.next(); - if (schema.getProperties() == null || !schema.getProperties().containsKey(propName)) { - props.put(propName, node.get(propName)); - } - } - return props; - } + private Map<String, JsonNode> getAdditionalProperties(JsonNode node) { + Map<String, JsonNode> props = new HashMap<>(); + Set<String> regexes = schema.getPatternProperties() != null + ? ((Map<String, ?>) schema.getPatternProperties()).keySet() + : Collections.emptySet(); + for (Iterator<String> it = node.fieldNames(); it.hasNext(); ) { + String propName = it.next(); + boolean declared = schema.getProperties() != null && schema.getProperties().containsKey(propName); + boolean matchesPattern = !regexes.isEmpty() && regexes.stream().anyMatch(r -> Pattern.compile(r).matcher(propName).matches()); + if (!declared && !matchesPattern) { + props.put(propName, node.get(propName)); + } + } + return props; + }core/src/main/java/com/predic8/membrane/core/openapi/validators/RequestHeaderParameterValidator.java (1)
54-55: Set status code for missing required headerAlign with schema-validation branch above; make error explicitly 400.
Apply this diff:
- return ValidationErrors.error(ctx.entity(param.getName()).entityType(HEADER_PARAMETER), format("Missing required header %s.", param.getName())); + return ValidationErrors.error( + ctx.statusCode(400).entity(param.getName()).entityType(HEADER_PARAMETER), + format("Missing required header %s.", param.getName()) + );core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberRestrictionValidator.java (1)
110-111: Minor punctuation fixRemove extra period before %s.
Apply this diff:
- return ValidationErrors.error(ctx, format("The value of %s should be greater than the exclusive minimum. %s", value, schema.getExclusiveMinimumValue())); + return ValidationErrors.error(ctx, format("The value of %s should be greater than the exclusive minimum %s.", value, schema.getExclusiveMinimumValue()));core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java (1)
191-214: Fix typo in user-facing messages (“ist” -> “is”).Minor but user-visible.
Apply:
- errors.add(ctx.statusCode(401), "Caller ist not authenticated with HTTP and %s.".formatted(BASIC())); + errors.add(ctx.statusCode(401), "Caller is not authenticated with HTTP and %s.".formatted(BASIC())); ... - errors.add(ctx.statusCode(401), "Caller ist not authenticated with HTTP and %s.".formatted(BEARER())); + errors.add(ctx.statusCode(401), "Caller is not authenticated with HTTP and %s.".formatted(BEARER()));core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (3)
46-47: Pattern accepts empty keys; consider ignoring empty names early.Prevents reporting “” as an invalid parameter later.
Apply within map construction (see detailed diff below on lines 160-175).
99-111: Handle OAS 3.0 schemas withouttypes(fallback totype).Avoids skipping validation when
getTypes()is null/empty.- Set<String> types = schema.getTypes(); // Try all e.g. type: [array, null] - // Maybe there is no type - if (types == null) { - return errors; - } + Set<String> types = schema.getTypes(); // e.g., [array, null] + if (types == null || types.isEmpty()) { + String t = schema.getType(); + types = (t != null) ? Set.of(t) : Set.of("string"); + }
160-175: Harden query parsing: catch invalid percent-encoding and skip empty keys.Prevents 500s on bad input; preserves raw value for later style/explode parsing.
- for (String p : query.split("&")) { + for (String p : query.split("&")) { Matcher m = QUERY_PARAMS_PATTERN.matcher(p); if (m.matches()) { - String key = decode(m.group(1), UTF_8); // Key can here be decoded - String value = m.group(2); // Do not decode here cause it has to be done after array or object splitting - List<String> ab = parameterMap.computeIfAbsent(key, k -> new ArrayList<>()); - ab.add(value); + String rawKey = m.group(1); + String value = m.group(2); // decode later after splitting + String key; + try { + key = decode(rawKey, UTF_8); + } catch (IllegalArgumentException iae) { + key = rawKey; // fallback to raw token + } + if (key.isEmpty()) continue; // ignore stray separators + List<String> ab = parameterMap.computeIfAbsent(key, k -> new ArrayList<>()); + ab.add(value); } }core/src/test/resources/openapi/specs/oas31/parameters/object.yaml (1)
1-75: Test spec vs. security scanners.CKV_OPENAPI_4/5 flags can be ignored for test fixtures. Prefer excluding test resources in the scanner config rather than altering the spec.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java (5)
28-30: Outdated TODO comment.This class implements object parameter parsing now; update or remove the TODO.
Apply:
-/** - * TODO implement for objects in parameters - */ +/** Parser for form-style object parameters (non-exploded). */
58-61: Silent fallback on unresolved schema; add a debug log.Returning {} hides misconfiguration; emit a debug to aid diagnosis.
Apply:
- if (parameterSchema == null) { - return FACTORY.objectNode(); - } + if (parameterSchema == null) { + log.debug("No schema resolved for parameter '{}'; returning empty object.", parameter.getName()); + return FACTORY.objectNode(); + }
62-65: Trim field names; skip empty keys.Whitespace around keys can break property lookups.
Apply:
- String fieldName = tokens.pollFirst(); + String fieldName = Optional.ofNullable(tokens.pollFirst()).orElse("").trim(); + if (fieldName.isEmpty()) continue; JsonNode json = scalarAsJson(tokens.pollFirst());
80-86: Use set() consistently.For consistency with other branches, prefer set() over replace().
Apply:
- if (apb) { - obj.replace(fieldName, json); - } + if (apb) { + obj.set(fieldName, json); + }
17-27: FACTORY is declared in AbstractParameterParser; remove the JsonUtil static importAbstractParameterParser defines
protected static final JsonNodeFactory FACTORY(core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java:27). JsonUtil declares a private FACTORY (core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java:24), so theimport static com.predic8.membrane.core.util.JsonUtil.*;is misleading — drop it if unused.core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java (1)
19-21: Import StandardCharsets for explicit UTF‑8 decoding.import java.net.*; +import java.nio.charset.StandardCharsets; import static com.predic8.membrane.core.util.JsonUtil.*;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
core/src/main/java/com/predic8/membrane/core/openapi/OpenAPIValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractBodyValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/AnyOfValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/ArrayValidator.java(3 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/NotValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/NullValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberRestrictionValidator.java(5 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/ObjectValidator.java(10 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/OneOfValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/RequestHeaderParameterValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/ResponseBodyValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java(5 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/StringValidator.java(5 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java(1 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/ExplodedArrayParameterParser.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/ParameterParser.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/JsonUtil.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/ConstValueTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java(6 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/CompositionTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/object.yaml(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedArrayParameterParser.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameterParser.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/BooleanValidator.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParser.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/ExclusiveMinMaxTest.java
- core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ExplodedArrayQueryParameterTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/CompositionTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/NullValidator.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
🧬 Code graph analysis (19)
core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberRestrictionValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/OneOfValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/NotValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/AnyOfValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/OpenAPIValidator.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ResponseBodyValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java (2)
core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
JsonUtil(22-78)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)
core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractBodyValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-132)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(44-237)
core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ArrayValidator.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java (1)
Utils(43-307)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (2)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)core/src/main/java/com/predic8/membrane/core/openapi/util/SchemaUtil.java (1)
SuppressWarnings(27-53)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/ConstValueTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/main/java/com/predic8/membrane/core/openapi/validators/RequestHeaderParameterValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (5)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(25-69)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java (1)
core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
JsonUtil(22-78)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml
[high] 1-73: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-73: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 19-23: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
core/src/test/resources/openapi/specs/oas31/parameters/object.yaml
[high] 1-76: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-76: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml
[high] 1-90: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-90: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 20-24: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (51)
core/src/test/resources/openapi/specs/oas31/parameters/explode-false.yaml (4)
45-53: Confirm object field requirements.additionalProperties: false + no required means R/G/B are optional. If tests expect all channels present, add required: [R, G, B]; otherwise keep as is.
61-63: Good fix: minimum/maximum are correct for JSON Schema.
65-67: OAS 3.1 nullability usage looks correct.type: [array, "null"] is valid JSON Schema in 3.1 for nullable arrays.
1-3: Security warnings from scanners are not applicable to test specs.This file is a focused test fixture; omitting global/operation security is fine. No action needed.
core/src/test/java/com/predic8/membrane/core/openapi/oas31/ConstValueTest.java (2)
28-28: LGTM: Consolidated OpenAPITestUtils static import.This aligns the test with the shared helpers and improves readability.
51-51: LGTM: Use of var for local inference is fine here.core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (4)
22-22: LGTM on import updates.Static imports for test utilities and
singletonListimprove readability.Also applies to: 24-24
51-51: Rename to OpenAPI 3.0 acknowledged.Test intent reads clearly.
130-130: LGTM: getUniqueIdNoCollision uses parsed OpenAPI via test utils.Clear and robust.
109-116: Verified: both referenced YAML files exist under test resources.
core/src/test/resources/openapi/specs/oas31/request-reference.yaml and core/src/test/resources/openapi/specs/oas31/references/request-reference.yaml are present — no change required.core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml (2)
1-5: Well-formed OAS 3.1 test spec — LGTMFits the PR’s goal of exercising exploded/form array and basic query params.
1-89: Exclude test OpenAPI specs from Checkov (suppress CKV_OPENAPI_4/5/21) or scope Checkov to production specs.
No Checkov config or workflow was found in this repo; if your CI runs Checkov, add an exclusion for core/src/test/resources/openapi (or suppress CKV_OPENAPI_4/5/21 for test files) to avoid false positives.core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (2)
236-237: List.getFirst() requires Java 21 — confirm baselineIf target JDK < 21, use get(0).
Apply this diff if you need pre‑21 compatibility:
- errors.add(new ValidationError(ctx.addJSONpointerSegment(missingProperties.getFirst()), format("Required property %s is missing.", missingProperties.getFirst()))); + errors.add(new ValidationError(ctx.addJSONpointerSegment(missingProperties.get(0)), format("Required property %s is missing.", missingProperties.get(0))));
66-68: Remove canValidate() call inside validate() to avoid unintended exceptions (duplicate from earlier review)Calling canValidate() here can throw on bad inputs and is redundant with instanceof.
Apply this diff:
- if (canValidate(obj) == null || !(obj instanceof ObjectNode node)) { + if (!(obj instanceof ObjectNode node)) { return ValidationErrors.error(ctx.statusCode(400), format("Value %s is not an object.", obj)); }core/src/main/java/com/predic8/membrane/core/openapi/validators/ResponseBodyValidator.java (1)
128-129: LGTM: migrated to ValidationErrors.error(...)Consistent with the new factory; behavior unchanged.
core/src/main/java/com/predic8/membrane/core/openapi/OpenAPIValidator.java (1)
89-93: LGTM: error factory migrationGood switch to ValidationErrors.error(...) with proper context.
core/src/main/java/com/predic8/membrane/core/openapi/validators/NotValidator.java (1)
40-41: LGTM: error factory migrationSwap to ValidationErrors.error(...) is correct.
core/src/main/java/com/predic8/membrane/core/openapi/validators/OneOfValidator.java (1)
52-53: LGTM: error factory migrationConsistent with ValidationErrors API change.
core/src/main/java/com/predic8/membrane/core/openapi/validators/AnyOfValidator.java (1)
51-52: LGTM: error factory migrationMatches the repo‑wide change.
core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java (2)
145-148: Factory rename usage looks correct.
216-221: Factory rename usage looks correct.core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractBodyValidator.java (2)
45-56: Guard against null schema in JSON branch; avoid NPE and return validator result directly.If
mediaTypeObj.getSchema()is null,new SchemaValidator(...).validate(...)will NPE.Suggested fix:
- if ( APPLICATION_JSON_CONTENT_TYPE.match(mediaType)) { - if (mediaTypeObj.getSchema() != null && mediaTypeObj.getSchema().get$ref() != null) { - ctx = ctx.schemaType(mediaTypeObj.getSchema().get$ref()); - } - return errors.add(new SchemaValidator(api, mediaTypeObj.getSchema()).validate(ctx, message.getBody())); - } + if (APPLICATION_JSON_CONTENT_TYPE.match(mediaType)) { + Schema schema = mediaTypeObj.getSchema(); + if (schema == null) + return errors; // no schema -> nothing to validate + if (schema.get$ref() != null) + ctx = ctx.schemaType(schema.get$ref()); + return new SchemaValidator(api, schema).validate(ctx, message.getBody()); + }
95-103: Good: switched toValidationErrors.error(...)for wrong media type path.core/src/main/java/com/predic8/membrane/core/openapi/validators/ArrayValidator.java (3)
31-31: Interface migration toJsonSchemaValidatorlooks good.
63-69: Message grammar fixes LGTM.
88-90: uniqueItems error construction viaerror(...)LGTM.Null-safe addition pattern is preserved via
add(ValidationErrors).core/src/main/java/com/predic8/membrane/core/openapi/validators/StringValidator.java (3)
29-31: Interface migration toJsonSchemaValidatorlooks good.
40-49:canValidateflow is clearer; behavior preserved.
187-195: Const/enum/pattern message tweaks LGTM.core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberValidator.java (2)
31-51: Switch to BigDecimal for detection is a solid precision win.
57-76: Validation logic is precise and null-safe; usingerror(...)is consistent.core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (3)
234-236: UsinggetRawQuery()is correct for preserving percent-encoding for later parsing.Please confirm tests cover cases with:
- invalid percent-encoding in keys (now gracefully handled if you apply the hardening)
- empty-value parameters (e.g.,
?flag&arr=&arr=1)
194-196: Detect query params viain="query"(covers $ref) instead ofinstanceof QueryParameter.Prevents missing parameters defined generically or by $ref.
- Set<Parameter> getAllQueryParameters(Operation operation) { - return getAllParameter(operation).stream().filter(p -> p instanceof QueryParameter).collect(toSet()); - } + Set<Parameter> getAllQueryParameters(Operation operation) { + return getAllParameter(operation).stream() + .filter(Objects::nonNull) + .filter(p -> "query".equalsIgnoreCase(p.getIn())) + .collect(toCollection(LinkedHashSet::new)); + }
225-232: Null-safe check forParameter.getRequired()to avoid NPE.
getRequired()is nullable in OAS; auto-unboxing can throw.- return parameters.stream() - .filter(Parameter::getRequired).map(Parameter::getName).collect(toSet()); + return parameters.stream() + .filter(p -> Boolean.TRUE.equals(p.getRequired())) + .map(Parameter::getName) + .collect(toSet());core/src/main/java/com/predic8/membrane/core/openapi/validators/IntegerValidator.java (1)
26-77: Integer detection/validation logic is robust and null-safe; interface migration LGTM.core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
31-35: Approve — rename to error() applied; no remaining ValidationErrors.create(...) usages found.
Searched for "ValidationErrors.create" and static imports across the repo; no matches.core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (11)
16-16: LGTM! Added Jackson TextNode import for test data.The addition of Jackson TextNode imports is appropriate for the test refactoring that replaces raw strings with TextNode wrappers for query parameter values.
25-28: LGTM! Added necessary static imports.The new static imports are appropriate for:
Request.*factory methods for creating request objectsOpenAPITestUtils.*for test utilitiesOpenAPIUtil.*for utility functionsQueryParameterValidator.getQueryStringfor direct access in tests
36-48: LGTM! Added ValidationContext initialization and improved setup.The addition of the
ValidationContextfield and its initialization in setup is necessary for the updated test methods that require this context. The explicit initialization ensures tests have proper validation context.
50-53: LGTM! Added empty query parameter test.This test appropriately verifies that an empty query string (with only
?) yields zero validated parameters, which is a good edge case to test.
76-83: LGTM! Refactored to use getGET helper method.The introduction of the
getGET()helper method improves code reusability and makes the test more readable. The test logic remains correct for verifying referenced parameters.
86-105: LGTM! Updated tests with TextNode wrappers.The conversion from raw string values to Jackson
TextNodewrappers is appropriate for the refactored validation logic. This ensures proper JSON node representation in the test data.
108-115: LGTM! Test method correctly validates security scheme collection.The test appropriately verifies that the
securitySchemeApiKeyQueryParamNamesmethod correctly extracts API key query parameter names from security schemes.
117-128: LGTM! Added query string extraction tests.Both tests appropriately verify the
getQueryStringmethod behavior:
- Basic query string extraction functionality
- Preservation of raw encoding (important for proper parameter parsing later)
131-174: LGTM! Well-organized nested test class with comprehensive coverage.The
UtilMethodsnested test class provides good coverage of utility methods with clear test separation. The helper methodgetPathItem()appropriately loads a different OpenAPI spec for testing parameter functionality.
176-178: LGTM! Preserved legacy helper method.The
operationHasParamWithNameutility method is appropriately preserved as a static helper for parameter name validation in tests.
60-60: Renamed getAllParameterSchemas → getAllParameter — verified.No remaining calls to getAllParameterSchemas; getAllParameter is declared in core/src/main/java/com/predic8/membrane/core/openapi/validators/AbstractParameterValidator.java:50 and used in core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:195 and core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java:67.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java (2)
43-43: Potential JDK compatibility: List.getFirst() requires Java 21.Use index access to keep compatibility with Java 17 builds.
Apply:
- Collections.addAll(tokens, values.getFirst().split(",")); + Collections.addAll(tokens, values.get(0).split(","));⛔ Skipped due to learnings
Learnt from: predic8 PR: membrane/api-gateway#2068 File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90 Timestamp: 2025-08-22T13:18:09.463Z Learning: The Membrane API Gateway project uses Java 21, so modern Java 21 features like List.getFirst() are valid and should be used instead of the older get(0) approach.Learnt from: predic8 PR: membrane/api-gateway#2068 File: core/src/test/java/com/predic8/membrane/core/interceptor/balancer/ClusterBalancerTest.java:73-90 Timestamp: 2025-08-22T13:18:09.463Z Learning: The Membrane API Gateway project has mixed Java version configurations in different modules, but the team uses Java 21 in practice. Modern Java 21 features like List.getFirst() are appropriate and should be used instead of older alternatives like List.get(0).
33-37: Confirmed: explode=true selects ExplodedObjectParameterParser — no change required.
AbstractParameterParser returns new ExplodedObjectParameterParser when parameter.getExplode() is true (core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java).core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java (2)
25-28: Guard against empty/multi-valued scalars; pick first; decode as UTF‑8.Avoid NoSuchElementException when absent, handle repeats deterministically (first-one-wins), and specify UTF‑8 for decoding.
@Override public JsonNode getJson() throws ParameterParsingException { - return scalarAsJson(URLDecoder.decode(getValuesForParameter().getFirst())); + var vs = getValuesForParameter(); + if (vs == null || vs.isEmpty()) { + // Absence -> JSON null; required/nullable handled upstream + return scalarAsJson(null); + } + // Multi-valued scalar: first-one-wins (align with other parsers) + String value = vs.getFirst(); + return scalarAsJson(URLDecoder.decode(value, StandardCharsets.UTF_8)); }
25-28: No change required — decoding here is correct.QueryParameterValidator.getParameterMapFromQuery(...) decodes only the parameter name and leaves the parameter values percent-encoded before calling setValues(...), so ScalarParameterParser must decode the value (URLDecoder.decode(...)). Files: core/src/main/java/.../QueryParameterValidator.java (getParameterMapFromQuery) and core/src/main/java/.../ScalarParameterParser.java.
core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberRestrictionValidator.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java
Show resolved
Hide resolved
...main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java (2)
203-209: BEARER auth message still says “ist” — fix to “is” for consistency.Align with the BASIC branch and fix the typo.
Apply this diff:
- errors.add(ctx.statusCode(401), "Caller ist not authenticated with HTTP and %s.".formatted(BEARER())); + errors.add(ctx.statusCode(401), "Caller is not authenticated with HTTP and %s.".formatted(BEARER()));
195-201: Avoid potential NPE when HTTP scheme is missing.schemeDefinition.getScheme() may be null in malformed specs; guard it and return a server‑side config error instead of throwing.
Apply this diff:
- switch (schemeDefinition.getScheme().toLowerCase()) { + String httpScheme = schemeDefinition.getScheme(); + if (httpScheme == null) { + // OpenAPI misconfiguration on server side + return getValidationErrorsProblemServerSide(ctx); + } + switch (httpScheme.toLowerCase()) {core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (2)
119-126: Assert rec not null before using.Same guard as above.
void referencesDepth() { OpenAPIRecord rec = getOpenAPIRecord("oas31/references/deep/deep.oas.yaml", "deep-refs-v1-0-0"); + assertNotNull(rec); assertEquals("Deep Refs", rec.api.getInfo().getTitle()); assertEquals(V31, rec.api.getSpecVersion()); assertNotNull(getMail(rec)); }
109-116: Add assertNotNull(rec) and restore missing test resources
- Add assertion in core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java -> referencesRelativeFilesInSameDirectory():
void referencesRelativeFilesInSameDirectory() { OpenAPIRecord rec = getOpenAPIRecord("oas31/references/request-reference.yaml", "demo-v1-0-0"); + assertNotNull(rec); assertEquals("Demo", rec.api.getInfo().getTitle()); assertEquals(V31, rec.api.getSpecVersion()); assertNotNull(getMail(rec)); }
- Restore or correct these missing test resources referenced by the tests:
src/test/resources/openapi/specs/customers.yml
src/test/resources/openapi/specs/fruitshop-swagger-2.0.json
src/test/resources/openapi/specs/fruitshop-api-v2-openapi-3.yml
src/test/resources/openapi/specs/oas31/request-reference.yaml
src/test/resources/openapi/specs/oas31/references/request-reference.yaml
src/test/resources/openapi/specs/oas31/references/deep/deep.oas.yaml
🧹 Nitpick comments (13)
core/src/test/java/com/predic8/membrane/core/openapi/validators/security/AndOrSecurityValidatorTest.java (1)
74-74: Grammar nit: prefer “HTTP Basic (authentication) scheme”.Current text “HTTP and basic scheme” reads awkward. Consider aligning the production message (and this assertion) to “Caller is not authenticated with HTTP Basic scheme” for clarity.
core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java (1)
106-110: Prefer collect over reduce for mutable accumulator.Using collect improves clarity and avoids misuse of reduce for mutation.
Apply this diff:
- return requirement.keySet().stream() // Names of SecurityRequirements - .map(requirementName -> checkSingleRequirement(ctx, requirement, request, requirementName)) - .reduce(new ValidationErrors(), ValidationErrors::add); + return requirement.keySet().stream() // Names of SecurityRequirements + .map(requirementName -> checkSingleRequirement(ctx, requirement, request, requirementName)) + .collect(ValidationErrors::new, ValidationErrors::add, ValidationErrors::add);core/src/test/java/com/predic8/membrane/core/transport/http/Http2ClientTest.java (1)
41-45: Configure the connection first; avoid magic numbers in-line.Build the connection, set its timeout, then attach it. Also prefer digit separators for readability.
Apply:
- config.setConnection(getConnectionConfiguration()); - config.getConnection().setSoTimeout(10000); + ConnectionConfiguration conn = getConnectionConfiguration(); + conn.setSoTimeout(10_000); + config.setConnection(conn);core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml (2)
10-14: Make query param style explicit for arrays (clarity, future‑proofing).Explicitly set style: form on array query params to avoid relying on defaults across 3.0/3.1 and specs/tools.
Apply this diff:
- name: number in: query + style: form explode: true schema: $ref: "#/components/schemas/Numbers" - name: string description: String param in: query + style: form explode: true schema: type: array items: type: string - name: const in: query + style: form explode: true allowReserved: true schema: type: array items: type: string enum: ["foo","bar","baz", "äöü","ä=#"] - name: bool in: query + style: form explode: true schema: $ref: "#/components/schemas/Booleans"Also applies to: 15-23, 24-31, 32-36
75-76: Consistency nit: unify 200 response quoting and description casing.Use the same quoting and description casing as above.
Apply this diff:
- '200': - description: Ok + "200": + description: OKcore/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
70-71: Preserve deterministic order for unknown-field reporting.Use LinkedHashSet to keep insertion order (helps predictable error messages).
- Set<String> fields = new HashSet<>(parameterMap.keySet()); + Set<String> fields = new LinkedHashSet<>(parameterMap.keySet());core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
76-82: Guard against nullvaluesmap.Defensive check avoids NPE if
setValueswasn’t called.- return values.getOrDefault(paramName, List.of()); + return values != null ? values.getOrDefault(paramName, List.of()) : List.of();core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java (2)
41-45: Use the shared OBJECT constant for consistency.Avoid mixing string literals and constants.
- colorParser = AbstractParameterParser.instance(validator.getApi(), "object", color); + colorParser = AbstractParameterParser.instance(validator.getApi(), OBJECT, color);
78-79: Remove stray stdout in tests.Avoid noisy test output.
- System.out.println("fields = " + fields);core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (4)
38-42: Strongly type getMail() to Schema<?> for clarity.Avoids returning raw Object and improves readability.
- private static Object getMail(OpenAPIRecord rec) { + private static io.swagger.v3.oas.models.media.Schema<?> getMail(OpenAPIRecord rec) { return rec.api.getPaths().get("/users").getPost() .getRequestBody().getContent().get(APPLICATION_JSON) .getSchema().getProperties().get("email"); }
44-48: Fail fast if record id not found in helper.Improves test diagnostics vs. NPEs downstream.
- private static OpenAPIRecord getOpenAPIRecord(String fileName, String id) { - OpenAPISpec spec = new OpenAPISpec(); - spec.setLocation(fileName); - return factory.create(singletonList(spec)).get(id); - } + private static OpenAPIRecord getOpenAPIRecord(String fileName, String id) { + OpenAPISpec spec = new OpenAPISpec(); + spec.setLocation(fileName); + Map<String, OpenAPIRecord> recs = factory.create(singletonList(spec)); + OpenAPIRecord rec = recs.get(id); + assertNotNull(rec, "Did not find OpenAPIRecord for id '" + id + "' (location: " + fileName + ")"); + return rec; + }
52-56: Avoid double-brace initialization in tests.Cleaner, avoids anonymous subclasses and potential pitfalls.
- Collection<OpenAPISpec> specs = new ArrayList<>(); - specs.add(new OpenAPISpec() {{ - setLocation("customers.yml"); - }}); + OpenAPISpec spec = new OpenAPISpec(); + spec.setLocation("customers.yml"); + Collection<OpenAPISpec> specs = singletonList(spec);
97-101: Avoid double-brace initialization here as well.Consistent with earlier refactor.
- OpenAPIRecord rec = factory.create(new ArrayList<>() {{ - add(new OpenAPISpec() {{ - setLocation("oas31/request-reference.yaml"); - }}); - }}).get("demo-v1-0-0"); + OpenAPISpec spec = new OpenAPISpec(); + spec.setLocation("oas31/request-reference.yaml"); + OpenAPIRecord rec = factory.create(singletonList(spec)).get("demo-v1-0-0");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
core/src/main/java/com/predic8/membrane/core/openapi/validators/NotValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberRestrictionValidator.java(5 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java(10 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/RequestHeaderParameterValidator.java(3 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java(6 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.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/AbstractParameterParser.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/ParameterParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/oas31/ConstValueTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java(6 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/IntegerValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/SchemaValidatorTest.java(4 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/security/AndOrSecurityValidatorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/transport/http/Http2ClientTest.java(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java
- distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/NumberRestrictionValidator.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParser.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/IntegerValidatorTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/RequestHeaderParameterValidator.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractArrayParameterParser.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/NotValidator.java
- core/src/test/java/com/predic8/membrane/core/openapi/oas31/ConstValueTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/QueryParameterValidator.java
🧬 Code graph analysis (6)
core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java (5)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(27-83)core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java (3)
Nested(45-107)Nested(48-106)Nested(89-105)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectAdditionalParameterTest.java (1)
Nested(39-85)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterTest.java (1)
Nested(48-67)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (3)
core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java (1)
Utils(43-307)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)core/src/main/java/com/predic8/membrane/core/openapi/util/SchemaUtil.java (1)
SuppressWarnings(27-53)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (5)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(33-107)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(27-83)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml
[high] 1-89: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-89: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 20-24: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (35)
core/src/test/java/com/predic8/membrane/core/openapi/validators/security/AndOrSecurityValidatorTest.java (1)
74-74: LGTM: fixed typo in expected error text.Assertion now matches the corrected message.
core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java (4)
145-148: Correct factory method usage for server-side config errors.Switch to ValidationErrors.error(...) is correct and aligns with the updated API.
183-186: Scope message typo fixed.Grammar corrected to “is not in scope …”. Good.
195-201: BASIC auth message fixed.Grammar corrected; status 401 is appropriate for authentication failures.
216-221: Correct factory method and status code for OAuth2/JWT missing.Using ValidationErrors.error(...) with 401 is appropriate here.
core/src/test/java/com/predic8/membrane/core/transport/http/Http2ClientTest.java (1)
41-45: Timeout units are consistent — both use milliseconds.ConnectionConfiguration#setSoTimeout and setKeepAliveTimeout are documented as milliseconds; setSoTimeout(10000) and setKeepAliveTimeout(100) are therefore millisecond values (keepAlive = 100ms). No change required.
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml (3)
6-44: LGTM: solid 3.1 test fixture for query-array explode cases.Good coverage for array params (including Unicode and reserved chars) and required param scoping. No blockers.
1-6: About Checkov hints: safe to ignore for test fixtures; confirm CI doesn’t gate on them.Security blocks and maxItems are unnecessary in test resources; ensure Checkov doesn’t fail the build for files under core/src/test/resources.
If needed, exclude this path in CI or add repo‑level Checkov config to ignore CKV_OPENAPI_4, CKV_OPENAPI_5, CKV_OPENAPI_21 for test specs. Want a PR to adjust CI/config?
23-31: Verified — Unicode and reserved chars handled; no change required.ExplodedArrayQueryParameterTest and ArrayQueryParameterTest (using core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml) assert both raw "äöü" and percent-encoded "%C3%A4%3D%23" are accepted and validated against the enum.
core/src/main/java/com/predic8/membrane/core/openapi/validators/ObjectValidator.java (6)
57-63: LGTM! Fixed null-safety and typo.The null-safety issue and typo reported in previous reviews have been corrected.
105-113: LGTM! Fixed null reference handling.The null-safety issue reported in previous reviews has been properly addressed - the method now checks for null
refSchemaand returns an appropriate error instead of passing null toSchemaValidator.
152-180: Clean refactor of additional properties validation.The method has been well-refactored to handle different
additionalPropertiestypes properly. The boolean handling logic is correct and the error messages are clear.
186-199: Solid implementation of additional properties detection.The logic correctly identifies properties that are neither declared in the schema nor match any pattern properties. The pattern matching approach is appropriate.
69-71: Fix canValidate() usage in validate() to prevent exceptions.The code still calls
canValidate(obj)inside validate(), which can throw exceptions for bad inputs. This is the same issue flagged in previous reviews.Apply this diff to fix the issue:
- if (canValidate(obj) == null || !(obj instanceof ObjectNode node)) { + if (!(obj instanceof ObjectNode node)) {
119-123: Add null-safety check for api.getComponents().The method could throw NPE if
api.getComponents()returns null.Apply this diff to add proper null checking:
private Schema<?> getBaseSchema(String propertyValue) { - if (api.getComponents() == null || api.getComponents().getSchemas() == null) + if (api.getComponents() == null || api.getComponents().getSchemas() == null) return null; return api.getComponents().getSchemas().get(propertyValue); }Wait, the code already has the null check. This is correct.
core/src/test/java/com/predic8/membrane/core/openapi/validators/SchemaValidatorTest.java (4)
34-39: LGTM! Interface migration to JsonSchemaValidator.The migration from
IJSONSchemaValidatortoJsonSchemaValidatoris consistent with the broader API refactoring across the codebase.
53-59: LGTM! Updated test method signature and assertions.The method signature and test logic have been properly updated to work with the new
JsonSchemaValidatorinterface.
95-97: LGTM! Clearer test case comments.The explicit type comments for Double and Long test cases improve test readability.
115-117: LGTM! Updated test method.The test method has been properly renamed and updated to use the new utilities.
core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java (4)
38-38: LGTM! Interface migration to JsonSchemaValidator.The interface change from
IJSONSchemaValidatortoJsonSchemaValidatoraligns with the broader API migration across the validator suite.
148-149: LGTM! Updated error construction.The migration from
ValidationErrors.create(...)toValidationErrors.error(...)is consistent with the updated ValidationErrors API.Also applies to: 192-193
122-122: LGTM! Explicit generic type usage.The explicit cast to
ArrayList<String>provides better type safety than using raw types.
200-210: LGTM! Updated return type.The return type change from
List<IJSONSchemaValidator>toList<JsonSchemaValidator>is consistent with the interface migration.core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (5)
133-140: Good null-safe type handling.Fallback to single
typeor"string"avoids NPEs for OAS 3.0 schemas.
231-233: LGTM: raw query extraction is correct here.Using
getRawQuery()preserves delimiters for later splitting; confirmed by tests thatRequest.get().path(...)retains the query.
193-195: Detect query params byin="query"(notinstanceof QueryParameter).This misses generic/
$refed parameters and can skip valid query params.Apply:
- Set<Parameter> getAllQueryParameters(Operation operation) { - return getAllParameter(operation).stream().filter(p -> p instanceof QueryParameter).collect(toSet()); - } + Set<Parameter> getAllQueryParameters(Operation operation) { + return getAllParameter(operation).stream() + .filter(Objects::nonNull) + .filter(p -> "query".equalsIgnoreCase(p.getIn())) + .collect(toCollection(LinkedHashSet::new)); + }
222-229: Possible NPE onParameter::getRequired. Use null‑safe Boolean check.Auto‑unboxing can throw NPE when
requiredis unspecified.Apply:
- return parameters.stream() - .filter(Parameter::getRequired).map(Parameter::getName).collect(toSet()); + return parameters.stream() + .filter(Objects::nonNull) + .filter(p -> Boolean.TRUE.equals(p.getRequired())) + .map(Parameter::getName) + .collect(toSet());
165-181: Robustness: guard against invalid percent‑encoding in keys.
URLDecoder.decode(...)throwsIllegalArgumentExceptionon malformed input; current code will crash on bad queries.Apply:
- Map<String, List<String>> parameterMap = new HashMap<>(); + Map<String, List<String>> parameterMap = new LinkedHashMap<>(); @@ - if (m.matches()) { - String key = decode(m.group(1), UTF_8); // Key can here be decoded - if (key.isEmpty()) continue; // ignore stray separators - String value = m.group(2); // Do not decode here cause it has to be done after array or object splitting - List<String> ab = parameterMap.computeIfAbsent(key, k -> new ArrayList<>()); - ab.add(value); - } + if (m.matches()) { + String rawKey = m.group(1); + String rawValue = m.group(2); + String key; + try { + key = decode(rawKey, UTF_8); // decode key early + } catch (IllegalArgumentException iae) { + key = rawKey; // fallback to raw if malformed + } + if (key.isEmpty()) continue; // ignore stray separators + String value = rawValue; // value decoded later by parsers + List<String> vals = parameterMap.computeIfAbsent(key, k -> new ArrayList<>()); + vals.add(value); + }Optional: also accept semicolon separators per RFC 1866 form-style queries.
- for (String p : query.split("&")) { + for (String p : query.split("[&;]")) {core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
62-69: Null-safe explode default matches OAS (form defaults to true).Correctly avoids autounboxing NPE and applies spec default.
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (6)
22-22: Good move: centralize OpenAPI parsing via test utils.Reduces duplication and keeps parsing options consistent across tests.
24-24: LGTM on singletonList import.Used appropriately in helper.
51-62: LGTM: OAS 3.0 test.Asserts look correct for 3.0.
130-131: LGTM: getUniqueId without collision.Using OpenAPITestUtils.getApi() keeps this test focused and robust.
136-138: LGTM: collision path covered.Simple pre-population of the map is sufficient for the test intent.
65-70: Add explicit not-null assertion for rec.Prevents NPE masking failures.
void readAndParseSwagger2() { OpenAPIRecord rec = getOpenAPIRecord("fruitshop-swagger-2.0.json", "fruit-shop-api-swagger-2-v1-0-0"); + assertNotNull(rec); assertNotNull(rec); assertEquals("Fruit Shop API Swagger 2", rec.api.getInfo().getTitle()); assertEquals(V30, rec.api.getSpecVersion()); }Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (20)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (2)
198-214: Make parsing robust and deterministic: preserve key order and tolerate bad encoding.
- Use LinkedHashMap to preserve insertion order (stable error ordering).
- Guard URLDecoder against IllegalArgumentException and fall back to raw key.
Apply:
- private static @NotNull Map<String, List<String>> getParameterMapFromQuery(String query) { - Map<String, List<String>> parameterMap = new HashMap<>(); + private static @NotNull Map<String, List<String>> getParameterMapFromQuery(String query) { + Map<String, List<String>> parameterMap = new LinkedHashMap<>(); if (query == null || query.isEmpty()) { return parameterMap; } for (String p : query.split("&")) { Matcher m = QUERY_PARAMS_PATTERN.matcher(p); if (m.matches()) { - var key = decode(m.group(1), UTF_8); // Key can here be decoded + String rawKey = m.group(1); + String key; + try { + key = decode(rawKey, UTF_8); // Key can be decoded here + } catch (IllegalArgumentException ex) { + key = rawKey; + } if (key.isEmpty()) continue; // ignore stray separators var value = m.group(2); // Do not decode here cause it has to be done after array or object splitting var ab = parameterMap.computeIfAbsent(key, k -> new ArrayList<>()); ab.add(value); } } return parameterMap; }
233-246: Avoid duplicate object-type checks; reuse OpenAPIUtil.hasObjectType.Use the existing util instead of a local isObjectType(), then drop the helper below.
Apply:
- if (isObjectType(schema) && schema.getProperties() != null) { + if (OpenAPIUtil.hasObjectType(schema) && schema.getProperties() != null) { schema.getProperties().forEach((name, ignored) -> names.add(name)); }And remove the local isObjectType(...) method (Lines 248-251).
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
99-109: Support parameters defined via content (schema may be null).OAS allows parameter.content with schema instead of parameter.schema. Consider resolving from content as fallback.
Apply:
- public static Schema<?> resolveSchema(OpenAPI api, Parameter p) { - Schema<?> schema = p.getSchema(); + public static Schema<?> resolveSchema(OpenAPI api, Parameter p) { + Schema<?> schema = p.getSchema(); if (schema == null) { - return null; + if (p.getContent() != null && !p.getContent().isEmpty()) { + var mt = p.getContent().values().iterator().next(); + schema = mt != null ? mt.getSchema() : null; + } + if (schema == null) return null; } if (schema.get$ref() != null) { if (api.getComponents() == null || api.getComponents().getSchemas() == null) return null; return api.getComponents().getSchemas().get(getComponentLocalNameFromRef(schema.get$ref())); } return schema; }core/src/test/resources/openapi/specs/query-params.yml (1)
41-41: YAML commas spacing for readability.Add spaces after commas in enum array.
Apply:
- enum: ["Bonn","New York","Delhi"] + enum: ["Bonn", "New York", "Delhi"]Also applies to: 31-41
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (3)
68-70: Use matching validator for the path under test.Leverage objectValidator for /object to avoid path/operation mismatch.
Apply:
- assertEquals(0, citiesValidator.validate(ctx, Request.get().path("/object?"), getGET("/object")).size()); + assertEquals(0, objectValidator.validate(ctx, Request.get().path("/object?"), getGET("/object")).size());
251-253: Don’t assert implementation class; assert intent (in="query").This test will fail if parameters are generic Parameter with in="query". Make the test resilient.
Apply:
- assertTrue(qp.stream().allMatch(p -> p instanceof QueryParameter)); + assertTrue(qp.stream().allMatch(p -> "query".equalsIgnoreCase(p.getIn())));
232-239: Nit: typo in test name.Consider renaming to ignoreAdditionalFromObject for consistency.
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml (1)
31-31: YAML commas spacing for readability.Add spaces after commas to satisfy linters.
Apply:
- enum: ["foo","bar","baz", "äöü","ä=#"] + enum: ["foo", "bar", "baz", "äöü", "ä=#"]core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java (2)
192-193: Message phrasing: “type” vs “format”.This branch reports type errors, not format. Adjust wording.
Apply:
- return ValidationErrors.error(ctx, "%s is not of %s format.".formatted(value, type)); + return ValidationErrors.error(ctx, "%s is not of type %s.".formatted(value, type));
200-209: Micro-opt: reuse validator instances.getValidatorClasses() builds a new list each time. Consider a static constant to avoid allocations on hot paths.
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (2)
68-70: Align validator with operation under test.Use objectValidator for /object.
Apply:
- assertEquals(0, citiesValidator.validate(ctx, Request.get().path("/object?"), getGET("/object")).size()); + assertEquals(0, objectValidator.validate(ctx, Request.get().path("/object?"), getGET("/object")).size());
251-253: Make assertion robust: check in="query" instead of instanceof.Prevents false failures when parser returns generic Parameter.
Apply:
- assertTrue(qp.stream().allMatch(p -> p instanceof QueryParameter)); + assertTrue(qp.stream().allMatch(p -> "query".equalsIgnoreCase(p.getIn())));core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java (1)
47-49: Preserve trailing empty tokens when splittingUsing split(",", -1) keeps trailing empties, allowing correct null-as-empty handling for the last pair.
- Collections.addAll(tokens, values.getFirst().split(",")); + Collections.addAll(tokens, values.getFirst().split(",", -1));core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
66-72: Avoid NPE when values map is not setDefensive guard if setValues() was not called.
protected List<String> getValues() { String paramName = parameter.getName(); if (paramName == null) { return List.of(); } - return values.getOrDefault(paramName, List.of()); + if (values == null) return List.of(); + return values.getOrDefault(paramName, List.of()); }core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java (1)
191-213: Consider 500 for unsupported HTTP auth schemesAn unsupported scheme in the OpenAPI definition is a server-side misconfiguration; 500 may be more appropriate than 401.
- return error(ctx.statusCode(401), "Scheme %s is not supported".formatted(schemeDefinition.getScheme())); + return error(ctx.statusCode(500), "HTTP auth scheme '%s' is not supported.".formatted(schemeDefinition.getScheme()));core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java (5)
37-41: Close resource: use helper that manages the InputStream.
parseOpenAPI(getResourceAsStream(...))leaves the stream open. PrefergetApi(...)which closes the stream.- var apiRecord = new OpenAPIRecord( - parseOpenAPI(getResourceAsStream(this, "/openapi/specs/oas31/parameters/explode-false.yaml")), - new OpenAPISpec() - ); + var apiRecord = new OpenAPIRecord( + getApi(this, "/openapi/specs/oas31/parameters/explode-false.yaml"), + new OpenAPISpec() + );
75-75: Make assertion resilient to quoting variations.Error messages may change quoting style; avoid asserting on surrounding quotes.
- assertTrue(e0.getMessage().contains("'foo,bar'")); + assertTrue(e0.getMessage().contains("foo,bar"));
95-95: Reduce brittleness of message assertion.Relying on exact phrasing/quoting is fragile. Check key tokens instead.
- assertTrue(err.get(0).getMessage().contains("\"foo\" is of type string")); + assertTrue(err.get(0).getMessage().contains("foo")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("type")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("string"));
102-102: Relax assertion to tolerate wording changes.Message wording for null handling varies; assert on essentials.
- assertTrue(err.get(0).getMessage().contains("null is of type")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("null")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("type"));
69-76: Consider relocating this test to the Invalid group.
rawQueryIsUsedToSplitParametersexpects an error; placing it underValidis misleading. Move it underInvalidor rename the grouping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
core/src/main/java/com/predic8/membrane/core/interceptor/ratelimit/RateLimitInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/OperationValidator.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java(8 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java(4 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedArrayParameterParser.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/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java(5 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/security/AndOrSecurityValidatorTest.java(1 hunks)core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml(1 hunks)core/src/test/resources/openapi/specs/query-params.yml(1 hunks)docs/ROADMAP.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/ratelimit/RateLimitInterceptor.java
🚧 Files skipped from review as they are similar to previous changes (5)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedArrayParameterParser.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/security/AndOrSecurityValidatorTest.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ScalarParameterParser.java
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ExplodedObjectParameterParser.java
- core/src/test/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParserTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/QueryParameterValidator.java
🧬 Code graph analysis (9)
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java (1)
Utils(43-307)
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-126)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-132)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-126)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(47-266)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java (2)
core/src/main/java/com/predic8/membrane/core/util/JsonUtil.java (1)
JsonUtil(22-78)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-126)
core/src/main/java/com/predic8/membrane/core/openapi/validators/SecurityValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (5)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-126)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(26-80)
core/src/main/java/com/predic8/membrane/core/openapi/validators/SchemaValidator.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/query-params.yml
[medium] 39-43: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml
[high] 1-89: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-89: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 20-24: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 YAMLlint (1.37.1)
core/src/test/resources/openapi/specs/query-params.yml
[warning] 41-41: too few spaces after comma
(commas)
[warning] 41-41: too few spaces after comma
(commas)
⏰ 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 (14)
docs/ROADMAP.md (1)
23-25: Clarify behavior and wiring for unknownQueryParameters.Document what “report” does (log-only? include in response body?). Also note where this setting is read (validator class/config) and whether it affects request blocking by default (currently defaults to accept).
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (5)
63-92: Good: raw query preserved and multi-value map.Preserving raw encoding via getRawQuery and deferring value decoding is correct for explode/form parsing.
79-86: Good: skip unknowns when exploded object with additionalProperties is present.Matches OAS semantics; prevents false positives for dynamic keys.
263-265: Good: use getRawQuery() to preserve encoding.Keeps “%2C” intact for later splitting.
254-261: Fix NPE risk on Parameter.getRequired().getRequired() is nullable; method reference auto-unboxes and can NPE.
Apply:
- return parameters.stream() - .filter(Parameter::getRequired).map(Parameter::getName).collect(toSet()); + return parameters.stream() + .filter(p -> Boolean.TRUE.equals(p.getRequired())) + .map(Parameter::getName) + .collect(toSet());
226-228: Detect query params by in="query" (not instanceof).instanceof QueryParameter can miss generic Parameter/$ref’d entries. Filter by getIn().
Apply:
- Set<Parameter> getAllQueryParameters(Operation operation) { - return getAllParameter(operation).stream().filter(p -> p instanceof QueryParameter).collect(toSet()); - } + Set<Parameter> getAllQueryParameters(Operation operation) { + return getAllParameter(operation).stream() + .filter(Objects::nonNull) + .filter(p -> "query".equalsIgnoreCase(p.getIn())) + .collect(toCollection(LinkedHashSet::new)); + }core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
118-125: Correct explode default for form; false otherwise.Implementation aligns with spec defaults. Nice.
core/src/test/resources/openapi/specs/query-params.yml (1)
88-106: Spec additions look good for test coverage.Object params (explode true/false) and additionalProperties=true cases are well represented.
core/src/test/resources/openapi/specs/oas31/parameters/simple.yaml (1)
1-88: Spec looks good to exercise explode behavior and required params.Covers arrays, booleans, allowReserved, and required handling.
core/src/main/java/com/predic8/membrane/core/openapi/validators/OperationValidator.java (1)
52-52: Switched to unified QueryParameterValidator.validate — looks goodConsistent with the new validator API; no concerns.
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ObjectParameterParser.java (2)
62-66: Good: resolve schema once outside the loopPre-resolving avoids redundant lookups and fixes the prior early-return issue. Also switched to obj.set(...). LGTM.
66-75: Decode field names before property lookupProperty names can be percent-encoded. Decode the field name prior to getProperty() and insertion to avoid misclassification into additionalProperties.
Apply:
- String fieldName = Optional.ofNullable(tokens.pollFirst()).orElse("").trim(); + String fieldName = decode(Optional.ofNullable(tokens.pollFirst()).orElse(""), UTF_8).trim(); if (fieldName.isEmpty()) continue; - var json = scalarAsJson(decode(requireNonNullElse(tokens.pollFirst(), NULL), UTF_8)); + var json = scalarAsJson(decode(requireNonNullElse(tokens.pollFirst(), "null"), UTF_8));core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
74-79: Confirmed: core module compiled with Java 21 — no change requiredRoot POM sets <javac.source>/<javac.target> = 21 and core/pom.xml uses ${javac.source}, so List.getFirst() is supported.
core/src/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java (1)
79-86: No action required — build is configured for UTF‑8. root pom.xml declares <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> and <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>; core/pom.xml also sets <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>.
...rc/test/java/com/predic8/membrane/core/openapi/oas31/parameters/ArrayQueryParameterTest.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIRecordFactoryTest.java
Show resolved
Hide resolved
.../src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (17)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
51-53: Use the existing (method, path) constructor and diamond for consistency.This keeps it consistent with other factory methods and avoids an extra call.
- public static <T extends Body> Request<T> get(String path) { - return new Request<T>("GET").path(path); - } + public static <T extends Body> Request<T> get(String path) { + return new Request<>("GET", path); + }core/src/test/resources/openapi/specs/query-params.yml (5)
42-49: Consider bounding array length and validating item count.Add maxItems to constrain input size; helps validators and fuzz resistance. Keep maxLength: 3 for items as-is.
schema: type: array + maxItems: 100 # adjust as needed items: type: string maxLength: 3If maxItems is intentionally unbounded for test coverage, ignore. Confirm.
55-66: Be explicit about object serialization to avoid query key collisions.With query defaults (style: form, explode: true), car serializes as color=...&brand=...&power=..., losing the car prefix. If you intend car[color]=..., use deepObject.
- in: query name: car + style: deepObject + explode: true schema: type: object properties: color: type: string brand: type: string power: type: integerIf the test intentionally targets form+explode behavior without deepObject, keep as-is. Confirm intent.
67-78: Make style explicit for object with explode=false (clarity, fewer ambiguities).Default is form for query, but being explicit improves readability.
- in: query name: pet + style: form explode: false schema: type: object properties: age: type: number kind: type: string
79-85: Header object: document serialization explicitly.Default is style: simple, explode: false. Being explicit helps implementers and tests.
- in: header name: plant + style: simple + explode: false schema: type: object properties: size: type: numberAlso verify your validator supports object serialization in headers (some stacks don’t).
91-102: Add an example to exercise additionalProperties parsing.Helps tests/assertions for form+explode=true objects with extra keys.
- name: foo in: query style: form explode: true + example: + a: 1 + b: "hi" + extraKey: "value" schema: type: object additionalProperties: true properties: a: type: number b: type: stringdistribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/APIKeyWithOpenAPIExampleTest.java (1)
70-77: Also assert the response Content-Type for problem details.Helps guard against regressions (e.g., text/html on error). Prefer a lenient match to support application/problem+json.
Apply this diff:
.statusCode(403) + .contentType(containsString("json")) .body("title", equalTo("OpenAPI message validation failed")) .body("type", equalTo("https://membrane-api.io/problems/user/validation")) .body("status", equalTo(403))distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.java (3)
17-31: Consolidate Hamcrest static imports; drop CoreMatchers.hasItem and duplicates.Avoid mixed CoreMatchers/Matchers to prevent ambiguity and reduce noise.
Apply this diff:
-import static org.hamcrest.CoreMatchers.hasItem; -import static org.hamcrest.Matchers.allOf; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.*; +import static org.hamcrest.Matchers.*;
41-41: Unnecessary throws on test method.JUnit 5 doesn’t require throwing checked exceptions here. Consider removing for clarity.
Apply this diff:
- void validRequest() throws Exception { + void validRequest() {
316-329: Targeted query-parameter validation is solid.Good use of GPath + hasEntry to assert the limit overflow error.
Optionally also assert the response is JSON/problem+json:
- res.then().assertThat() - .statusCode(400) + res.then().assertThat() + .statusCode(400) + .contentType(containsString("json")) .body("validation.errors['REQUEST/QUERY_PARAMETER/limit']", hasItem(allOf( hasEntry(equalTo("schemaType"), equalTo("integer")), hasEntry(equalTo("message"), containsString("maximum of 100")) )));core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (3)
227-229: Detect query params by in="query" (covers $ref and generic Parameter). Preserve order.Using instanceof QueryParameter can miss valid entries.
Apply:
- Set<Parameter> getAllQueryParameters(Operation operation) { - return getAllParameter(operation).stream().filter(p -> p instanceof QueryParameter).collect(toSet()); - } + Set<Parameter> getAllQueryParameters(Operation operation) { + return getAllParameter(operation).stream() + .filter(Objects::nonNull) + .filter(p -> "query".equalsIgnoreCase(p.getIn())) + .collect(toCollection(LinkedHashSet::new)); + }Note: If you adopt this, update tests that assert instanceof QueryParameter to assert in="query".
199-215: Be resilient to invalid percent-encoding in keys (don’t blow up whole request).URLDecoder.decode throws IllegalArgumentException on malformed input.
Apply:
- var key = decode(m.group(1), UTF_8); // Key can here be decoded + var rawKey = m.group(1); + String key; + try { + key = decode(rawKey, UTF_8); // decode key + } catch (IllegalArgumentException iae) { + key = rawKey; // fallback to raw if invalid encoding + }
111-123: Add null-safety for parameters during scan.Defensive guard avoids accidental NPE if a null sneaks in.
Apply:
- for (Parameter p : getAllQueryParameters(operation)) { + for (Parameter p : getAllQueryParameters(operation)) { + if (p == null) continue;core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
123-128: Avoid order-dependence in security-scheme query param keys.Map iteration order is not guaranteed across implementations.
Apply:
- assertEquals(List.of("api-key", "x-api-key"), citiesValidator.securitySchemeApiKeyQueryParamNames(spec)); + assertEquals(List.of("api-key", "x-api-key"), + citiesValidator.securitySchemeApiKeyQueryParamNames(spec).stream().sorted().toList());
247-252: Don’t assert on implementation class; assert on in="query".Makes tests robust if the implementation stops using QueryParameter subclasses.
Apply:
- assertTrue(qp.stream().allMatch(p -> p instanceof QueryParameter)); + assertTrue(qp.stream().allMatch(p -> "query".equalsIgnoreCase(p.getIn())));
231-238: Fix typo in test name.Minor readability polish.
Apply:
- void ignoreAddtionalFromObject() { + void ignoreAdditionalFromObject() {
66-69: Prefer isEmpty() over size() == 0.Clearer intent.
Apply:
- assertEquals(0, objectValidator.validate(ctx, get("/object?"), getGET("/object")).size()); + assertTrue(objectValidator.validate(ctx, get("/object?"), getGET("/object")).isEmpty());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(2 hunks)core/src/test/resources/openapi/specs/query-params.yml(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.java(6 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/APIKeyWithOpenAPIExampleTest.java(1 hunks)docs/ROADMAP.md(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/ROADMAP.md
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/model/Request.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
🧬 Code graph analysis (3)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (5)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-132)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(26-80)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.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/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-136)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-132)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(48-261)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/query-params.yml
[medium] 39-43: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (java)
- GitHub Check: Automated tests
🔇 Additional comments (7)
core/src/test/resources/openapi/specs/query-params.yml (2)
50-52: Responses block for /cities looks good.
86-87: Responses block for /object looks good.distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/APIKeyWithOpenAPIExampleTest.java (1)
70-77: Good move to structured RFC7807-style assertions.The assertions validate the problem-details envelope and the scoped error precisely.
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/openapi/OpenAPIValidationExampleTest.java (2)
162-162: Message wording update looks consistent.Matches the new phrasing “does not match regex pattern ...”.
197-197: Message normalization LGTM.“Object has the additional property” reads cleaner and aligns with typical validator phrasing.
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (2)
258-260: Good: preserve raw query for downstream parsing.Returning getRawQuery() keeps encoding intact for array/object parsing.
254-256: Fix NPE risk on Parameter.getRequired().getRequired() is nullable; autounboxing can throw NPE.
Apply:
- return parameters.stream() - .filter(Parameter::getRequired).map(Parameter::getName).collect(toSet()); + return parameters.stream() + .filter(Objects::nonNull) + .filter(p -> Boolean.TRUE.equals(p.getRequired())) + .map(Parameter::getName) + .collect(toSet());
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java
Show resolved
Hide resolved
.../src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParsingException.java (2)
17-20: Add serialVersionUID to the Exception subclass.
Prevents serialization warnings and future‑proofs the type.public class ParameterParsingException extends Exception { + private static final long serialVersionUID = 1L; public ParameterParsingException(String message) { super(message); } public ParameterParsingException(String message, Throwable cause) { super(message, cause); } }
17-20: Consider adding standard convenience constructors (optional).
Improves ergonomics and allows constructing without message or with suppression control (useful for performance-sensitive parse failures).public class ParameterParsingException extends Exception { private static final long serialVersionUID = 1L; + public ParameterParsingException() { super(); } public ParameterParsingException(String message) { super(message); } + public ParameterParsingException(Throwable cause) { super(cause); } public ParameterParsingException(String message, Throwable cause) { super(message, cause); } + public ParameterParsingException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) { + super(message, cause, enableSuppression, writableStackTrace); + } }core/src/test/resources/openapi/specs/query-params.yml (2)
39-41: Optional: add maxItems for arrays (satisfy linter CKV_OPENAPI_21).Not required for functionality; silences static analysis noise.
schema: type: array + maxItems: 10 items: type: string enum: ["Bonn", "New York", "Delhi"]
81-88: Be explicit about header serialization.Header defaults are style: simple, explode: false. Declare them explicitly to reduce ambiguity.
- in: header name: plant + style: simple + explode: false schema: type: object properties: size: type: numbercore/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (1)
167-171: Verify serialization form for object with style=form, explode=false.Spec-conform encoding is key,value pairs (e.g., pet=kind,mammal,age,9). You’re testing pet=...age=9. If intentional (lenient parser), consider adding a test for the strict form too.
Would you like me to add an extra test asserting pet=kind,mammal,age,9 is valid as well?
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
199-215: Harden key decoding against bad percent-encoding.Prevent IllegalArgumentException; fall back to raw key. Values remain raw by design for later parsing.
- if (m.matches()) { - var key = decode(m.group(1), UTF_8); // Key can here be decoded + if (m.matches()) { + String rawKey = m.group(1); + String key; + try { + key = decode(rawKey, UTF_8); // Key can here be decoded + } catch (IllegalArgumentException iae) { + key = rawKey; // fallback on invalid encoding + } if (key.isEmpty()) continue; // ignore stray separators - var value = m.group(2); // Do not decode here cause it has to be done after array or object splitting + var value = m.group(2); // keep raw; decode later after array/object splitting var ab = parameterMap.computeIfAbsent(key, k -> new ArrayList<>()); ab.add(value); }
📜 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/model/Request.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParsingException.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(2 hunks)core/src/test/resources/openapi/specs/query-params.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java
- core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/QueryParameterValidator.java
🧬 Code graph analysis (2)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-136)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-136)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(48-261)
core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (5)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-136)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationContext.java (1)
ValidationContext(23-229)core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java (1)
ValidationErrors(25-130)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/AbstractParameterParser.java (1)
AbstractParameterParser(26-80)
🪛 Checkov (3.2.334)
core/src/test/resources/openapi/specs/query-params.yml
[medium] 39-43: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (7)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParsingException.java (2)
1-14: License header looks good.
Matches standard Apache 2.0 header; no issues.
17-20: Confirm checked vs unchecked exception.
OpenAPIParsingException is unchecked (core/src/main/java/com/predic8/membrane/core/openapi/OpenAPIParsingException.java) while ParameterParsingException is checked (core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParsingException.java). If parsing errors are expected to be handled at call sites keep it checked; otherwise make ParameterParsingException extend RuntimeException to avoid propagating throws clauses.core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (2)
68-71: LGTM: empty query handled correctly.Covers the empty-query edge case for a path with optional params.
132-139: LGTM: unknown param message/assertions.Asserts both “invalid” and “unknown”, aligned with validator output.
core/src/test/resources/openapi/specs/query-params.yml (1)
39-41: Add missing item type for enum in array items.Explicitly declare the item type to avoid validator incompatibilities.
schema: type: array - items: - enum: ["Bonn", "New York", "Delhi"] + items: + type: string + enum: ["Bonn", "New York", "Delhi"]core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (2)
254-256: Fix potential NPE: Parameter.getRequired() may be null.Null autounboxing can crash when ‘required’ is unspecified.
- return parameters.stream() - .filter(Parameter::getRequired).map(Parameter::getName).collect(toSet()); + return parameters.stream() + .filter(p -> Boolean.TRUE.equals(p.getRequired())) + .map(Parameter::getName) + .collect(toSet());
227-229: Detect query params viain="query"(notinstanceof).Covers $ref and generic Parameter; preserves order and avoids missing params.
- Set<Parameter> getAllQueryParameters(Operation operation) { - return getAllParameter(operation).stream().filter(p -> p instanceof QueryParameter).collect(toCollection(LinkedHashSet::new)); - } + Set<Parameter> getAllQueryParameters(Operation operation) { + return getAllParameter(operation).stream() + .filter(Objects::nonNull) + .filter(OpenAPIUtil::isQueryParameter) + .collect(toCollection(LinkedHashSet::new)); + }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParsingException.java (4)
17-18: Avoid wildcard import; use explicitjava.io.SerialKeeps the public API surface cleaner and avoids accidental type shadowing.
Apply this diff:
-import java.io.*; +import java.io.Serial;
24-26: Add a cause-only constructor for convenienceCommon when rethrowing underlying parsing errors.
Apply this diff:
public ParameterParsingException(String message) { super(message); } public ParameterParsingException(String message, Throwable cause) { super(message, cause); } + public ParameterParsingException(Throwable cause) { super(cause); } }
19-26: Add minimal Javadoc since this is a public API typeImproves discoverability and IDE help.
Apply this diff:
+/** + * Thrown when parsing an OpenAPI parameter value (style/explode/form; arrays/objects) fails. + * Intended for use by parameter parser implementations and validators. + */ public class ParameterParsingException extends Exception { @@ - public ParameterParsingException(String message) { super(message); } - public ParameterParsingException(String message, Throwable cause) { super(message, cause); } + /** Creates an exception with a message describing the parsing failure. */ + public ParameterParsingException(String message) { super(message); } + /** Creates an exception with a message and the underlying cause. */ + public ParameterParsingException(String message, Throwable cause) { super(message, cause); }
19-19: Extend ParameterParsingException from RuntimeException
Align with peer OpenAPI exceptions (e.g. OpenAPIParsingException) to avoid pervasive checked throws.File: core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParsingException.java
-public class ParameterParsingException extends Exception { +public class ParameterParsingException extends RuntimeException {core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
51-53: Add symmetric builder overloads for other HTTP verbs (optional).The GET overload is useful. For API consistency, consider adding
post(String),put(String),delete(String),patch(String), andtrace(String)overloads as well. Also consider a brief Javadoc note thatpathmay include a raw query string and is not decoded.core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
73-80: Remove unused parameter from helper (or generalize).
getParameterSchemas(QueryParameterValidator val)ignores itsvalarg and always uses/cities. Either remove the parameter and field-capturecitiesValidator, or pass anOperationso it works for any path.Minimal clean-up option:
- private List<Parameter> getParameterSchemas(QueryParameterValidator val) { - return val.getAllParameter(getGET("/cities")); - } + private List<Parameter> getParameterSchemas() { + return citiesValidator.getAllParameter(getGET("/cities")); + }Call site update (outside the selected range):
var parameterSchemas = getParameterSchemas();Also applies to: 82-84
132-139: Make message assertions resilient to wording/case changes.Use case-insensitive checks to avoid brittle failures if capitalization or phrasing changes slightly.
- assertTrue(err.get(0).getMessage().contains("invalid")); - assertTrue(err.get(0).getMessage().contains("unknown")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("invalid")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("unknown"));
247-254: Avoid passing null API into QueryParameterValidator in util tests.These tests will break if
QueryParameterValidatorstarts requiringapi(several methods already do). Use the actualOpenAPIinstance you read for the PathItem to keep tests robust.void get_QueryParameters() { - var pathItem = getPathItem("/array"); - var qpv = new QueryParameterValidator(null, pathItem); + var api = getApi(this, "/openapi/specs/oas31/parameters/simple.yaml"); + var pathItem = getPath(api, "/array"); + var qpv = new QueryParameterValidator(api, pathItem); var qp = qpv.getAllQueryParameters(pathItem.getGet()); assertEquals(4, qp.size()); assertTrue(qp.stream().allMatch(OpenAPIUtil::isQueryParameter)); }void get_QueryParameter_WithName() { - var pathItem = getPathItem("/array"); - QueryParameterValidator qpv = new QueryParameterValidator(null, pathItem); + var api = getApi(this, "/openapi/specs/oas31/parameters/simple.yaml"); + var pathItem = getPath(api, "/array"); + QueryParameterValidator qpv = new QueryParameterValidator(api, pathItem); assertEquals("String param", qpv.getQueryParameter(pathItem.getGet(), STRING).getDescription()); }void get_QueryParameter_Absent_ReturnsNull() { - var pathItem = getPathItem("/array"); - var qpv = new QueryParameterValidator(null, pathItem); + var api = getApi(this, "/openapi/specs/oas31/parameters/simple.yaml"); + var pathItem = getPath(api, "/array"); + var qpv = new QueryParameterValidator(api, pathItem); assertNull(qpv.getQueryParameter(pathItem.getGet(), "absent")); }void get_Required_QueryParameters() { - var pathItem = getPathItem("/required"); - var qpv = new QueryParameterValidator(null, pathItem); + var api = getApi(this, "/openapi/specs/oas31/parameters/simple.yaml"); + var pathItem = getPath(api, "/required"); + var qpv = new QueryParameterValidator(api, pathItem); var rq = qpv.getRequiredQueryParameters(pathItem.getGet()); assertEquals(2, rq.size()); }Also applies to: 258-262, 265-269, 272-277
279-284: Prefer using public helpers over accessing validator internals.Avoid referencing
citiesValidator.apidirectly. UsegetPathItem("/object")to keep the test decoupled from internal fields.- var propertyNames = citiesValidator.getPossibleObjectPropertiesNamesForOperation( getPath( citiesValidator.api, "/object").getGet()); + var propertyNames = citiesValidator.getPossibleObjectPropertiesNamesForOperation(getPathItem("/object").getGet());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParsingException.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.745Z
Learnt from: predic8
PR: membrane/api-gateway#2156
File: core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java:129-131
Timestamp: 2025-09-19T11:12:26.745Z
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/model/Request.java
🧬 Code graph analysis (1)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-136)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-136)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(48-261)
⏰ 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 (5)
core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParsingException.java (2)
19-26: Good: Dedicated exception type for parameter parsingIntroducing a focused exception improves error signaling and keeps parsing concerns separate. Constructors cover the common cases.
21-23: No change required — project baseline is Java 21, so@Serialis supported.core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (3)
56-66: Good: per-path validators setup and prior bug fixed.Constructing validators per PathItem and fixing
objectValidatorto useobjectPathItemlooks correct.
286-289: LGTM: raw query extraction happy-path.Asserts expected behavior of
getQueryString(Request).
291-297: LGTM: raw encoding preservation test.Validates the intentional use of
getRawQuery()in production.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
51-85: Keep spacing around arguments consistent.Everywhere else in this class we leave a space after commas in argument lists. Please add the space here as well to stay aligned with the local style.
- return new Request<>("GET",path); + return new Request<>("GET", path); @@ - return new Request<>("POST",path); + return new Request<>("POST", path); @@ - return new Request<>("PUT",path); + return new Request<>("PUT", path); @@ - return new Request<>("DELETE",path); + return new Request<>("DELETE", path);core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (7)
74-79: Reduce brittleness of “8 parameters” assertion.Hard-coding the exact count can make the test brittle when the spec evolves. Prefer asserting required names are present, or ≥ minimum expected.
108-116: Simplify map creation (no need for extra HashMap).The method doesn’t mutate the input; an immutable Map is fine.
Apply:
- assertFalse(citiesValidator.validateAdditionalQueryParameters( - ctx, - new HashMap<>(Map.of("bar", new TextNode("2315124"))), + assertFalse(citiesValidator.validateAdditionalQueryParameters( + ctx, + Map.of("bar", new TextNode("2315124")), new OpenAPI().components(new Components() {{ addSecuritySchemes("schemaA", new SecurityScheme().type(APIKEY).name("api-key").in(QUERY)); }}) ).isEmpty());
149-157: Make assertion more robust with JSON Pointer.Pin the error location to the offending field to reduce dependence on message text.
Apply:
var err = additionalValidator.validate(ctx, get().path("/additional?a=wrong&b=txt&additional=t"), additionalPathItem.getGet()); assertEquals(1, err.size()); assertTrue(err.get(0).getMessage().contains("is not a number")); assertTrue(err.get(0).getMessage().contains("wrong")); + assertEquals("/a", err.get(0).getContext().getJSONpointer());
193-201: Assert error location for array item.Checking the pointer makes the test less sensitive to wording changes.
Apply:
var err = citiesValidator.validate(ctx, get().path("/cities?city=Bonn&city=Bielefeld&limit=10"), citiesPathItem.getGet()); assertEquals(1, err.size()); assertTrue(err.get(0).getMessage().contains("enum")); assertTrue(err.get(0).getMessage().contains("Bielefeld")); + assertTrue(err.get(0).getContext().getJSONpointer().contains("/city"));
228-235: Avoid shadowing ctx; reuse the shared context.Keeps tests consistent and avoids accidental divergence in defaults.
Apply:
- var ctx = new ValidationContext(); - var err = addValidator.validate(ctx, get().path("/additional?a=1&b=txt&additional=t"), pi.getGet()); + var err = addValidator.validate(ctx, get().path("/additional?a=1&b=txt&additional=t"), pi.getGet());
244-249: Don’t pass null api to QueryParameterValidator in util tests.Future changes may dereference api; pass the real API to avoid latent NPEs.
Apply:
- var qpv = new QueryParameterValidator(null, pathItem); + var qpv = new QueryParameterValidator(validator.getApi(), pathItem)- QueryParameterValidator qpv = new QueryParameterValidator(null, pathItem); + QueryParameterValidator qpv = new QueryParameterValidator(validator.getApi(), pathItem);- var qpv = new QueryParameterValidator(null, pathItem); + var qpv = new QueryParameterValidator(validator.getApi(), pathItem);- var qpv = new QueryParameterValidator(null, pathItem); + var qpv = new QueryParameterValidator(validator.getApi(), pathItem);Also applies to: 255-257, 261-264, 269-272
274-279: Avoid accessing internal api field; use validator.getApi().Improves encapsulation and reduces tight coupling to protected/package-private members.
Apply:
- var propertyNames = citiesValidator.getPossibleObjectPropertiesNamesForOperation( getPath( citiesValidator.api, "/object").getGet()); + var propertyNames = citiesValidator.getPossibleObjectPropertiesNamesForOperation( + getPath(validator.getApi(), "/object").getGet());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java(1 hunks)core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParsingException.java(1 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/parameters/ParameterParsingException.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T11:12:26.787Z
Learnt from: predic8
PR: membrane/api-gateway#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/model/Request.java
🧬 Code graph analysis (1)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-156)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-136)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(48-261)
⏰ 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/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
61-63: Correct PathItem used for objectValidator (prior bug fixed).The validator is now constructed with objectPathItem as intended.
68-70: Good empty-query coverage.Validates no errors for a trailing '?' as expected.
127-134: Expectation updated to include “unknown” and “invalid”.Matches the current validator message.
282-293: Good verification of raw query preservation.These checks ensure percent-encoding is preserved pre-parse.
Consider adding two quick cases to tighten coverage:
- No-query path returns null.
- Stray separators and key-only flags are handled.
Proposed additions:
@Test void get_QueryString_NoQuery_ReturnsNull() { assertNull(getQueryString(get().path("/foo"))); } @Test void get_QueryString_StrayAndFlag() { // Leading/trailing separators and key-only flag should be preserved by getQueryString assertEquals("flag&=v1&&k=&k2=v2&", getQueryString(get().path("/foo?flag&=v1&&k=&k2=v2&"))); }
.../src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (6)
74-79: Rename misleading local var (not schemas).The value is a list of Parameters, not Schemas. Rename to avoid confusion.
- var parameterSchemas = citiesValidator.getAllParameter(getGET("/cities")); - assertEquals(8, parameterSchemas.size()); + var parameters = citiesValidator.getAllParameter(getGET("/cities")); + assertEquals(8, parameters.size()); // All Parameters must have a name. Referenced params do not have a name. - assertFalse(parameterSchemas.stream().anyMatch(param -> param.getName() == null)); + assertFalse(parameters.stream().anyMatch(param -> param.getName() == null));
219-223: Make assertion robust and explicit for maxLength.“axLength” is brittle. Prefer a case-insensitive check for “maxlength”.
- assertTrue(err.get(0).getMessage().contains("axLength")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("maxlength")); assertTrue(err.get(0).getMessage().contains("Jack")); assertTrue(err.get(0).getMessage().contains("4"));
231-237: Reuse prebuilt additionalValidator instead of re-instantiating.Reduces duplication and speeds tests slightly.
- var pi = validator.getApi().getPaths().get("/additional"); - var addValidator = new QueryParameterValidator(validator.getApi(), pi); - var err = addValidator.validate(ctx, get().path("/additional?a=1&b=txt&additional=t"), pi.getGet()); + var err = additionalValidator.validate(ctx, get().path("/additional?a=1&b=txt&additional=t"), additionalPathItem.getGet());
245-250: Avoid passing null API to QueryParameterValidator.Several util tests construct QueryParameterValidator with a null api. This couples tests to the current internal behavior and may break if methods start requiring api (e.g., for $ref or component resolution). Pass the actual OpenAPI instance to keep tests future-proof.
Apply this pattern (example for get_QueryParameters):
- var pathItem = getPathItem("/array"); - var qpv = new QueryParameterValidator(null, pathItem); - var qp = qpv.getAllQueryParameters(pathItem.getGet()); + var api = getApi(this, "/openapi/specs/oas31/parameters/simple.yaml"); + var pathItem = getPath(api, "/array"); + var qpv = new QueryParameterValidator(api, pathItem); + var qp = qpv.getAllQueryParameters(pathItem.getGet());Do the same in:
- get_QueryParameter_WithName (lines 255-258)
- get_QueryParameter_Absent_ReturnsNull (lines 261-265)
- get_Required_QueryParameters (lines 269-273)
Also applies to: 255-258, 261-265, 269-273
276-280: Don’t reach into internals; use objectPathItem already available.Avoid using citiesValidator.api from tests. Use the prepared PathItem.
- var propertyNames = citiesValidator.getPossibleObjectPropertiesNamesForOperation( getPath( citiesValidator.api, "/object").getGet()); + var propertyNames = citiesValidator.getPossibleObjectPropertiesNamesForOperation(objectPathItem.getGet());
196-202: Remove all debug System.out.println calls from tests.
Eliminate everySystem.out.println(...)incore/src/test(e.g., QueryParameterValidatorTest.java:198) to prevent noisy output.- System.out.println("err = " + err);core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (2)
94-97: Remove unnecessary cast.Schema#getProperties() already returns Map<String, Schema>. The cast is redundant.
Apply:
- public static Schema<?> getProperty(Schema<?> schema, String propertyName) { - if (schema == null || schema.getProperties() == null) return null; - return (Schema<?>) schema.getProperties().get(propertyName); - } + public static Schema<?> getProperty(Schema<?> schema, String propertyName) { + if (schema == null || schema.getProperties() == null) return null; + return schema.getProperties().get(propertyName); + }
32-34: Avoid cross-layer coupling: drop static import from JsonSchemaValidator.OpenAPIUtil should not depend on validator internals. Prefer a local constant ("object") or Parameter.Type where available.
If you keep the method change above, you can replace OBJECT with "object" and remove this static import. No behavior change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java(3 hunks)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java(2 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java(3 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-157)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(36-135)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(48-261)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java (1)
Utils(43-307)
⏰ 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 (6)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (3)
55-63: Good fix: validators now bound to correct PathItems.Constructors now use the intended PathItem (earlier bug with objectValidator using additionalPathItem is corrected).
97-116: API-key additional query parameter handling looks correct.Valid/invalid cases reflect security scheme collection and filtering as expected.
127-134: Expectation for “unknown” token in message is aligned.The test now checks for both “invalid” and “unknown” as previously discussed.
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
132-134: LGTM: simple and clear query check.core/src/test/java/com/predic8/membrane/core/interceptor/schemavalidation/SOAPMessageValidatorInterceptorTest.java (2)
46-46: JUnit 5 visibility changes look good.Package-private @test methods are valid in JUnit Jupiter. Consistent modernization.
Also applies to: 77-77, 92-92, 100-100, 115-115, 130-130, 144-144
84-84: No changes required—project uses Java 21 toolchain, ‘var’ is supported.
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java
Show resolved
Hide resolved
…into openapi-validator-explode-2155
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (3)
96-99: Unnecessary cast in getProperty()Map value type is Schema; the explicit cast isn’t needed.
- return (Schema<?>) schema.getProperties().get(propertyName); + return schema.getProperties().get(propertyName);
125-128: Clarify boolean precedence in hasObjectType()Current expression is correct but a bit dense. Split for readability.
- return (schema.getTypes() != null && (schema.getTypes().contains(OBJECT)) || OBJECT.equals(schema.getType())); + if (schema.getTypes() != null) return schema.getTypes().contains(OBJECT); + return OBJECT.equals(schema.getType());
130-141: Add null‑safety to isExplode()Make defensive against null parameter to avoid surprises when used elsewhere.
- public static boolean isExplode(Parameter parameter) { + public static boolean isExplode(Parameter parameter) { + if (parameter == null) return false; if (parameter.getExplode() == null) { Parameter.StyleEnum style = getStyle(parameter); return style == FORM || style == DEEPOBJECT; } return parameter.getExplode(); }core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (1)
214-222: Stabilize substring for maxLength assertion“axLength” works but is brittle. Prefer case-insensitive “maxLength”.
- assertTrue(err.get(0).getMessage().contains("axLength")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("maxlength"));
📜 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/util/OpenAPIUtil.java(2 hunks)core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
core/src/main/java/com/predic8/membrane/core/openapi/model/Request.java (1)
Request(27-157)core/src/test/java/com/predic8/membrane/core/openapi/util/OpenAPITestUtils.java (1)
OpenAPITestUtils(34-98)core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
OpenAPIUtil(37-146)core/src/main/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidator.java (1)
QueryParameterValidator(48-261)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (1)
core/src/main/java/com/predic8/membrane/core/openapi/util/Utils.java (1)
Utils(43-307)
⏰ 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 (7)
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java (3)
82-85: Null‑safe path lookup looks good
87-94: Parameter lookup by name: concise and null‑safe
101-120: Schema resolution flow is robust (null/content/$ref)Consider adding a test where parameter.content has multiple media types to ensure JSON is preferred after fixing the key (e.g., both application/xml and application/json present).
core/src/test/java/com/predic8/membrane/core/openapi/validators/QueryParameterValidatorTest.java (4)
238-245: Nice: exercising getAllQueryParameters and isQueryParameter together
248-253: Good: querying by name and asserting description
270-275: Correct: property names collected from object schema
127-134: Fix case-sensitive assertion for “unknown”Production message starts with “Unknown…”, so this assertion fails.
- assertTrue(err.get(0).getMessage().contains("unknown")); + assertTrue(err.get(0).getMessage().toLowerCase().contains("unknown"));
core/src/main/java/com/predic8/membrane/core/openapi/util/OpenAPIUtil.java
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Dependency
Tests
Documentation