From f1cc22e13143cdb6c58c19a0b3f6e857a4ee6973 Mon Sep 17 00:00:00 2001 From: Antoine Wood Date: Sun, 14 Jul 2024 13:15:38 -0400 Subject: [PATCH 01/12] 708 - Add Natural Key Support to Tags model. --- taggit/managers.py | 15 +++++++ taggit/models.py | 35 ++++++++++++++++- tests/tests.py | 98 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 146 insertions(+), 2 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index 0ff8e976..6d02eda5 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -1,5 +1,6 @@ import uuid from operator import attrgetter +from typing import List from django.conf import settings from django.contrib.contenttypes.fields import GenericRelation @@ -56,6 +57,20 @@ def clone(self): return type(self)(self.alias, self.col, self.content_types[:]) +class NaturalKeyManager(models.Manager): + def __init__(self, natural_key_fields: List[str], *args, **kwargs): + super().__init__(*args, **kwargs) + self.natural_key_fields = natural_key_fields + + def get_by_natural_key(self, *args): + if len(args) != len(self.model.natural_key_fields): + raise ValueError( + "Number of arguments does not match number of natural key fields." + ) + lookup_kwargs = dict(zip(self.model.natural_key_fields, args)) + return self.get(**lookup_kwargs) + + class _TaggableManager(models.Manager): # TODO investigate whether we can use a RelatedManager instead of all this stuff # to take advantage of all the Django goodness diff --git a/taggit/models.py b/taggit/models.py index 8d7f60bd..43064d43 100644 --- a/taggit/models.py +++ b/taggit/models.py @@ -1,3 +1,5 @@ +from typing import List + from django.conf import settings from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType @@ -15,7 +17,29 @@ def unidecode(tag): return tag -class TagBase(models.Model): +class NaturalKeyManager(models.Manager): + def __init__(self, natural_key_fields: List[str], *args, **kwargs): + super().__init__(*args, **kwargs) + self.natural_key_fields = natural_key_fields + + def get_by_natural_key(self, *args): + if len(args) != len(self.model.natural_key_fields): + raise ValueError( + "Number of arguments does not match number of natural key fields." + ) + lookup_kwargs = dict(zip(self.model.natural_key_fields, args)) + return self.get(**lookup_kwargs) + + +class NaturalKeyModel(models.Model): + def natural_key(self): + return (getattr(self, field) for field in self.natural_key_fields) + + class Meta: + abstract = True + + +class TagBase(NaturalKeyModel): name = models.CharField( verbose_name=pgettext_lazy("A tag name", "name"), unique=True, max_length=100 ) @@ -26,6 +50,9 @@ class TagBase(models.Model): allow_unicode=True, ) + natural_key_fields = ["name"] + objects = NaturalKeyManager(natural_key_fields) + def __str__(self): return self.name @@ -91,13 +118,15 @@ class Meta: app_label = "taggit" -class ItemBase(models.Model): +class ItemBase(NaturalKeyModel): def __str__(self): return gettext("%(object)s tagged with %(tag)s") % { "object": self.content_object, "tag": self.tag, } + objects = NaturalKeyManager(natural_key_fields=["name"]) + class Meta: abstract = True @@ -170,6 +199,7 @@ def tags_for(cls, model, instance=None, **extra_filters): class GenericTaggedItemBase(CommonGenericTaggedItemBase): object_id = models.IntegerField(verbose_name=_("object ID"), db_index=True) + natural_key_fields = ["object_id"] class Meta: abstract = True @@ -177,6 +207,7 @@ class Meta: class GenericUUIDTaggedItemBase(CommonGenericTaggedItemBase): object_id = models.UUIDField(verbose_name=_("object ID"), db_index=True) + natural_key_fields = ["object_id"] class Meta: abstract = True diff --git a/tests/tests.py b/tests/tests.py index 79489598..7b041db8 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -1,3 +1,4 @@ +import os from io import StringIO from unittest import mock @@ -1398,3 +1399,100 @@ def test_tests_have_no_pending_migrations(self): out = StringIO() call_command("makemigrations", "tests", dry_run=True, stdout=out) self.assertEqual(out.getvalue().strip(), "No changes detected in app 'tests'") + + +class NaturalKeyTests(TestCase): + @classmethod + def setUpTestData(cls): + cls.tag_names = ["circle", "square", "triangle", "rectangle", "pentagon"] + cls.filename = "test_data_dump.json" + cls.tag_count = len(cls.tag_names) + + def setUp(self): + self.tags = self._create_tags() + + def tearDown(self): + self._clear_existing_tags() + try: + os.remove(self.filename) + except FileNotFoundError: + pass + + @property + def _queryset(self): + return Tag.objects.filter(name__in=self.tag_names) + + def _create_tags(self): + return Tag.objects.bulk_create( + [Tag(name=shape, slug=shape) for shape in self.tag_names], + ignore_conflicts=True, + ) + + def _clear_existing_tags(self): + self._queryset.delete() + + def _dump_model(self, model): + model_label = model._meta.label + with open(self.filename, "w") as f: + call_command( + "dumpdata", + model_label, + natural_primary=True, + use_natural_foreign_keys=True, + stdout=f, + ) + + def _load_model(self): + call_command("loaddata", self.filename) + + def test_tag_natural_key(self): + """ + Test that tags can be dumped and loaded using natural keys. + """ + + # confirm count in the DB + self.assertEqual(self._queryset.count(), self.tag_count) + + # dump all tags to a file + self._dump_model(Tag) + + # Delete all tags + self._clear_existing_tags() + + # confirm all tags clear + self.assertEqual(self._queryset.count(), 0) + + # load the tags from the file + self._load_model() + + # confirm count in the DB + self.assertEqual(self._queryset.count(), self.tag_count) + + def test_tag_reloading_with_changed_pk(self): + """Test that tags are not reliant on the primary key of the tag model. + + Test that data is correctly loaded after database state has changed. + + """ + original_shape = self._queryset.first() + original_pk = original_shape.pk + original_shape_name = original_shape.name + new_shape_name = "hexagon" + + # dump all tags to a file + self._dump_model(Tag) + + # Delete the tag + self._clear_existing_tags() + + # create new tag with the same PK + Tag.objects.create(name=new_shape_name, slug=new_shape_name, pk=original_pk) + + # Load the tags from the file + self._load_model() + + # confirm that load did not overwrite the new_shape + self.assertEqual(Tag.objects.get(pk=original_pk).name, new_shape_name) + + # confirm that the original shape was reloaded with a different PK + self.assertNotEqual(Tag.objects.get(name=original_shape_name).pk, original_pk) From 743e9aef05e88eeaf8155b9b521a6d40639ce671 Mon Sep 17 00:00:00 2001 From: Antoine Wood Date: Sun, 14 Jul 2024 13:30:53 -0400 Subject: [PATCH 02/12] 708 - Update ChangeLog --- CHANGELOG.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 28f5cf50..874aa235 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -10,6 +10,7 @@ Changelog We believe that this should not cause a noticable performance change, and the number of queries involved should not change. * Add Django 5.0 support (no code changes were needed, but now we test this release). * Add Python 3.12 support +* Add Support for dumpdata/loaddata using Natural Keys 5.0.1 (2023-10-26) ~~~~~~~~~~~~~~~~~~ From 96394987f532b83e72ab69c5e96f3b32fa143d99 Mon Sep 17 00:00:00 2001 From: Antoine Wood Date: Mon, 15 Jul 2024 19:30:51 -0400 Subject: [PATCH 03/12] 708 - Remove Duplicate Manager Class --- taggit/models.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/taggit/models.py b/taggit/models.py index 43064d43..d30f2a23 100644 --- a/taggit/models.py +++ b/taggit/models.py @@ -9,6 +9,8 @@ from django.utils.translation import gettext_lazy as _ from django.utils.translation import pgettext_lazy +from taggit.managers import NaturalKeyManager + try: from unidecode import unidecode except ImportError: @@ -17,20 +19,6 @@ def unidecode(tag): return tag -class NaturalKeyManager(models.Manager): - def __init__(self, natural_key_fields: List[str], *args, **kwargs): - super().__init__(*args, **kwargs) - self.natural_key_fields = natural_key_fields - - def get_by_natural_key(self, *args): - if len(args) != len(self.model.natural_key_fields): - raise ValueError( - "Number of arguments does not match number of natural key fields." - ) - lookup_kwargs = dict(zip(self.model.natural_key_fields, args)) - return self.get(**lookup_kwargs) - - class NaturalKeyModel(models.Model): def natural_key(self): return (getattr(self, field) for field in self.natural_key_fields) From 1da39dc1876b49dea47f6eb0f2a64f3059063b6a Mon Sep 17 00:00:00 2001 From: Antoine Wood Date: Mon, 15 Jul 2024 19:34:07 -0400 Subject: [PATCH 04/12] 708 - Move Natural Key Manager back to models to avoid circular import --- taggit/managers.py | 14 -------------- taggit/models.py | 16 ++++++++++++++-- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index 6d02eda5..3c89ad91 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -57,20 +57,6 @@ def clone(self): return type(self)(self.alias, self.col, self.content_types[:]) -class NaturalKeyManager(models.Manager): - def __init__(self, natural_key_fields: List[str], *args, **kwargs): - super().__init__(*args, **kwargs) - self.natural_key_fields = natural_key_fields - - def get_by_natural_key(self, *args): - if len(args) != len(self.model.natural_key_fields): - raise ValueError( - "Number of arguments does not match number of natural key fields." - ) - lookup_kwargs = dict(zip(self.model.natural_key_fields, args)) - return self.get(**lookup_kwargs) - - class _TaggableManager(models.Manager): # TODO investigate whether we can use a RelatedManager instead of all this stuff # to take advantage of all the Django goodness diff --git a/taggit/models.py b/taggit/models.py index d30f2a23..43064d43 100644 --- a/taggit/models.py +++ b/taggit/models.py @@ -9,8 +9,6 @@ from django.utils.translation import gettext_lazy as _ from django.utils.translation import pgettext_lazy -from taggit.managers import NaturalKeyManager - try: from unidecode import unidecode except ImportError: @@ -19,6 +17,20 @@ def unidecode(tag): return tag +class NaturalKeyManager(models.Manager): + def __init__(self, natural_key_fields: List[str], *args, **kwargs): + super().__init__(*args, **kwargs) + self.natural_key_fields = natural_key_fields + + def get_by_natural_key(self, *args): + if len(args) != len(self.model.natural_key_fields): + raise ValueError( + "Number of arguments does not match number of natural key fields." + ) + lookup_kwargs = dict(zip(self.model.natural_key_fields, args)) + return self.get(**lookup_kwargs) + + class NaturalKeyModel(models.Model): def natural_key(self): return (getattr(self, field) for field in self.natural_key_fields) From 0de8a3890182c589d9d7a147558359d4449e3548 Mon Sep 17 00:00:00 2001 From: Antoine Wood Date: Mon, 15 Jul 2024 19:40:38 -0400 Subject: [PATCH 05/12] 708 - Remove unused List import --- taggit/managers.py | 1 - 1 file changed, 1 deletion(-) diff --git a/taggit/managers.py b/taggit/managers.py index 3c89ad91..0ff8e976 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -1,6 +1,5 @@ import uuid from operator import attrgetter -from typing import List from django.conf import settings from django.contrib.contenttypes.fields import GenericRelation From 46db2d6d83b8c0b2489c2dd6b26bf78c66c240a7 Mon Sep 17 00:00:00 2001 From: Antoine Wood Date: Wed, 17 Jul 2024 09:15:21 -0400 Subject: [PATCH 06/12] 708 - Clean up Natural Key Manager Implementation --- taggit/models.py | 8 ++------ tests/tests.py | 11 +++++++++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/taggit/models.py b/taggit/models.py index 43064d43..410d613e 100644 --- a/taggit/models.py +++ b/taggit/models.py @@ -18,10 +18,6 @@ def unidecode(tag): class NaturalKeyManager(models.Manager): - def __init__(self, natural_key_fields: List[str], *args, **kwargs): - super().__init__(*args, **kwargs) - self.natural_key_fields = natural_key_fields - def get_by_natural_key(self, *args): if len(args) != len(self.model.natural_key_fields): raise ValueError( @@ -51,7 +47,7 @@ class TagBase(NaturalKeyModel): ) natural_key_fields = ["name"] - objects = NaturalKeyManager(natural_key_fields) + objects = NaturalKeyManager() def __str__(self): return self.name @@ -125,7 +121,7 @@ def __str__(self): "tag": self.tag, } - objects = NaturalKeyManager(natural_key_fields=["name"]) + objects = NaturalKeyManager() class Meta: abstract = True diff --git a/tests/tests.py b/tests/tests.py index 7b041db8..38a14e40 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -1496,3 +1496,14 @@ def test_tag_reloading_with_changed_pk(self): # confirm that the original shape was reloaded with a different PK self.assertNotEqual(Tag.objects.get(name=original_shape_name).pk, original_pk) + + def test_get_by_natural_key(self): + # Test retrieval of tags by their natural key + for name in self.tag_names: + tag = Tag.objects.get_by_natural_key(name) + self.assertEqual(tag.name, name) + + def test_wrong_number_of_args(self): + # Test that get_by_natural_key raises an error when the wrong number of args is passed + with self.assertRaises(ValueError): + Tag.objects.get_by_natural_key() From 558ec17aafc27fdd30af1ed538810284244fea4a Mon Sep 17 00:00:00 2001 From: Antoine Wood Date: Wed, 17 Jul 2024 09:23:47 -0400 Subject: [PATCH 07/12] 708 - Remove unused import --- taggit/models.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/taggit/models.py b/taggit/models.py index 410d613e..17613c51 100644 --- a/taggit/models.py +++ b/taggit/models.py @@ -1,5 +1,3 @@ -from typing import List - from django.conf import settings from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType From 5ead6243419597eeb2f0c1e929dc9316ed7d2cec Mon Sep 17 00:00:00 2001 From: Antoine Wood Date: Sun, 21 Jul 2024 21:07:19 -0400 Subject: [PATCH 08/12] 708 - natural_key return type changed to string instead of generator to allow deserialization with natural-foreign key --- taggit/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/taggit/models.py b/taggit/models.py index 17613c51..091d733b 100644 --- a/taggit/models.py +++ b/taggit/models.py @@ -27,7 +27,7 @@ def get_by_natural_key(self, *args): class NaturalKeyModel(models.Model): def natural_key(self): - return (getattr(self, field) for field in self.natural_key_fields) + return [getattr(self, field) for field in self.natural_key_fields] class Meta: abstract = True From c2147e8f6a6ac0507279a4fd2c171c6dae67beb4 Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Thu, 25 Jul 2024 22:19:31 +1000 Subject: [PATCH 09/12] Add documentation details --- CHANGELOG.rst | 2 +- docs/index.rst | 1 + docs/testing.rst | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 docs/testing.rst diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 874aa235..24bc6c62 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -10,7 +10,7 @@ Changelog We believe that this should not cause a noticable performance change, and the number of queries involved should not change. * Add Django 5.0 support (no code changes were needed, but now we test this release). * Add Python 3.12 support -* Add Support for dumpdata/loaddata using Natural Keys +* Add support for dumpdata/loaddata using natural keys 5.0.1 (2023-10-26) ~~~~~~~~~~~~~~~~~~ diff --git a/docs/index.rst b/docs/index.rst index 611c28d3..db978980 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -13,6 +13,7 @@ tagging to your project easy and fun. forms admin serializers + testing api faq custom_tagging diff --git a/docs/testing.rst b/docs/testing.rst new file mode 100644 index 00000000..a18763e7 --- /dev/null +++ b/docs/testing.rst @@ -0,0 +1,14 @@ +Testing +======= + +Natural Key Support +------------------- +We have added `natural key support `_ to the Tag model in the Django taggit library. This allows you to identify objects by human-readable identifiers rather than by their database ID:: + + python manage.py dumpdata taggit.Tag --natural-foreign --natural-primary > tags.json + + python manage.py loaddata tags.json + +By default tags use the name field as the natural key. + +You can customize this in your own custom tag model by setting the ``natural_key_fields`` property on your model the required fields. From 15ed6e55ae5af6b92bc2a464dddc2f6414189800 Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Thu, 25 Jul 2024 22:41:52 +1000 Subject: [PATCH 10/12] Clean up documentation --- CHANGELOG.rst | 2 +- docs/admin.rst | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8073a416..53cc11ec 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -8,9 +8,9 @@ Changelog The previous behavior for this was that by default tag items were not ordered. In practice tag items often end up ordered by creation date anyways, just due to how databases work, but this was not a guarantee. If you wish to have the old behavior, set ``ordering=[]`` to your ``TaggableManager`` instance. We believe that this should not cause a noticable performance change, and the number of queries involved should not change. +* Added the ability to merge tags via the admin * Add Django 5.0 support (no code changes were needed, but now we test this release). * Add Python 3.12 support -* Added functionality for tag merging 5.0.1 (2023-10-26) ~~~~~~~~~~~~~~~~~~ diff --git a/docs/admin.rst b/docs/admin.rst index 22242c33..cb8084ca 100644 --- a/docs/admin.rst +++ b/docs/admin.rst @@ -42,10 +42,9 @@ method to the :class:`~django.contrib.admin.ModelAdmin`, using Merging tags in the admin -========================= +~~~~~~~~~~~~~~~~~~~~~~~~~ -Functionality has been added to the admin app to allow for tag "merging". -Really what is happening is a "find and replace" where the selected tags are being used. +Functionality has been added to the admin app to allow for tag "merging". Multiple tags can be selected, and all of their usages will be replaced by the tag that you choose to use. To merge your tags follow these steps: From c6c7bb219d81a4c9a191505486dc5241c03b7357 Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Fri, 26 Jul 2024 10:59:37 +1000 Subject: [PATCH 11/12] Fix issue when merging tags when the item has both the old and new tag --- taggit/admin.py | 17 ++++++--- .../admin/taggit/merge_tags_form.html | 2 +- tests/test_admin.py | 36 +++++++++++++++++++ 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/taggit/admin.py b/taggit/admin.py index f55e5624..408a3d42 100644 --- a/taggit/admin.py +++ b/taggit/admin.py @@ -27,7 +27,7 @@ def get_urls(self): path( "merge-tags/", self.admin_site.admin_view(self.merge_tags_view), - name="merge_tags", + name="taggit_tag_merge_tags", ), ] return custom_urls + urls @@ -58,9 +58,18 @@ def merge_tags_view(self, request): tag = Tag.objects.get(id=tag_id) tagged_items = TaggedItem.objects.filter(tag=tag) for tagged_item in tagged_items: - tagged_item.tag = new_tag - TaggedItem.objects.filter(tag=tag).update(tag=new_tag) - # tag.delete() #this will delete the selected tags after merge...leaving out for now + if TaggedItem.objects.filter( + tag=new_tag, + content_type=tagged_item.content_type, + object_id=tagged_item.object_id, + ).exists(): + # we have the new tag as well, so we can just + # remove the tag association + tagged_item.delete() + else: + # point this taggedItem to the new one + tagged_item.tag = new_tag + tagged_item.save() self.message_user(request, "Tags have been merged", level="success") # clear the selected_tag_ids from session after merge is complete diff --git a/taggit/templates/admin/taggit/merge_tags_form.html b/taggit/templates/admin/taggit/merge_tags_form.html index 224cfc95..3a57d4be 100644 --- a/taggit/templates/admin/taggit/merge_tags_form.html +++ b/taggit/templates/admin/taggit/merge_tags_form.html @@ -6,7 +6,7 @@
{% csrf_token %} {% for field in form %}
diff --git a/tests/test_admin.py b/tests/test_admin.py index 3b33c829..be835623 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -1,6 +1,7 @@ from django.contrib.auth.models import User from django.test import TestCase from django.urls import reverse +from taggit.models import Tag from .models import Food @@ -10,6 +11,11 @@ def setUp(self): super().setUp() self.apple = Food.objects.create(name="apple") self.apple.tags.add("Red", "red") + self.pear = Food.objects.create(name="pear") + self.pear.tags.add("red", "RED") + self.peach = Food.objects.create(name="peach") + self.peach.tags.add("red", "Yellow") + user = User.objects.create_superuser( username="admin", email="admin@mailinator.com", password="password" ) @@ -40,3 +46,33 @@ def test_get_change(self): reverse("admin:tests_food_change", args=(self.apple.pk,)) ) self.assertEqual(response.status_code, 200) + + def test_tag_merging(self): + response = self.client.get(reverse("admin:taggit_tag_changelist")) + + # merging red and RED into Red + pks_to_select = [Tag.objects.get(name="red").pk, Tag.objects.get(name="RED").pk] + response = self.client.post( + reverse("admin:taggit_tag_changelist"), + data={"action": "render_tag_form", "_selected_action": pks_to_select}, + ) + # we're redirecting + self.assertEqual(response.status_code, 302) + # make sure what we expected got into the session keys + assert "selected_tag_ids" in self.client.session.keys() + self.assertEqual( + self.client.session["selected_tag_ids"], ",".join(map(str, pks_to_select)) + ) + + # let's do the actual merge operation + response = self.client.post( + reverse("admin:taggit_tag_merge_tags"), {"new_tag_name": "Red"} + ) + self.assertEqual(response.status_code, 302) + + # time to check the result of the merges + self.assertSetEqual({tag.name for tag in self.apple.tags.all()}, {"Red"}) + self.assertSetEqual({tag.name for tag in self.pear.tags.all()}, {"Red"}) + self.assertSetEqual( + {tag.name for tag in self.peach.tags.all()}, {"Yellow", "Red"} + ) From ba489937851110ac290097524c0a3e50d2981c3d Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Fri, 26 Jul 2024 11:12:38 +1000 Subject: [PATCH 12/12] isort fix --- tests/test_admin.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_admin.py b/tests/test_admin.py index be835623..d36a6249 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -1,6 +1,7 @@ from django.contrib.auth.models import User from django.test import TestCase from django.urls import reverse + from taggit.models import Tag from .models import Food