From 4eb0e727b9bb35e78e91102c0b8a1c2b988dd842 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Thu, 28 May 2026 14:26:00 -0400 Subject: [PATCH] Fixes #22301: Avoid reverse relation name collision among tagged models (#22323) --- netbox/extras/managers.py | 29 ++++++++++++++- netbox/extras/tests/test_tags.py | 49 ++++++++++++++++++++++++++ netbox/netbox/models/features.py | 10 +++--- netbox/utilities/testing/base.py | 4 ++- netbox/utilities/testing/filtersets.py | 2 +- 5 files changed, 87 insertions(+), 7 deletions(-) diff --git a/netbox/extras/managers.py b/netbox/extras/managers.py index bd8ccf1ca..450af466e 100644 --- a/netbox/extras/managers.py +++ b/netbox/extras/managers.py @@ -1,10 +1,11 @@ from django.db import router from django.db.models import signals -from taggit.managers import _TaggableManager +from taggit.managers import TaggableManager, _TaggableManager from taggit.utils import require_instance_manager __all__ = ( 'NetBoxTaggableManager', + 'NetBoxTaggableManagerField', ) @@ -65,3 +66,29 @@ class NetBoxTaggableManager(_TaggableManager): pk_set=new_ids, using=db, ) + + +class NetBoxTaggableManagerField(TaggableManager): + """ + Subclass of taggit's TaggableManager that interpolates `%(app_label)s` and `%(class)s` in + `related_name`. taggit's contribute_to_class() bypasses Django's RelatedField, which is what + normally performs this substitution, so without this two taggable models that share a class + name (e.g. from different plugins) collide on Tag's reverse accessor. + """ + def contribute_to_class(self, cls, name): + super().contribute_to_class(cls, name) + if not cls._meta.abstract and self.remote_field.related_name: + self.remote_field.related_name = self.remote_field.related_name % { + 'class': cls.__name__.lower(), + 'app_label': cls._meta.app_label.lower(), + } + + def deconstruct(self): + # Emit the upstream taggit path and omit related_name so existing migrations remain + # equivalent and no AlterField is produced for every TagsMixin consumer. related_name + # has no effect on the database schema; it is reapplied on model load. Only safe while + # this subclass adds no field attributes that affect schema — if that changes, restore + # the real path so migrations capture the diff. + name, _path, args, kwargs = super().deconstruct() + kwargs.pop('related_name', None) + return name, 'taggit.managers.TaggableManager', args, kwargs diff --git a/netbox/extras/tests/test_tags.py b/netbox/extras/tests/test_tags.py index 7de2b4889..f2f61f4ec 100644 --- a/netbox/extras/tests/test_tags.py +++ b/netbox/extras/tests/test_tags.py @@ -1,7 +1,12 @@ +from django.apps import apps +from django.db import models +from django.test import SimpleTestCase from django.urls import reverse from rest_framework import status from dcim.models import Site +from extras.managers import NetBoxTaggableManagerField +from netbox.models.features import TagsMixin from utilities.testing import APITestCase, create_tags @@ -77,3 +82,47 @@ class TaggedItemTestCase(APITestCase): self.assertEqual(len(response.data['tags']), 0) site = Site.objects.get(pk=response.data['id']) self.assertEqual(len(site.tags.all()), 0) + + +class TagsMixinCollisionTestCase(SimpleTestCase): + """ + Two TagsMixin-derived models that share a class name (e.g. introduced by separate plugins) + must not collide on Tag's reverse accessor. Regression test for #22301. + """ + def _make_taggable_model(self, app_label, name='Foo'): + # Build a model dynamically and register a cleanup to remove it from the global app + # registry so subsequent test runs (and other tests in this process) don't see ghost + # models leak in via apps.get_models(). + model = type(name, (TagsMixin, models.Model), { + '__module__': self.__class__.__module__, + 'Meta': type('Meta', (), {'app_label': app_label}), + }) + self.addCleanup(apps.all_models[app_label].pop, name.lower(), None) + self.addCleanup(apps.clear_cache) + return model + + def test_same_named_taggable_models_in_different_apps(self): + foo_a = self._make_taggable_model('dcim') + foo_b = self._make_taggable_model('ipam') + + # Without the fix, Django's system checks raise fields.E304 on these two models. + self.assertEqual(foo_a.check(), []) + self.assertEqual(foo_b.check(), []) + + # Each model should resolve to its own unique related_name on the Tag relation. + rn_a = foo_a._meta.get_field('tags').remote_field.related_name + rn_b = foo_b._meta.get_field('tags').remote_field.related_name + self.assertNotEqual(rn_a, rn_b) + + def test_deconstruct_emits_upstream_path(self): + """ + NetBoxTaggableManagerField.deconstruct() must emit the upstream taggit path and drop + related_name, so existing migrations remain valid and no AlterField is produced for + every TagsMixin consumer. + """ + model = self._make_taggable_model('dcim', name='DeconstructProbe') + field = model._meta.get_field('tags') + self.assertIsInstance(field, NetBoxTaggableManagerField) + _name, path, _args, kwargs = field.deconstruct() + self.assertEqual(path, 'taggit.managers.TaggableManager') + self.assertNotIn('related_name', kwargs) diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index 993828c1b..1556a22a1 100644 --- a/netbox/netbox/models/features.py +++ b/netbox/netbox/models/features.py @@ -9,13 +9,12 @@ from django.db import models from django.db.models import Q from django.utils import timezone from django.utils.translation import gettext_lazy as _ -from taggit.managers import TaggableManager from core.choices import JobStatusChoices, ObjectChangeActionChoices from core.models import ObjectType from extras.choices import * from extras.constants import CUSTOMFIELD_EMPTY_VALUES -from extras.managers import NetBoxTaggableManager +from extras.managers import NetBoxTaggableManager, NetBoxTaggableManagerField from extras.utils import is_taggable from netbox.config import get_config from netbox.constants import CORE_APPS @@ -489,12 +488,15 @@ class JournalingMixin(models.Model): class TagsMixin(models.Model): """ Enables support for tag assignment. Assigned tags can be managed via the `tags` attribute, - which is a `NetBoxTaggableManager` instance. + which is a `NetBoxTaggableManager` instance. The field is a `NetBoxTaggableManagerField`, + which performs `%(app_label)s` / `%(class)s` interpolation on `related_name` to avoid + reverse-accessor collisions between same-named models in different apps (e.g. plugins). """ - tags = TaggableManager( + tags = NetBoxTaggableManagerField( through='extras.TaggedItem', ordering=('weight', 'name'), manager=NetBoxTaggableManager, + related_name='%(app_label)s_%(class)s_tagged+', ) class Meta: diff --git a/netbox/utilities/testing/base.py b/netbox/utilities/testing/base.py index 1eb1a17b3..12e815f2e 100644 --- a/netbox/utilities/testing/base.py +++ b/netbox/utilities/testing/base.py @@ -165,7 +165,9 @@ class ModelTestCase(TestCase): continue # Handle ManyToManyFields - if value and type(field) in (ManyToManyField, ManyToManyRel, TaggableManager): + if value and ( + type(field) in (ManyToManyField, ManyToManyRel) or isinstance(field, TaggableManager) + ): # Resolve reverse M2M relationships if isinstance(field, ManyToManyRel): value = getattr(instance, field.related_name).all() diff --git a/netbox/utilities/testing/filtersets.py b/netbox/utilities/testing/filtersets.py index bb200a43e..7b1a7d038 100644 --- a/netbox/utilities/testing/filtersets.py +++ b/netbox/utilities/testing/filtersets.py @@ -82,7 +82,7 @@ class BaseFilterSetTests: return [(f'{filter_name}_id', django_filters.ModelMultipleChoiceFilter)] # Tag manager - if type(field) is TaggableManager: + if isinstance(field, TaggableManager): return [('tag', TagFilter)] # Unable to determine the correct filter class