fix(schema): keep annotations across encodings - #7203
Closed
spencerbeggs wants to merge 11 commits into
Closed
Conversation
- Carry documentation annotations from the type side of an encoding chain onto the representation, so a schema that encodes to a different shape keeps its title, description and examples - Let annotations closer to the encoded side win, since a link that rewrites the data may already have folded the type side description into its own Signed-off-by: C. Spencer Beggs <spencer@beggs.codes>
🦋 Changeset detectedLatest commit: 15020bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
spencerbeggs
changed the title
- Assert the carry at the representation layer: documentation keys travel, expected and the other behavioral keys stay behind, and the encoded side wins a collision - Cover the paths the JSON Schema tests missed: a referenced encoded definition, a two-link encoding chain, and default and examples values that are not valid JSON Signed-off-by: C. Spencer Beggs <spencer@beggs.codes>
- Type the encoding-chain test against the real signatures: numeric examples for NumberFromString, and passthroughSubtype for the Date to unknown link - Update the HttpApi OpenAPI snapshot, where the UserEncoded component schema now carries the description its Schema.Class declares Signed-off-by: C. Spencer Beggs <spencer@beggs.codes>
Contributor
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
Effect-TS#7336 compacts a lone inlineable check into its parent instead of wrapping it in `allOf`. That PR updated every expectation present on main, but the annotated `Schema.BigInt` assertion is added by this branch, so the merge left it in the pre-compaction shape and it was the only failing test on Node and Deno. Signed-off-by: C. Spencer Beggs <spencer@beggs.codes>
Contributor
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.
Fixes #7192.
This is a fix I put together for a problem I ran into; the analysis below is my reading of the code, not something a maintainer has signed off on. I have tried to make the premises, the judgement calls and the gaps easy to check separately, so any one of them can be rejected without discarding the rest. Happy to take this in a different direction.
What I am proposing
Schema.toJsonSchemaDocumentkeepstitle,description,examplesand the other JSON Schema annotations on schemas whose encoded form differs from their type.Schema.Numberwas the reported case, and it ends up behaving likeSchema.StringandSchema.NullOralready do:Root cause, as I read it
The annotation is not lost in the JSON Schema compiler — it never reaches it.
internal/schema/toRepresentation.tsbuilds every representation fromSchemaAST.getLastEncoding(input), which walks to the final link of the encoding chain and then reads annotations off that node alone.Schema.Number.toCodecJson()callsreplaceEncoding(this, [numberToJson(this.checks)]), so the annotatedNumbernode is the type side, and the encoded side is a freshUnion([finite, nonFiniteLiterals])carrying no annotations. Everything on the type side is dropped before the JSON Schema pass runs.That also accounts for the other observations in the issue.
StringandNullOrhave no encoding, soinput === encodedand nothing is lost.FiniteandIntshort-circuittoCodecJson()back tothis, so they are in the same position and keep the annotation. And the 4-branchanyOfintoJsonSchemaDocument.tsis unrelated: the 2-branch shape the caller sees comes from the encodedUnion, not from that code path.Scope
Schema.Numberis not the only schema affected — as far as I can tell this applies to every schema with an encoding link.Schema.BigInt,Schema.Date,Schema.URL,Schema.Option,Schema.ReadonlyMap,Schema.Unknown,Schema.Void,Schema.Undefined,Schema.ObjectKeywordandbigintliterals all dropped their annotations the same way, and six existing tests asserted that behavior. Those are updated here, and the fix covers all of them rather than special-casingNumber.If you would rather scope this to
Numberonly, say so — that is a smaller change and I am not attached to the broad version.The fix, and the two judgement calls in it
toRepresentationcarries the type-side annotations forward across the encoding chain. Two decisions in there are mine, and either can be reversed cheaply:1. Only
jsonSchemaAnnotationKeystravel.representation,expected,identifier, theto*hooks and everything else stay bound to the node that declared them. Carryingexpectedin particular looked wrong to me: it describes the decoded value, soSchema.Option(...)would start emittingdescription: "Option"for its encoded union undergenerateDescriptions: true.2. Annotations closer to the encoded side win.
unstable/ai/internal/structured-output.tscomposes the type-side description into its own ("Tuple encoded as an object with numeric string keys …; description"). My first attempt let the type side win and broke fourAnthropicStructuredOutput/OpenAiStructuredOutputtests, which is what convinced me that a link rewriting the shape of the data should get to describe the result. It also lines up withresolveReferenceIdentifier, which prefers the encoded identifier and falls back to the input's. If you read the precedence the other way, it is a one-line flip inwithCarriedAnnotations, but the structured-output description composition would need reworking to match.Tests
Assertions were checked by mutation rather than by inspection — each row below is a single-line change to the fix, re-run against the full workspace suite on this branch:
toJsonSchemaDocument(14),toRepresentation(4),OpenApiGenerator(3),HttpApi(1)toRepresentation,AnthropicStructuredOutput,OpenAiStructuredOutputjsonSchemaAnnotationKeystoRepresentation,toRepresentations,toJsonSchemaMultiDocument,toJsonSchemaDocumenttoRepresentationtoRepresentationtoRepresentation,toJsonSchemaDocument,HttpApitest/schema/representation/toRepresentation.test.tsholds the unit-level assertions (carry, key filtering, precedence, the reference path, a two-linkUnknown > Declaration > Stringchain).test/schema/toJsonSchemaDocument.test.tsholds the user-visible ones, including thatSchema.Date.annotate({ default: new Date(0) })still drops the non-JSON value rather than emitting it.Known gap: annotations that land on a check
.annotate()targets the last check when a schema has one, and checks do not travel to the encoded side of a transformation. So this case is still annotation-free and is not fixed here:I left it out on purpose. The obvious fix is to read through
InternalAnnotations.resolveinstead ofast.annotationsincarriedAnnotations. I tried that and measured it, and it trades one class of dropped annotation for another, becauseresolvereturns only the last check's annotations and does not merge them with the node's:resolveFiniteFromString.annotate({description}){type:"string"}❌{type:"string", description:"d"}✅Number.annotate({description}).check(isGreaterThan(0)){anyOf:[…], description:"d"}✅{anyOf:[…]}❌BigInt.annotate({description}).check(…){type:"string", description:"d", pattern:…}✅{type:"string", pattern:…}❌Number.check(isGreaterThan(0)).annotate({description}){anyOf:[{…, description:"d"}, …]}{anyOf:[{…, description:"d"}, …], description:"d"}(duplicated)Worth flagging clearly: the full suite passes either way. No test currently defends either the regression in rows 2–3 or the duplication in row 4, so green CI does not settle this one — it is a behavior question, not a test question.
A merge of the two (
{...ast.annotations, ...lastCheckAnnotations}) would close the gap without the regression, and I am glad to add it. What stops me from doing it unilaterally is row 4: it leaves adescriptionboth inside the finite branch and at the top level, and deciding whether that is redundant-but-fine or actually wrong is really the question of what a check annotation means on an encoded shape. That reads to me like your call rather than mine, and it is adjacent to #7336. Happy to fold it into this PR, split it into a follow-up, or drop it entirely.Interaction with #7336
Rebased on top of the check compaction. One expectation in
toJsonSchemaDocument.test.tsneeded updating: the annotatedSchema.BigIntassertion is added by this branch, so it was not covered by #7336's sweep and still expected the pre-compaction shape.The compaction also supersedes something I claimed in an earlier version of this description:
Schema.Finite.annotate({description})no longer nests the annotation underallOf, it now reads{ type: "number", description: "d" }. That paragraph is gone.Verification
On the current head of this branch:
pnpm check— clean.pnpm lint(oxlint +dprint check) — clean.pnpm test-types --target '>=5.9'— 182 files, 2144 tests, 5270 assertions.One snapshot moved and I think it is worth a look rather than a rubber stamp:
HttpApi's OpenAPI fixture now emits"description": "Some description for User"on theUserEncodedcomponent schema.Useris aSchema.Classdeclaring that description, and the encoded component was dropping it while the OpenAPI response objects already carried it through a different path. The two agree now, but if the previous asymmetry was deliberate I have missed the reason.Questions for reviewers
Schema.Number?