amadan

carlos / platform Public

edges: Task 4 fix round 1: a three-way steering read, the home map's field use mapped to its candidate test, untagged and case-folded hosts fields flagged

The steering test (renamed TestSteeringDesiredStateIgnoresOfflineMoves) reads
the fixture three ways: Hosts is pinned in each, and the desired state and the
converged records are equal across all three because desiredState reads a
host's entry only for the replica mark (stated). The edge.buildCandidates
offline_moves use maps to TestFixtureHoldIsACandidateNeverServed. The detector
names a field's key as encoding/json does (tag name, else the field name,
case-insensitive; "-" and unexported fields skipped) and also flags an untagged
BoxID; fixture package untagged proves it. No new site in real code; the
exclusion list is unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 5f8a63f60494b9b4616c528685c83dbccdf70de3 parent 96e01f8
3 files changed, +145 −33
  • internal/edgesfile/audit_test.go +53 −15
  • internal/edgesfile/testdata/audit/untagged/untagged.go +59 −0
  • internal/steering/offline_moves_test.go +33 −18
diff --git a/internal/edgesfile/audit_test.go b/internal/edgesfile/audit_test.go
index a958841f0..f6d2bfb0d 100644
--- a/internal/edgesfile/audit_test.go
+++ b/internal/edgesfile/audit_test.go
@@ -32,7 +32,9 @@ import (
// (ii) any json.Unmarshal or (*json.Decoder).Decode whose target type,
// after pointer, alias and named-type resolution (and through a
// slice, array or map element and an embedded struct), has a
-// top-level field tagged json:"hosts" or json:"box_id", in any
+// top-level field decoded from the key "hosts" or "box_id" (the tag's
+// name or, untagged, the field's own, matched case-insensitively as
+// encoding/json matches; an untagged BoxID is flagged too), in any
// package under internal/. A decode helper whose target is one of its
// own interface-typed or type-parameter-typed parameters is followed
// to its callers, so a site through such a helper is judged by the
@@ -105,8 +107,8 @@ var edgesReadInventory = map[string]string{
"internal/console.(*Server).foldedEdgeIPs -> via internal/console.(*Server).realFoldEdgeIPs": "internal/console.TestConsoleEdgeIPFoldIsUnchangedByOfflineMoves",
// internal/steering, which keeps its own decoder.
- "internal/steering.parseEdgesFileView -> json decode into internal/steering.edgesFileView": "internal/steering.TestSteeringDesiredStateIgnoresTheHold",
- "internal/steering.ConvergeDeployment -> via internal/steering.parseEdgesFileView": "internal/steering.TestSteeringDesiredStateIgnoresTheHold",
+ "internal/steering.parseEdgesFileView -> json decode into internal/steering.edgesFileView": "internal/steering.TestSteeringDesiredStateIgnoresOfflineMoves",
+ "internal/steering.ConvergeDeployment -> via internal/steering.parseEdgesFileView": "internal/steering.TestSteeringDesiredStateIgnoresOfflineMoves",
// internal/hostactors: the anonymous box-id reader.
"internal/hostactors.Directory.Referenced -> json decode into an anonymous struct": "internal/hostactors.TestReferencedIgnoresOfflineMoves",
@@ -117,7 +119,7 @@ var edgesReadInventory = map[string]string{
"internal/edge.parseEdgesFile -> json decode into internal/edge.edgesFileView": "internal/host.TestHomeMapFoldServesTheRouteNotTheHold",
"internal/edge.FoldedHome -> via internal/edge.parseEdgesFile": "internal/host.TestHomeMapFoldServesTheRouteNotTheHold",
"internal/edge.(*homeMap).refreshHosts -> via internal/edge.parseEdgesFile": "internal/host.TestHomeMapFoldServesTheRouteNotTheHold",
- "internal/edge.buildCandidates -> uses internal/edge.edgesFileView.offline_moves": "internal/edge.TestHoldingViewDecodesTheWriterFixture",
+ "internal/edge.buildCandidates -> uses internal/edge.edgesFileView.offline_moves": "internal/edge.TestFixtureHoldIsACandidateNeverServed",
// internal/host: the writer's own shape, read back.
"internal/host.seedEdgesFileGate -> json decode into internal/host.edgesFile": "internal/host.TestEdgesFileGateRoundTripsOfflineMoves",
@@ -159,7 +161,7 @@ var edgesReadExclusions = map[string]string{
// the audit loads them by path.
const auditFixtureDir = "internal/edgesfile/testdata/audit"
-var auditFixturePackages = []string{"unrelated", "boxid", "wrapper", "alias", "helper", "holds"}
+var auditFixturePackages = []string{"unrelated", "boxid", "wrapper", "alias", "helper", "holds", "untagged"}
type auditLoad struct {
root string
@@ -321,19 +323,46 @@ func edgesShaped(t types.Type, seen map[types.Type]bool) (bool, string) {
func structShaped(st *types.Struct, seen map[types.Type]bool) bool {
for i := 0; i < st.NumFields(); i++ {
f := st.Field(i)
- name, _, _ := strings.Cut(reflect.StructTag(st.Tag(i)).Get("json"), ",")
- if name == "hosts" || name == "box_id" {
- return true
+ name, tagged, decoded := jsonName(st, i)
+ if !decoded {
+ continue
}
- if f.Embedded() && name == "" {
+ if f.Embedded() && !tagged {
if ok, _ := edgesShaped(f.Type(), seen); ok {
return true
}
+ continue
+ }
+ // encoding/json matches keys to names case-insensitively, so an
+ // untagged Hosts (or a tag "HOSTS") decodes "hosts". An untagged
+ // BoxID does not decode "box_id" (the underscore), but it is a
+ // box-id reader's evident intent, so it is flagged too.
+ if strings.EqualFold(name, "hosts") || strings.EqualFold(name, "box_id") || (!tagged && f.Name() == "BoxID") {
+ return true
}
}
return false
}
+// jsonName is field i's key as encoding/json decodes it: the tag's name,
+// else the field's own name. tagged says the tag named it; decoded is false
+// for a field json never fills (tag "-", or unexported and not embedded).
+func jsonName(st *types.Struct, i int) (name string, tagged, decoded bool) {
+ f := st.Field(i)
+ tag := reflect.StructTag(st.Tag(i)).Get("json")
+ if tag == "-" {
+ return "", false, false
+ }
+ if !f.Exported() && !f.Embedded() {
+ return "", false, false
+ }
+ name, _, _ = strings.Cut(tag, ",")
+ if name != "" {
+ return name, true, true
+ }
+ return f.Name(), false, true
+}
+
// decodeTarget returns the target expression of a json.Unmarshal or a
// (*json.Decoder).Decode call, or nil.
func decodeTarget(info *types.Info, call *ast.CallExpr) ast.Expr {
@@ -550,7 +579,7 @@ func runEdgesAudit(pkgs []*packages.Package, exclusions map[string]string) *edge
if sel == nil || sel.Kind() != types.FieldVal {
return
}
- if owner, tag := fieldTag(sel.Recv(), sel.Index()); tag == "offline_moves" {
+ if owner, tag := fieldTag(sel.Recv(), sel.Index()); strings.EqualFold(tag, "offline_moves") {
add(u, readSite{key: u.name + " -> uses " + owner + ".offline_moves", pos: u.pkg.Fset.Position(n.Sel.Pos()), kind: "iv"}, top)
}
case *ast.CallExpr:
@@ -671,7 +700,7 @@ func fieldTag(recv types.Type, index []int) (string, string) {
if !ok || i >= st.NumFields() {
return "", ""
}
- name, _, _ = strings.Cut(reflect.StructTag(st.Tag(i)).Get("json"), ",")
+ name, _, _ = jsonName(st, i)
t = st.Field(i).Type()
}
return owner, name
@@ -699,11 +728,17 @@ func offlineShaped(t types.Type, seen map[types.Type]bool) bool {
t = u.Underlying()
case *types.Struct:
for i := 0; i < u.NumFields(); i++ {
- name, _, _ := strings.Cut(reflect.StructTag(u.Tag(i)).Get("json"), ",")
- if name == "offline_moves" {
- return true
+ name, tagged, decoded := jsonName(u, i)
+ if !decoded {
+ continue
+ }
+ if u.Field(i).Embedded() && !tagged {
+ if offlineShaped(u.Field(i).Type(), seen) {
+ return true
+ }
+ continue
}
- if u.Field(i).Embedded() && name == "" && offlineShaped(u.Field(i).Type(), seen) {
+ if strings.EqualFold(name, "offline_moves") {
return true
}
}
@@ -915,6 +950,9 @@ func TestEdgesFileReadDetectorFindsEveryShape(t *testing.T) {
fixtureKey("holds", "pass -> via "+fx+"holds.read"),
fixtureKey("holds", "read -> json decode into "+fx+"holds.file"),
fixtureKey("unrelated", "ReadRoster -> json decode into "+fx+"unrelated.Roster"),
+ fixtureKey("untagged", "Folded -> json decode into "+fx+"untagged.folded"),
+ fixtureKey("untagged", "ID -> json decode into "+fx+"untagged.idOnly"),
+ fixtureKey("untagged", "Plain -> json decode into "+fx+"untagged.plain"),
fixtureKey("unrelated", "Served -> edgesfile.Parse"),
fixtureKey("wrapper", "Chooser -> edgesfile.LiveSnapshot"),
fixtureKey("wrapper", "Taken -> via "+fx+"wrapper.routes"),
diff --git a/internal/edgesfile/testdata/audit/untagged/untagged.go b/internal/edgesfile/testdata/audit/untagged/untagged.go
new file mode 100644
index 000000000..69783ff60
--- /dev/null
+++ b/internal/edgesfile/testdata/audit/untagged/untagged.go
@@ -0,0 +1,59 @@
+// Package untagged is a fixture of the edges-file read audit: fields that
+// decode "hosts" without a json:"hosts" tag (encoding/json matches an
+// untagged field's own name, and every name, case-insensitively), an
+// untagged BoxID, and two fields json never fills.
+package untagged
+
+import "encoding/json"
+
+type plain struct {
+ V int
+ Hosts map[string]json.RawMessage
+}
+
+type idOnly struct {
+ BoxID string
+}
+
+type folded struct {
+ H map[string]json.RawMessage `json:"HOSTS"`
+}
+
+// skipped and unexported carry a hosts field json never decodes.
+type skipped struct {
+ Hosts map[string]string `json:"-"`
+}
+
+type unexported struct {
+ hosts map[string]string
+}
+
+// Plain decodes an untagged Hosts.
+func Plain(body []byte) int {
+ var f plain
+ _ = json.Unmarshal(body, &f)
+ return len(f.Hosts)
+}
+
+// ID decodes an untagged BoxID.
+func ID(body []byte) string {
+ var f idOnly
+ _ = json.Unmarshal(body, &f)
+ return f.BoxID
+}
+
+// Folded decodes a tag that folds to hosts.
+func Folded(body []byte) int {
+ var f folded
+ _ = json.Unmarshal(body, &f)
+ return len(f.H)
+}
+
+// Never decodes the two that json never fills: no site.
+func Never(body []byte) int {
+ var s skipped
+ var u unexported
+ _ = json.Unmarshal(body, &s)
+ _ = json.Unmarshal(body, &u)
+ return len(s.Hosts) + len(u.hosts)
+}
diff --git a/internal/steering/offline_moves_test.go b/internal/steering/offline_moves_test.go
index df64dbb14..783210965 100644
--- a/internal/steering/offline_moves_test.go
+++ b/internal/steering/offline_moves_test.go
@@ -77,36 +77,51 @@ func fixtureVariants(t *testing.T, edit map[string]any) holdVariants {
}
// parseEdgesFileView keeps its own decoder (internal/edgesfile's doc names
-// it), and since #162 it reads the per-host replica mark from Hosts: the one
-// way a host's entry changes steering. The holding entries never reach
-// Hosts, so the desired state, and what ConvergeDeployment writes, are the
-// same with and without offline_moves for a steered route and for the held
-// alias steered as a route.
-func TestSteeringDesiredStateIgnoresTheHold(t *testing.T) {
+// it), read three ways. Hosts is the served route and the certificate-only
+// entry with and without offline_moves, and the held names with the hold
+// counted as served: that Hosts set check is what pins this reader, because
+// desiredState reads a host's entry only for the replica mark (#162). A
+// plain served entry therefore changes no record, so the desired state and
+// what ConvergeDeployment writes are equal across all three variants (the
+// served variant does not discriminate here, by design), for a steered
+// route and for the held alias steered as a route.
+func TestSteeringDesiredStateIgnoresOfflineMoves(t *testing.T) {
captureLog(t)
v := fixtureVariants(t, map[string]any{"region": "eu-west-2"})
- with, err := parseEdgesFileView(v.with)
- if err != nil {
- t.Fatal(err)
- }
- var names []string
- for h := range with.Hosts {
- names = append(names, h)
+ parse := func(body []byte) (edgesFileView, []string) {
+ f, err := parseEdgesFileView(body)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var names []string
+ for h := range f.Hosts {
+ names = append(names, h)
+ }
+ sort.Strings(names)
+ return f, names
}
- sort.Strings(names)
+ with, names := parse(v.with)
if !reflect.DeepEqual(names, []string{"*.certonly.example.net", fxServed}) {
t.Fatalf("edgesFileView.Hosts = %v, want the served route and the certificate-only entry (the hold %s and its alias %s are never hosts)", names, fxHeld, fxHeldAlias)
}
- without, _ := parseEdgesFileView(v.without)
+ without, _ := parse(v.without)
if !reflect.DeepEqual(with, without) {
t.Fatalf("parse with offline_moves = %+v, without = %+v", with, without)
}
+ served, servedNames := parse(v.served)
+ if !reflect.DeepEqual(servedNames, []string{"*.certonly.example.net", fxHeld, fxHeldAlias, fxServed}) {
+ t.Fatalf("test bug: counted as served, the hold must be in Hosts: %v", servedNames)
+ }
+
routes := []registry.Route{steeredRoute(fxServed), steeredRoute(fxHeldAlias)}
- other := edgesBody(t, "betty", "5.6.7.8", "us-west-2")
dw := desiredState(routes, []edgesFileView{with}, "example.com")
if len(dw) != 3 || !reflect.DeepEqual(dw, desiredState(routes, []edgesFileView{without}, "example.com")) {
t.Fatalf("desired state with offline_moves = %+v, want three records and the same without", dw)
}
+ if !reflect.DeepEqual(dw, desiredState(routes, []edgesFileView{served}, "example.com")) {
+ t.Fatal("a plain served entry changed the desired state: desiredState now reads hosts beyond the replica mark, so this test must discriminate there too")
+ }
+ other := edgesBody(t, "betty", "5.6.7.8", "us-west-2")
converged := func(body []byte) []string {
f := newFake()
if err := ConvergeDeployment(context.Background(), f, testZone, routes, [][]byte{body, other}, "example.com"); err != nil {
@@ -117,7 +132,7 @@ func TestSteeringDesiredStateIgnoresTheHold(t *testing.T) {
cw := converged(v.with)
want := []string{"cart.example.com.|A|annie", "cart.example.com.|A|betty", "edge.example.com.|A|annie", "edge.example.com.|A|betty",
"shop.example.com.|A|annie", "shop.example.com.|A|betty"}
- if !reflect.DeepEqual(cw, want) || !reflect.DeepEqual(cw, converged(v.without)) {
- t.Fatalf("converged with offline_moves %v, want %v and the same without", cw, want)
+ if !reflect.DeepEqual(cw, want) || !reflect.DeepEqual(cw, converged(v.without)) || !reflect.DeepEqual(cw, converged(v.served)) {
+ t.Fatalf("converged with offline_moves %v, want %v and the same without and served", cw, want)
}
}