Count stale root rows in the ltree path check

Both counts joined each row to its parent, so a root never appeared in either:
--check reported a root with a corrupted path or sort_path as OK, and told the
operator there was nothing to rebuild. Worse where the root has children, since
a child consistent with its corrupted parent is not counted either, leaving an
entire subtree wrong and nothing reported.

Check roots against what populate_paths_sql() gives them, a path of their own
padded id and a sort_path of their own name, and every other row against its
parent as before.

The report now says rows are out of date rather than that they disagree with
their parent, which a root does not have.

docs/administration/management-commands.md

@@ -59,8 +59,8 @@ python3 netbox/manage.py rebuild_ltree_paths --check
 ```
 
 ```no-highlight
-dcim.location: 5 path, 5 sort_path row(s) disagree with their parent
-dcim.region: 2 sort_path row(s) disagree with their parent
+dcim.location: 5 path, 5 sort_path row(s) out of date
+dcim.region: 2 sort_path row(s) out of date
 ...
 
 Needs rebuilding: dcim.location dcim.region

netbox/utilities/management/commands/rebuild_ltree_paths.py

@@ -113,7 +113,7 @@ class Command(BaseCommand):
         if stale_sort_paths:
             damage.append(f'{stale_sort_paths} sort_path')
         self.stdout.write(self.style.WARNING(
-            f"{model._meta.label_lower}: {', '.join(damage)} row(s) disagree with their parent"
+            f"{model._meta.label_lower}: {', '.join(damage)} row(s) out of date"
         ))
         return True
 

netbox/utilities/mptt_to_ltree.py

@@ -121,30 +121,44 @@ UPDATE "{table}" SET path = t.path FROM t WHERE "{table}".id = t.id;
 
 def count_stale_rows_sql(table, sort_path=False):
     """
-    Return SQL counting the rows in `table` whose `path` disagrees with their parent's,
-    and (when `sort_path` is set) the rows whose `sort_path` does.
+    Return SQL counting the rows in `table` whose `path` disagrees with the hierarchy, and
+    (when `sort_path` is set) the rows whose `sort_path` does.
 
     A reparent leaves `path` wrong, a rename leaves `sort_path` wrong, and while the
     cascade trigger is missing either can happen without the other, so both are counted
-    separately.
+    separately. Roots are checked against what `populate_paths_sql()` would give them (a
+    path of their own padded id, and a sort_path of their own name) and every other row
+    against its parent: a root has no parent to compare with, but it can still be wrong.
 
-    This answers "does this table need rebuilding", not "how many rows are damaged". Only
-    a row which disagrees with its own parent is counted: the descendants below it agree
-    with their parents and are not, though they are equally stale. Treat any non-zero
-    result as the whole table needing a rebuild, and do not use it to decide which rows to
-    touch.
+    This answers "does this table need rebuilding", not "how many rows are damaged". Where
+    an object has moved, the objects below it agree with their own parent and are not
+    counted, though they are equally stale. Treat any non-zero result as the whole table
+    needing a rebuild, and do not use it to decide which rows to touch.
     """
-    stale_sort_path = (
-        f'SELECT count(*) FROM "{table}" c JOIN "{table}" p ON c.parent_id = p.id'
-        f' WHERE c.sort_path <> p.sort_path || chr(9) || c.name'
-        if sort_path else 'SELECT 0'
+    root_path = (
+        f'SELECT id FROM "{table}"'
+        f" WHERE parent_id IS NULL"
+        f" AND path <> lpad(id::text, {_PATH_LABEL_WIDTH}, '0')::ltree"
     )
+    child_path = (
+        f'SELECT c.id FROM "{table}" c JOIN "{table}" p ON c.parent_id = p.id'
+        f" WHERE c.path <> p.path || lpad(c.id::text, {_PATH_LABEL_WIDTH}, '0')::ltree"
+    )
+    if sort_path:
+        root_sort_path = (
+            f'SELECT id FROM "{table}" WHERE parent_id IS NULL AND sort_path <> name'
+        )
+        child_sort_path = (
+            f'SELECT c.id FROM "{table}" c JOIN "{table}" p ON c.parent_id = p.id'
+            f' WHERE c.sort_path <> p.sort_path || chr(9) || c.name'
+        )
+        stale_sort_path = f'SELECT count(*) FROM ({root_sort_path} UNION ALL {child_sort_path}) s'
+    else:
+        stale_sort_path = 'SELECT 0'
+
     return f"""
 SELECT
-    (
-        SELECT count(*) FROM "{table}" c JOIN "{table}" p ON c.parent_id = p.id
-        WHERE c.path <> p.path || lpad(c.id::text, {_PATH_LABEL_WIDTH}, '0')::ltree
-    ) AS stale_paths,
+    (SELECT count(*) FROM ({root_path} UNION ALL {child_path}) p) AS stale_paths,
     ({stale_sort_path}) AS stale_sort_paths;
 """
 

netbox/utilities/tests/test_management_commands.py

@@ -160,6 +160,52 @@ class RebuildLtreePathsTestCase(TestCase):
 
         self.assertIn('1 path', out.getvalue())
 
+    def test_check_reports_a_stale_root(self):
+        """
+        A root has no parent to be compared against, so a check which only joins children
+        to parents never examines it and reports a corrupt root as clean.
+        """
+        root = Region.objects.create(name='Solo', slug='solo-rlp')
+        Region.objects.filter(pk=root.pk).update(
+            path='9999999999999999999', sort_path='WRONG',
+        )
+        out = StringIO()
+
+        call_command('rebuild_ltree_paths', 'dcim.region', '--check', stdout=out)
+
+        output = out.getvalue()
+        self.assertIn('1 path, 1 sort_path', output)
+        self.assertNotIn('dcim.region: OK', output)
+
+    def test_rebuilds_a_stale_root(self):
+        root = Region.objects.create(name='Solo', slug='solo-rlp')
+        Region.objects.filter(pk=root.pk).update(
+            path='9999999999999999999', sort_path='WRONG',
+        )
+
+        call_command('rebuild_ltree_paths', 'dcim.region', stdout=StringIO())
+
+        root.refresh_from_db()
+        self.assertEqual(root.path, str(root.pk).zfill(19))
+        self.assertEqual(root.sort_path, 'Solo')
+
+    def test_check_reports_a_stale_root_whose_child_agrees_with_it(self):
+        """
+        The child of a corrupt root can be consistent with that root, so a parent-only
+        comparison sees nothing wrong anywhere in the subtree.
+        """
+        root = Region.objects.create(name='Solo', slug='solo-rlp')
+        child = Region.objects.create(name='Sub', slug='sub-rlp', parent=root)
+        Region.objects.filter(pk=root.pk).update(path='9999999999999999999')
+        Region.objects.filter(pk=child.pk).update(
+            path=f'9999999999999999999.{str(child.pk).zfill(19)}',
+        )
+        out = StringIO()
+
+        call_command('rebuild_ltree_paths', 'dcim.region', '--check', stdout=out)
+
+        self.assertIn('1 path', out.getvalue())
+
     def test_rejects_a_model_which_is_not_hierarchical(self):
         with self.assertRaises(CommandError):
             call_command('rebuild_ltree_paths', 'dcim.site')