diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 4163ae4..b9ec408 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -12,4 +12,4 @@ permissions: contents: read jobs: analyze: - uses: mapgie/android-Actions/.github/workflows/codeql.yml@1a6f42d3abb98e085e01ec0803a74e0902b55ff3 + uses: mapgie/android-Actions/.github/workflows/codeql.yml@55fd01778230063aca02eb208c0a73e33541c497 diff --git a/LESSONS.md b/LESSONS.md index 89af38e..c83784b 100644 --- a/LESSONS.md +++ b/LESSONS.md @@ -1592,3 +1592,31 @@ Things that came with it: - **Diff the list, don't replace it.** Saving a chore sends the wanted ids; `nfcTagChanges` (pure, tested) turns that into attach, detach, and "taken by another chore", checked whole before any write so a refusal never half-applies. + +--- + +## 69. An idempotent schema file must also be safe in pieces, and read as safe + +#58 made `schema.sql` idempotent with `DROP POLICY IF EXISTS` before each +`CREATE POLICY`. Correct, but Supabase's SQL Editor flags any query with `DROP` +as destructive, so a person pasting a section to apply one change is asked to +confirm something that sounds like data loss. Reasonably, they hesitate. + +Then the section they did paste (the `nfc_tags` block from #68) failed: its +one-time copy read `tags.nfc_id`, a column an *earlier* section adds, and their +database had never had that section applied. Run whole, the file was fine; run +in pieces, it assumed state it did not create. + +Rules that fall out: +- **Create only what is missing instead of drop-and-recreate.** Policies check + `pg_policies`; a CHECK is replaced only when `pg_constraint` shows it missing + or lacking the newest value (`pg_get_constraintdef(oid) LIKE '%''value''%'`). + The re-run is then a true no-op, and the file has no `DROP POLICY` to alarm + anyone. `SchemaSyncTest` fails if one comes back. +- **A section that reads a column checks that the column exists**, and knows what + the data meant before it did (here: `tag_id` was the sticker id, a UUID meant + no tag). People apply schema files by the section, not only by the file. +- **Test DDL against a real Postgres when one is to hand.** The container has + Postgres 16 binaries (`/usr/lib/postgresql/16/bin`); `initdb` under the + `postgres` user in its home dir (the scratchpad path is too long for a socket) + ran the fresh, old-shape, partial and re-run cases in seconds. diff --git a/app/src/test/java/com/mapgie/dash/data/model/SchemaSyncTest.kt b/app/src/test/java/com/mapgie/dash/data/model/SchemaSyncTest.kt index 89589d1..b3a4687 100644 --- a/app/src/test/java/com/mapgie/dash/data/model/SchemaSyncTest.kt +++ b/app/src/test/java/com/mapgie/dash/data/model/SchemaSyncTest.kt @@ -99,6 +99,29 @@ class SchemaSyncTest { ) } + @Test + fun `the nfc_tags carry-across works on a database that never got tags nfc_id`() { + // Run on its own, the section can meet a tags table without nfc_id; the + // copy must check for the column and fall back to tag_id. + assertTrue( + "supabase/schema.sql must copy nfc_tags from tag_id when tags.nfc_id is missing", + Regex("""INSERT INTO nfc_tags[^;]*SELECT tag_id, label, tag_id FROM tags""", RegexOption.IGNORE_CASE) + .containsMatchIn(schema), + ) + } + + @Test + fun `applying the schema never drops a policy, a table or a column`() { + // Supabase's SQL Editor flags DROP as destructive. Policies are created + // only when missing (pg_policies), so a re-run drops nothing. + for (statement in listOf("DROP POLICY", "DROP TABLE", "DROP COLUMN")) { + assertTrue( + "supabase/schema.sql contains $statement; create only what is missing instead", + !Regex("""(?m)^[^-\n]*\b$statement\b""", RegexOption.IGNORE_CASE).containsMatchIn(schema), + ) + } + } + @Test fun `every table is granted to the anon role for the Data API`() { // From 2026 Supabase stops auto-exposing public tables to the Data API, so diff --git a/changelog/unreleased/schema-no-drops.json b/changelog/unreleased/schema-no-drops.json new file mode 100644 index 0000000..08813c7 --- /dev/null +++ b/changelog/unreleased/schema-no-drops.json @@ -0,0 +1,7 @@ +{ + "bump": "patch", + "fixed": [ + "The Supabase schema no longer drops and recreates its access policies on every run, so the SQL Editor stops warning that it is destructive.", + "Applying only the saved NFC tags part of the schema now works on a project that never had the earlier NFC tag column." + ] +} diff --git a/supabase/README.md b/supabase/README.md index d59f5a7..5c06da1 100644 --- a/supabase/README.md +++ b/supabase/README.md @@ -3,10 +3,12 @@ `schema.sql` is the single source of truth for the shared Supabase project the Android app and the taskDash web app talk to. It creates the tables (`owners`, `tags`, `nfc_tags`, `scans`, `todos`), their policies, grants and constraints. Every statement -is **idempotent** (tables/indexes use `IF NOT EXISTS`, policies are dropped then -created, constraints dropped then added, grants re-granted), so applying the whole -file to a database that already has some or all of it is safe and never touches row -data. +is **idempotent** (tables/indexes use `IF NOT EXISTS`, policies are created only +when missing, a CHECK is replaced only when it is missing or out of date, grants +re-granted), so applying the whole file to a database that already has some or all +of it is safe. No statement drops a table, a column, a row or a policy. The only row +writes are the one-time fills that carry a chore's NFC tag across (see the file). +Each section also works when run on its own. ## Applying it by hand diff --git a/supabase/schema.sql b/supabase/schema.sql index a21d47b..195ab4e 100644 --- a/supabase/schema.sql +++ b/supabase/schema.sql @@ -3,8 +3,10 @@ -- The whole tables + app schema, and the single source of truth for it. Every -- statement is idempotent (safe to run any number of times against a database -- that already has some or all of it): tables and indexes use IF NOT EXISTS, --- policies are dropped-then-created, constraints are dropped-then-added, grants --- re-grant. So you can apply it two ways, and both are safe: +-- policies are created only when missing, a CHECK is replaced only when it is +-- missing or out of date, grants re-grant. No statement drops a table, a +-- column, a row or a policy. Any section can also be run on its own. So you +-- can apply it two ways, and both are safe: -- • by hand: paste the whole file into the SQL Editor (Project → SQL Editor → -- New query) and Run; -- • automatically: the "Deploy Supabase schema" GitHub Action @@ -32,14 +34,22 @@ CREATE TABLE IF NOT EXISTS owners ( ALTER TABLE owners ENABLE ROW LEVEL SECURITY; -DROP POLICY IF EXISTS "anon read owners" ON owners; -CREATE POLICY "anon read owners" ON owners FOR SELECT TO anon USING (true); -DROP POLICY IF EXISTS "anon insert owners" ON owners; -CREATE POLICY "anon insert owners" ON owners FOR INSERT TO anon WITH CHECK (true); -DROP POLICY IF EXISTS "anon update owners" ON owners; -CREATE POLICY "anon update owners" ON owners FOR UPDATE TO anon USING (true) WITH CHECK (true); -DROP POLICY IF EXISTS "anon delete owners" ON owners; -CREATE POLICY "anon delete owners" ON owners FOR DELETE TO anon USING (true); +-- Created only when missing, so a re-run drops nothing (LESSONS #69). +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'owners' AND policyname = 'anon read owners') THEN + CREATE POLICY "anon read owners" ON owners FOR SELECT TO anon USING (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'owners' AND policyname = 'anon insert owners') THEN + CREATE POLICY "anon insert owners" ON owners FOR INSERT TO anon WITH CHECK (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'owners' AND policyname = 'anon update owners') THEN + CREATE POLICY "anon update owners" ON owners FOR UPDATE TO anon USING (true) WITH CHECK (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'owners' AND policyname = 'anon delete owners') THEN + CREATE POLICY "anon delete owners" ON owners FOR DELETE TO anon USING (true); + END IF; +END $$; -- Add one row per household member, e.g.: -- INSERT INTO owners (handle) VALUES ('alex'), ('sam'); @@ -97,14 +107,22 @@ CREATE INDEX IF NOT EXISTS tags_archived_idx ON tags(archived_at); ALTER TABLE tags ENABLE ROW LEVEL SECURITY; -DROP POLICY IF EXISTS "anon read tags" ON tags; -CREATE POLICY "anon read tags" ON tags FOR SELECT TO anon USING (true); -DROP POLICY IF EXISTS "anon insert tags" ON tags; -CREATE POLICY "anon insert tags" ON tags FOR INSERT TO anon WITH CHECK (true); -DROP POLICY IF EXISTS "anon update tags" ON tags; -CREATE POLICY "anon update tags" ON tags FOR UPDATE TO anon USING (true) WITH CHECK (true); -DROP POLICY IF EXISTS "anon delete tags" ON tags; -CREATE POLICY "anon delete tags" ON tags FOR DELETE TO anon USING (true); +-- Created only when missing, so a re-run drops nothing (LESSONS #69). +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'tags' AND policyname = 'anon read tags') THEN + CREATE POLICY "anon read tags" ON tags FOR SELECT TO anon USING (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'tags' AND policyname = 'anon insert tags') THEN + CREATE POLICY "anon insert tags" ON tags FOR INSERT TO anon WITH CHECK (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'tags' AND policyname = 'anon update tags') THEN + CREATE POLICY "anon update tags" ON tags FOR UPDATE TO anon USING (true) WITH CHECK (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'tags' AND policyname = 'anon delete tags') THEN + CREATE POLICY "anon delete tags" ON tags FOR DELETE TO anon USING (true); + END IF; +END $$; -- ───────────────────────────────────────────────────────────────────────── -- nfc_tags — every NFC tag the app has saved, one row per physical tag. @@ -116,8 +134,9 @@ CREATE POLICY "anon delete tags" ON tags FOR DELETE TO anon USING (true); -- -- This replaces tags.nfc_id, which held one tag per chore. That column stays, -- untouched, for app versions that still read it. The table is created, and --- filled from tags.nfc_id, only on the run that creates it, so a tag the app --- detaches later is never re-attached by a re-run. +-- filled from tags.nfc_id (or tag_id, on a database that never got nfc_id), +-- only on the run that creates it, so a tag the app detaches later is never +-- re-attached by a re-run. -- ───────────────────────────────────────────────────────────────────────── DO $$ BEGIN @@ -133,9 +152,22 @@ BEGIN chore_tag_id text REFERENCES tags(tag_id) ON DELETE SET NULL, created_at timestamptz DEFAULT now() ); - INSERT INTO nfc_tags (nfc_id, name, chore_tag_id) - SELECT nfc_id, label, tag_id FROM tags WHERE nfc_id IS NOT NULL - ON CONFLICT (nfc_id) DO NOTHING; + -- Run on its own, this section can meet a tags table that never got + -- nfc_id (the block above adds it). Then tag_id is still the sticker's + -- id, and an app-made UUID means the chore has no tag. + IF EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'public' AND table_name = 'tags' AND column_name = 'nfc_id' + ) THEN + INSERT INTO nfc_tags (nfc_id, name, chore_tag_id) + SELECT nfc_id, label, tag_id FROM tags WHERE nfc_id IS NOT NULL + ON CONFLICT (nfc_id) DO NOTHING; + ELSE + INSERT INTO nfc_tags (nfc_id, name, chore_tag_id) + SELECT tag_id, label, tag_id FROM tags + WHERE tag_id !~* '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$' + ON CONFLICT (nfc_id) DO NOTHING; + END IF; END IF; END $$; @@ -143,14 +175,22 @@ CREATE INDEX IF NOT EXISTS nfc_tags_chore_idx ON nfc_tags(chore_tag_id); ALTER TABLE nfc_tags ENABLE ROW LEVEL SECURITY; -DROP POLICY IF EXISTS "anon read nfc_tags" ON nfc_tags; -CREATE POLICY "anon read nfc_tags" ON nfc_tags FOR SELECT TO anon USING (true); -DROP POLICY IF EXISTS "anon insert nfc_tags" ON nfc_tags; -CREATE POLICY "anon insert nfc_tags" ON nfc_tags FOR INSERT TO anon WITH CHECK (true); -DROP POLICY IF EXISTS "anon update nfc_tags" ON nfc_tags; -CREATE POLICY "anon update nfc_tags" ON nfc_tags FOR UPDATE TO anon USING (true) WITH CHECK (true); -DROP POLICY IF EXISTS "anon delete nfc_tags" ON nfc_tags; -CREATE POLICY "anon delete nfc_tags" ON nfc_tags FOR DELETE TO anon USING (true); +-- Created only when missing, so a re-run drops nothing (LESSONS #69). +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'nfc_tags' AND policyname = 'anon read nfc_tags') THEN + CREATE POLICY "anon read nfc_tags" ON nfc_tags FOR SELECT TO anon USING (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'nfc_tags' AND policyname = 'anon insert nfc_tags') THEN + CREATE POLICY "anon insert nfc_tags" ON nfc_tags FOR INSERT TO anon WITH CHECK (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'nfc_tags' AND policyname = 'anon update nfc_tags') THEN + CREATE POLICY "anon update nfc_tags" ON nfc_tags FOR UPDATE TO anon USING (true) WITH CHECK (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'nfc_tags' AND policyname = 'anon delete nfc_tags') THEN + CREATE POLICY "anon delete nfc_tags" ON nfc_tags FOR DELETE TO anon USING (true); + END IF; +END $$; -- ───────────────────────────────────────────────────────────────────────── -- scans — log of NFC taps. Each scan marks the matching tag as "done now". @@ -166,14 +206,22 @@ CREATE INDEX IF NOT EXISTS scans_scanned_at_idx ON scans(scanned_at); ALTER TABLE scans ENABLE ROW LEVEL SECURITY; -DROP POLICY IF EXISTS "anon read scans" ON scans; -CREATE POLICY "anon read scans" ON scans FOR SELECT TO anon USING (true); -DROP POLICY IF EXISTS "anon insert scans" ON scans; -CREATE POLICY "anon insert scans" ON scans FOR INSERT TO anon WITH CHECK (true); -DROP POLICY IF EXISTS "anon update scans" ON scans; -CREATE POLICY "anon update scans" ON scans FOR UPDATE TO anon USING (true) WITH CHECK (true); -DROP POLICY IF EXISTS "anon delete scans" ON scans; -CREATE POLICY "anon delete scans" ON scans FOR DELETE TO anon USING (true); +-- Created only when missing, so a re-run drops nothing (LESSONS #69). +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'scans' AND policyname = 'anon read scans') THEN + CREATE POLICY "anon read scans" ON scans FOR SELECT TO anon USING (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'scans' AND policyname = 'anon insert scans') THEN + CREATE POLICY "anon insert scans" ON scans FOR INSERT TO anon WITH CHECK (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'scans' AND policyname = 'anon update scans') THEN + CREATE POLICY "anon update scans" ON scans FOR UPDATE TO anon USING (true) WITH CHECK (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'scans' AND policyname = 'anon delete scans') THEN + CREATE POLICY "anon delete scans" ON scans FOR DELETE TO anon USING (true); + END IF; +END $$; -- ───────────────────────────────────────────────────────────────────────── -- todos — shared task list, used by both taskDash (web) and the Android app. @@ -201,14 +249,22 @@ CREATE INDEX IF NOT EXISTS todos_owner_idx ON todos(owner); ALTER TABLE todos ENABLE ROW LEVEL SECURITY; -DROP POLICY IF EXISTS "anon read todos" ON todos; -CREATE POLICY "anon read todos" ON todos FOR SELECT TO anon USING (true); -DROP POLICY IF EXISTS "anon insert todos" ON todos; -CREATE POLICY "anon insert todos" ON todos FOR INSERT TO anon WITH CHECK (true); -DROP POLICY IF EXISTS "anon update todos" ON todos; -CREATE POLICY "anon update todos" ON todos FOR UPDATE TO anon USING (true) WITH CHECK (true); -DROP POLICY IF EXISTS "anon delete todos" ON todos; -CREATE POLICY "anon delete todos" ON todos FOR DELETE TO anon USING (true); +-- Created only when missing, so a re-run drops nothing (LESSONS #69). +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'todos' AND policyname = 'anon read todos') THEN + CREATE POLICY "anon read todos" ON todos FOR SELECT TO anon USING (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'todos' AND policyname = 'anon insert todos') THEN + CREATE POLICY "anon insert todos" ON todos FOR INSERT TO anon WITH CHECK (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'todos' AND policyname = 'anon update todos') THEN + CREATE POLICY "anon update todos" ON todos FOR UPDATE TO anon USING (true) WITH CHECK (true); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_policies WHERE schemaname = 'public' AND tablename = 'todos' AND policyname = 'anon delete todos') THEN + CREATE POLICY "anon delete todos" ON todos FOR DELETE TO anon USING (true); + END IF; +END $$; -- ───────────────────────────────────────────────────────────────────────── -- Data API exposure (table grants) @@ -244,10 +300,26 @@ GRANT SELECT, INSERT, UPDATE, DELETE ON todos TO anon, authenticated, service_r -- an older database (created before 'eventually' was allowed, or before -- repeat_unit existed) is brought in line; a no-op once it already matches. -- ───────────────────────────────────────────────────────────────────────── -ALTER TABLE todos DROP CONSTRAINT IF EXISTS todos_due_period_check; -ALTER TABLE todos ADD CONSTRAINT todos_due_period_check - CHECK (due_period IN ('today', 'this_week', 'this_month', 'eventually')); +DO $$ +BEGIN + -- Replaced only when missing or lacking the newest value the app writes, so + -- a database already in line is left alone. When a value is added to a CHECK + -- below, change the LIKE to look for it. + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = 'todos_due_period_check' AND pg_get_constraintdef(oid) LIKE '%''eventually''%' + ) THEN + ALTER TABLE todos DROP CONSTRAINT IF EXISTS todos_due_period_check; + ALTER TABLE todos ADD CONSTRAINT todos_due_period_check + CHECK (due_period IN ('today', 'this_week', 'this_month', 'eventually')); + END IF; -ALTER TABLE tags DROP CONSTRAINT IF EXISTS tags_repeat_unit_check; -ALTER TABLE tags ADD CONSTRAINT tags_repeat_unit_check - CHECK (repeat_unit IN ('week', 'month', 'year')); + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = 'tags_repeat_unit_check' AND pg_get_constraintdef(oid) LIKE '%''year''%' + ) THEN + ALTER TABLE tags DROP CONSTRAINT IF EXISTS tags_repeat_unit_check; + ALTER TABLE tags ADD CONSTRAINT tags_repeat_unit_check + CHECK (repeat_unit IN ('week', 'month', 'year')); + END IF; +END $$;