diff --git a/docs/administration/management-commands.md b/docs/administration/management-commands.md index 2989e766a..954d961e4 100644 --- a/docs/administration/management-commands.md +++ b/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 diff --git a/netbox/utilities/management/commands/rebuild_ltree_paths.py b/netbox/utilities/management/commands/rebuild_ltree_paths.py index 60e8dccdc..3de783c74 100644 --- a/netbox/utilities/management/commands/rebuild_ltree_paths.py +++ b/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 diff --git a/netbox/utilities/mptt_to_ltree.py b/netbox/utilities/mptt_to_ltree.py index ca2a75fb9..a4e6d85d2 100644 --- a/netbox/utilities/mptt_to_ltree.py +++ b/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; """ diff --git a/netbox/utilities/tests/test_management_commands.py b/netbox/utilities/tests/test_management_commands.py index 46440cb5c..fced0f1c7 100644 --- a/netbox/utilities/tests/test_management_commands.py +++ b/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')