From 3fa6c52b437b5916aad79b52d44c2b1ede52141e Mon Sep 17 00:00:00 2001 From: Cameron Hobbs Date: Mon, 15 Jun 2026 13:49:51 +0100 Subject: [PATCH] Fix key for loaded/consumed to avoid cross-app collisions --- ARCHITECTURE.md | 12 +++-- README.md | 2 +- eagle/sinks.py | 40 +++++++++++++--- eagle/unused/marker.py | 30 ++++++------ eagle/unused/state.py | 5 +- eagle/unused/tracker.py | 13 +++-- tests/collision_app/__init__.py | 0 tests/collision_app/models.py | 14 ++++++ tests/factories.py | 8 ++++ tests/test_cross_app_collision.py | 79 +++++++++++++++++++++++++++++++ tests/test_project/settings.py | 3 +- 11 files changed, 172 insertions(+), 34 deletions(-) create mode 100644 tests/collision_app/__init__.py create mode 100644 tests/collision_app/models.py create mode 100644 tests/test_cross_app_collision.py diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 7cc0d17..cb8f4ed 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -182,8 +182,8 @@ _state: ContextVar[_CollectorState] @dataclass class _CollectorState: active: bool # is a request being tracked? - loaded: dict[(model_name, cache_name), LoadedRelation] # kind + call-site - consumed: set[(model_name, cache_name)] # relations that were accessed + loaded: dict[(model_label, cache_name), LoadedRelation] # kind + call-site + consumed: set[(model_label, cache_name)] # relations that were accessed ``` - **Context-local** (a `contextvars.ContextVar`) so concurrent @@ -193,8 +193,12 @@ class _CollectorState: request's `begin_request()` wipe another's state mid-`await`). `begin_request()` / `end_request()` bind a *fresh* state object rather than mutating in place, so a request starting while another is suspended cannot clobber the suspended request's tracking. -- **Keyed by `(model_name, cache_name)`** — model *name* (not class) and the ORM cache key - for the relation. This is why `mark_considered` accepts a model name string. +- **Keyed by `(model_label, cache_name)`** — the model's `_meta.label` + (`app_label.ModelName`) and the ORM cache key for the relation. The label, rather than the + bare class name, keeps same-named models in different apps (`billing.Comment` vs + `blog.Comment`) from colliding and masking each other's warnings. `mark_considered` resolves + its argument to this label, accepting a model class, an `app_label.ModelName` string, or a + bare class name (resolved via the app registry when unambiguous). - **First write wins** for `loaded`, so the originally captured call-site survives repeated loads. diff --git a/README.md b/README.md index 25ca2a8..4504352 100644 --- a/README.md +++ b/README.md @@ -160,7 +160,7 @@ Reach for one of these escape hatches: ### `mark_considered` — mark relations as accessed imperatively -Call it with the model first (a class or its name string), followed by one or more relation cache names. Eagle then treats those relations as accessed for the rest of the current request: +Call it with the model first, followed by one or more relation cache names. Eagle then treats those relations as accessed for the rest of the current request. The model can be a class, an `app_label.ModelName` label string, or a bare class name; pass the class or the label to disambiguate when two apps define a model with the same class name: ```python from eagle import mark_considered diff --git a/eagle/sinks.py b/eagle/sinks.py index 027571b..0568626 100644 --- a/eagle/sinks.py +++ b/eagle/sinks.py @@ -3,6 +3,7 @@ from collections.abc import Callable from typing import ParamSpec, TypeVar +from django.apps import apps from django.db.models import Model from eagle.logger import logger @@ -14,17 +15,42 @@ def _normalize_model(model: type[Model] | str) -> str: """ - Return the string model name regardless of whether *model* is a class or already a string. + Resolve *model* to its Django label (``app_label.ModelName``) so it matches tracked keys. + + Tracking keys relations on ``model._meta.label`` to keep same-named models in different apps + distinct, so the public escape hatches must resolve their argument to that same labelled form. Args: - model: A Django model class or its string name. + model: A Django model class, a label string (``app_label.ModelName``), or a bare class name. Returns: - The model's ``__name__`` if a class is provided, otherwise the value cast to str. + The model's ``_meta.label``. A class or labelled string resolves directly; a bare class name + resolves via the app registry when exactly one model matches. The input is returned unchanged + when it cannot be resolved (an unmatched or ambiguous bare name), which simply fails to match. """ if isinstance(model, type): - return model.__name__ - return str(model) + return model._meta.label + + name = str(model) + + if "." in name: + try: + return apps.get_model(name)._meta.label + except (LookupError, ValueError): + return name + + matches = [registered._meta.label for registered in apps.get_models() if registered.__name__ == name] + if len(matches) == 1: + return matches[0] + + if len(matches) > 1: + logger.warning( + "mark_considered/may_access got ambiguous model name %r matching %s; " + "pass the model class or a labelled 'app_label.ModelName' string to disambiguate.", + name, + matches, + ) + return name def mark_considered(model: type[Model] | str, *cache_names: str) -> None: @@ -32,7 +58,7 @@ def mark_considered(model: type[Model] | str, *cache_names: str) -> None: Suppress warnings for *cache_names* on *model* in the current request. Args: - model: The Django model class or its string name. + model: The Django model class, a label string (``app_label.ModelName``), or a bare class name. """ normalized = _normalize_model(model) logger.debug("Marking considered %s, %s.", normalized, cache_names) @@ -44,7 +70,7 @@ def may_access(model: type[Model] | str, *cache_names: str) -> Callable[[Callabl Decorator that suppresses warnings for *cache_names* on *model* after the decorated function returns. Args: - model: The Django model class or its string name. + model: The Django model class, a label string (``app_label.ModelName``), or a bare class name. Returns: A decorator that wraps the function and calls mark_considered_internal after it executes. diff --git a/eagle/unused/marker.py b/eagle/unused/marker.py index f23f883..4130a66 100644 --- a/eagle/unused/marker.py +++ b/eagle/unused/marker.py @@ -6,12 +6,12 @@ from eagle.unused.state import LoadedRelation, collector -def _record_loaded(model_name: str, cache_name: str, kind: str, location: str | None) -> None: +def _record_loaded(model_label: str, cache_name: str, kind: str, location: str | None) -> None: """ Register a relation as loaded; first write wins so repeated loads don't overwrite the original location. Args: - model_name: Django model class name. + model_label: Django model label (``app_label.ModelName``). cache_name: ORM cache key for the relation (e.g. ``_author_cache``). kind: Relation type string, either ``"select_related"`` or ``"prefetch_related"``. location: Call-site string (``"file:line"``) captured when the queryset was built, or None. @@ -20,7 +20,7 @@ def _record_loaded(model_name: str, cache_name: str, kind: str, location: str | return collector.loaded.setdefault( - (model_name, cache_name), + (model_label, cache_name), LoadedRelation( kind=kind, location=location, @@ -28,33 +28,33 @@ def _record_loaded(model_name: str, cache_name: str, kind: str, location: str | ) -def _record_consumed(model_name: str, cache_name: str) -> None: +def _record_consumed(model_label: str, cache_name: str) -> None: """ Mark a relation key as consumed so it will not trigger an unused warning. Args: - model_name: Django model class name. + model_label: Django model label (``app_label.ModelName``). cache_name: ORM cache key for the relation. """ if not collector.active: - logger.debug("Tried recording consumed %s, %s... but collector is not active.", model_name, cache_name) + logger.debug("Tried recording consumed %s, %s... but collector is not active.", model_label, cache_name) return - collector.consumed.add((model_name, cache_name)) - logger.debug("Recorded consumed %s, %s.", model_name, cache_name) + collector.consumed.add((model_label, cache_name)) + logger.debug("Recorded consumed %s, %s.", model_label, cache_name) -def mark_considered_internal(model_name: str, *cache_names: str) -> None: +def mark_considered_internal(model_label: str, *cache_names: str) -> None: """ - Suppress warnings for *cache_names* on *model_name* without going through the public API. + Suppress warnings for *cache_names* on *model_label* without going through the public API. Args: - model_name: Django model class name. + model_label: Django model label (``app_label.ModelName``). """ if not collector.active: return - collector.consumed.update((model_name, cache_name) for cache_name in cache_names) + collector.consumed.update((model_label, cache_name) for cache_name in cache_names) def init_state(instance: Model, location: str | None, locations: dict[str, str] | None = None) -> None: @@ -111,7 +111,7 @@ def mark_select_related(instance: Model, cache_name: str) -> None: return _record_loaded( - model_name=instance.__class__.__name__, + model_label=instance._meta.label, cache_name=cache_name, kind="select_related", location=resolve_location(instance._state, cache_name), @@ -139,7 +139,7 @@ def mark_prefetched(instances: Iterable[Model], cache_name: str | None) -> None: continue _record_loaded( - model_name=instance.__class__.__name__, + model_label=instance._meta.label, cache_name=cache_name, kind="prefetch_related", location=resolve_location(state, cache_name), @@ -160,7 +160,7 @@ def mark_consumed(instance: Model, cache_name: str) -> None: return _record_consumed( - instance.__class__.__name__, + instance._meta.label, cache_name, ) logger.debug("Marked consumed %s, %s.", instance.__class__.__name__, cache_name) diff --git a/eagle/unused/state.py b/eagle/unused/state.py index 3a30563..25729f5 100644 --- a/eagle/unused/state.py +++ b/eagle/unused/state.py @@ -10,6 +10,9 @@ class LoadedRelation: location: str | None +# Keyed by ``(model_label, cache_name)`` where ``model_label`` is ``model._meta.label`` +# (``app_label.ModelName``). Using the labelled form rather than the bare class name keeps +# same-named models in different apps (e.g. ``billing.Comment`` vs ``blog.Comment``) distinct. RelationKey = tuple[str, str] @@ -61,7 +64,7 @@ def active(self) -> bool: @property def loaded(self) -> dict[RelationKey, LoadedRelation]: """ - Relations eager-loaded during this request, keyed by ``(model_name, cache_name)``. + Relations eager-loaded during this request, keyed by ``(model_label, cache_name)``. Returns: The mutable map of loaded relations for the current context. diff --git a/eagle/unused/tracker.py b/eagle/unused/tracker.py index 61d92ac..31c3e65 100644 --- a/eagle/unused/tracker.py +++ b/eagle/unused/tracker.py @@ -29,21 +29,24 @@ def end_request() -> None: if not collector.active: return - for (model_name, cache_name), relation in sorted(collector.loaded.items()): - key = (model_name, cache_name) - + for key, relation in sorted(collector.loaded.items()): if key in collector.consumed: continue + model_label, cache_name = key + # Keys carry the full label (app_label.ModelName); ignore rules and warning messages + # speak in the bare class name, which is the segment after the final dot. + model_name = model_label.rsplit(".", 1)[-1] + if should_ignore( model_name, cache_name, relation.location, ): - logger.debug("Ignoring %s, %s, %s", model_name, cache_name, relation.location) + logger.debug("Ignoring %s, %s, %s", model_label, cache_name, relation.location) continue - logger.debug("Found unused %s, %s, %s", model_name, cache_name, relation.location) + logger.debug("Found unused %s, %s, %s", model_label, cache_name, relation.location) _emit_unused_warning( model_name=model_name, cache_name=cache_name, diff --git a/tests/collision_app/__init__.py b/tests/collision_app/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/collision_app/models.py b/tests/collision_app/models.py new file mode 100644 index 0000000..5d7a8f9 --- /dev/null +++ b/tests/collision_app/models.py @@ -0,0 +1,14 @@ +from django.db import models + +from test_project.models import Location + + +class Eagle(models.Model): + """ + An eagle in a second tracked app whose class name deliberately collides with ``test_project.Eagle``. + + Used by the cross-app collision regression test: it carries a ``location`` relation with the same + cache name as ``test_project.Eagle.location`` so the two only stay distinct when keyed by label. + """ + + location = models.ForeignKey(Location, models.CASCADE, null=True, related_name="collision_visitors") diff --git a/tests/factories.py b/tests/factories.py index 4a71864..2a7614e 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -2,6 +2,7 @@ import factory +from collision_app.models import Eagle as CollisionEagle from excluded_app.models import Burrow from test_project.models import Climate, Eagle, Eaglet, Location @@ -50,3 +51,10 @@ class Meta: eagle = factory.SubFactory(EagleFactory) depth = 3 + + +class CollisionEagleFactory(factory.django.DjangoModelFactory): + class Meta: + model = CollisionEagle + + location = factory.SubFactory(LocationFactory) diff --git a/tests/test_cross_app_collision.py b/tests/test_cross_app_collision.py new file mode 100644 index 0000000..28b2cfa --- /dev/null +++ b/tests/test_cross_app_collision.py @@ -0,0 +1,79 @@ +import pytest + +from collision_app.models import Eagle as CollisionEagle +from eagle import UnusedRelatedAccess, mark_considered, unused +from test_project.models import Eagle +from tests.base import MayAccessHelperTestBase +from tests.factories import CollisionEagleFactory, EagleFactory + + +class TestCrossAppCollision(MayAccessHelperTestBase): + """ + Same-named models in different apps must not share loaded/consumed keys. + + ``test_project.Eagle`` and ``collision_app.Eagle`` have the same class name and a ``location`` + relation with the same cache name. Keys are built from ``model._meta.label`` so a consumed + relation on one app's model cannot mask an unused one on the other's. + """ + + def test_consumed_relation_does_not_mask_unused_on_same_named_model(self): + # collision_app.Eagle.location is loaded and accessed -> consumed. + consumed = CollisionEagleFactory() + loaded_collision = CollisionEagle.objects.select_related("location").get(pk=consumed.pk) + assert loaded_collision.location is not None + + # test_project.Eagle.location is loaded but never accessed -> unused. + unused_eagle = EagleFactory() + Eagle.objects.select_related("location").filter(pk=unused_eagle.pk).first() + + # When keyed by bare class name both relations collided on ("Eagle", "location"), so the + # consumed collision_app load masked this unused test_project load and no warning fired. + with pytest.raises(UnusedRelatedAccess) as exc_info: + unused.end_request() + assert 'select_related("location")' in str(exc_info.value) + + def test_consumed_relation_on_test_project_model_does_not_mask_collision_app(self): + # Reverse roles: test_project.Eagle.location is consumed, collision_app.Eagle.location is unused. + consumed = EagleFactory() + loaded_eagle = Eagle.objects.select_related("location").get(pk=consumed.pk) + assert loaded_eagle.location is not None + + unused_collision = CollisionEagleFactory() + CollisionEagle.objects.select_related("location").filter(pk=unused_collision.pk).first() + + with pytest.raises(UnusedRelatedAccess) as exc_info: + unused.end_request() + assert 'select_related("location")' in str(exc_info.value) + + def test_consuming_both_same_named_models_emits_no_warning(self): + # Both apps' relations are loaded and accessed; ending the request must stay silent. + from_test_project = EagleFactory() + loaded_eagle = Eagle.objects.select_related("location").get(pk=from_test_project.pk) + assert loaded_eagle.location is not None + + from_collision = CollisionEagleFactory() + loaded_collision = CollisionEagle.objects.select_related("location").get(pk=from_collision.pk) + assert loaded_collision.location is not None + + # A surviving unused relation would raise here (warnings are errors); silence means success. + unused.end_request() + + def test_mark_considered_labelled_form_suppresses_matching_model(self): + eagle = EagleFactory() + Eagle.objects.select_related("location").filter(pk=eagle.pk).first() + + # The labelled "app_label.ModelName" form resolves to the same key the load was recorded under. + mark_considered("test_project.Eagle", "location") + + unused.end_request() + + def test_mark_considered_labelled_form_does_not_suppress_other_app(self): + eagle = EagleFactory() + Eagle.objects.select_related("location").filter(pk=eagle.pk).first() + + # Marking the same-named model in a different app must not suppress this load. + mark_considered("collision_app.Eagle", "location") + + with pytest.raises(UnusedRelatedAccess) as exc_info: + unused.end_request() + assert 'select_related("location")' in str(exc_info.value) diff --git a/tests/test_project/settings.py b/tests/test_project/settings.py index 09f12d0..ba633c2 100644 --- a/tests/test_project/settings.py +++ b/tests/test_project/settings.py @@ -13,6 +13,7 @@ "eagle", "test_project", "excluded_app", + "collision_app", ] EAGLE_ENABLED = True @@ -30,7 +31,7 @@ "DEFAULT_RENDERER_CLASSES": ["rest_framework.renderers.JSONRenderer"], } -MIGRATION_MODULES = {"test_project": None, "excluded_app": None} +MIGRATION_MODULES = {"test_project": None, "excluded_app": None, "collision_app": None} DEFAULT_AUTO_FIELD = "django.db.models.AutoField"