From ce0223006b7ef3daf543316f9a3268f42976253b Mon Sep 17 00:00:00 2001 From: Suriyakumarvijayanayagam Date: Mon, 7 Sep 2026 12:17:49 +0530 Subject: [PATCH] References are immutable, because clients now store them 012 turned three descriptive columns into identifiers other systems keep: in agent.json on a shop counter, in a saved URL, in a scheduled report. All three were already treated as stable and none of it was enforced. - clients.slug is an MQTT topic segment the broker ACL is written against. Rename one and that tenant's whole estate is silently refused by the broker, with no way to tell the agents. - sites.slug is what a shop PC calls itself - agent.json holds "site_id": "chennai", never the uuid. A rename orphans the PC from the shop it is standing in. - site_cameras.camera_id lands in visits.camera_id, which is text and not a foreign key. A rename orphans every visit already attributed to the old name: the footfall is still there and no longer joins to a camera. This was half-enforced in handleUpdateCamera and nowhere else, which is the shape of a rule that holds until somebody adds a second write path. - visitors.number is assigned once from the tenant's counter and read back as V-42. A trigger, not a CHECK: a CHECK cannot see the old row and the rule is about the transition. The DISPLAY name is deliberately not frozen - "TeNext Chennai", "Front door" - it is what a person reads, nothing keys on it, and a system that cannot fix a typo in a shop's name has confused the two. Also records why the uuid stays where a slug would do. The length was never the problem; needing it was, and that is fixed. Replacing it would touch eight foreign keys on a live database to shorten a field clients are already told not to use, and a sequential id would make any future tenancy hole walkable by counting. It is NOT because ids must be minted offline - sites, visitors and visits are all created server-side with a database in hand, and claiming otherwise would defend the status quo rather than explain it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HViLj9gYNRtSr7YVZmW5sn --- API.md | 9 ++ CLAUDE.md | 47 +++++++++ .../internal/store/api_immutable_live_test.go | 57 +++++++++++ .../013_references_are_immutable.sql | 97 +++++++++++++++++++ 4 files changed, 210 insertions(+) create mode 100644 server/internal/store/api_immutable_live_test.go create mode 100644 server/migrations/013_references_are_immutable.sql diff --git a/API.md b/API.md index 22a3c9d..005b44d 100644 --- a/API.md +++ b/API.md @@ -43,6 +43,15 @@ An unknown reference in a **path** is `404`; an unknown one in a **query filter* is `400`, because the collection itself was fine and it was the filter that was wrong. +**A reference never changes.** A shop's slug, a camera's id and a customer's +number are immutable in the database, so it is safe to store one — in a saved +URL, a config file or a scheduled report. The **display name** beside it +(`"TeNext Chennai"`, `"Front door"`) is free to change and should be; do not key +on it. + +The uuid is still returned everywhere and still works. Use it if you want a key +you never have to think about; use the reference when a person will read it. + --- ## 1. Signing in diff --git a/CLAUDE.md b/CLAUDE.md index 6fd3b4e..29cd8fa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2047,6 +2047,53 @@ the engine's own diagnostic dashboard. human name. The prop carrying it is `customerRef`, not `ref` — React reserves that name, so it would never have reached the component. +### Why the uuid stays, when the slug would do + +Asked directly: `site_id` is 36 characters, why not a small number? + +The honest answer is that **the length was never the problem — needing it was**, +and that is already fixed: `?site=chennai` and `/api/sites/chennai/check` work, +and the shop PC has always identified itself by slug (`agent.json` holds +`"site_id": "chennai"`, never the uuid). The uuid in a *response* is the stable +key for a client that wants to store one. + +Two reasons not to replace it, and one reason that is NOT among them: + +- **Enumeration.** `/api/sites/3/check` makes any future tenancy hole walkable + by counting; a uuid makes it require a leak first. Every handler scopes by the + session's client today, so this is defence in depth rather than the control — + but this database holds biometric templates, and defence in depth is the point + of a second layer. +- **The payoff is now zero.** Eight tables carry a foreign key to `sites(id)`, + against a live database, to make a field shorter that a client is already told + not to use. +- **NOT because ids must be minted offline.** Sites, visitors and visits are all + created server-side with a database in hand. That argument holds for the + agent's `event_id` — which is derived precisely so it needs no coordination — + and it does not hold here; claiming it would be a defence of the status quo + rather than a reason for it. + +What DID need fixing is that the references were only stable by accident. +Migration 013 makes `clients.slug`, `sites.slug`, `site_cameras.camera_id` and +`visitors.number` immutable in the database, because 012 turned them from +descriptive columns into identifiers other systems store: + +- `clients.slug` is an MQTT topic segment the broker ACL is written against. + Rename one and that tenant's whole estate is silently refused by the broker, + with no way to tell the agents. +- `sites.slug` is what a shop PC calls itself. A rename orphans the PC from the + shop it is standing in. +- `site_cameras.camera_id` lands in `visits.camera_id`, which is text and not a + foreign key. A rename orphans every visit already attributed to the old name: + the footfall is still there and no longer joins to a camera. This was + half-enforced in `handleUpdateCamera` and nowhere else — the shape of a rule + that holds until somebody adds a second write path. + +A trigger rather than a CHECK, because a CHECK cannot see the old row and the +rule is about the transition. **The display name is deliberately NOT frozen** — +"TeNext Chennai", "Front door" — it is what a person reads, nothing keys on it, +and a system that cannot fix a typo in a shop's name has confused the two. + ### Three uuids on one arrival, three different answers Asked of the row the feed actually returns, and they do not get the same reply: diff --git a/server/internal/store/api_immutable_live_test.go b/server/internal/store/api_immutable_live_test.go new file mode 100644 index 0000000..86ae74f --- /dev/null +++ b/server/internal/store/api_immutable_live_test.go @@ -0,0 +1,57 @@ +package store + +import ( + "context" + "strings" + "testing" +) + +// A reference clients are told to use must not be able to change underneath +// them. +// +// 012 turned three descriptive columns into IDENTIFIERS other people store: in +// agent.json on a shop counter, in a saved URL, in a scheduled report. All +// three were already treated as stable and none of it was enforced - the +// camera case was half-enforced in one handler and nowhere else, which is the +// shape of a rule that holds until somebody adds a second write path. +// +// Only a real database can test this: the rule is a trigger, and an in-memory +// fake would happily agree with any implementation. +func TestLiveAReferenceCannotBeRenamed(t *testing.T) { + st := liveStore(t) + ctx := context.Background() + site := seedAgentSite(t, st, "frozen-"+stamp()) + + if _, err := st.pool.Exec(ctx, ` + INSERT INTO site_cameras (client_id, site_id, camera_id, label, host) + VALUES ($1::uuid, $2::uuid, 'Office1', 'Front door', '10.0.0.5')`, + site.ClientID, site.SiteID); err != nil { + t.Fatal(err) + } + + for _, c := range []struct{ what, sql string }{ + {"a shop's slug", `UPDATE sites SET slug = 'moved' WHERE id = $1::uuid`}, + {"a camera's id", `UPDATE site_cameras SET camera_id = 'Office2' WHERE site_id = $1::uuid`}, + } { + _, err := st.pool.Exec(ctx, c.sql, site.SiteID) + if err == nil { + t.Fatalf("%s was renamed - it is a reference other systems store", c.what) + } + if !strings.Contains(err.Error(), "cannot be changed") { + t.Fatalf("%s: unexpected error %v", c.what, err) + } + } + + // The DISPLAY name is not frozen and must not be. It is what a person + // reads, nothing keys on it, and a system that cannot fix a typo in a + // shop's name has confused the two. + if _, err := st.pool.Exec(ctx, + `UPDATE sites SET name = 'Renamed Shop' WHERE id = $1::uuid`, site.SiteID); err != nil { + t.Fatalf("a shop's display name must stay editable: %v", err) + } + if _, err := st.pool.Exec(ctx, + `UPDATE site_cameras SET label = 'Back door' WHERE site_id = $1::uuid`, + site.SiteID); err != nil { + t.Fatalf("a camera's label must stay editable: %v", err) + } +} diff --git a/server/migrations/013_references_are_immutable.sql b/server/migrations/013_references_are_immutable.sql new file mode 100644 index 0000000..b1ef802 --- /dev/null +++ b/server/migrations/013_references_are_immutable.sql @@ -0,0 +1,97 @@ +-- The references clients are now told to use must not be able to change. +-- +-- 012 made `chennai`, `Office1` and `V-42` first-class: every route that takes +-- an id takes one of these instead, and the documentation tells a client to +-- prefer them. That turns three columns which were merely descriptive into +-- IDENTIFIERS other people store - in an agent's config file on a shop counter, +-- in a saved URL, in a report somebody scheduled. +-- +-- All three were already treated as stable, and none of it was enforced: +-- +-- * `clients.slug` appears in MQTT topics as bv/./... and the +-- broker ACL is written against it. Renaming one silently stops that +-- tenant's estate from being able to publish, and the agents cannot be told +-- - they would simply be refused by the broker. +-- * `sites.slug` is what a shop PC calls itself: agent.json holds +-- "site_id": "chennai". A rename orphans the PC from the shop it is +-- standing in. +-- * `site_cameras.camera_id` is what lands in `visits.camera_id`, which is a +-- text column and not a foreign key. Renaming it orphans every visit +-- already attributed to the old name - the footfall is still there and no +-- longer joins to a camera. +-- +-- The camera case was half-enforced in one handler (`handleUpdateCamera` nils +-- CameraID before saving) and nowhere else, which is the shape of a rule that +-- holds until somebody adds a second write path. This is the backstop, in the +-- one place every write has to go through. +-- +-- Deliberately a trigger and not a CHECK: a CHECK cannot see the old row, and +-- the rule is about the transition, not the value. +-- +-- Note what this does NOT freeze. `name` - "TeNext Chennai", "Front door" - is +-- free to change and always should be: it is what a person reads, it is not what +-- anything keys on, and conflating the two is how a system ends up unable to fix +-- a typo in a shop's name. + +BEGIN; + +CREATE OR REPLACE FUNCTION reference_is_immutable() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + RAISE EXCEPTION + '% is a public reference and cannot be changed (% -> %); ' + 'create a new row instead, or change the display name', + TG_ARGV[0], OLD.slug, NEW.slug + USING ERRCODE = 'check_violation'; +END; +$$; + +-- camera_id lives in its own function only because the column is named +-- differently; splitting it keeps the message honest about which value moved. +CREATE OR REPLACE FUNCTION camera_reference_is_immutable() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + RAISE EXCEPTION + 'camera_id is a public reference and cannot be changed (% -> %); ' + 'every visit already recorded names the old one. Change the label instead', + OLD.camera_id, NEW.camera_id + USING ERRCODE = 'check_violation'; +END; +$$; + +DROP TRIGGER IF EXISTS clients_slug_immutable ON clients; +CREATE TRIGGER clients_slug_immutable + BEFORE UPDATE OF slug ON clients + FOR EACH ROW WHEN (OLD.slug IS DISTINCT FROM NEW.slug) + EXECUTE FUNCTION reference_is_immutable('clients.slug'); + +DROP TRIGGER IF EXISTS sites_slug_immutable ON sites; +CREATE TRIGGER sites_slug_immutable + BEFORE UPDATE OF slug ON sites + FOR EACH ROW WHEN (OLD.slug IS DISTINCT FROM NEW.slug) + EXECUTE FUNCTION reference_is_immutable('sites.slug'); + +DROP TRIGGER IF EXISTS site_cameras_id_immutable ON site_cameras; +CREATE TRIGGER site_cameras_id_immutable + BEFORE UPDATE OF camera_id ON site_cameras + FOR EACH ROW WHEN (OLD.camera_id IS DISTINCT FROM NEW.camera_id) + EXECUTE FUNCTION camera_reference_is_immutable(); + +-- A visitor's number is assigned once from the tenant's counter and read back +-- as V-42. Nothing writes it after the insert; this says so. +DROP TRIGGER IF EXISTS visitors_number_immutable ON visitors; +CREATE OR REPLACE FUNCTION visitor_number_is_immutable() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + RAISE EXCEPTION + 'visitors.number is a public reference and cannot be changed (% -> %)', + OLD.number, NEW.number + USING ERRCODE = 'check_violation'; +END; +$$; +CREATE TRIGGER visitors_number_immutable + BEFORE UPDATE OF number ON visitors + FOR EACH ROW WHEN (OLD.number IS DISTINCT FROM NEW.number) + EXECUTE FUNCTION visitor_number_is_immutable(); + +COMMIT;