fix(registry): let a removed ID type be used again - #437
Conversation
Removing an ID through a change request keeps the record and marks it Invalid rather than deleting it. Two separate checks counted that dead row, so once an ID had been removed the registrant was left with an Invalid ID and no way to add a valid one of the same type — which defeats the point of the Remove action. Both had to change; fixing either alone still leaves the user stuck. UNIQUE(partner_id, id_type_id) on spp.registry.id becomes a partial unique index over rows WHERE status IS DISTINCT FROM 'invalid'. A table constraint cannot express the condition. IS DISTINCT FROM rather than != is what keeps a NULL status blocking: those are IDs added straight through the registry, which are live records, not removed ones. The change request add path searched for any ID of the type regardless of status and refused on the strength of it. It now looks for a live one, so the Remove-then-Add sequence the ticket describes completes. Uniqueness is asserted before the write rather than through @api.constrains. Constraints run on flush, by which point the INSERT has already hit the index and the user sees a raw psycopg UniqueViolation instead of a sentence naming the ID type. The index stays as the race-safe guarantee. Covers the spec exactly: a live ID still reserves its type, an ID with no status still reserves its type, successive removals leave several Invalid rows behind without blocking, and flipping an Invalid row back to valid while a live one exists is refused. Note for upgrades: init() drops the old constraint explicitly rather than relying on the ORM noticing it is no longer declared, so an existing database needs spp_registry upgraded, not merely restarted. OP#1136
…-after-cr-removal
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #437 +/- ##
==========================================
- Coverage 75.01% 72.74% -2.27%
==========================================
Files 524 560 +36
Lines 35301 38323 +3022
==========================================
+ Hits 26480 27878 +1398
- Misses 8821 10445 +1624
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
The analysis and the shape of the fix are right — both checks fixed together (either alone leaves the user stuck), IS DISTINCT FROM for the NULL-status registry-added IDs, and a readable pre-write assert in front of the race-safe index. What I verified:
status = 'invalid'is exactly what the CR removal writes (update_id.py:117), and the selection has no other terminal value to worry about.- The old constraint's generated name is correct: Odoo 19's
TableObject.full_nameproduces{table}_{attribute-minus-underscore}(odoo/orm/table_objects.py), sospp_registry_id_unique_partner_id_typeis the right target — and Odoo does not auto-drop constraints that disappear from the model (apply_to_databaseonly adds/updates declared ones), so your explicit drop is genuinely required, not just prudent. - Existing data cannot violate the new index: the old constraint was strictly stronger, so
CREATE UNIQUE INDEXsucceeds on any database that held it. - Tests cover both sides of both checks, the multiple-invalid-rows case, and the flip-back-to-valid refusal.
Two blockers:
1. CI is red — and the better fix is a construct you'll like
Semgrep (odoo-sql-injection-fstring, both the pre-commit hook and the standalone Semgrep OSS check) fires on the CREATE INDEX f-string in init() — the known repo pitfall: SQL literal even when the composition is a compile-time constant. But rather than a nosemgrep, Odoo 19 has the idiomatic construct for exactly this:
_unique_active_id_type = models.UniqueIndex(
"(partner_id, id_type_id) WHERE status IS DISTINCT FROM 'invalid'"
)models.UniqueIndex supports WHERE clauses — the docstring's own example in odoo/orm/table_objects.py is a partial index ((group_id, active) WHERE active IS TRUE), and core uses the pattern in res_device and account_bank_statement_line. That deletes the raw CREATE INDEX (and most of init()), gets you framework-managed drop/recreate when the definition changes, and takes pylint's missing-return on init with it. The only SQL left is the one-time legacy-constraint DROP, as a literal string.
2. Version bumps + upgrade path — the fix currently reaches fresh installs only
This is a real schema change on two released modules, and neither manifest moved (spp_registry 19.0.2.1.4, spp_change_request_v2 19.0.3.1.1). Your own upgrade note says an existing database needs spp_registry upgraded — but with no version bump, nothing ever triggers that upgrade, so Remove-then-Add stays broken everywhere the bug actually lives. Per the schema-change rule (and Edwin's merge-time-assignment ruling), please bump both modules with HISTORY entries; the legacy-constraint drop fits naturally in migrations/<version>/ as literal SQL (with UniqueIndex handling creation declaratively, the migration is just the drop).
With those two, this is an approve — the core of the fix doesn't need to change.
Replaces the raw CREATE INDEX in init() with models.UniqueIndex, which takes the WHERE clause the rule needs. The framework now creates, drops and recreates the index as the definition changes, init() goes away entirely, and there is no SQL string left for the injection check to flag — which is what CI was red on. The old unconditional constraint still has to be dropped explicitly: Odoo adds and updates the constraints a model declares but never removes one that has simply stopped being declared, and left in place it would keep refusing exactly the Remove-then-Add sequence this fixes. That moves to migrations/19.0.2.2.0/pre-migration.py as literal SQL, before the ORM reconciles the table. Version bumps with changelog entries: spp_registry 19.0.2.2.0 (schema change) and spp_change_request_v2 19.0.3.1.2. Without them nothing triggers the upgrade, so the fix would only ever have reached fresh installs — not the databases where the bug lives. Also documents the sudo in _assert_id_type_free, which semgrep flagged separately: the rule is about the registrant's data rather than the acting user's visibility, so a clashing ID the user cannot read must still block the write.
|
Thanks — both blockers done, and CI is green (34 checks). Pushed as 1. _unique_active_id_type = models.UniqueIndex(
"(partner_id, id_type_id) WHERE status IS DISTINCT FROM 'invalid'"
)The framework now owns creation and recreation, there is no SQL string left for the injection rule to see, and pylint's While there, semgrep also flagged a second, pre-existing finding on the same file: the 2. Version bumps — And I verified the upgrade path rather than assuming it, since "reaches fresh installs only" was the actual defect:
Suites: Back to you for re-review — I have not merged this one. The previous four in this batch were bump-only and I merged them on your "with the bump, this is an approve"; this one changes the shape of the fix, so it deserves your eyes on the new construct before it lands. |
3 participants
Why is this change needed?
Removing an ID through a change request keeps the record and marks it Invalid rather than deleting it. Two separate checks counted that dead row, so once an ID had been removed the registrant was left with an Invalid ID and no way to add a valid one of the same type — which defeats the point of the Remove action (OP#1136).
Both checks had to change; fixing either alone still leaves the user stuck.
How was the change implemented?
UNIQUE(partner_id, id_type_id)onspp.registry.idbecomes a partial unique index over rowsWHERE status IS DISTINCT FROM 'invalid'. A table constraint cannot express the condition.IS DISTINCT FROMrather than!=is what keeps a NULL status blocking: those are IDs added straight through the registry, which are live records, not removed ones.@api.constrains. Constraints run on flush, by which point the INSERT has already hit the index and the user sees a raw psycopgUniqueViolationinstead of a sentence naming the ID type. The index stays as the race-safe guarantee.Note for upgrades:
init()drops the old constraint explicitly rather than relying on the ORM noticing it is no longer declared, so an existing database needsspp_registryupgraded, not merely restarted.New unit tests
spp_registry/tests/test_reg_id.py— a live ID still reserves its type, an ID with no status still reserves its type, successive removals leave several Invalid rows behind without blocking, and flipping an Invalid row back to valid while a live one exists is refused.spp_change_request_v2/tests/test_update_id_strategy.py— the add path accepts a type whose only existing row is Invalid, and still refuses one with a live row.Unit tests executed by the author
Run per module, the way CI's matrix does, on this branch after merging
19.0in:spp_registry— 250 tests, 0 failed, 0 errorsspp_change_request_v2— 331 tests, 0 failed, 0 errorsHow to test manually
Related links