Skip to content

add JsonRPCProtection plugin#2963

Open
christiangoerdes wants to merge 35 commits into
masterfrom
json-rpc-protection
Open

add JsonRPCProtection plugin#2963
christiangoerdes wants to merge 35 commits into
masterfrom
json-rpc-protection

Conversation

@christiangoerdes
Copy link
Copy Markdown
Collaborator

@christiangoerdes christiangoerdes commented May 28, 2026

Summary by CodeRabbit

  • New Features

    • JSON-RPC protection interceptor with request validation, explicit allow/deny rules, and pattern-based method matching
    • Batch handling configurable (enable/disable, max size, empty/oversize rejection)
    • Method-specific params validation via regex→schema mappings (YAML/XML support, first-match wins)
    • Improved error responses and a now-public JSON-RPC request helper
  • Tests

    • Expanded tests covering rule precedence, rejections, batch behavior/limits, param validation, and invalid configs

Review Change Stack

@christiangoerdes
Copy link
Copy Markdown
Collaborator Author

/ok-to-test

@christiangoerdes christiangoerdes marked this pull request as draft May 28, 2026 12:30
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 28, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a JsonRPCProtectionInterceptor with regex-based Allow/Deny rules, batch controls, per-method JSON Schema param validation, a validator returning structured ValidationError records, public JSONRPCRequest factory exposure, tests and test schemas, plus a YAML introspector change to hide excluded child elements.

Changes

JSON-RPC Request Protection

Layer / File(s) Summary
Permission rule contract and implementations
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Rule.java, core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Allow.java, core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Deny.java
Adds abstract Rule with regex method matching, setMethod validation, matches(...), and getMethod(); adds Allow (permits=true) and Deny (permits=false) subclasses.
Batch configuration and constraints
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/BatchRule.java
Adds BatchRule with enabled and validated maxSize properties (rejects null/<=0).
JsonRPCParams schema mapping and loading
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.java
Adds mapping from method-pattern → schema path, compiles patterns, resolves and loads JSON/YAML schemas with a Membrane loader, initializes validators, and exposes getSchema(method).
JsonRPCValidator parsing and logic
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java
Parses JSON into single or batch requests, enforces batch enable/size rules, parses JSONRPCRequest objects, applies Rule list to each request, validates params against per-method Schema, and returns a ValidationError record for the first validation failure.
JsonRPCProtectionInterceptor implementation and validation
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java
Interceptor handles POST bodies, delegates to JsonRPCValidator, logs and writes JSON-RPC error responses (single or batch), and exposes batch, rules, and params configuration; caches validator and initializes params.
Public API change
core/src/main/java/com/predic8/membrane/core/jsonrpc/JSONRPCRequest.java
JSONRPCRequest.fromNode(JsonNode) visibility changed to public static to allow reuse by validator/parsers.
Tests and JSON Schema fixtures
core/src/test/java/.../JsonRPCProtectionInterceptorTest.java, core/src/test/resources/json/rpc/*
Adds JUnit tests for rule precedence, deny behavior, batch enable/limit, invalid regex, params-schema validation (including regex method patterns), plus two schema fixtures used by tests.

YAML Introspector

Layer / File(s) Summary
McYamlIntrospector JSON child visibility
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java
Child-element detection now filters @MCChildElement by excludeFromJson via isJsonVisibleChild(...), updating isStructured, getChildSetters, matchesJsonChildElementKey, hasChildren, and getChildSetter.

Sequence Diagram

sequenceDiagram
  participant Client as Client (HTTP POST)
  participant Interceptor as JsonRPCProtectionInterceptor
  participant Validator as JsonRPCValidator
  participant RuleEngine as Rule matcher

  Client->>Interceptor: POST body with JSON-RPC payload
  Interceptor->>Validator: validate(body)
  Validator->>Validator: parse payload (single or batch)
  Validator->>RuleEngine: for each request -> matches(method) & permits()
  alt Validation error or unauthorized
    Validator->>Interceptor: ValidationError
    Interceptor->>Client: HTTP 200 with JSON-RPC error (single or array)
  else All requests valid
    Interceptor->>Client: CONTINUE (forward request)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

7.x

Suggested reviewers

  • rrayst
  • predic8

Poem

🐰 I sniff the JSON, hop through each rule,
Allowing safe calls while Deny keeps the mule,
Batches I count, schemas I test,
Errors I tidy so clients rest,
I guard the gateway — nibble, then drool.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.05% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a new JsonRPCProtection plugin to the codebase, which is substantiated by the addition of multiple new JSON-RPC interception classes and validation components.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch json-rpc-protection

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@membrane-ci-server
Copy link
Copy Markdown

This pull request needs "/ok-to-test" from an authorized committer.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java`:
- Around line 103-106: The setter JsonRPCProtectionInterceptor.setParams should
guard against a null argument to avoid config-time NPEs: update
setParams(Map<String,String> params) to check if the incoming params is null
(and simply return) before calling this.params.putAll(...); also ensure the
instance field this.params is initialized (e.g., non-null default) so putAll is
safe when a non-null map is provided.
- Around line 80-88: The response currently includes the RuntimeException
message (e.getMessage()) in JsonRPCProtectionInterceptor when building the
ValidationError for createErrorResponse; remove the exception message from the
client-facing error and replace it with a generic string (e.g. "Invalid JSON-RPC
payload") while keeping the original exception logged server-side
(log.debug/log.warn) for diagnostics; update the code in
JsonRPCProtectionInterceptor (the catch(RuntimeException e) block that calls
createErrorResponse(new ValidationError(...))) to stop interpolating
e.getMessage() into the ValidationError and instead pass the generic message.

In `@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Rule.java`:
- Around line 15-16: The matches method in Rule (public boolean matches(String
method)) can NPE when method is null; update it to return false if method is
null before using methodPattern.matcher(method). Concretely, in Rule.matches add
a null-check for the method parameter (e.g., if (method == null) return false)
and then keep the existing methodPattern != null &&
methodPattern.matcher(method).matches() logic so null method names are treated
as non-matches.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f2a0e788-43d7-4e66-9f95-bedf3c9bcba0

📥 Commits

Reviewing files that changed from the base of the PR and between 537418e and 69d37c7.

📒 Files selected for processing (6)
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Allow.java
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/BatchRule.java
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Deny.java
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Rule.java
  • core/src/test/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptorTest.java

Comment thread core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Rule.java Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java (1)

46-58: 💤 Low value

Consider caching the JsonRPCValidator instance.

Line 57 creates a new JsonRPCValidator on every request. Since batchRule and rules are configuration-time immutable, the validator could be instantiated once (lazily after init() or after setters are called) and reused, avoiding per-request allocation overhead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java`
around lines 46 - 58, The handleRequest method is creating a new
JsonRPCValidator for every request; cache a single JsonRPCValidator instance in
JsonRPCProtectionInterceptor (e.g., a private volatile field like
jsonRpcValidator) and instantiate it once using the immutable batchRule and
rules either in init() or via a synchronized/lazy initializer after setters are
called; then have handleRequest call jsonRpcValidator.validate(body) instead of
new JsonRPCValidator(...). Ensure thread-safety when initializing the cached
JsonRPCValidator.
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java (1)

123-140: 💤 Low value

Default-allow semantics when no rule matches.

If no rule matches the method (including when rules is empty), validation returns null allowing the request through. This "first-match-wins with implicit allow" design requires users to add a catch-all Deny method=".*" rule to enforce deny-by-default.

Consider documenting this behavior clearly, or alternatively returning an error when a method matches no rules (deny-by-default), which is typically safer for security-critical APIs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java`
around lines 123 - 140, The current validateMethod in JsonRPCValidator returns
null (allows) when no Rule in rules matches, causing implicit allow-by-default;
change to deny-by-default by returning a ValidationError (same form as the
existing denial) when the loop completes without a match: construct and return
new ValidationError(payloadType, request, 403, ERR_METHOD_NOT_FOUND, "JSON-RPC
method '%s' is not allowed.".formatted(request.getMethod())) instead of null;
keep existing behavior where a matching rule that permits() returns null and a
matching denying rule returns the same ValidationError so only the "no match"
path is altered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java`:
- Around line 49-65: In JsonRPCValidator, the catch blocks for
JsonProcessingException and RuntimeException currently embed exception messages
into the ValidationError returned to clients; change both catch handlers to
return a generic client-facing message like "Invalid JSON-RPC payload"
(preserving payload type via getPayloadType(body) or PayloadType.SINGLE) and
move the detailed exception text into server-side logging (e.g., log the
exception in the catch before returning the ValidationError). Update the
JsonProcessingException handler (which now uses e.getOriginalMessage()) and the
RuntimeException handler (which now uses e.getMessage()) to avoid leaking
internal details by returning the generic message and logging the exception
object instead.
- Around line 142-144: JsonRPCValidator.parseRequest currently serializes the
JsonNode to String then calls JSONRPCRequest.parse, causing unnecessary
round-trip parsing; change JSONRPCRequest by exposing its private
fromNode(JsonNode) as package-private or add a public/static parse(JsonNode)
overload, then update JsonRPCValidator.parseRequest to call that new
fromNode/parse(JsonNode) method instead of serializing to String so the JsonNode
is parsed directly; ensure method names referenced are
JSONRPCRequest.fromNode(JsonNode) (now non-private) and
JsonRPCValidator.parseRequest(JsonNode).

---

Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java`:
- Around line 46-58: The handleRequest method is creating a new JsonRPCValidator
for every request; cache a single JsonRPCValidator instance in
JsonRPCProtectionInterceptor (e.g., a private volatile field like
jsonRpcValidator) and instantiate it once using the immutable batchRule and
rules either in init() or via a synchronized/lazy initializer after setters are
called; then have handleRequest call jsonRpcValidator.validate(body) instead of
new JsonRPCValidator(...). Ensure thread-safety when initializing the cached
JsonRPCValidator.

In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java`:
- Around line 123-140: The current validateMethod in JsonRPCValidator returns
null (allows) when no Rule in rules matches, causing implicit allow-by-default;
change to deny-by-default by returning a ValidationError (same form as the
existing denial) when the loop completes without a match: construct and return
new ValidationError(payloadType, request, 403, ERR_METHOD_NOT_FOUND, "JSON-RPC
method '%s' is not allowed.".formatted(request.getMethod())) instead of null;
keep existing behavior where a matching rule that permits() returns null and a
matching denying rule returns the same ValidationError so only the "no match"
path is altered.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fc6a7617-e385-4fae-9153-310a89927817

📥 Commits

Reviewing files that changed from the base of the PR and between 69d37c7 and 9b7da29.

📒 Files selected for processing (2)
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java

@christiangoerdes christiangoerdes marked this pull request as ready for review May 28, 2026 13:31
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java (1)

68-76: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Preserve batch payload type in the runtime fallback.

The method already computes the payload type before parsing, but this branch hardcodes PayloadType.SINGLE. A runtime failure during batch validation will therefore be serialized as a single JSON-RPC error object instead of a batch error array.

🐛 Minimal fix
         } catch (RuntimeException e) {
             log.debug("Invalid JSON-RPC payload.", e);
             return new ValidationError(
-                    PayloadType.SINGLE,
+                    getPayloadType(body),
                     null,
                     400,
                     ERR_INVALID_REQUEST,
                     "Invalid JSON-RPC payload"
             );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java`
around lines 68 - 76, The runtime-catch in JsonRPCValidator currently returns a
ValidationError with PayloadType.SINGLE, which ignores the precomputed payload
type and causes batch payloads to be serialized as a single error; update the
catch to use the previously computed payloadType (the variable calculated before
parsing) when constructing the ValidationError so batch requests produce a batch
error array instead of a single JSON-RPC error object.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java (1)

44-49: ⚡ Quick win

Avoid initializing param schemas twice during interceptor startup.

init() calls params.init(...), then createValidator() does the same work again. That doubles schema resolution/compilation for every configured mapping at startup.

♻️ Minimal cleanup
     `@Override`
     public void init() {
         super.init();
-        params.init(router.getResolverMap(), router.getConfiguration().getUriFactory(), getBeanBaseLocation());
         validator = createValidator();
     }

Also applies to: 111-113

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java`
around lines 44 - 49, The params schema is being initialized twice: once in
init() via params.init(router.getResolverMap(),
router.getConfiguration().getUriFactory(), getBeanBaseLocation()) and again
inside createValidator(); remove the duplicate by choosing one location to
perform params.init — e.g., keep params.init(...) in init() and update
createValidator() to assume params are already initialized (remove or guard the
call there), or alternatively move params.init(...) into createValidator() and
delete it from init(); ensure validator is still assigned by createValidator()
and that no other code path calls params.init() redundantly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.java`:
- Around line 43-52: The current `@MCOtherAttributes` Map<String,String> params in
JsonRPCParams (and its setter setParams) exposes regex keys as XML attribute
names which is invalid; fix by switching to an explicit child-element model:
replace the Map params with a List<Param> where Param is a small POJO (fields
e.g. String method, String schema) and annotate the list/Param with the XML
element annotations (e.g. `@MCElement/`@MCAttribute on Param) so config becomes
<param method="regex" schema="..."/>; update JsonRPCParams#get/set to use
List<Param> and migrate any code that referenced the Map to build a runtime map
from that list. Alternatively, if preserving the Map is required, add validation
in setParams(Map) to detect illegal XML attribute characters in keys (e.g. '^',
'*', '\\', spaces, etc.) and throw a clear IllegalArgumentException explaining
regex keys are not allowed in XML.

---

Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java`:
- Around line 68-76: The runtime-catch in JsonRPCValidator currently returns a
ValidationError with PayloadType.SINGLE, which ignores the precomputed payload
type and causes batch payloads to be serialized as a single error; update the
catch to use the previously computed payloadType (the variable calculated before
parsing) when constructing the ValidationError so batch requests produce a batch
error array instead of a single JSON-RPC error object.

---

Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java`:
- Around line 44-49: The params schema is being initialized twice: once in
init() via params.init(router.getResolverMap(),
router.getConfiguration().getUriFactory(), getBeanBaseLocation()) and again
inside createValidator(); remove the duplicate by choosing one location to
perform params.init — e.g., keep params.init(...) in init() and update
createValidator() to assume params are already initialized (remove or guard the
call there), or alternatively move params.init(...) into createValidator() and
delete it from init(); ensure validator is still assigned by createValidator()
and that no other code path calls params.init() redundantly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7a3a04b8-3f23-4f8f-8c76-446e54504db9

📥 Commits

Reviewing files that changed from the base of the PR and between 9b7da29 and 329ee81.

📒 Files selected for processing (8)
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.java
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Rule.java
  • core/src/main/java/com/predic8/membrane/core/jsonrpc/JSONRPCRequest.java
  • core/src/test/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptorTest.java
  • core/src/test/resources/json/rpc/echo-params.schema.json
  • core/src/test/resources/json/rpc/generic-rpc-params.schema.json
✅ Files skipped from review due to trivial changes (2)
  • core/src/test/resources/json/rpc/echo-params.schema.json
  • core/src/test/resources/json/rpc/generic-rpc-params.schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Rule.java

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 28, 2026

Actionable comments posted: 0

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.java (1)

107-113: 💤 Low value

Consider creating SchemaRegistry once and reusing it for all schemas.

createSchemaRegistry(resolver) is invoked inside loadSchema() for each configured method pattern. Since the registry configuration is identical across all entries, creating it once in init() and passing it to loadSchema() would reduce initialization overhead.

♻️ Proposed refactor
     public void init(Resolver resolver, URIFactory uriFactory, String beanBaseLocation) {
         List<Param> effectiveMappings = getEffectiveMappings();
         if (effectiveMappings.isEmpty()) {
             schemas = List.of();
             return;
         }
         if (resolver == null || uriFactory == null) {
             throw new ConfigurationException("Cannot initialize JSON-RPC param schemas without resolver context.");
         }

+        SchemaRegistry schemaRegistry = createSchemaRegistry(resolver);
         schemas = effectiveMappings.stream()
                 .map(entry -> new CompiledSchema(
                         entry.getMethod(),
                         compilePattern(entry.getMethod()),
-                        loadSchema(entry.getMethod(), entry.getSchema(), resolver, uriFactory, beanBaseLocation)
+                        loadSchema(entry.getMethod(), entry.getSchema(), schemaRegistry, uriFactory, beanBaseLocation)
                 ))
                 .toList();
     }

And update loadSchema signature accordingly:

-    private static Schema loadSchema(String methodPattern, String schemaPath, Resolver resolver, URIFactory uriFactory, String beanBaseLocation) {
+    private static Schema loadSchema(String methodPattern, String schemaPath, SchemaRegistry schemaRegistry, URIFactory uriFactory, String beanBaseLocation) {
         // ...
-            Schema schema = createSchemaRegistry(resolver).getSchema(
+            Schema schema = schemaRegistry.getSchema(

Also applies to: 129-148

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.java`
around lines 107 - 113, The code repeatedly calls createSchemaRegistry(resolver)
inside loadSchema(...) for every entry; instead create the SchemaRegistry once
in init() (e.g., SchemaRegistry schemaRegistry =
createSchemaRegistry(resolver)), store it in a local variable, and pass that
same schemaRegistry into loadSchema(...) when building CompiledSchema in the
effectiveMappings stream and in the other loop (the second block around lines
129-148). Update loadSchema's signature to accept SchemaRegistry (remove
internal createSchemaRegistry call) and ensure both call sites (the mapping ->
new CompiledSchema(...) creation and the other loadSchema usages) use the shared
schemaRegistry to avoid recreating identical registries per entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.java`:
- Around line 107-113: The code repeatedly calls createSchemaRegistry(resolver)
inside loadSchema(...) for every entry; instead create the SchemaRegistry once
in init() (e.g., SchemaRegistry schemaRegistry =
createSchemaRegistry(resolver)), store it in a local variable, and pass that
same schemaRegistry into loadSchema(...) when building CompiledSchema in the
effectiveMappings stream and in the other loop (the second block around lines
129-148). Update loadSchema's signature to accept SchemaRegistry (remove
internal createSchemaRegistry call) and ensure both call sites (the mapping ->
new CompiledSchema(...) creation and the other loadSchema usages) use the shared
schemaRegistry to avoid recreating identical registries per entry.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e7c0039a-7550-423c-ba05-306d03760700

📥 Commits

Reviewing files that changed from the base of the PR and between 5bd32da and 4fd835a.

📒 Files selected for processing (3)
  • annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.java
  • core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.java
  • core/src/test/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • core/src/test/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptorTest.java

Comment thread core/src/main/java/com/predic8/membrane/core/util/allowdeny/Allow.java Outdated
christiangoerdes and others added 16 commits June 1, 2026 13:54
… parameter validation, and batch size limits
…d update associated logic, tests, and documentation
…`error` with updated tests and supporting classes
…lace with `schemaValidation` supporting `params`, `response`, and `error` validation. Update tests and tutorials accordingly.
…cumentation with descriptions and examples, and update schema handling logic and tests
if (segment.startsWith(".")) {
return segment.substring(1);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc sample ...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract

}

if (segment.startsWith("['") && segment.endsWith("']")) {
return segment.substring(2, segment.length() - 2)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract

}

if (segment.startsWith("[") && segment.endsWith("]")) {
return segment.substring(1, segment.length() - 1);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract

throw unsupported(wanted, key, node);
}

private Object convertAnySetterValue(ParsingContext<?> ctx, Method setter, JsonNode node, String key) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc

return path;
}

private static String toJsonPathProperty(String property) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs

* <p>In YAML, the entries are written as a map under <code>schemaValidation.methods</code>.</p>
*/
@MCElement(name = "methods", component = false, id = "json-rpc-method-definitions")
public class JsonRPCMethodDefinitions {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonRPCMethodsDefinitions

* to validate successful upstream responses.</p>
*/
@MCElement(name = "method", component = false, id = "json-rpc-method-schema")
public class JsonRPCSchemas {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename

Map<String, Schema> resolvedParamSchemas = new LinkedHashMap<>();
Map<String, Schema> resolvedResponseSchemas = new LinkedHashMap<>();

for (Map.Entry<String, JsonRPCSchemas> entry : methods.getMethods().entrySet()) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants