fix(schema): share representation identity across rebuilt containers of one declaration - #6838
Closed
aryasaatvik wants to merge 2 commits into
Closed
Conversation
HttpApiSchema.status annotated the schema with httpApiStatus, forking the AST while the fork kept its identifier. A named schema reused with a non-default status then published duplicate OpenAPI components for one declared name (previously: Duplicate identifier throw). The status describes how one endpoint uses the schema, not what the schema is, so it now rides the AST key context — which representation identity already excludes — via annotateKey. Declaration-site statuses (the HttpApiError classes) keep working through the annotation fallback.
…ation A memoized AST derivation (toCodecJson) rebuilds a node whenever any child changes - e.g. Unknown lowering to Json - allocating fresh property-signature wrappers and arrays per top-level input. Two ASTs that differ only in their key context (a named schema and its HttpApiSchema.status copy) are separate derivation inputs, so their rebuilt containers can never be reference-equal and representation identity split them into Widget and Widget_1. Container wrappers are pure structure with no identity of their own. When two ASTs hold the same annotations object - the token of one user declaration - their container slots now compare by contents instead of by reference. Anonymous nodes and nodes with different annotation objects keep pure reference identity, so referentially distinct declarations still suffix.
🦋 Changeset detectedLatest commit: 571d0c7 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 |
Author
|
Consolidated into #6837 — that PR now carries both commits (the httpapi key-context fix and this representation-identity fix) as companion halves of one invariant. |
Contributor
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — two commits that fix duplicate OpenAPI component generation when a declared schema is reused with HttpApiSchema.status().
HttpApiSchema.statusswitched toannotateKey: StoreshttpApiStatusin key context (excluded from representation identity) instead of annotations, with an annotation fallback forHttpApiErrorclasses.toRepresentation.tsstructural slot comparison: When two ASTs share the same annotations object, container slots (PropertySignature,IndexSignature, arrays) compare structurally rather than by reference, so derivation rebuilds (e.g.toCodecJsonofUnknown → Json) don't fork identity.- Tests: One regression test at the representation level, two end-to-end OpenAPI tests. All existing tests pass — invariants for anonymous nodes, referentially distinct declarations, and recursive fallback definitions are preserved.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
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
1 participant
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.
Stacked on #6837 — the branch includes its commit; only the last commit (
Share representation identity across rebuilt containers of one declaration) is new here. The two are companion halves of one invariant: #6837 stopsHttpApiSchema.statusfrom forking the annotations slot; this stops derivation rebuilds from forking the container slots.Problem
SchemaRepresentationresolves component identity by reference: an AST's identity key is thecomposition of the object identities of its slots, with
contextexcluded. That works as long asevery reuse of a declared schema reaches the representation as reference-shared structure.
Memoized derivations break that assumption whenever the derivation is non-identity.
toCodecJsonlowers
UnknowntoJsonby rewriting the child node, which forces the parent rebuild to allocatefresh
PropertySignaturewrappers and a freshpropertySignaturesarray — once per top-levelderivation input. A named schema and its context-only copy (
HttpApiSchema.statusafter #6837) aretwo derivation inputs, so their rebuilt containers can never be reference-equal, and the identity
key diverges at exactly that slot:
Remove the
metadatafield and the pair unifies — the failure is keyed to whether any child'sderivation is non-identity, which makes it look nondeterministic from the API author's seat. Any
schema carrying
Schema.Unknown(a JSON metadata column, aRecord(String, Unknown)) hits it inthe default CRUD shape.
Fix
Container wrappers —
propertySignatures/indexSignaturesarrays,PropertySignature,IndexSignature— are pure structure: they carry no annotations, checks, or context of their own,so they have no identity to defend. When two ASTs hold the same annotations object — the token
of one user declaration — their container slots now compare by contents (element-wise reference
identity) instead of by container reference.
The gate is what keeps existing semantics intact:
with distinct ASTs" is unchanged.
.annotate({ identifier: "Value" })calls create two annotations objects, so referentiallydistinct declarations still suffix (
Value/Value_1).split: their containers differ at the child references themselves ("does not deduplicate fallback
definitions with distinct recursive dependencies" is unchanged).
Scope: one function in
internal/schema/toRepresentation.ts; no AST or derivation changes.Tests
toRepresentations.test.ts: "shares a named reference across context-only copies whosederivation rebuilds a container" — pure representation-level regression, fails with
["Widget", "Widget_1"]refs before the fix.OpenApi.test.ts: "emits one component for a named schema containing Unknown reused withHttpApiSchema.status" — end-to-end CRUD shape, both
200and201referencing oneWidgetcomponent. Depends on fix(httpapi): reuse named schemas across response statuses #6837 (without it the copies also differ at the annotations slot).
packages/effect/test/schema+packages/effect/test/unstable/httpapi: 2220 tests green;tsc -b packages/effectand lint clean.