From 22c2886da2b764f28b3b1c3e00ad56ae5750b593 Mon Sep 17 00:00:00 2001 From: Valentijn Scholten Date: Tue, 4 Aug 2026 23:47:44 +0200 Subject: [PATCH 1/2] refactor(dedupe): one definition of the location prefetch Which location relation to prefetch is a property of what Finding.get_locations() reads, not of any individual caller, but it was spelled out separately at every call site. That drifts, in both directions: the hash-recompute paths disagreed about it for months, and the batch dedupe loader prefetched the deprecated endpoints relation, which raises under V3_FEATURE_LOCATIONS on an instance migrated from endpoints. Adds location_prefetch_lookups() to dojo/location/queries.py, alongside vulnerability_id_prefetch, and routes build_candidate_scope_queryset and the dedupe command through it. The command's V3 branch prefetched "locations", one hop, where get_locations() reads location.url; it now prefetches the whole path like the other call sites, and its two near-identical select_related/prefetch_related blocks collapse into one. get_finding_models_for_deduplication is left alone deliberately: #15508 fixes the crash there and re-baselines the perf query counts it shifts. Measured that change independently here -- +2 queries on each V3 step, and -104 on the second import -- which matches the numbers in that PR. A TODO marks the one-line swap to the helper once it lands. The prefix argument exists for callers that page a model reaching the finding through a relation rather than paging Finding itself. --- dojo/finding/deduplication.py | 20 ++-- dojo/location/queries.py | 23 +++++ dojo/management/commands/dedupe.py | 48 ++++----- unittests/test_dedupe_location_prefetch.py | 108 +++++++++++++++++++++ 4 files changed, 154 insertions(+), 45 deletions(-) create mode 100644 unittests/test_dedupe_location_prefetch.py diff --git a/dojo/finding/deduplication.py b/dojo/finding/deduplication.py index d1b10682cbe..da488ba5db0 100644 --- a/dojo/finding/deduplication.py +++ b/dojo/finding/deduplication.py @@ -11,6 +11,7 @@ from django.db.models.query_utils import Q from dojo.celery import app +from dojo.location.queries import location_prefetch_lookups from dojo.models import Endpoint_Status, Finding, System_Settings from dojo.vulnerability.queries import vulnerability_id_prefetch @@ -36,19 +37,13 @@ def get_finding_models_for_deduplication(finding_ids): # are_locations_duplicates reads the new finding's locations (V3) or endpoints (V2) # once per candidate pair, so the pair loop N+1s unless the right relation is - # prefetched here. The V3 prefetch pulls the reference -> Location -> URL subtype - # chain that finding_locations() walks. Prefetching the endpoints m2m under V3 is - # not an option either: the Endpoint model is deprecated there and its __init__ - # raises, so hydrating legacy rows would crash the batch. ("endpoints" is the V2 - # relation — TODO: delete it after the move to Locations.) - location_prefetch = "locations__location__url" if settings.V3_FEATURE_LOCATIONS else "endpoints" - + # prefetched here. location_prefetch_lookups() is the single definition of that relation. return list( Finding.objects.filter(id__in=finding_ids) .only(*Finding.DEDUPLICATION_FIELDS) .select_related("test", "test__engagement", "test__engagement__product", "test__test_type") .prefetch_related( - location_prefetch, + *location_prefetch_lookups(), # Prefetch duplicates of each finding to avoid N+1 when set_duplicate iterates Prefetch( "original_finding", @@ -384,13 +379,10 @@ def build_candidate_scope_queryset(test, mode="deduplication", service=None): ) queryset = Finding.objects.filter(scope_q) - if settings.V3_FEATURE_LOCATIONS: - prefetch_list = ["locations__location__url", vulnerability_id_prefetch(), "finding_cwe_set", "found_by"] - else: - # TODO: Delete this after the move to Locations - # Base prefetches for both modes - prefetch_list = ["endpoints", vulnerability_id_prefetch(), "finding_cwe_set", "found_by"] + prefetch_list = [*location_prefetch_lookups(), vulnerability_id_prefetch(), "finding_cwe_set", "found_by"] + if not settings.V3_FEATURE_LOCATIONS: + # TODO: Delete this after the move to Locations # Prefetch all endpoint statuses with their endpoint for reimport mode. # The non-special filtering (excluding false_positive, out_of_scope, risk_accepted) # is done in Python by EndpointManager.get_non_special_endpoint_statuses(). diff --git a/dojo/location/queries.py b/dojo/location/queries.py index 63abeefce8d..cfef084542d 100644 --- a/dojo/location/queries.py +++ b/dojo/location/queries.py @@ -1,5 +1,6 @@ import logging +from django.conf import settings from django.db.models import ( Case, CharField, @@ -30,6 +31,28 @@ def get_auth_filter(key): return None logger = logging.getLogger(__name__) +def location_prefetch_lookups(prefix: str = "") -> list[str]: + """ + Prefetch lookups for the location relation that the hash and deduplication paths read + through ``Finding.get_locations()``, for the location model actually in use. + + Endpoint rows are not deleted by the move to Locations, and ``Endpoint.__init__`` raises + ``NotImplementedError`` once ``V3_FEATURE_LOCATIONS`` is on (see + ``Endpoint.allow_endpoint_init``). So prefetching the endpoint relation under V3 hydrates + the deprecated model for every surviving row and kills the caller -- on a migrated + instance, not on a fresh one, which is why it is easy to miss. Under V3 ``get_locations()`` + reads URL locations and never touches endpoints, so the endpoint prefetch is dead weight + there in any case. + + :param prefix: relation path to the Finding, e.g. ``"finding__"`` when paging a model that + reaches the finding through a relation. + """ + if settings.V3_FEATURE_LOCATIONS: + return [f"{prefix}locations__location__url"] + # TODO: Delete this after the move to Locations + return [f"{prefix}endpoints"] + + def get_authorized_locations(permission, queryset=None, user=None): impl = get_auth_filter("location.get_authorized_locations") if impl: diff --git a/dojo/management/commands/dedupe.py b/dojo/management/commands/dedupe.py index 36d46a1919b..78f42d4086b 100644 --- a/dojo/management/commands/dedupe.py +++ b/dojo/management/commands/dedupe.py @@ -14,6 +14,7 @@ get_finding_models_for_deduplication, hashcode_values_writer, ) +from dojo.location.queries import location_prefetch_lookups from dojo.models import Finding, Product from dojo.utils import ( calculate_grade, @@ -92,37 +93,22 @@ def _run_dedupe(self, *, restrict_to_parsers, hash_code_only, dedupe_only, dedup findings = Finding.objects.all().filter(id__gt=0).exclude(duplicate=True) logger.info("######## Will process the full database with %d findings ########", findings.count()) - if settings.V3_FEATURE_LOCATIONS: - # Prefetch related objects for synchronous deduplication - findings = findings.select_related( - "test", "test__engagement", "test__engagement__product", "test__test_type", - ).prefetch_related( - "locations", - # vulnerability id store feeds hash_code computation for parsers whose - # HASHCODE_FIELDS_PER_SCANNER includes vulnerability_ids; prefetch to avoid - # a per-finding query in get_vulnerability_ids(). - vulnerability_id_prefetch(), - Prefetch( - "original_finding", - queryset=Finding.objects.only("id", "duplicate_finding_id").order_by("-id"), - ), - ) - else: - # TODO: Delete this after the move to Locations - # Prefetch related objects for synchronous deduplication - findings = findings.select_related( - "test", "test__engagement", "test__engagement__product", "test__test_type", - ).prefetch_related( - "endpoints", - # vulnerability id store feeds hash_code computation for parsers whose - # HASHCODE_FIELDS_PER_SCANNER includes vulnerability_ids; prefetch to avoid - # a per-finding query in get_vulnerability_ids(). - vulnerability_id_prefetch(), - Prefetch( - "original_finding", - queryset=Finding.objects.only("id", "duplicate_finding_id").order_by("-id"), - ), - ) + # Prefetch related objects for synchronous deduplication + findings = findings.select_related( + "test", "test__engagement", "test__engagement__product", "test__test_type", + ).prefetch_related( + # location relation for the parsers that hash it; the lookup differs per location + # model, and prefetching the deprecated endpoint relation under V3 raises. + *location_prefetch_lookups(), + # vulnerability id store feeds hash_code computation for parsers whose + # HASHCODE_FIELDS_PER_SCANNER includes vulnerability_ids; prefetch to avoid + # a per-finding query in get_vulnerability_ids(). + vulnerability_id_prefetch(), + Prefetch( + "original_finding", + queryset=Finding.objects.only("id", "duplicate_finding_id").order_by("-id"), + ), + ) # Phase 1: update hash_codes without deduplicating if not dedupe_only: diff --git a/unittests/test_dedupe_location_prefetch.py b/unittests/test_dedupe_location_prefetch.py new file mode 100644 index 00000000000..fbd7e7d3295 --- /dev/null +++ b/unittests/test_dedupe_location_prefetch.py @@ -0,0 +1,108 @@ +""" +The location prefetch the dedupe and rehash querysets need, in one place. + +Which location relation to prefetch is a property of what ``Finding.get_locations()`` reads, not +of any individual caller, but it was spelled out separately at every call site. That drifts: the +hash-recompute paths disagreed about it for months, and the batch dedupe loader prefetched the +deprecated ``endpoints`` relation, which raises under ``V3_FEATURE_LOCATIONS`` once an instance has +migrated from endpoints (see #15508). + +``location_prefetch_lookups()`` is that decision, once. These assertions are about queryset +shape, so they need no fixtures and cannot flake on machine speed. +""" +import logging + +from django.db.models import Prefetch +from django.test import override_settings +from django.utils import timezone + +from dojo.finding.deduplication import build_candidate_scope_queryset +from dojo.location.queries import location_prefetch_lookups +from dojo.models import Engagement, Product, Product_Type, Test, Test_Type + +from .dojo_test_case import DojoTestCase + +logger = logging.getLogger(__name__) + + +class TestLocationPrefetchLookups(DojoTestCase): + + """The lookup has to follow the location model in use -- no database needed.""" + + @override_settings(V3_FEATURE_LOCATIONS=True) + def test_v3_prefetches_url_locations_and_not_endpoints(self): + lookups = location_prefetch_lookups() + self.assertEqual(lookups, ["locations__location__url"]) + self.assertNotIn("endpoints", lookups) + + @override_settings(V3_FEATURE_LOCATIONS=False) + def test_pre_v3_prefetches_endpoints(self): + # TODO: Delete this after the move to Locations + self.assertEqual(location_prefetch_lookups(), ["endpoints"]) + + @override_settings(V3_FEATURE_LOCATIONS=True) + def test_it_reaches_the_url_the_hash_actually_reads(self): + """ + ``get_locations()`` reads ``location_ref.location.url``, so stopping the prefetch at + ``locations`` leaves a query per reference. The lookup has to traverse the whole path. + """ + self.assertEqual(location_prefetch_lookups(), ["locations__location__url"]) + + @override_settings(V3_FEATURE_LOCATIONS=True) + def test_prefix_reaches_the_finding_through_a_relation(self): + """For callers that page a model reaching the finding through a relation.""" + self.assertEqual( + location_prefetch_lookups("finding__"), + ["finding__locations__location__url"], + ) + + +def _lookup_strings(queryset): + """The lookup path of every prefetch on a queryset, Prefetch objects included.""" + return [ + lookup.prefetch_through if isinstance(lookup, Prefetch) else str(lookup) + for lookup in queryset._prefetch_related_lookups + ] + + +class TestDedupeQuerysetsUseTheHelper(DojoTestCase): + + """Every dedupe/rehash queryset picks its location lookup the same way.""" + + def setUp(self): + super().setUp() + product_type = Product_Type.objects.create(name="Org for prefetch shape") + product = Product.objects.create( + name="Product for prefetch shape", + description="shape fixture", + prod_type=product_type, + ) + engagement = Engagement.objects.create( + name="Engagement", + product=product, + target_start=timezone.now(), + target_end=timezone.now(), + ) + self.test = Test.objects.create( + engagement=engagement, + test_type=Test_Type.objects.get_or_create(name="Manual Test")[0], + target_start=timezone.now(), + target_end=timezone.now(), + ) + + @override_settings(V3_FEATURE_LOCATIONS=True) + def test_candidate_scope_queryset_under_v3(self): + for mode in ("deduplication", "reimport"): + with self.subTest(mode=mode): + lookups = _lookup_strings(build_candidate_scope_queryset(self.test, mode=mode)) + self.assertIn("locations__location__url", lookups) + self.assertNotIn("endpoints", lookups) + + @override_settings(V3_FEATURE_LOCATIONS=False) + def test_candidate_scope_queryset_pre_v3(self): + # TODO: Delete this after the move to Locations + for mode in ("deduplication", "reimport"): + with self.subTest(mode=mode): + lookups = _lookup_strings(build_candidate_scope_queryset(self.test, mode=mode)) + self.assertIn("endpoints", lookups) + self.assertNotIn("locations__location__url", lookups) From 096e5129738a08a074c4b6c973fac90e314c8f9d Mon Sep 17 00:00:00 2001 From: Valentijn Scholten Date: Wed, 5 Aug 2026 08:42:37 +0200 Subject: [PATCH 2/2] refactor(dedupe): give the dedupe command extension points instead of a fork Distributions that store extra hash fields or need extra scoping have maintained near-verbatim copies of this command, and a copy is what lets the two drift. The vulnerability-id prefetch was fixed in a copy first and had to be fixed again elsewhere months later; the location prefetch was fixed elsewhere and never reached a copy, leaving it prefetching the deprecated endpoint relation that raises under V3_FEATURE_LOCATIONS. Such a distribution can now subclass this command and override a hook instead. The hooks are deliberately narrow -- extra arguments, extra scope and its description, the hash generator, an extra recompute pass, the two batch-dedupe entry points, and product grading -- so what an edition adds stays visible and a stale override is a signature mismatch rather than silent drift. Behaviour is unchanged: every default does what the code did before. The scope build is also unified, replacing the parser/no-parser if/else with one queryset that filters down, which is what makes an extra scope filter composable. unittests/test_dedupe_command_hooks.py covers both halves of the contract: every hook has a working default, and a subclass's hooks are actually reached by the run -- including that --dedupe_only skips the extra hash pass and that both batch paths and grading go through their hooks. This command had no tests at all before. --- dojo/management/commands/dedupe.py | 88 +++++++++--- unittests/test_dedupe_command_hooks.py | 177 +++++++++++++++++++++++++ 2 files changed, 246 insertions(+), 19 deletions(-) create mode 100644 unittests/test_dedupe_command_hooks.py diff --git a/dojo/management/commands/dedupe.py b/dojo/management/commands/dedupe.py index 78f42d4086b..20b10aadc0e 100644 --- a/dojo/management/commands/dedupe.py +++ b/dojo/management/commands/dedupe.py @@ -44,6 +44,49 @@ class Command(BaseCommand): help = 'Usage: manage.py dedupe [--parser "Parser1 Scan" --parser "Parser2 Scan"...] [--hash_code_only] [--dedupe_only] [--dedupe_sync] [--dedupe_batch_mode]' + # ---------------------------------------------------------------- extension points + # Editions that ship extra hash fields or extra scoping subclass this command and + # override the hooks below rather than forking the whole run. Keeping them narrow is + # the point: a fork drifts silently (the location prefetch and the vulnerability-id + # prefetch each had to be fixed twice because of one), while an override that goes + # stale is a signature mismatch. + + def add_extra_arguments(self, parser): + """Extra CLI arguments. Defaults to none.""" + + def apply_extra_scope(self, findings, options): + """Narrow the finding scope further from ``options``. Defaults to no narrowing.""" + return findings + + def describe_extra_scope(self, options): + """Human-readable fragments describing what ``apply_extra_scope`` narrowed to.""" + return [] + + def hash_code_generator(self): + """The callable that recomputes one finding's hash_code in place.""" + return generate_hash_code + + def recompute_extra_hashes(self, findings, writer): + """Recompute hashes stored outside Finding.hash_code. Defaults to none.""" + + def dedupe_batch_sync(self, findings): + """Deduplicate one already-loaded batch of findings, in this thread.""" + dedupe_batch_of_findings(findings) + + def dedupe_batch_async(self, finding_ids): + """Queue one batch of finding ids for deduplication.""" + from dojo.celery_dispatch import dojo_dispatch_task # noqa: PLC0415 circular import + + dojo_dispatch_task(do_dedupe_batch_task, finding_ids) + + def grade_product(self, product): + """Recalculate one product's grade after a synchronous dedupe run.""" + from dojo.celery_dispatch import dojo_dispatch_task # noqa: PLC0415 circular import + + dojo_dispatch_task(calculate_grade, product.id) + + # --------------------------------------------------------------------------------- + def add_arguments(self, parser): parser.add_argument( "--parser", @@ -61,37 +104,46 @@ def add_arguments(self, parser): default=True, help="Deduplicate in batches (similar to import), works with both sync and async modes (default: True)", ) + self.add_extra_arguments(parser) def handle(self, *args, **options): - restrict_to_parsers = options["parser"] hash_code_only = options["hash_code_only"] dedupe_only = options["dedupe_only"] dedupe_sync = options["dedupe_sync"] dedupe_batch_mode = options.get("dedupe_batch_mode", True) # Default to True (batch mode enabled) - # Wrap with pghistory context for audit trail + # Wrap with pghistory context for audit trail, so the hash_code churn this command + # produces is attributable to source="dedupe_command" in the audit log instead of + # being indistinguishable from ordinary import/save activity. with pghistory.context( source="dedupe_command", dedupe_sync=dedupe_sync, ): self._run_dedupe( - restrict_to_parsers=restrict_to_parsers, + options=options, hash_code_only=hash_code_only, dedupe_only=dedupe_only, dedupe_sync=dedupe_sync, dedupe_batch_mode=dedupe_batch_mode, ) - def _run_dedupe(self, *, restrict_to_parsers, hash_code_only, dedupe_only, dedupe_sync, dedupe_batch_mode): + def _run_dedupe(self, *, options, hash_code_only, dedupe_only, dedupe_sync, dedupe_batch_mode): """Internal method to run the dedupe logic within pghistory context.""" + restrict_to_parsers = options["parser"] + + # filter on id to make counts not slow on mysql, and exclude duplicates to avoid + # reprocessing findings that are already marked as duplicates + findings = Finding.objects.all().filter(id__gt=0).exclude(duplicate=True) if restrict_to_parsers is not None: - findings = Finding.objects.filter(test__test_type__name__in=restrict_to_parsers).exclude(duplicate=True) - logger.info("######## Will process only parsers %s and %d findings ########", *restrict_to_parsers, findings.count()) - else: - # add filter on id to make counts not slow on mysql - # exclude duplicates to avoid reprocessing findings that are already marked as duplicates - findings = Finding.objects.all().filter(id__gt=0).exclude(duplicate=True) - logger.info("######## Will process the full database with %d findings ########", findings.count()) + findings = findings.filter(test__test_type__name__in=restrict_to_parsers) + findings = self.apply_extra_scope(findings, options) + + scope = ([f"parsers={restrict_to_parsers}"] if restrict_to_parsers else []) + self.describe_extra_scope(options) + logger.info( + "######## Will process %d findings%s ########", + findings.count(), + f" ({', '.join(scope)})" if scope else " (full database)", + ) # Prefetch related objects for synchronous deduplication findings = findings.select_related( @@ -115,7 +167,9 @@ def _run_dedupe(self, *, restrict_to_parsers, hash_code_only, dedupe_only, dedup logger.info("######## Start Updating Hashcodes (foreground) ########") hash_code_writer = hashcode_values_writer if settings.MASS_HASH_CODE_USE_SQL_WRITER else None - mass_model_updater(Finding, findings, generate_hash_code, fields=["hash_code"], order="asc", log_prefix="hash_code computation ", writer=hash_code_writer) + mass_model_updater(Finding, findings, self.hash_code_generator(), fields=["hash_code"], order="asc", log_prefix="hash_code computation ", writer=hash_code_writer) + + self.recompute_extra_hashes(findings, hash_code_writer) logger.info("######## Done Updating Hashcodes########") @@ -146,9 +200,7 @@ def _run_dedupe(self, *, restrict_to_parsers, hash_code_only, dedupe_only, dedup # in async mode the background task that grades products every hour will pick it up logger.debug("Updating grades for products...") for product in Product.objects.all(): - from dojo.celery_dispatch import dojo_dispatch_task # noqa: PLC0415 circular import - - dojo_dispatch_task(calculate_grade, product.id) + self.grade_product(product) logger.info("######## Done deduplicating (%s) ########", ("foreground" if dedupe_sync else "tasks submitted to celery")) else: @@ -195,13 +247,11 @@ def _dedupe_batch_mode(self, findings_queryset, *, dedupe_sync: bool = True): # Synchronous: load findings and process immediately batch_findings = get_finding_models_for_deduplication(batch_finding_ids) logger.debug(f"Deduplicating batch of {len(batch_findings)} findings for test {test_id}") - dedupe_batch_of_findings(batch_findings) + self.dedupe_batch_sync(batch_findings) else: # Asynchronous: submit task with finding IDs logger.debug(f"Submitting async batch task for {len(batch_finding_ids)} findings for test {test_id}") - from dojo.celery_dispatch import dojo_dispatch_task # noqa: PLC0415 circular import - - dojo_dispatch_task(do_dedupe_batch_task, batch_finding_ids) + self.dedupe_batch_async(batch_finding_ids) total_processed += len(batch_finding_ids) batch_finding_ids = [] diff --git a/unittests/test_dedupe_command_hooks.py b/unittests/test_dedupe_command_hooks.py new file mode 100644 index 00000000000..979d8e53551 --- /dev/null +++ b/unittests/test_dedupe_command_hooks.py @@ -0,0 +1,177 @@ +""" +The extension points ``manage.py dedupe`` offers, and that its own run goes through them. + +Editions that store extra hash fields or need extra scoping subclass this command instead of +forking it. The fork is what let the two copies drift -- the location prefetch and the +vulnerability-id prefetch each had to be fixed twice because of one -- so this pins the contract a +subclass relies on: every hook has a working default, and the run actually calls it. + +No fixtures beyond one product tree, and the recompute is stubbed, so nothing here depends on +deduplication actually running. +""" +import logging +from unittest.mock import patch + +from django.contrib.auth import get_user_model +from django.test import override_settings +from django.utils import timezone + +from dojo.management.commands.dedupe import Command, generate_hash_code +from dojo.models import Engagement, Finding, Product, Product_Type, Test, Test_Type + +from .dojo_test_case import DojoTestCase + +logger = logging.getLogger(__name__) + +User = get_user_model() + +DEFAULT_OPTIONS = { + "parser": None, + "hash_code_only": True, + "dedupe_only": False, + "dedupe_sync": False, + "dedupe_batch_mode": True, +} + + +class TestDedupeCommandHookDefaults(DojoTestCase): + + """Every hook has a default, so the base command is usable without a subclass.""" + + def setUp(self): + super().setUp() + self.command = Command() + + def test_extra_scope_defaults_to_no_narrowing(self): + queryset = Finding.objects.all() + self.assertIs(self.command.apply_extra_scope(queryset, {}), queryset) + + def test_extra_scope_description_defaults_to_nothing(self): + self.assertEqual(self.command.describe_extra_scope({}), []) + + def test_the_default_hash_generator_is_the_module_level_one(self): + self.assertIs(self.command.hash_code_generator(), generate_hash_code) + + def test_there_are_no_extra_hashes_by_default(self): + """The base edition stores only Finding.hash_code, so this is a no-op.""" + self.assertIsNone(self.command.recompute_extra_hashes(Finding.objects.none(), None)) + + def test_adding_extra_arguments_defaults_to_nothing(self): + """A subclass that adds no arguments must not have to define the hook.""" + with patch("argparse.ArgumentParser.add_argument") as mock_add: + self.assertIsNone(self.command.add_extra_arguments(None)) + mock_add.assert_not_called() + + +@override_settings(V3_FEATURE_LOCATIONS=True) +class TestDedupeCommandCallsItsHooks(DojoTestCase): + + """A subclass's hooks have to actually be reached by the run.""" + + def setUp(self): + super().setUp() + self.user, _ = User.objects.get_or_create(username="admin") + product_type = Product_Type.objects.create(name="Org for dedupe hooks") + self.product = Product.objects.create( + name="Product for dedupe hooks", + description="hook fixture", + prod_type=product_type, + ) + engagement = Engagement.objects.create( + name="Engagement", + product=self.product, + target_start=timezone.now(), + target_end=timezone.now(), + ) + self.test = Test.objects.create( + engagement=engagement, + test_type=Test_Type.objects.get_or_create(name="Manual Test")[0], + target_start=timezone.now(), + target_end=timezone.now(), + ) + Finding.objects.create( + test=self.test, + title="Finding for dedupe hooks", + severity="High", + description="hook fixture", + mitigation="n/a", + impact="n/a", + reporter=self.user, + ) + + def _subclass(self): + """A subclass recording what the run asked of it.""" + calls = [] + + class Subclassed(Command): + def apply_extra_scope(self, findings, options): + calls.append(("apply_extra_scope", options.get("marker"))) + return findings.filter(title__startswith="Finding") + + def describe_extra_scope(self, options): + calls.append(("describe_extra_scope", None)) + return ["marker=on"] + + def hash_code_generator(self): + calls.append(("hash_code_generator", None)) + return generate_hash_code + + def recompute_extra_hashes(self, findings, writer): + calls.append(("recompute_extra_hashes", None)) + + return Subclassed(), calls + + def test_the_hash_phase_goes_through_the_hooks(self): + command, calls = self._subclass() + + command.handle(**DEFAULT_OPTIONS, marker="on") + + called = [name for name, _ in calls] + self.assertIn("apply_extra_scope", called) + self.assertIn("describe_extra_scope", called) + self.assertIn("hash_code_generator", called) + self.assertIn( + "recompute_extra_hashes", called, + msg="an edition with extra hash fields must get its second pass", + ) + + def test_extra_scope_receives_the_parsed_options(self): + """A subclass reads its own arguments out of options, so they have to be passed through.""" + command, calls = self._subclass() + + command.handle(**DEFAULT_OPTIONS, marker="on") + + self.assertIn(("apply_extra_scope", "on"), calls) + + def test_dedupe_only_skips_the_extra_hash_pass(self): + """--dedupe_only means recompute nothing, including the edition's extra hashes.""" + command, calls = self._subclass() + + with patch.object(Command, "_dedupe_batch_mode"): + command.handle(**{**DEFAULT_OPTIONS, "hash_code_only": False, "dedupe_only": True}, marker="on") + + self.assertNotIn("recompute_extra_hashes", [name for name, _ in calls]) + + def test_the_batch_dedupe_hooks_are_used(self): + """Both batch paths route through the overridable hooks rather than calling directly.""" + command, _ = self._subclass() + + with ( + patch.object(Command, "dedupe_batch_async") as mock_async, + patch("dojo.management.commands.dedupe.get_system_setting", return_value=True), + ): + command.handle(**{**DEFAULT_OPTIONS, "hash_code_only": False}, marker="on") + + mock_async.assert_called() + + def test_grading_goes_through_its_hook_in_sync_mode(self): + command, _ = self._subclass() + + with ( + patch.object(Command, "grade_product") as mock_grade, + patch.object(Command, "_dedupe_batch_mode"), + patch("dojo.management.commands.dedupe.get_system_setting", return_value=True), + ): + command.handle(**{**DEFAULT_OPTIONS, "hash_code_only": False, "dedupe_sync": True}, marker="on") + + mock_grade.assert_called()