feat: implement OP#12 schema-vs-schema validation#11
Merged
Artifizer merged 8 commits intoGlobalTypeSystem:mainfrom Feb 17, 2026
Merged
feat: implement OP#12 schema-vs-schema validation#11Artifizer merged 8 commits intoGlobalTypeSystem:mainfrom
Artifizer merged 8 commits intoGlobalTypeSystem:mainfrom
Conversation
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds GtsStore.validateSchemaAgainstParent with deep schema resolution and constraint comparison, normalizes/prunes x-gts-ref and combinators, expands x-gts-ref combinator traversal/evaluation (containsXGtsRef, duplicate implementation present), exposes POST /validate-schema and POST /validate-entity endpoints, and bumps package version to 0.2.0. Changes
Sequence DiagramsequenceDiagram
participant Client as rgba(70,130,180,0.5) Client
participant Server as rgba(34,139,34,0.5) Server
participant Store as rgba(255,165,0,0.5) GtsStore
participant XG as rgba(147,112,219,0.5) x-gts-ref
participant DB as rgba(220,20,60,0.5) DB
Client->>Server: POST /validate-schema { schema_id } or /validate-entity { entity_id | gts_id }
activate Server
Server->>DB: fetch entity by id
DB-->>Server: entity (schema or instance)
alt entity is schema
Server->>Store: validateSchemaAgainstParent(schema_id)
activate Store
Store->>Store: resolve & normalize schema (strip x-gts-ref, prune combinators)
Store->>XG: evaluate combinators (allOf/anyOf/oneOf) using containsXGtsRef
XG-->>Store: validation result / errors
Store-->>Server: ValidationResult
deactivate Store
else entity is instance
Server->>Store: validateInstance(entity)
Store-->>Server: ValidationResult
end
Server-->>Client: return ValidationResult or structured error
deactivate Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/store.ts`:
- Around line 227-237: The current combinator cleanup indiscriminately removes
all empty object subschemas from normalized.oneOf/anyOf/allOf; instead, only
remove subschemas that became empty due to stripping x-gts-ref. When you strip
x-gts-ref earlier, mark those subschemas (e.g., add a non-serializing sentinel
property like __gtsStrippedRef = true or store their path in a Set). Then update
this combinator loop to filter out only empty objects that also carry that
sentinel (or are present in the Set), leaving intentionally empty {} branches
intact; finally ensure you remove the sentinel/Set entries after cleanup so no
runtime artifacts remain.
In `@src/x-gts-ref.ts`:
- Around line 85-133: The anyOf/oneOf handling currently runs x-gts-ref checks
across only the ref-containing branches and rejects if none pass even when a
non-x-gts-ref branch actually validated; change the logic to first determine
which anyOf/oneOf branches actually validate the instance (e.g. call the schema
validator used elsewhere or add an isValidAgainstSchema(instance, subSchema,
rootSchema) helper), then only run containsXGtsRef + visitInstance on those
branches that both validate and contain x-gts-ref; push errors from
branchResults only when the branch validated but its x-gts-ref checks failed,
and for oneOf ensure you count only validated branches when computing
passingCount (refer to containsXGtsRef, visitInstance, branchResults, and
errors).
GeraBart
force-pushed
the
fix/oneOf_fix
branch
from
9a926d6 to
a9befc8
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/x-gts-ref.ts`:
- Around line 85-133: The anyOf/oneOf logic currently treats a branch as
"passing" based solely on x-gts-ref errors collected by visitInstance, so a
branch that fails general JSON Schema constraints can still be counted as
passing; fix by first performing a full schema validation of each branch and
only run visitInstance / consider x-gts-ref results for branches that actually
validate. Concretely, for each subSchema in schema.anyOf/schema.oneOf (and after
filtering with containsXGtsRef), run the existing full-schema validator (e.g.,
this.ajv.validate or the project’s validateAgainstSchema method) against
instance; if the branch does not validate, ignore it for counting and do not
treat its x-gts-ref errors as a pass (you may still optionally collect x-gts-ref
errors only for branches that validate); if it does validate, then call
this.visitInstance(instance, subSchema, path, rootSchema, branchErrors) and use
branchErrors.length === 0 to determine passing for anyOf/oneOf logic (adjust the
anyPassed and passingCount calculations accordingly).
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
GeraBart
force-pushed
the
fix/oneOf_fix
branch
from
a9befc8 to
d478734
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/gts.test.ts (1)
417-810: Add a mixed-branch test to lock intended semantics.Current coverage is strong, but there’s no case where a non‑
x-gts-refbranch validates while allx-gts-refbranches fail. Adding such a test would clarify the expected behavior for mixed combinators and protect future changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gts.test.ts` around lines 417 - 810, Add a new test under the "oneOf with mixed x-gts-ref and plain branches" suite that asserts a non-x-gts-ref branch can validate even when all x-gts-ref branches fail: create a schema (use the existing $$id 'gts.test.pkg.ns.mixed_oneof.v1~' pattern) with oneOf: [{ 'x-gts-ref': 'gts.test.pkg.ns.target_a.*' }, { minLength: 1 }], instantiate a GTS with new GTS({ validateRefs: false }), register only a plain string target/instance (but do NOT register the target_a entries so the x-gts-ref branch fails), register the schema and an instance whose ref meets minLength but does not match any x-gts-ref pattern, call g.validateInstance(instance.gtsId) and expect res.ok toBe(true); reference GTS, g.register, and g.validateInstance to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/x-gts-ref.ts`:
- Around line 85-133: The anyOf/oneOf handling currently only inspects branches
with containsXGtsRef and treats a branch as "passing" based solely on
visitInstance (which doesn't run full structural validation), causing valid
instances that match a non-x-gts-ref branch to be rejected; change the logic to
first determine which anyOf/oneOf branches structurally validate the instance
(e.g., using the schema validator you already use elsewhere or a call that
performs full Ajv validation), then for branches that both validate structurally
and contain x-gts-ref run visitInstance to collect XGtsRefValidationError
results, and finally aggregate errors only if no structurally-valid branch
passes the x-gts-ref checks (for anyOf) or if zero or >1 structurally-valid
branches pass the x-gts-ref checks (for oneOf), updating the code paths that
reference visitInstance, containsXGtsRef, and XGtsRefValidationError to reflect
this two-step (structural then x-gts-ref) validation and error aggregation.
---
Nitpick comments:
In `@tests/gts.test.ts`:
- Around line 417-810: Add a new test under the "oneOf with mixed x-gts-ref and
plain branches" suite that asserts a non-x-gts-ref branch can validate even when
all x-gts-ref branches fail: create a schema (use the existing $$id
'gts.test.pkg.ns.mixed_oneof.v1~' pattern) with oneOf: [{ 'x-gts-ref':
'gts.test.pkg.ns.target_a.*' }, { minLength: 1 }], instantiate a GTS with new
GTS({ validateRefs: false }), register only a plain string target/instance (but
do NOT register the target_a entries so the x-gts-ref branch fails), register
the schema and an instance whose ref meets minLength but does not match any
x-gts-ref pattern, call g.validateInstance(instance.gtsId) and expect res.ok
toBe(true); reference GTS, g.register, and g.validateInstance to locate where to
add the test.
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
GeraBart
force-pushed
the
fix/oneOf_fix
branch
from
d478734 to
1db461a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/gts.test.ts (2)
458-475: Consider extracting repeated fixture registration into a helper.The registration block (lines 459-464) is repeated verbatim in many tests within this suite. While the current approach ensures test isolation, a helper function would reduce duplication without sacrificing clarity.
💡 Suggested helper approach
// Add at the top of the describe block function setupGtsWithFixtures(): GTS { const g = new GTS({ validateRefs: false }); g.register(refTargetA); g.register(refTargetB); g.register(instanceA); g.register(instanceB); return g; } // Then in tests: const g = setupGtsWithFixtures(); g.register(schema);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gts.test.ts` around lines 458 - 475, Extract the repeated registration block into a helper function (e.g., setupGtsWithFixtures) defined at the top of the describe block that creates new GTS({ validateRefs: false }), registers refTargetA, refTargetB, instanceA, and instanceB, and returns the GTS instance; then update tests like the one using GTS and schema to call this helper (const g = setupGtsWithFixtures()) and only register test-specific items such as schema and inst, ensuring each test still creates its own GTS instance for isolation while removing duplication.
779-808: Clarify the non-standard oneOf semantics in comments.This test documents that non-x-gts-ref branches are excluded from oneOf counting. In standard JSON Schema, the value would match both branches (x-gts-ref pattern and
minLength: 1), causing oneOf to fail. Consider adding a comment explaining this deliberate departure from standard semantics for future maintainers.📝 Suggested comment addition
describe('oneOf with mixed x-gts-ref and plain branches', () => { + // NOTE: When evaluating oneOf with x-gts-ref branches, only branches containing + // x-gts-ref are counted for the "exactly one" requirement. Plain JSON Schema + // branches are validated separately and don't contribute to the oneOf count. test('non-x-gts-ref branches do not interfere with counting', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/gts.test.ts` around lines 779 - 808, Add a brief comment above this test (or directly above the schema variable) explaining that GTS intentionally treats oneOf differently by excluding non-x-gts-ref branches from the oneOf match count, so the pattern branch ('x-gts-ref') is evaluated independently of the plain JSON Schema branch (e.g., { minLength: 1 }) and thus the instance is considered a single match; reference the test's schema variable and the GTS class/validateInstance flow so future maintainers know this is a deliberate, non-standard deviation from JSON Schema semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/gts.test.ts`:
- Around line 458-475: Extract the repeated registration block into a helper
function (e.g., setupGtsWithFixtures) defined at the top of the describe block
that creates new GTS({ validateRefs: false }), registers refTargetA, refTargetB,
instanceA, and instanceB, and returns the GTS instance; then update tests like
the one using GTS and schema to call this helper (const g =
setupGtsWithFixtures()) and only register test-specific items such as schema and
inst, ensuring each test still creates its own GTS instance for isolation while
removing duplication.
- Around line 779-808: Add a brief comment above this test (or directly above
the schema variable) explaining that GTS intentionally treats oneOf differently
by excluding non-x-gts-ref branches from the oneOf match count, so the pattern
branch ('x-gts-ref') is evaluated independently of the plain JSON Schema branch
(e.g., { minLength: 1 }) and thus the instance is considered a single match;
reference the test's schema variable and the GTS class/validateInstance flow so
future maintainers know this is a deliberate, non-standard deviation from JSON
Schema semantics.
806f5d0 to
b60c04f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/store.ts (1)
1317-1331: Redundant nested condition check.The nested condition on lines 1321-1326 is redundant. When
overlay.additionalProperties === undefinedis true (line 1319), the inner check on line 1324 will always be true.♻️ Proposed simplification
// Check additionalProperties if (baseResolved.additionalProperties === false) { if (overlay.additionalProperties === undefined) { - // Derived omits AP when base has false → fail (loosening) - if (Object.keys(overlayProps).length > 0 || overlay.additionalProperties === undefined) { - // Only fail if the overlay is actually present as a separate subschema - // Check if overlay explicitly omitted it - if (overlay.additionalProperties === undefined) { - errors.push('Base has additionalProperties: false but derived does not restate it'); - } - } + // Derived omits additionalProperties when base has false → fail (loosening) + errors.push('Base has additionalProperties: false but derived does not restate it'); } else if (overlay.additionalProperties === true) { errors.push('Cannot loosen additionalProperties from false to true'); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store.ts` around lines 1317 - 1331, The nested redundant check is that inside the block where overlay.additionalProperties === undefined you re-check overlay.additionalProperties === undefined again; simplify by removing the inner conditional and directly push the error when overlay.additionalProperties is undefined and overlayProps is non-empty: i.e., in the branch that tests baseResolved.additionalProperties === false and overlay.additionalProperties === undefined, only test Object.keys(overlayProps).length > 0 and then push the message into errors (leave the separate else-if overlay.additionalProperties === true branch as-is); reference variables: baseResolved, overlay, overlayProps, errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 35-36: The markdown warning is caused by underscores in the
filename examples being parsed as emphasis; edit the README entries under "YAML
support" and "TypeSpec support" and wrap the filenames in inline code or escape
the underscores/asterisk so they are literal (e.g., replace "_.yml, _.yaml" with
`_.yml`, `_.yaml` and replace "*.tsp" or "\*.tsp" with `*.tsp`), making sure the
checklist lines "YAML support" and "TypeSpec support" show the filenames as code
literals.
In `@src/server/server.ts`:
- Around line 595-602: handleValidateSchema is missing input validation for
schema_id; add a runtime check in the handleValidateSchema method (the handler
for ValidateSchemaBody) to ensure request.body.schema_id is present and
non-empty before calling
this.store['store'].validateSchemaAgainstParent(schema_id). If schema_id is
missing or invalid, return a 400 Bad Request (or throw a Fastify error) with a
clear message such as "Missing required field: schema_id" so callers get a
proper validation error instead of "Entity not found: undefined".
---
Nitpick comments:
In `@src/store.ts`:
- Around line 1317-1331: The nested redundant check is that inside the block
where overlay.additionalProperties === undefined you re-check
overlay.additionalProperties === undefined again; simplify by removing the inner
conditional and directly push the error when overlay.additionalProperties is
undefined and overlayProps is non-empty: i.e., in the branch that tests
baseResolved.additionalProperties === false and overlay.additionalProperties ===
undefined, only test Object.keys(overlayProps).length > 0 and then push the
message into errors (leave the separate else-if overlay.additionalProperties ===
true branch as-is); reference variables: baseResolved, overlay, overlayProps,
errors.
e7e4d72 to
2978d4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/server/types.ts (1)
76-83: Model “one-of” input to prevent{}bodies.
ValidateEntityBodycurrently allows an empty object, which undermines the intended contract and shifts all validation to runtime. Consider a union (or XOR) type to enforce at least one ID at compile time.♻️ Suggested type shape
-export interface ValidateEntityBody { - entity_id?: string; - gts_id?: string; -} +export type ValidateEntityBody = + | { entity_id: string; gts_id?: never } + | { gts_id: string; entity_id?: never };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/types.ts` around lines 76 - 83, ValidateEntityBody currently permits an empty object; replace the loose interface with a discriminated union so callers must provide exactly one identifier at compile time (e.g., a union of a shape with entity_id present and gts_id absent vs. a shape with gts_id present and entity_id absent). Update the exported type name ValidateEntityBody to this union (or use a reusable XOR/OneOf utility type) and ensure any code referencing ValidateEntityBody (validators, handlers) compiles against the new stricter type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/store.ts`:
- Around line 1103-1112: The code calls resolveSchemaFully(parentEntity.content)
without verifying parentEntity is actually a schema or has content; update the
parent resolution to validate parentEntity.type (e.g., === 'schema' or the
project's canonical schema discriminator) and that parentEntity.content is
present before calling resolveSchemaFully, and if the check fails return the
existing error shape (e.g., { id: schemaId, ok: false, error: ... }); use
symbols parentRef, parentId, parentEntity, this.get(...), parentEntity.content,
resolveSchemaFully and schemaId to locate and implement the guard.
- Around line 1321-1334: The current additionalProperties check incorrectly
requires derived schemas to restate additionalProperties: false; update the
validation in the additionalProperties block so that when
baseResolved.additionalProperties === false you only error if
overlay.additionalProperties === true (i.e. the overlay explicitly loosens the
constraint), and do not push an error when overlay.additionalProperties is
undefined or overlayProps exist; modify the logic around baseResolved, overlay,
overlayProps and errors to remove the “must restate false” condition and only
report "Cannot loosen additionalProperties from false to true" when
overlay.additionalProperties === true.
- Around line 1250-1272: The current use of Object.assign on overlay.properties
(inside the schema.allOf loop and the top-level properties merge) overwrites
earlier property constraints when multiple subschemas touch the same property;
instead iterate each property key and merge per-property objects: for each key
in sub.properties (and for schema.properties) if overlay.properties[key] exists
and both values are objects, shallow-merge fields into overlay.properties[key]
(e.g., Object.assign(overlay.properties[key], subProp)), otherwise set
overlay.properties[key] = subProp; keep the existing handling for
overlay.required and overlay.additionalProperties unchanged.
---
Nitpick comments:
In `@src/server/types.ts`:
- Around line 76-83: ValidateEntityBody currently permits an empty object;
replace the loose interface with a discriminated union so callers must provide
exactly one identifier at compile time (e.g., a union of a shape with entity_id
present and gts_id absent vs. a shape with gts_id present and entity_id absent).
Update the exported type name ValidateEntityBody to this union (or use a
reusable XOR/OneOf utility type) and ensure any code referencing
ValidateEntityBody (validators, handlers) compiles against the new stricter
type.
2978d4e to
8453dee
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/store.ts (1)
1379-1409: Consider the impact ofhasNewConstraintsallowing constraint drops.The logic at lines 1396-1404 allows dropping base constraints (e.g.,
maxLength) if the derived schema adds any new constraint keyword. This means a derived schema addingminItemscould effectively "unlock" droppingmaxLengthwithout an error.If this is intentional for allOf semantics (base constraints remain in effect anyway), consider adding a clarifying comment. Otherwise, this could allow loosening validation in unexpected ways.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store.ts` around lines 1379 - 1409, The current logic uses hasNewConstraints (computed from CONSTRAINT_KEYWORDS, baseConstraintKeys, derivedConstraintKeys) to allow dropping base constraints like maxLength if any other new constraint exists in the derived schema; if that is not intentional, change the check in the max-constraints loop (the branch that currently does "if (derived[kw] === undefined) { if (!hasNewConstraints) { errors.push(...)" ) to always push an error for dropped constraints (remove the hasNewConstraints gating) so any missing base[kw] in derived triggers errors.push(`Property '${propPath}' drops constraint '${kw}'`); alternatively, if the original behavior is intentional, add a clarifying comment above CONSTRAINT_KEYWORDS/hasNewConstraints explaining why any new constraint in the derived schema is allowed to permit dropping other base constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/store.ts`:
- Around line 1332-1339: The current check incorrectly treats a missing
overlay.additionalProperties as an error when baseResolved.additionalProperties
=== false; update the logic in the same validation block (referencing
baseResolved, overlay, additionalProperties and errors) to only push an error if
the derived/overlay explicitly sets additionalProperties === true while the
baseResolved has additionalProperties === false; do not push an error when
overlay.additionalProperties is undefined (inherited behavior), so remove or
change the branch that adds the "does not restate it" message.
---
Nitpick comments:
In `@src/store.ts`:
- Around line 1379-1409: The current logic uses hasNewConstraints (computed from
CONSTRAINT_KEYWORDS, baseConstraintKeys, derivedConstraintKeys) to allow
dropping base constraints like maxLength if any other new constraint exists in
the derived schema; if that is not intentional, change the check in the
max-constraints loop (the branch that currently does "if (derived[kw] ===
undefined) { if (!hasNewConstraints) { errors.push(...)" ) to always push an
error for dropped constraints (remove the hasNewConstraints gating) so any
missing base[kw] in derived triggers errors.push(`Property '${propPath}' drops
constraint '${kw}'`); alternatively, if the original behavior is intentional,
add a clarifying comment above CONSTRAINT_KEYWORDS/hasNewConstraints explaining
why any new constraint in the derived schema is allowed to permit dropping other
base constraints.
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
GeraBart
force-pushed
the
fix/oneOf_fix
branch
from
8453dee to
9ca48d7
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/store.ts`:
- Around line 1332-1338: The check in src/store.ts incorrectly treats
overlay.additionalProperties === undefined as an error when
baseResolved.additionalProperties === false; update the logic around
baseResolved.additionalProperties and overlay.additionalProperties (the block
that currently pushes "Base has additionalProperties: false but derived does not
restate it") so that you only error when the overlay explicitly loosens the
constraint (overlay.additionalProperties === true) and do not error when
overlay.additionalProperties is undefined (inheritance case); keep the existing
error for the explicit true case and allow overlay to omit the property to
inherit the base behavior.
Signed-off-by: GeraBart <246844849+GeraBart@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
fix of #10
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests