Skip to content

fix(db): allow supplier CIA readings (#161) + migration-name guardrail#163

Merged
unidoc-ahall merged 1 commit into
masterfrom
fix/161-supplier-reading
Jul 13, 2026
Merged

fix(db): allow supplier CIA readings (#161) + migration-name guardrail#163
unidoc-ahall merged 1 commit into
masterfrom
fix/161-supplier-reading

Conversation

@unidoc-ahall

Copy link
Copy Markdown
Contributor

Two related changes for 0.7.1 — closes #161 and #162.

1. Fix: supplier CIA readings (#161)

Saving a CIA reading for a Supplier failed with entity_readings_entity_type_check (SQLSTATE 23514). The supplier reading path shipped (POST /suppliers/:id/readings forces entity_type = "supplier" server-side), but the entity_readings entity_type CHECK only listed risk, legal_requirement, asset, system. The status CHECK on the same table already carried supplier statuses — the entity_type set was just never updated.

The 0.7.1 release migration (v0.7.1.sql — one migration file per release) widens the constraint. A migration in a patch is the documented exception: a DB CHECK constraint has no non-schema fix. DROP … IF EXISTS + re-ADD is safe on fresh DBs too. Regression test TestSupplierReadings covers the exact reported flow (the gap that let it ship — we had supplier review tests, not supplier reading tests).

2. Guardrail: migration naming (#162)

So this can't recur: tests/test_migrations.py (pure filesystem, no fixtures) fails just test and CI on any migration not named <timestamp>_vX.Y.Z.sql. Grandfathers initial_schema.sql and change_type_check.sql.

just build-go green. No API/handler change — the write path was already correct.

…ype check (#161)

POST /suppliers/:id/readings sets entity_type 'supplier', but the entity_readings
entity_type CHECK only listed risk/legal_requirement/asset/system, so every
supplier reading failed with SQLSTATE 23514. The status CHECK on the same table
already carries supplier statuses — the entity_type set was simply never updated.

The 0.7.1 release migration (v0.7.1.sql — one migration file per release) widens
the constraint to include 'supplier'. Migration in a patch is the documented
exception here: a DB CHECK constraint has no non-schema fix. Adds
TestSupplierReadings covering the reported flow: create supplier -> save CIA
reading -> verify classification + last_review, plus reader-role 403.

Closes #161.

@unidoc-alip unidoc-alip left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, minimal fix for #161: the entity_readings.entity_type CHECK constraint (initial_schema.sql) never listed 'supplier', even though POST /suppliers/:id/readings has always forced entity_type = "supplier" server-side (api_readings.go:425) — confirmed by reading both. The new v0.7.1.sql migration correctly widens the constraint via DROP CONSTRAINT IF EXISTS + re-ADD, and TestSupplierReadings exercises the exact reported flow, including RBAC (reader → 403). The migration-naming guardrail (#162) is a solid filesystem-only test that all 4 existing migration files already pass. go build ./... is clean. No concerns worth blocking on.


Findings

No findings — reviewed clean.

Verification performed

  • Confirmed the bug: migrations/20260327000000_initial_schema.sql:1237 defines entity_readings.entity_type CHECK (... IN ('risk','legal_requirement','asset','system')) — no 'supplier', matching the reported SQLSTATE 23514.
  • Confirmed the write path already sends entity_type = "supplier": internal/isms/api/api_readings.go:425 (handleCreateSupplierReading), unconditionally, regardless of request body — the fix is purely schema-side as the PR claims, no handler change needed.
  • Confirmed writeSupplierFromReading (api_readings.go:454-483) updates Confidentiality/Integrity/Availability/LastReview/NextReview on the supplier row exactly as TestSupplierReadings.test_03_supplier_updated asserts.
  • Confirmed RBAC: handleCreateSupplierReading calls requireRole(c, "admin", "manager") (api_readings.go:407) — the new test_05_reader_cannot_submit (403) is consistent with existing enforcement, not new behavior.
  • Ran the new naming regex (^\d{14}_v\d+\.\d+\.\d+\.sql$) against all 4 files currently in migrations/: all pass (2 grandfathered exactly as listed, 2 match the pattern). No spurious breakage of CI from this PR.
  • go build ./... — clean, no errors.
  • pytest unavailable in this environment so tests/test_migrations.py / tests/test_readings.py were not executed live; logic was verified by static read instead.

LGTM

@unidoc-ahall
unidoc-ahall merged commit 18ab812 into master Jul 13, 2026
2 checks passed
@unidoc-ahall
unidoc-ahall deleted the fix/161-supplier-reading branch July 13, 2026 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CIA reading for supplier entity fails --> entity_readings_entity_type_check violated

2 participants