You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Isolate caller-owned schemas during Lambda validation - #170
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.
#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.
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:
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.
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.
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 installinto 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 offlineclean verifyon integration head78bfc68passed: 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.