Read a numeric natural key as a name before reading it as an id

An Azure device import died part-way with a foreign-key error, and the crash
was the lucky outcome.

The Azure adapter routes a device that already exists to its real asset by
writing that asset's assetnumber into the assetid column of the CSV it
generates, expecting the engine to look it up. The resolver instead takes any
all-digit string at face value as a literal id. This site numbers operations
0NNN, so numeric assetnumbers are ordinary here: `0704` was read as id 704,
which does not exist, hence the error. `0600` would have been read as id 600 -
a printer - and bound a PC to it silently, reporting success.

Two changes, both needed.

The resolver now tries the label first for an all-digit string and only falls
back to reading it as an id when nothing is named that. The face-value
behaviour exists for a real reason - exports round-trip and carry real ids, as
its original comment says - so it stays as the fallback rather than being
removed. Somebody writing `0600` in a column keyed on assetnumber means the
asset called 0600, and binding the row to an unrelated record is far worse than
failing.

_existing_hostname_map now hands over the assetid rather than the assetnumber.
Giving the resolver the id it is going to treat the value as makes the
passthrough correct by construction instead of correct by luck.

Verified against a restored copy of the production database: 234 devices, 221
matching an existing computer and updating in place, 13 new, no problems. The
same run previously failed on the foreign key.

Neither half was wrong alone. The passthrough dates from the original CSV
import and the hostname map from the wilm merge months later; they only meet
at a site whose natural keys are numeric.

Scope: product

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
 
 

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))

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