From b255dceaa8189bbedec6ceabdd1679aff77554e6 Mon Sep 17 00:00:00 2001 From: dborup Date: Sat, 18 Jul 2026 21:52:23 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20flip=20geo=5Ffilter=20node-list=20defaul?= =?UTF-8?q?t=20polarity=20=E2=80=94=20safe-by-default=20for=20upgraders?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7cc5aae's GeoFilterAppliesToNodeList had the polarity backwards: it defaulted to false, meaning geo_filter would NOT apply to the node list unless a deployment explicitly opted in — but every existing deployment that already had geo_filter configured (predating this field entirely) would decode that field as false too, since it's simply absent from their config.json. They'd silently get the new "show everything" behavior on upgrade with no way back short of reading release notes and adding a new config key — exactly the breaking change the opt-in gate was meant to prevent, just one layer deeper. Renamed to GeoFilterExemptNodeList (still defaults false): geo_filter now applies to the node list by DEFAULT when configured, matching the long-standing #730 behavior exactly, for every deployment predating this field. Only a deployment that explicitly sets geoFilterExemptNodeList=true (i.e. one adopting geo_filter fresh, purely for foreign_advert classification/analytics) gets the non-filtering default. ?geoFilter=0/1 still overrides either default for a single request. Co-Authored-By: Claude Sonnet 5 --- cmd/server/config.go | 19 ++--- cmd/server/nodes_geofilter_optin_test.go | 88 +++++++++++++----------- cmd/server/openapi.go | 2 +- cmd/server/routes.go | 31 +++++---- 4 files changed, 76 insertions(+), 64 deletions(-) diff --git a/cmd/server/config.go b/cmd/server/config.go index cd767c54..e259a31e 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -142,14 +142,17 @@ type Config struct { // GOMEMLIMIT environment variable, when set, takes precedence. Runtime *RuntimeConfig `json:"runtime,omitempty"` GeoFilter *GeoFilterConfig `json:"geo_filter,omitempty"` - // GeoFilterAppliesToNodeList restores the pre-opt-in behavior where - // configuring geo_filter alone made GET /api/nodes exclude out-of- - // polygon nodes by default (no ?geoFilter=1 needed). Off by default — - // deployments that already relied on the old always-on filtering - // (added under #730) can set this to keep it; new deployments get the - // non-surprising default of geo_filter not silently changing what the - // node list/map returns. - GeoFilterAppliesToNodeList bool `json:"geoFilterAppliesToNodeList,omitempty"` + // GeoFilterExemptNodeList opts a deployment OUT of the long-standing + // #730 behavior where configuring geo_filter also makes GET /api/nodes + // (and therefore the live map) exclude out-of-polygon nodes by + // default. Off by default — an existing config.json with geo_filter + // already set (i.e. every deployment predating this field) keeps + // getting exactly the filtering it always had; a deployment that wants + // geo_filter purely for foreign_advert classification/analytics, + // without also decluttering the node list/map, sets this to true. + // Per-request ?geoFilter=0 / ?geoFilter=1 overrides either default for + // that one call. + GeoFilterExemptNodeList bool `json:"geoFilterExemptNodeList,omitempty"` Areas map[string]AreaEntry `json:"areas,omitempty"` diff --git a/cmd/server/nodes_geofilter_optin_test.go b/cmd/server/nodes_geofilter_optin_test.go index cbdd310b..e2f54bd8 100644 --- a/cmd/server/nodes_geofilter_optin_test.go +++ b/cmd/server/nodes_geofilter_optin_test.go @@ -6,12 +6,14 @@ import ( "testing" ) -// Regression test: configuring geo_filter alone must not change what -// GET /api/nodes returns. Before this fix, setting geo_filter silently -// hid every node outside the polygon that hadn't yet been re-tagged -// foreign_advert=1 by the ingestor (which only happens on that node's -// next ADVERT) — including the live map, which lists straight off this -// endpoint. The filter is now opt-in via ?geoFilter=1. +// Regression test: geo_filter's node-list-declutter behavior (#730) is +// preserved by default for every deployment that already had geo_filter +// configured — GeoFilterExemptNodeList (default false) only lets a NEW +// adopter of geo_filter (using it purely for foreign_advert classification/ +// analytics) opt OUT of also hiding out-of-polygon nodes from +// GET /api/nodes (and therefore the live map, which lists straight off +// this endpoint). Per-request ?geoFilter=0/1 overrides either default for +// a single call. func TestHandleNodes_GeoFilterExcludedByDefault(t *testing.T) { apiKey := "a-strong-api-key-for-testing" srv, router, _ := setupGeoFilterServer(t, apiKey) @@ -40,30 +42,19 @@ func TestHandleNodes_GeoFilterExcludedByDefault(t *testing.T) { } return out } - - t.Run("default request returns every node regardless of geo_filter", func(t *testing.T) { - req := httptest.NewRequest("GET", "/api/nodes?limit=50", nil) + get := func(qs string) map[string]bool { + t.Helper() + req := httptest.NewRequest("GET", "/api/nodes?limit=50"+qs, nil) w := httptest.NewRecorder() router.ServeHTTP(w, req) if w.Code != 200 { t.Fatalf("expected 200, got %d", w.Code) } - got := names(w) - for _, want := range []string{"InsideNode", "OutsideUntagged", "OutsideTagged"} { - if !got[want] { - t.Errorf("expected %s in default (unfiltered) response, got %v", want, got) - } - } - }) + return names(w) + } - t.Run("geoFilter=1 excludes untagged out-of-polygon nodes but keeps foreign-tagged ones", func(t *testing.T) { - req := httptest.NewRequest("GET", "/api/nodes?limit=50&geoFilter=1", nil) - w := httptest.NewRecorder() - router.ServeHTTP(w, req) - if w.Code != 200 { - t.Fatalf("expected 200, got %d", w.Code) - } - got := names(w) + t.Run("default request preserves the long-standing #730 filtering (deployments predating this field see no change)", func(t *testing.T) { + got := get("") if !got["InsideNode"] { t.Error("expected InsideNode (within polygon) to be present") } @@ -71,29 +62,44 @@ func TestHandleNodes_GeoFilterExcludedByDefault(t *testing.T) { t.Error("expected OutsideTagged (foreign_advert=1) to be present even though it's outside the polygon — #730") } if got["OutsideUntagged"] { - t.Error("expected OutsideUntagged (outside polygon, not yet foreign-tagged) to be excluded when geoFilter=1") + t.Error("expected OutsideUntagged (outside polygon, not foreign-tagged) to be excluded by default, matching the pre-existing #730 behavior") } }) - t.Run("GeoFilterAppliesToNodeList=true restores the pre-opt-in always-on behavior without needing ?geoFilter=1", func(t *testing.T) { - srv.cfg.GeoFilterAppliesToNodeList = true - defer func() { srv.cfg.GeoFilterAppliesToNodeList = false }() + t.Run("geoFilter=0 forces the filter off for a single request", func(t *testing.T) { + got := get("&geoFilter=0") + if !got["OutsideUntagged"] { + t.Error("expected OutsideUntagged to be present when explicitly overriding with geoFilter=0") + } + }) - req := httptest.NewRequest("GET", "/api/nodes?limit=50", nil) - w := httptest.NewRecorder() - router.ServeHTTP(w, req) - if w.Code != 200 { - t.Fatalf("expected 200, got %d", w.Code) - } - got := names(w) - if !got["InsideNode"] { - t.Error("expected InsideNode (within polygon) to be present") - } - if !got["OutsideTagged"] { - t.Error("expected OutsideTagged (foreign_advert=1) to be present even though it's outside the polygon — #730") + t.Run("geoFilter=1 is a no-op restating the default (still excludes untagged, keeps foreign-tagged)", func(t *testing.T) { + got := get("&geoFilter=1") + if !got["InsideNode"] || !got["OutsideTagged"] { + t.Error("expected InsideNode and OutsideTagged to remain present") } if got["OutsideUntagged"] { - t.Error("expected OutsideUntagged to be excluded by default when GeoFilterAppliesToNodeList=true, without needing ?geoFilter=1") + t.Error("expected OutsideUntagged to still be excluded with geoFilter=1") + } + }) + + t.Run("GeoFilterExemptNodeList=true makes the default request return everything, without needing ?geoFilter=0", func(t *testing.T) { + srv.cfg.GeoFilterExemptNodeList = true + defer func() { srv.cfg.GeoFilterExemptNodeList = false }() + + got := get("") + if !got["InsideNode"] || !got["OutsideTagged"] || !got["OutsideUntagged"] { + t.Errorf("expected every node present when GeoFilterExemptNodeList=true, got %v", got) + } + }) + + t.Run("geoFilter=1 still overrides GeoFilterExemptNodeList=true for a single request", func(t *testing.T) { + srv.cfg.GeoFilterExemptNodeList = true + defer func() { srv.cfg.GeoFilterExemptNodeList = false }() + + got := get("&geoFilter=1") + if got["OutsideUntagged"] { + t.Error("expected OutsideUntagged to be excluded when geoFilter=1 overrides an exempt deployment") } }) } diff --git a/cmd/server/openapi.go b/cmd/server/openapi.go index 36c12843..a1f335d2 100644 --- a/cmd/server/openapi.go +++ b/cmd/server/openapi.go @@ -80,7 +80,7 @@ func routeDescriptions() map[string]routeMeta { QueryParams: []paramMeta{ {Name: "role", Description: "Filter by node role", Type: "string"}, {Name: "status", Description: "Filter by status (active/stale/offline)", Type: "string"}, - {Name: "geoFilter", Description: "Set to \"1\" to exclude nodes outside the configured geo_filter (unless foreign_advert-tagged). Opt-in — omitting it returns every node regardless of geo_filter, unless the deployment's config.json sets geoFilterAppliesToNodeList=true to restore the pre-opt-in always-on behavior.", Type: "string"}, + {Name: "geoFilter", Description: "Overrides the deployment's geo_filter node-list default for this one request: \"1\" excludes nodes outside the configured geo_filter (unless foreign_advert-tagged), \"0\" returns every node regardless. Omitting it uses the deployment default — geo_filter applies to the node list unless config.json sets geoFilterExemptNodeList=true.", Type: "string"}, }}, "GET /api/nodes/search": {Summary: "Search nodes", Description: "Search nodes by name or public key prefix.", Tag: "nodes", QueryParams: []paramMeta{{Name: "q", Description: "Search query", Type: "string", Required: true}}}, "GET /api/nodes/bulk-health": {Summary: "Bulk node health", Description: "Returns health status for all nodes in one call.", Tag: "nodes"}, diff --git a/cmd/server/routes.go b/cmd/server/routes.go index 513a7773..168ed63c 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -1426,20 +1426,23 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) { } } } - // Opt-in by default (?geoFilter=1), unless the deployment has - // GeoFilterAppliesToNodeList set to restore the pre-existing always-on - // behavior. Configuring geo_filter alone must not, by default, change - // what /api/nodes (and therefore the live map, which lists straight off - // this endpoint) returns — any node outside the polygon and not YET - // foreign_advert-tagged (which only happens on that node's next ADVERT - // after geo_filter was configured) would otherwise silently vanish from - // every view the moment geo_filter was set, with no per-request way to - // see them anyway. A deployment that intentionally relies on the old - // #730 declutter behavior can set GeoFilterAppliesToNodeList to keep - // it. geo_filter's own ingestor-side tagging (and the explicit - // prune-geo-filter admin flow) are unaffected either way — this only - // gates the passive declutter view. - applyGeoFilter := s.cfg.GeoFilterAppliesToNodeList || q.Get("geoFilter") == "1" + // geo_filter applies to the node list (and therefore the live map, + // which lists straight off this endpoint) BY DEFAULT when configured — + // the long-standing #730 declutter behavior. Every deployment that + // already had geo_filter set before GeoFilterExemptNodeList existed + // keeps getting exactly that, unchanged: the field is absent from + // their config.json, which decodes to false, which preserves the + // default. A deployment that wants geo_filter purely for + // foreign_advert classification/analytics — without also hiding + // out-of-polygon nodes from the map — opts out via + // GeoFilterExemptNodeList. ?geoFilter=0 / ?geoFilter=1 overrides + // either default for a single request. geo_filter's own ingestor-side + // tagging (and the explicit prune-geo-filter admin flow) are + // unaffected either way — this only gates the passive declutter view. + applyGeoFilter := !s.cfg.GeoFilterExemptNodeList + if v := q.Get("geoFilter"); v != "" { + applyGeoFilter = v == "1" + } if s.cfg.GeoFilter != nil && applyGeoFilter { filtered := nodes[:0] for _, node := range nodes {