Skip to content

Isolate caller-owned schemas during Lambda validation - #170

Merged
stevehu merged 4 commits into
masterfrom
fix-schema-validation-isolation
Oct 27, 2026
Merged

stevehu merged 4 commits into
masterfrom
fix-schema-validation-isolation

Conversation

@stevehu

@stevehu stevehu commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Schema validation attached API components to the caller's JSON tree. Deep-copy the supplied schema and API components before registry construction so validation leaves caller-owned trees unchanged.

Tests cover valid and invalid referenced schemas, preservation of caller components, and concurrent validation of one shared input through validators with conflicting component definitions and different strictness profiles. The new tests fail against the original implementation and pass with the fix. This proves input mutation, not a reproduced production request race.

Validation: full stacked reactor clean install into a task-owned Maven cache passed: 70 tests, 2 skipped, zero failures or errors. No live AWS execution.

Remote CI: https://github.com/networknt/light-aws-lambda/actions/runs/40449769617 passed. Downloaded reports confirm 70 tests, 2 skipped, zero failures/errors; the strict artifact audit has no failures and no Jackson 2 dependencies.

#169 is merged as 8406f403bec5e52fcb042978970ed285f2c80f67. This PR now targets master and includes that merged commit, retaining master's reviewed mapper files. The resulting diff is still only SchemaValidator and SchemaIsolationTest. Local full offline clean verify on integration head 78bfc68 passed: 70 tests, 2 skipped, zero failures/errors. Fresh CI is running; the earlier run above covers the original stacked head.

Next merge this PR after review and green current-head CI. Native companion networknt/light-lambda-native#190 must then repin to the actual AWS master merge SHA and rerun source/native CI before leaving draft.

Review follow-up: constructor Javadoc documents snapshot semantics and instructs callers to construct a new validator after specification changes. The isolation tests use explicit imports, explain the inline document's resolution base, and name the concurrent case as input isolation. All 3 focused isolation tests pass, with zero skips/failures/errors. This is not proof of registry-cache thread safety.

Per-call components copying remains deliberately conservative: sharing the constructor snapshot with downstream registry code needs a read-only audit and a representative large-spec benchmark before changing it. No performance claim is made here. The pre-existing non-object-schema cast and its ERR11003 behavior are unchanged and outside this input-isolation fix.

@stevehu
stevehu changed the base branch from fix-shared-lambda-runtime-mapper to master October 27, 2026 13:01
@stevehu

stevehu commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Review of head 78bfc68. I read the diff and the surrounding validator and caller code and ran nothing; both test checks had no result yet when I looked. I found no blockers.

The fix addresses what I raised on #168: doValidate now deep-copies the caller's schema and attaches a copy of the API components, so validation no longer mutates the shared overlay tree. The tests cover valid and invalid referenced schemas, caller-owned components staying unchanged, and repeated and concurrent validation across validators with conflicting components and different strictness profiles.

Five minor points:

  1. Per-call copy cost. Every doValidate call deep-copies the whole components tree again (jsonNode.deepCopy()), although the constructor already stores a private snapshot and that node is only read after being attached. RequestValidator calls validate once per parameter and body per request, so a large components section is copied several times per request on the hot path. Attaching the constructor's snapshot to the per-call schema copy should keep the isolation for the cost of only the small schema copy; a test with a large components fixture would settle whether it matters.
  2. Snapshot semantics. The constructor now holds a construction-time snapshot, so a validator no longer follows later in-place changes to the OpenApi3 overlay. Nothing observable changes today because LambdaSchemaValidator builds the helper and validator together, but a one-line Javadoc note would record the contract.
  3. Pre-existing, next to the change. ((ObjectNode) validationSchema).set(...) throws ClassCastException for a boolean schema (true/false) when components exist, and the broad catch (Exception) returns ERR11003. Skipping the attachment unless the schema is an ObjectNode would handle it.
  4. Test style. SchemaIsolationTest uses a wildcard static import and an inline java.nio.file.Path.of(...); the parse URL is a placeholder that is never read.
  5. Concurrency test claim. It shows inputs are not mutated, but it cannot catch a race in the registry cache because the assertions are about outcomes and every task uses the same three validators. The PR text already limits the claim; a name such as "input isolation" would match it.

@stevehu

stevehu commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Review of head 4aee8bf. I read the delta and ran nothing. The test check had no result yet on this head when I looked.

My earlier points look addressed or accepted: the constructor Javadoc now records the snapshot semantics, the test uses explicit assertion imports and a Path import with a comment that the parser does not read the placeholder file, and the concurrency test is renamed to concurrentValidationKeepsCallerInputsIsolated, which matches what it proves. Keeping the defensive copies until there is separate evidence is reasonable, since isolation is the point of the PR, and leaving the existing boolean-schema behavior unchanged is fine. No new points from me.

@stevehu
stevehu merged commit 5913c7c into master Oct 27, 2026
2 checks passed
@stevehu
stevehu deleted the fix-schema-validation-isolation branch October 27, 2026 13:20
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.

1 participant