feat(generator): unions expose a base even when the spec inlines the shared properties - #59
Open
giraffesyo wants to merge 3 commits into
Open
feat(generator): unions expose a base even when the spec inlines the shared properties#59giraffesyo wants to merge 3 commits into
giraffesyo wants to merge 3 commits into
Conversation
…shared properties Base() keyed on allOf, so it never fired for producers that flatten composition: the branches reach the generator with identical inlined properties and no allOf anywhere, and a spec written that way got no accessor at all. The analyzer now falls back to intersecting the variants' declared fields and synthesizes <Union>Base from what they all agree on, with the accessor copying those fields out of the variant Value holds. Variants that do compose a shared schema still name it directly, which keeps returning the type the caller already knows. Fields match on everything that shapes the generated Go: name, wire name, type, required-ness, pointer-ness, omitempty, read/write-only, deprecation. The discriminator is left out, since it is how the variants differ and a spec that spells the base out keeps it out of the shared schema too. Synthesis needs two distinct struct variants, so a nullable anyOf does not duplicate its only variant, and the name goes through the naming scope so a spec's own PetBase keeps it. Variant structs are untouched: embedding the synthesized base into them would break every keyed composite literal in already-released clients. complex-schemas.yaml now yields 18 types rather than 17. Circle and Rectangle both declare shapeType and the body union dispatches on kind, so shapeType is a property the variants share and gets a base.
The in-place filter was correct, the write index never leading the read index, but a reader has to prove that before moving on. The intersection runs once per union at analysis time.
A consumer reading ClusterCreateBase in a generated package cannot tell it from a schema the spec defines, which matters because its field set is a function of what the variants happen to share: an unrelated edit to one variant moves it. Also guards the field copy the synthesis does. ir.Field holds only strings and bools today, so copying with *f is complete, and a reference member added later would alias into the base silently rather than failing to compile.
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
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.
Follow-up to #56, which shipped in v0.2.7 but keys on
allOf: producers that flatten composition emit branches with identical inlined properties and noallOfanywhere, so those specs got noBase()at all.What changed
The analyzer now has two paths, in order:
<Union>Basefrom what they all agree on.The inlined repro from the issue now generates:
Rules
age: integerin one branch andage: stringin another drops out, as does a property one branch omits.kindwould spawn a base holding nothing but the tag. It also makes the two paths agree: a spec that spells the base out throughallOfputskindin the branches, not the base.UnknownDiscriminator()still covers the unknown case.anyOf: [Person, null]does not duplicatePerson.PetBasekeeps it and the synthesized one gets a distinct name.Dog{ID: ...}) in already-released clients.One behavior change to review
testdata/complex-schemas.yamlnow yields 18 types rather than 17.CircleandRectangleboth declareshapeType, and the request-body union dispatches onkind, soshapeTypeis a property the variants genuinely share and gets a base. That is the rule working, and the test expectation is updated with that note.Ask 2 (hoisting onto the wrapper) is not in this PR
Not implemented, for three reasons:
Valueis the decoded variant; hoisted fields would be a second copy.p.Name = "x"; json.Marshal(p)writes the old name, becauseMarshalJSONmarshalsValue. Making the wrapper authoritative on marshal does not fix the in-Go divergence, it moves it.TestUnionIsComparablepins this today. Gating hoisting on "the shared fields happen to be comparable" would make the generated shape depend on incidental spec details.Value,Raw,Base,MarshalJSON.The nil return is also load-bearing under the unknown-variant rule: for an unrecognized discriminator there is no decoded variant to read shared fields from, and hoisted fields would read as zero values instead.
Tests
internal/analyzer/unionbases_test.go: synthesis from inlined properties, composed base still preferred, only identical properties survive the intersection, discriminator-only shares nothing, synthesized name avoids the spec's own, and two struct variants are required.internal/generator/e2e_union_base_test.go: generates the inlined spec, then compiles and runs a test that compares*p.Base()against the expectedPetBasefor both variants, checksnilfor an unrecognizedkind, and confirms the variant keeps its own fields and round-trips through JSON.gofmt,go vet ./..., andgo test ./...pass.