From 7081ea5d41473f3eea74b86f4892dbb054024c4d Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Mon, 31 Aug 2026 15:55:31 -0400 Subject: [PATCH] Fix numeric natural keys resolving to the wrong asset on import --- shopdb/core/api/settings.py | 23 +++++++++++++------- shopdb/core/services/csvimport.py | 35 ++++++++++++++++++++++++++++--- tests/test_csv_import.py | 26 +++++++++++++++++++++++ 3 files changed, 74 insertions(+), 10 deletions(-) diff --git a/shopdb/core/api/settings.py b/shopdb/core/api/settings.py index 4daaecb6..e36afc21 100644 --- a/shopdb/core/api/settings.py +++ b/shopdb/core/api/settings.py @@ -334,23 +334,32 @@ def download_import_template(tablename): def _existing_hostname_map(): - """Case-insensitive hostname -> assetnumber map for existing computers. + """Case-insensitive hostname -> assetid map for existing computers. - Azure device-group exports align on hostname (their displayName), while the - import engine keys assets on assetnumber. This lets the Azure adapter route - a matched device to its real asset so the import updates in place. + Azure and Thor exports align on hostname (their displayName), while the + import engine keys assets on assetnumber. This lets those adapters route a + matched device to its real asset so the import updates in place. + + THE ID, NOT THE ASSETNUMBER, and that is not a style choice. The resolver + takes any all-digit value at face value as a literal id rather than looking + it up, so an assetnumber like `0600` resolved to asset 600 - a different + asset entirely, or none. This site numbers operations 0NNN, so those + assetnumbers are real and common: one of them pointed a PC at a printer's + record, and another at an id that does not exist, which is the foreign-key + error an Azure import died on. Handing over the id the resolver is going to + treat it as makes the passthrough correct by construction. """ assets = db.metadata.tables['assets'] computers = db.metadata.tables['computers'] query = sa_select( - computers.c.hostname, assets.c.assetnumber + computers.c.hostname, assets.c.assetid ).select_from( computers.join(assets, computers.c.assetid == assets.c.assetid) ).where(computers.c.hostname.isnot(None)) mapping = {} - for hostname, assetnumber in db.session.execute(query): + for hostname, assetid in db.session.execute(query): if hostname: - mapping[hostname.strip().lower()] = assetnumber + mapping[hostname.strip().lower()] = assetid return mapping diff --git a/shopdb/core/services/csvimport.py b/shopdb/core/services/csvimport.py index 7e093028..149f07ae 100644 --- a/shopdb/core/services/csvimport.py +++ b/shopdb/core/services/csvimport.py @@ -427,12 +427,41 @@ class Resolver: def resolve(self, tablename, idcolumn, value): if value is None: return None - # A number is taken at face value: exports round-trip, and some sites do - # know their ids. - if isinstance(value, int) or (isinstance(value, str) and value.strip().isdigit()): + # An int is an id. It cannot be anything else. + if isinstance(value, int): return int(value) label = _label_column(tablename) + + # An ALL-DIGIT STRING IS AMBIGUOUS, and used to be taken at face value + # as an id. That is wrong wherever a site's natural keys are numeric, + # and this one numbers operations 0NNN: assetnumber `0600` was read as + # id 600, which is a different asset - a printer, in the case that + # found this - or none at all, which surfaced as a foreign-key error + # partway through an Azure device import. + # + # The label wins when one matches. Somebody writing `0600` in a column + # keyed on assetnumber means the asset called 0600, and silently + # binding a row to an unrelated record is far worse than the error. + # Fall through to the id reading only when no label matches, which + # keeps round-tripped exports and sites that do know their ids working. + if isinstance(value, str) and value.strip().isdigit(): + if label: + key = (tablename, value.strip().lower()) + if key in self._cache: + return self._cache[key] + table = db.metadata.tables[tablename] + row = db.session.execute( + table.select().where( + db.func.lower(db.func.trim(table.c[label])) + == value.strip().lower()) + ).first() + if row is not None: + found = getattr(row, idcolumn) + self._cache[key] = found + return found + return int(value) + if not label: raise ValueError( "cannot look up %s by name - give the numeric %s instead" % (tablename, idcolumn)) diff --git a/tests/test_csv_import.py b/tests/test_csv_import.py index 0cd4956d..078b116a 100644 --- a/tests/test_csv_import.py +++ b/tests/test_csv_import.py @@ -335,3 +335,29 @@ def test_reconcile_still_binds_normalized_exact(db): _db.session.rollback() + + +def test_a_numeric_assetnumber_is_read_as_a_name_not_an_id(db): + """This site numbers operations 0NNN, so numeric natural keys are real. + + An all-digit string used to be taken at face value as an id. `0600` became + id 600 - a different asset, or none - which is how an Azure device import + died on a foreign key partway through, and how a PC could instead have been + bound silently to a printer's record. The label wins when one matches. + """ + resolver = Resolver() + seed_reference(resolver) + import_csv('assets', 'assetnumber,assettypeid\n0600,Machine\n', + resolver=resolver, commit=True) + realid = _db.session.execute(_db.text( + "select assetid from assets where assetnumber = '0600'")).scalar() + + # Force the ambiguity: an id that is NOT the one named 0600. + assert resolver.resolve('assets', 'assetid', '0600') == realid + + +def test_a_number_naming_nothing_is_still_read_as_an_id(db): + """Round-tripped exports and sites that know their ids keep working.""" + resolver = Resolver() + seed_reference(resolver) + assert resolver.resolve('assets', 'assetid', '999999') == 999999