Conversation
Collaborator
Integration test reportCommit: 557ecbb
Top 3 slowest tests (at least 2 minutes):
|
denik
changed the title
denik
force-pushed
the
denik/no-same-depth-shadows
branch
from
September 13, 2026 14:44
500f482 to
8983cb4
Compare
A same-depth collision occurs when two anonymous embedded structs at the same embedding level both declare the same json name. encoding/json calls this ambiguous and serializes neither field; structaccess cannot read or write it. The test enumerates every resource type in config.Resources, walks the anonymous-embed tree breadth-first, and reports any json name that appears in two or more embeds at the same level. Three resource types have existing collisions (pipelines.id, apps.id/url, alerts.id — all from BaseResource fields colliding with identically-named SDK fields) that are listed in a documented allowlist. A new collision fails the test and must be either fixed or explicitly added to the list with an explanation. Depth-mismatch shadows (a direct named field overriding a same-named embedded field at a deeper level, as in ClusterPolicy.Definition overriding compute.CreatePolicy.Definition) are intentional and handled correctly by both encoding/json and structaccess: the shallower field wins. This test does not flag those. Co-authored-by: Isaac
…Alert
Three resource types had two anonymous embedded structs both declaring the same
json name at the same embedding depth:
- Pipeline: BaseResource.ID and CreatePipeline.Id both carry json:"id"
- App: BaseResource.{ID,URL} and apps.App.{Id,Url} carry the same names
- Alert: BaseResource.ID and AlertV2.Id both carry json:"id"
encoding/json calls same-depth collisions ambiguous and serializes neither
field, which means the field is silently unreachable from the wire format and
from structaccess.
Fix: add an explicit depth-0 field directly on the resource struct for each
colliding name. The depth-0 field wins over both embedded depth-1 declarations
in both encoding/json and structaccess. The fix does not change the json name
or the field semantics — only the declaration depth changes, from embedded to
direct.
Tests that constructed these structs with `BaseResource: resources.BaseResource{ID: "X"}`
now also set `ID: "X"` directly, because the promoted name now resolves to the
new depth-0 field instead of the embedded one.
The guard test (shadow_test.go) now has an empty allowlist, because there are
no remaining same-depth collisions in any resource type.
Co-authored-by: Isaac
TestNoSameDepthJSONShadows belongs with the other resource type invariant tests in resources_types_test.go rather than in a standalone file. The pattern is the same as TestResourceTypesZeroValueFieldsSerialize: walk all resource types and assert a structural property.
…ce type
BaseResource.ID had the same json:"id" name as several SDK types (CreatePipeline,
apps.App, AlertV2), causing same-depth ambiguity where encoding/json serialized
neither field. The fix in the previous commit added explicit depth-0 ID fields
on the three affected types; this commit makes the pattern consistent: every
resource type now declares ID directly, and BaseResource no longer carries it.
BaseResource retains ModifiedStatus, URL, and Lifecycle — these are bundle-invented
json names that will never collide with SDK fields. ID was the sole offender.
After this change:
- No resource type has a dead BaseResource.ID slot alongside a depth-0 ID.
- TestNoSameDepthJSONShadows passes with an empty allowlist by construction:
BaseResource cannot contribute an id collision because it no longer has one.
- Tests that constructed structs with BaseResource{ID: "x"} now set ID: "x"
directly on the resource struct instead.
Co-authored-by: Isaac
libs/structs/structaccess/bundle_test.go still constructed a Job with
BaseResource{ID: "jobid", URL: "joburl"}, which no longer compiles since ID
was removed from BaseResource. Move ID to the Job struct directly.
…D tag test
- Remove blank lines left over from the BaseResource{ID: "..."} replacement
- Drop the knownSameDepthCollisions allowlist (it was empty; the map is gone)
- Simplify TestNoSameDepthJSONShadows — no stale/known logic needed
- Update sameDepthCollisions to skip collisions already shadowed by a depth-0
direct field: App.URL at depth 0 correctly shadows the same-depth collision
between BaseResource.URL and apps.App.Url at depth 1, so encoding/json and
structaccess both work correctly
- Add TestResourceIDFieldTags: asserts every BaseResource-embedding resource
type has ID declared directly at depth 0 with the right tags
(json:"id,omitempty" bundle:"readonly")
denik
force-pushed
the
denik/no-same-depth-shadows
branch
from
September 14, 2026 14:38
b298b16 to
19ba275
Compare
- Remove the stale knownSameDepthCollisions var - Convert NumField/Field loops to Type.Fields() iteration (modernize linter) - gofmt three test files that had stale blank lines
- Fix last NumField/Field loop in TestResourceIDFieldTags - Remove stale shadow comments from pipeline.go, apps.go, alerts.go (BaseResource no longer has ID, so the "shadows" description is outdated)
Replace the structural hasBaseResource check with a named constant for the one resource group that has no user-facing ID (internal_immutable_snapshots). Explicit is better than implicit: a new resource type that somehow lacks BaseResource but still needs an ID will now fail the test rather than silently passing.
denik
force-pushed
the
denik/no-same-depth-shadows
branch
from
September 14, 2026 20:49
7a6fdb4 to
557ecbb
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
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.
Three resource types had an unresolvable
json:"id"collision: bothBaseResourceand the embedded SDK type (CreatePipeline,apps.App,AlertV2) declared it at the same embedding depth.encoding/jsoncalls that ambiguous and serializes neither — the field was silently unreachable.Fix: remove
IDfromBaseResourceand declare it directly on each resource type. Every resource now has a depth-0IDfield that wins unambiguously.BaseResourcekeepsURL,ModifiedStatus, andLifecycle— bundle-invented json names that will never collide with SDK fields.Guard:
TestNoSameDepthJSONShadowsinresources_types_test.gowalks every resource type breadth-first and fails if any json name is claimed by two anonymous embeds at the same depth without a depth-0 direct field resolving it.TestResourceIDFieldTagsasserts every resource hasIDat depth 0 withjson:"id,omitempty"andbundle:"readonly".This pull request and its description were written by Isaac.