fix: annotate resolved schemas with dialect-aware id keyword#912
Conversation
| $this->assertEquals('454f423bd7edddf0bc77af4130ed9161', md5(json_encode($schema))); | ||
| $this->assertEquals('object', $schema->type); | ||
| $this->assertObjectHasAttribute('$id', $schema); |
There was a problem hiding this comment.
$id instead of id?
There was a problem hiding this comment.
This was from a 9 years ago commit by a maintainer which is no longer active. I belief this to be a quick way to check for the expected result. These test now no longer confirm the id or $id value which would be even better. Could you add that an assertion for that?
There was a problem hiding this comment.
Good point — added value assertions ("id" or "$id" based on the active Draft) in 85bfc72.
There was a problem hiding this comment.
Pull request overview
Fixes UriRetriever::retrieve() unconditionally stamping the Draft-4 id keyword onto every URI-resolved schema. It now inspects the loaded root schema's $schema keyword and uses id only for Draft-3/4, defaulting to $id otherwise (matching the library's Draft-6 default dialect). The corresponding test no longer relies on a brittle md5 of the serialized schema.
Changes:
- Import
DraftIdentifiersinUriRetrieverand detect the dialect from the root schema before pointer resolution. - Stamp the resolved URI onto either
id(Draft-3/4) or$id(everything else) inUriRetriever::retrieve(). - Replace the md5-of-JSON assertion in
testRetrieveSchemaFromPackagewith structural assertions ontypeand the presence of$id.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/JsonSchema/Uri/UriRetriever.php | Detect dialect from root schema's $schema and annotate with id for Draft-3/4, $id otherwise. |
| tests/Uri/UriRetrieverTest.php | Switch the package-retrieval test from a brittle md5 hash to structural assertions reflecting the new $id annotation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DannyvdSluijs
left a comment
DannyvdSluijs
left a comment
There was a problem hiding this comment.
Thanx for the issue report and the PR. I think it would be best to improve the tests ever so slightly.
| $this->assertEquals('454f423bd7edddf0bc77af4130ed9161', md5(json_encode($schema))); | ||
| $this->assertEquals('object', $schema->type); | ||
| $this->assertObjectHasAttribute('$id', $schema); |
There was a problem hiding this comment.
This was from a 9 years ago commit by a maintainer which is no longer active. I belief this to be a quick way to check for the expected result. These test now no longer confirm the id or $id value which would be even better. Could you add that an assertion for that?
UriRetriever::retrieve() unconditionally stamped resolved schemas with the draft-04 "id" keyword. Draft-6 renamed this to "$id", but the annotation was never updated. Detect the dialect from the root schema's $schema keyword before pointer resolution. Draft-3/4 schemas get "id"; Draft-6+ and schemas without an explicit $schema keyword get "$id" (matching the library's default). Internally safe because SchemaStorage::findSchemaIdInObject() already reads both keywords. Fixes the issue for external consumers of resolved schemas (e.g. strict Draft-7 validators like Ajv that reject the unknown "id" keyword). Fixes jsonrainbow#911 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…word The dialect comparison now strips the trailing # from the $schema value and compares against withoutFragment() constants, so schemas declaring e.g. "http://json-schema.org/draft-04/schema" (without #) correctly get `id` instead of falling through to `$id`. Adds 5 tests covering Draft-04 and Draft-07 with/without fragment, plus the no-$schema default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per review feedback, the retriever tests confirmed only the presence of the id/$id attribute, not its value. Add an assertion for the stamped resolved URI to each affected test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DannyvdSluijs
force-pushed
the
fix/911-uri-retriever-dialect-aware-id
branch
from
85bfc72 to
25ec63d
Compare
|
🚀 thanks! |
Summary
UriRetriever::retrieve()now reads the root schema's$schemakeyword to determine whether to annotate withid(Draft-3/4) or$id(Draft-6+)$schemakeyword default to$id, matching the library's Draft-6 default dialecttestRetrieveSchemaFromPackageto assert structure instead of a brittle md5 hashContext
UriRetriever::retrieve()unconditionally stamped$jsonSchema->id = $resolvedUrion every resolved schema. Draft-6 renamedidto$id, but the annotation was never updated. This leaks a Draft-4 keyword into schemas resolved under Draft-6/7, causing errors in strict validators (e.g. Ajv:NOT SUPPORTED: keyword "id", use "$id" for schema ID).The dialect is detected from the root schema before pointer resolution, so sub-schemas inherit the correct keyword from their parent document.
Internally safe —
SchemaStorage::findSchemaIdInObject()already reads bothidand$id.Test plan
id, Draft-07 →$id, no$schema→$idFixes #911