feat(#10): pre-flight check for external-id (xmlid) collisions - #224
Merged
Conversation
added 2 commits
August 1, 2026 14:23
`to_xmlid` maps spaces/commas/pipes/newlines to `_`, so two distinct source ids (e.g. "a b" and "a,b") can collapse to the same Odoo external id. Because load() upserts on the xmlid, such rows are silently merged into one record — the later row overwrites the earlier. This was undetected. Add a pre-flight check that scans the id column and aborts *before* anything is written when distinct ids collide (listing the offending ids), unless the new `--allow-xmlid-collisions` flag is passed (then it warns and proceeds). Blank ids are left alone (each is an independent create, no merge), but a blank-id row that also carries relational (Pass-2) data is warned about — Pass 2 can't link relations to a record with no external id. Per review discussion, fluvo does NOT positionally guess the link (parallel batches / --groupby / sort / retries make row order unstable), it surfaces the problem instead. Tests cover collision-abort, opt-out, no-collision, and both blank-id cases. Docs: a warning admonition under the External ID section.
…isions arg Adding the parameter changed the exact arg-list text in run_import's baselined DOC103 entry; insert allow_xmlid_collisions to match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review follow-up #10 (design agreed in discussion).
Problem
to_xmlidsanitizes each id for Odoo by mapping spaces/commas/pipes/newlines to_. So two different source ids collapse to the same external id —a banda,bboth becomea_b. Since Odoo'sload()upserts on the external id, those rows are silently merged into one record (the later row overwrites the earlier). Real, silent data loss — and it was undetected.Fix
A new pre-flight check (
xmlid_collision_check) scans the id column and, when distinct ids collide, aborts before anything is written and lists the offending ids. Opt out with--allow-xmlid-collisions(then it warns and proceeds).Design calls from the discussion:
--groupby/ self-referential sort / retry+bisect make row order unstable, so "line N of Pass 1 = line N of Pass 2" would silently link the wrong records. We surface the problem instead of guessing.Tests
TestXmlidCollisionCheck: collision-abort,--allow-xmlid-collisionsopt-out, no-collision pass, blank-id-with-relations warns, blank-id-without-relations is fine. Full suite: 1435 passed; mypy + ruff clean; no pydoclint-baseline churn. Docs: a warning admonition under "How External IDs Work".Noted follow-up (from the discussion)
Narrow what actually gets deferred: a many2many/one2many that points at records which already exist can go inline in the Pass-1
load()— no deferral, no external id required. Only relations pointing at other rows in the same import truly need an id + Pass 2. That would eliminate a lot of blank-id-with-relations cases entirely. I can file this as a tracked issue if you'd like.