Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.

Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 33 additions & 7 deletions eagle/sinks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -14,25 +15,50 @@

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:
"""
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)
Expand All @@ -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.
Expand Down
30 changes: 15 additions & 15 deletions eagle/unused/marker.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -20,41 +20,41 @@ 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,
),
)


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:
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand All @@ -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)
Expand Down
5 changes: 4 additions & 1 deletion eagle/unused/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]


Expand Down Expand Up @@ -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.
Expand Down
13 changes: 8 additions & 5 deletions eagle/unused/tracker.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Empty file added tests/collision_app/__init__.py
Empty file.
14 changes: 14 additions & 0 deletions tests/collision_app/models.py
Original file line number Diff line number Diff line change
@@ -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")
8 changes: 8 additions & 0 deletions tests/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
79 changes: 79 additions & 0 deletions tests/test_cross_app_collision.py
Original file line number Diff line number Diff line change
@@ -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)
3 changes: 2 additions & 1 deletion tests/test_project/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"eagle",
"test_project",
"excluded_app",
"collision_app",
]

EAGLE_ENABLED = True
Expand All @@ -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"

Expand Down
Loading