Check the structs packages against encoding/json - #6469
Draft
denik wants to merge 28 commits into
Draft
Conversation
denik
force-pushed
the
denik/structs-json-agreement
branch
6 times, most recently
from
September 1, 2026 20:30
bf4147b to
1f87973
Compare
FindStructFieldByKeyType already recursed into embedded structs, but Get and Set stopped after one level, so a field of resources.PostgresProject -- which embeds a config struct that embeds the SDK spec -- was reported as not found. Both now walk embedding recursively and track the struct that declares the field, which is also the one whose ForceSendFields governs it: an outer struct that shadows the name (PostgresProjectConfig) tracks only its own fields. Co-authored-by: Isaac
… does From the adversarial review. Get and Set searched embedded structs depth-first, so a name declared at two embedding depths could resolve to the deeper field -- while json.Marshal picks the shallower one. Reading and writing the field would then not be the field that gets serialized under that name. The search now goes one level of embedding at a time, and the test fails on the old behaviour. Co-authored-by: Isaac
…tree Follow-up to the earlier fix, which only separated the first level of embedding from the rest: a field three levels down in the first anonymous member still won over the same name two levels down in a later member, while encoding/json picks the shallower one. ValidatePattern had the same depth-first walk, so a path could validate against one field and then be read and written on another. Both now walk level by level and share the direct-field scan. Co-authored-by: Isaac
The ForceSendFields assertion reads the promoted field, and the comparison spells out that GetByString returns the empty string rather than nil -- an explicit "" and an absent field are not the same thing.
When two embedded structs declare one json name at the same depth, encoding/json calls that ambiguous and omits the field entirely. Get, Set and ValidatePattern picked the first match, so a caller could read and write a field that is never serialized. All three now report it as not found, which is what json does with it. Co-authored-by: Isaac
The repeated json tag is what the fixture exists to exercise. Co-authored-by: Isaac
From the review: a struct embedding a pointer to itself sent the embedded-field search round forever whenever the key was not found at all. Both the value and the type walk now skip a type they have already visited. Co-authored-by: Isaac
From the review: the cycle guard deduplicated types globally, so a diamond -- two embeds reaching one type, putting the name at the same depth twice -- was only visited once and resolved to a field encoding/json omits. Types are now excluded only from earlier levels, so the two paths within one level produce the two matches that make it ambiguous. Co-authored-by: Isaac
From the review: two embedded pointers declaring one json name at the same depth make it a name encoding/json omits, but the value walk skips a nil embed, so whether the name resolved depended on which pointers happened to be set. Get and Set could reach a field ValidatePattern rejects. The value walk now asks the type walk first, so all three agree on which names exist. Co-authored-by: Isaac
… index
Two disagreements with encoding/json, both found by the new agreement tests, and both
fixed by moving field resolution wholly onto the type and following the index chain it
produces -- which is how encoding/json itself resolves a name.
A name declared behind a nil embedded pointer and again deeper down resolved to the deeper
field. encoding/json picks the shallower declaration and then serializes nothing, because
the pointer is nil, so the deeper field is a field the wire format never carries: Get
returned a value that could not be sent and Set wrote where nothing would read. Resolving
on the type and then walking the value means a nil pointer on the winning path reads as an
absent field. Comparing the owner *type* is not enough, because one struct type can be
reachable by two paths.
An anonymous field carrying a json name is a named field to encoding/json -- it serializes
as a nested object under that name -- but the embedded search flattened it anyway. So
"value" resolved on a type that emits {"leaf":{"value":...}}, while "leaf.value", the path
that is actually on the wire, did not resolve at all. Both directions now agree.
The index-chain resolution also replaces the parallel breadth-first walk over values, so
the two searches can no longer drift: findFieldInStruct, embeddedStructs and
embeddedStructTypes are gone.
Co-authored-by: Isaac
Both walks flattened every anonymous field. encoding/json flattens an embed only when its json tag gives no name: a name makes it an ordinary field serialized as a nested object. So a tagged embed was walked as though its fields were the outer struct's, and structdiff would have reported a change at a path the wire format does not have. The type walk had the inverse bug: it keyed the decision on the tag being non-empty, so `json:",omitempty"` on an embed -- a tag that sets an option and leaves the name empty -- made it a nested field, while encoding/json still flattens it. Both now use structaccess.IsFlattenedEmbed, so the three searches cannot drift again. No resource type has either shape today: the refschema golden is unchanged. Co-authored-by: Isaac
Same rule as structwalk: encoding/json flattens an embed only when its json tag gives no name. structdiff flattened every anonymous field, so a change inside a tagged embed was reported at the outer level -- a path the wire format does not have, which the direct engine would then put in an update mask. equal.go gets the same predicate, which also means a tagged embed with json:"-" is skipped rather than compared. Co-authored-by: Isaac
Three more from the review, all in name resolution: At one depth, encoding/json prefers the field whose json tag names it over one that merely has the matching Go field name; only a genuine tie is ambiguous. The search counted both as matches and reported the name as not found, so a field the wire format does carry was unreachable. A field whose tag sets only an option -- `json:",omitempty"` -- has no json name, so encoding/json serializes it under its Go field name. The search matched tag names only, so such a field could not be resolved at all, while structwalk already emitted it under the Go name: the two disagreed about a field that is plainly on the wire. Only an anonymous *struct* is promoted. An embedded scalar, slice or interface is a member named after its type, but IsFlattenedEmbed called it an embed, so structwalk and structdiff placed its contents at the parent path. No resource type has any of these shapes: the refschema golden is unchanged. Co-authored-by: Isaac
…/json does
encoding/json walks an embedded type once per level however many members reach it, so a name
declared *below* a type reached by two routes is not ambiguous -- it resolves along the first
route. A name the duplicated type declares itself is ambiguous, and json serializes neither.
The search treated every route as independent, so it called the first case ambiguous and
reported a name as not found that the wire format does carry. Verified against json rather
than reasoned about: a diamond over the declaring type marshals to {}, while a diamond one
level above it marshals to {"value":"left"}.
The internal/readonly skip keeps its existing behaviour, with a comment recording that it
diverges from encoding/json: such a field shadows a same-named field further down, so
skipping it lets the deeper one win. resources.App is the live example. Rejecting the name
outright instead would make ${resources.apps.*.url} unresolvable, so that is a decision
about what internal means rather than a fix to make here.
Co-authored-by: Isaac
The fifth review round claimed a remaining tagged/untagged bug for a repeated embedded type. It does not reproduce: the counterexample used omitempty fields left at their zero value, so "omitted because empty" was indistinguishable from "not serialized at all". With values populated, all four combinations agree. They are worth keeping, since the reasoning is easy to get wrong in either direction, so the table asserts each against encoding/json rather than against a hand-written expectation: a repeated type declaring the name itself is annihilated, its tagged name losing to a sibling's untagged X under "X", a repeated untagged name not colliding with the tagged one at all, and the shallower of two routes winning. Co-authored-by: Isaac
encoding/json skips a field only when its json tag is exactly "-". A tag whose name part is
"-" followed by options -- json:"-,omitempty" -- names the field "-" and serializes it like
any other name. structtag's parsed name reports "-" for both, so every caller that branched
on the parsed name conflated them: structaccess could not resolve such a field, and
structwalk and structdiff left it out.
structaccess.IsSkippedField now makes the distinction from the raw tag, and the four packages
share it. The structwalk fixture already had two such fields, added as "fixture for odd tag
handling"; its expectation asserted they were skipped, which is what encoding/json does with
json:"-" and not with what they actually carry. Verified against json.Marshal:
{"-":"o","kept":"k"}.
No resource type has the shape -- the refschema golden is unchanged.
Co-authored-by: Isaac
encoding/json decides which fields exist on the wire, under which names, at which paths.
Every libs/structs package claims to speak that vocabulary -- structwalk enumerates it,
structaccess reads and writes it, structdiff reports changes in it -- so a disagreement is
a bug in one of them, or in the type.
The new package fills every field of a type with a non-zero value, marshals it, and
compares the result against structwalk's leaves and structaccess.Get/ValidatePath. The
test drives it off config.Resources by reflection, so a newly added resource is covered
without touching the test.
It finds two things today, both enumerated rather than fixed here:
- Eleven resource types embed a struct that declares MarshalJSON and declare none of
their own, so the embedded marshaler takes over and id, url, lifecycle and permissions
never reach the wire. Nothing marshals a config type today (bundle validate -o json
marshals the dyn tree), so this is latent.
- Free-form any fields and types that marshal themselves as a scalar (duration.Duration,
the SDK time wrapper) are never visited by structwalk, so structdiff cannot report
drift on them.
Co-authored-by: Isaac
These are the types the direct engine actually reads fields out of: the plan resolves a
${resources...} reference by walking them, the state file is the JSON encoding of
StateType, and a refresh decodes into RemoteType. A disagreement here is not latent the
way it is for the config types -- it is a field the plan cannot see or the state file
cannot carry.
assertJSONRoundTrip already covers whether a wrapper loses fields across Marshal ->
Unmarshal. This covers whether the packages and encoding/json name and reach the same
fields at all.
Both flavours pass for every registered resource once the EmbeddedSlice convention is
accounted for: __embed__ is transparent to the walkers by design, so the check follows
them rather than the literal wire key.
Co-authored-by: Isaac
The repo forbids sort.Strings in favour of the standard library's generic version.
…ape corpus
The corpus in libs/structs/internal/jsonshapes pairs a struct shape whose JSON behaviour is
easy to get wrong -- two levels of embedding, a shadowed name, a same-depth collision, a
diamond, a cyclic embed, an embed behind a nil pointer -- with the fields encoding/json
actually serializes for it. Its own test asserts those expectations against json.Marshal,
so the corpus cannot teach every consumer the same wrong answer.
Each package is then checked against it on its own terms: structaccess must read, write and
validate exactly what the wire carries, and the write assertion goes through json.Marshal so
a Set into a field the wire format ignores fails; structwalk must visit exactly those paths;
structdiff must report a change to each of them and none to a field encoding/json drops.
structpath and structtag have no corpus to check against, so they are pinned directly:
a rendered path must survive being parsed again (a map key with dots is the case that
matters), and a json tag must resolve to the name encoding/json chose for it.
Three disagreements are recorded rather than fixed, each as a ratchet that asserts the
disagreement is still present, so fixing one breaks the test and forces the entry out:
- structwalk visits every declaration of a shadowed embedded field, so it reports the
field twice while the wire format carries one value.
- structwalk and structdiff both expose an ambiguous embedded field that encoding/json
refuses to serialize, so the engine can plan an update that can never be sent.
- structaccess.Set will not descend through a nil embedded pointer, so a field
json.Unmarshal reaches by allocating it cannot be written.
Co-authored-by: Isaac
JSONLeaves had no caller once Check took the self-marshaling set from the internal helper, and KnownDivergence was never used. task fmt rewrote the reflection loop to reflect.TypeFor and Type.Fields.
From the adversarial review. Four ways the checks were weaker than they read: Filter dropped SelfMarshalingScalars from the report it returned, so the callers' check on that category could never fire and the category was silently ignored rather than reported. A known-divergence entry only filtered; nothing noticed when the underlying bug was fixed and the entry went stale. Filter now returns the entries that matched nothing and both tests fail on them. That immediately found two stale entries. Prefix coverage applied to every category, so an entry naming a field could absorb an unrelated Get or value failure at a path beneath it. It now applies only to the two walk categories, where a field lost wholesale really does take its leaves with it. The per-shape gaps asserted merely that *some* disagreement remained, which a different regression would satisfy. They now hold the exact current output, so any change in behaviour fails and the entry has to be revisited. Paths inside a free-form any field are a category of their own now, like self-marshaling scalars: structwalk does not traverse an interface and structaccess cannot validate a path through one, so the whole subtree is opaque and listing individual paths would only pin the filler's choice of map key. With that, the state and remote types need no recorded divergences at all. Co-authored-by: Isaac
A json name on an anonymous field makes it a named field to encoding/json; a tag that sets only an option leaves it flattened. Both shapes are in the corpus now, and Leaves flattens a shape's JSON to leaf paths so the corpus's expectations stay comparable once a shape nests. Also: coveredBy takes the longest matching entry rather than the first, so overlapping known-divergence entries are each credited with what they alone cover instead of one absorbing everything and leaving the other looking stale.
…gories
Three coverage holes from the final review round.
Check inventoried scalar leaves only, so a field encoding/json emits as {} or [] contributed
nothing to compare and a field the packages could not reach at all would have passed
unnoticed. Container paths are now collected too and each one has to resolve through
ValidatePath and Get. Every resource, state and remote type already satisfies that.
The free-form and self-marshaling categories were logged and cleared, so a newly introduced
blind spot passed silently. Both are ratcheted now, each at the level where the ratchet says
something:
- Free-form any fields are listed by name per resource. Which resources have one is
stable, so a new one has to be added here deliberately. That immediately caught a real
difference: cluster policies have two in the config type and none in the state type,
because the definition is normalized to the string the API takes before deploy.
- Self-marshaling scalars are ratcheted on the Go *types* that behave this way, not the
paths. A new timestamp field of duration.Duration or the SDK time wrapper says nothing;
a new type that hides itself from the walkers is a finding, and fails.
Co-authored-by: Isaac
Check recorded the walk's leaves in a map, so two visits to one path became one entry and a shadowed embedded field looked like agreement. That is the worst failure mode for a test whose whole job is to notice disagreement, and it was hiding real cases. Visits are counted now, and six resource types turn out to have one: apps (id, url, lifecycle.prevent_destroy), pipelines and alerts (id), and job_runs, clusters and sql_warehouses (lifecycle.prevent_destroy). Each embeds BaseResource alongside an SDK type that declares the same json name, or two structs that each carry a Lifecycle. encoding/json serializes the shallower field and nothing else, so the second visit is a field that cannot reach the wire under that name -- and structdiff reports a change at that path twice. Recorded per resource rather than fixed: making structwalk resolve a name the way encoding/json does is a change to the walk itself, and it would move the refschema golden. The state and remote types have none of these, so the duplication is confined to the config types. Co-authored-by: Isaac
FillNonZero skipped any field whose parsed json name was "-", which includes json:"-,omitempty" -- a field encoding/json does serialize, under the name "-". Left at its zero value it was then omitted from the marshal output, so the harness saw no disagreement and passed: a false pass on exactly the shape structaccess could not resolve. It uses structaccess.IsSkippedField now, and the corpus carries a dash-named field alongside the genuinely skipped one so all four packages are held to the distinction.
Two more false-pass paths in the harness itself. render normalised every scalar to its text, so a JSON string and a JSON number of the same digits compared equal. A field tagged json:",string" puts a number on the wire as "1" while the packages expose the int behind it -- a real difference in what is at the path, reported as agreement. The JSON side is decoded with UseNumber now and both sides render with their type, so number:1 and string:1 no longer match while 1, 1.0 and 1e0 still do. flatten silently rewrote the EmbeddedSlice convention: __embed__ carries the slice on the wire while the walkers put its elements at the parent path. Following the walkers is right -- it is how the state file and the engine's paths relate -- but doing it silently meant the harness could not notice a change to the convention. The rename is reported now, and both tests assert that __embed__ is the only key it ever applies to. Co-authored-by: Isaac
denik
force-pushed
the
denik/structaccess-embed-ambiguity
branch
from
September 2, 2026 09:40
0bf400f to
0324991
Compare
denik
force-pushed
the
denik/structs-json-agreement
branch
from
September 2, 2026 09:40
b1f5982 to
842ed2d
Compare
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 #6468.
encoding/jsondecides which fields exist on the wire, under which names, at which paths. Everylibs/structspackage claims to speak that vocabulary — structwalk enumerates it, structaccess reads and writes it, structdiff reports changes in it — so a disagreement is a bug in one of them, or in the type. Nothing checked that until now.Three layers:
bundle/config/structstestfills every field of a type, marshals it, and compares the result against structwalk's leaves and structaccess Get/ValidatePath. Driven offconfig.Resourcesby reflection, so a new resource is covered automatically.bundle/direct/dresources/structs_test.godoes the same for StateType and RemoteType — the types the plan actually reads fields out of.libs/structspackage is checked against a shared corpus of shapes that are easy to get wrong (two levels of embedding, a shadowed name, a same-depth collision, a diamond, a cyclic embed, an embed behind a nil pointer). The corpus asserts its own expectations againstjson.Marshal, so it cannot teach every consumer the same wrong answer.What it found, all recorded rather than fixed here, each as a ratchet that fails when the bug is fixed:
MarshalJSONand declare none of their own, so the embedded marshaler takes over andid,url,lifecycleandpermissionsnever reach the wire. Latent:bundle validate -o jsonmarshals the dyn tree.encoding/jsonrefuses to serialize.structaccess.Setwill not descend through a nil embedded pointer, so a fieldjson.Unmarshalreaches by allocating it cannot write.anyfields and self-marshaling scalars (duration.Duration, the SDK time wrapper) are never visited by structwalk.Reverting any commit from #6467 or #6468 turns these tests red.
This pull request and its description were written by Isaac.