From f5ba4b5a98e09b60bf7dda561705847d9dc85c39 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Tue, 10 Oct 2023 11:52:26 -0600 Subject: [PATCH 01/22] add TestTaggableManager --- tests/test_models.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/test_models.py b/tests/test_models.py index 33d44a53..c975d769 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -3,6 +3,19 @@ from tests.models import TestModel +class TestTaggableManager(TestCase): + def test_set_ordering(self): + """ + Test that the tags are set and returned exactly + """ + str_tags = ["red", "green", "delicious"] + sample_obj = TestModel.objects.create() + + sample_obj.tags.set(str_tags) + for idx, tag in enumerate(sample_obj.tags.all()): + self.assertEqual(tag.name, str_tags[idx]) + + class TestSlugification(TestCase): def test_unicode_slugs(self): """ From d96a21602d91d3e9ee00a76dd5ab36c7c0dd9be9 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 11 Oct 2023 11:10:17 -0600 Subject: [PATCH 02/22] fix _to_tag_model_instances to preserve order --- taggit/managers.py | 64 +++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index 4d7f0140..a866d9d0 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -206,18 +206,38 @@ def add(self, *tags, through_defaults=None, tag_kwargs=None, **kwargs): def _to_tag_model_instances(self, tags, tag_kwargs): """ Takes an iterable containing either strings, tag objects, or a mixture - of both and returns set of tag objects. + of both and returns a list of tag objects while preserving order. """ db = router.db_for_write(self.through, instance=self.instance) - str_tags = set() - tag_objs = set() + processed_tags = [] + tag_objs = [] + + case_insensitive = getattr(settings, "TAGGIT_CASE_INSENSITIVE", False) + manager = self.through.tag_model()._default_manager.using(db) for t in tags: if isinstance(t, self.through.tag_model()): - tag_objs.add(t) + if t not in tag_objs: + tag_objs.append(t) elif isinstance(t, str): - str_tags.add(t) + if t not in processed_tags: + processed_tags.append(t) + if case_insensitive: + try: + tag = manager.get(name__iexact=t, **tag_kwargs) + tag_objs.append(tag) + except self.through.tag_model().DoesNotExist: + lookup = {"name__iexact": t, **tag_kwargs} + tag, created = manager.get_or_create(**lookup, defaults={"name": t}) + tag_objs.append(tag) + else: + try: + tag = manager.get(name=t, **tag_kwargs) + tag_objs.append(tag) + except self.through.tag_model().DoesNotExist: + tag, created = manager.get_or_create(name=t, defaults={"name": t}) + tag_objs.append(tag) else: raise ValueError( "Cannot add {} ({}). Expected {} or str.".format( @@ -225,41 +245,9 @@ def _to_tag_model_instances(self, tags, tag_kwargs): ) ) - case_insensitive = getattr(settings, "TAGGIT_CASE_INSENSITIVE", False) - manager = self.through.tag_model()._default_manager.using(db) - - if case_insensitive: - # Some databases can do case-insensitive comparison with IN, which - # would be faster, but we can't rely on it or easily detect it. - existing = [] - tags_to_create = [] - - for name in str_tags: - try: - tag = manager.get(name__iexact=name, **tag_kwargs) - existing.append(tag) - except self.through.tag_model().DoesNotExist: - tags_to_create.append(name) - else: - # If str_tags has 0 elements Django actually optimizes that to not - # do a query. Malcolm is very smart. - existing = manager.filter(name__in=str_tags, **tag_kwargs) - - tags_to_create = str_tags - {t.name for t in existing} - - tag_objs.update(existing) - - for new_tag in tags_to_create: - if case_insensitive: - lookup = {"name__iexact": new_tag, **tag_kwargs} - else: - lookup = {"name": new_tag, **tag_kwargs} - - tag, create = manager.get_or_create(**lookup, defaults={"name": new_tag}) - tag_objs.add(tag) - return tag_objs + @require_instance_manager def names(self): return self.get_queryset().values_list("name", flat=True) From e0c88582473d9a5b177d6948147b9fb877e09d15 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 11 Oct 2023 11:54:26 -0600 Subject: [PATCH 03/22] ensure tag order is preserved --- taggit/managers.py | 4 +++- tests/test_models.py | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/taggit/managers.py b/taggit/managers.py index a866d9d0..8d667f0c 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -68,7 +68,9 @@ def __init__(self, through, model, instance, prefetch_cache_name, ordering=None) if ordering: self.ordering = ordering else: - self.ordering = [] + # default to ordering by the pk of the through table entry + related_name = self.through.tag.field.related_query_name() + self.ordering = [f"{related_name}__pk"] def is_cached(self, instance): return self.prefetch_cache_name in instance._prefetched_objects_cache diff --git a/tests/test_models.py b/tests/test_models.py index c975d769..c0252f89 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,5 +1,6 @@ from django.test import TestCase, override_settings +from taggit.models import Tag from tests.models import TestModel @@ -15,6 +16,19 @@ def test_set_ordering(self): for idx, tag in enumerate(sample_obj.tags.all()): self.assertEqual(tag.name, str_tags[idx]) + def test_set_mixed(self): + """ + Test with mixed str and obj tags + """ + tag_obj = Tag.objects.create(name='mcintosh') + str_tags = ["red", "green", "delicious"] + sample_obj = TestModel.objects.create() + + sample_obj.tags.set(str_tags + [tag_obj]) + results = str_tags + [tag_obj.name] + for idx, tag in enumerate(sample_obj.tags.all()): + self.assertEqual(tag.name, results[idx]) + class TestSlugification(TestCase): def test_unicode_slugs(self): From 0595a42e118644eef60a932656c543f836906e3d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 11 Oct 2023 17:57:24 +0000 Subject: [PATCH 04/22] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- taggit/managers.py | 9 ++++++--- tests/test_models.py | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index 8d667f0c..a6240a0b 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -231,14 +231,18 @@ def _to_tag_model_instances(self, tags, tag_kwargs): tag_objs.append(tag) except self.through.tag_model().DoesNotExist: lookup = {"name__iexact": t, **tag_kwargs} - tag, created = manager.get_or_create(**lookup, defaults={"name": t}) + tag, created = manager.get_or_create( + **lookup, defaults={"name": t} + ) tag_objs.append(tag) else: try: tag = manager.get(name=t, **tag_kwargs) tag_objs.append(tag) except self.through.tag_model().DoesNotExist: - tag, created = manager.get_or_create(name=t, defaults={"name": t}) + tag, created = manager.get_or_create( + name=t, defaults={"name": t} + ) tag_objs.append(tag) else: raise ValueError( @@ -249,7 +253,6 @@ def _to_tag_model_instances(self, tags, tag_kwargs): return tag_objs - @require_instance_manager def names(self): return self.get_queryset().values_list("name", flat=True) diff --git a/tests/test_models.py b/tests/test_models.py index c0252f89..7024a0ab 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -20,7 +20,7 @@ def test_set_mixed(self): """ Test with mixed str and obj tags """ - tag_obj = Tag.objects.create(name='mcintosh') + tag_obj = Tag.objects.create(name="mcintosh") str_tags = ["red", "green", "delicious"] sample_obj = TestModel.objects.create() From 5820930721724b79a5478bad4f233708cdbecf7d Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 16:38:41 -0400 Subject: [PATCH 05/22] missing arg --- taggit/managers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/taggit/managers.py b/taggit/managers.py index a6240a0b..8768e47c 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -241,7 +241,7 @@ def _to_tag_model_instances(self, tags, tag_kwargs): tag_objs.append(tag) except self.through.tag_model().DoesNotExist: tag, created = manager.get_or_create( - name=t, defaults={"name": t} + name=t, defaults={"name": t}, **tag_kwargs ) tag_objs.append(tag) else: From 122112b025589c6317063b63f0f943b4c0c4006e Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 16:42:05 -0400 Subject: [PATCH 06/22] clean up tests --- tests/test_models.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/test_models.py b/tests/test_models.py index 7024a0ab..3f66d1a6 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -13,8 +13,7 @@ def test_set_ordering(self): sample_obj = TestModel.objects.create() sample_obj.tags.set(str_tags) - for idx, tag in enumerate(sample_obj.tags.all()): - self.assertEqual(tag.name, str_tags[idx]) + self.assertEqual(str_tags, [tag.name for tag in sample_obj.tags.all()]) def test_set_mixed(self): """ @@ -26,8 +25,12 @@ def test_set_mixed(self): sample_obj.tags.set(str_tags + [tag_obj]) results = str_tags + [tag_obj.name] - for idx, tag in enumerate(sample_obj.tags.all()): - self.assertEqual(tag.name, results[idx]) + self.assertEqual(results, [tag.name for tag in sample_obj.tags.all()]) + + def test_duplicates(self): + sample_obj = TestModel.objects.create() + sample_obj.tags.set(["green", "green"]) + self.assertEqual(1, sample_obj.tags.count()) class TestSlugification(TestCase): From 9697a97e2f0bbbd7ab16310959407537cff5b6ad Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 16:45:36 -0400 Subject: [PATCH 07/22] fix conditional on ordering arg --- taggit/managers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/taggit/managers.py b/taggit/managers.py index 8768e47c..74c48977 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -65,7 +65,7 @@ def __init__(self, through, model, instance, prefetch_cache_name, ordering=None) self.model = model self.instance = instance self.prefetch_cache_name = prefetch_cache_name - if ordering: + if ordering is not None: self.ordering = ordering else: # default to ordering by the pk of the through table entry From 897ed6a1a6443aa11213ce046a1b6381765b50d6 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 16:47:45 -0400 Subject: [PATCH 08/22] make test more explicit --- tests/test_models.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_models.py b/tests/test_models.py index 3f66d1a6..2228f1ee 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -30,7 +30,8 @@ def test_set_mixed(self): def test_duplicates(self): sample_obj = TestModel.objects.create() sample_obj.tags.set(["green", "green"]) - self.assertEqual(1, sample_obj.tags.count()) + desired_result = ["green"] + self.assertEqual(desired_result, [tag.name for tag in sample_obj.tags.all()]) class TestSlugification(TestCase): From cb7d00ce642a5431f1f192fafebaf7e2714767fb Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 17:16:15 -0400 Subject: [PATCH 09/22] fix case insensitive handling and tag_kwargs --- taggit/managers.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index 74c48977..fba91d55 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -224,7 +224,6 @@ def _to_tag_model_instances(self, tags, tag_kwargs): tag_objs.append(t) elif isinstance(t, str): if t not in processed_tags: - processed_tags.append(t) if case_insensitive: try: tag = manager.get(name__iexact=t, **tag_kwargs) @@ -235,15 +234,20 @@ def _to_tag_model_instances(self, tags, tag_kwargs): **lookup, defaults={"name": t} ) tag_objs.append(tag) + finally: + processed_tags.append(t.lower()) else: try: tag = manager.get(name=t, **tag_kwargs) tag_objs.append(tag) except self.through.tag_model().DoesNotExist: + lookup = {"name": t, **tag_kwargs} tag, created = manager.get_or_create( - name=t, defaults={"name": t}, **tag_kwargs + **lookup, defaults={"name": t} ) tag_objs.append(tag) + finally: + processed_tags.append(t) else: raise ValueError( "Cannot add {} ({}). Expected {} or str.".format( From 0a8378b42708634cdf3af7db55abcf7953367784 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 17:31:19 -0400 Subject: [PATCH 10/22] fix processing logic --- taggit/managers.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index fba91d55..7b833094 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -220,11 +220,17 @@ def _to_tag_model_instances(self, tags, tag_kwargs): for t in tags: if isinstance(t, self.through.tag_model()): - if t not in tag_objs: - tag_objs.append(t) + if case_insensitive: + if t.name.lower() not in processed_tags: + tag_objs.append(t) + processed_tags.append(t.name.lower()) + else: + if t.name not in processed_tags: + tag_objs.append(t) + processed_tags.append(t.name) elif isinstance(t, str): - if t not in processed_tags: - if case_insensitive: + if case_insensitive: + if t.lower() not in processed_tags: try: tag = manager.get(name__iexact=t, **tag_kwargs) tag_objs.append(tag) @@ -236,7 +242,8 @@ def _to_tag_model_instances(self, tags, tag_kwargs): tag_objs.append(tag) finally: processed_tags.append(t.lower()) - else: + else: + if t not in processed_tags: try: tag = manager.get(name=t, **tag_kwargs) tag_objs.append(tag) From b29755101f9f0b806510886961fe0be547d786da Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Thu, 26 Oct 2023 16:15:44 -0400 Subject: [PATCH 11/22] fixed duplicates issue --- taggit/managers.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index 7b833094..9d48c3e7 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -6,6 +6,7 @@ from django.contrib.contenttypes.models import ContentType from django.db import connections, models, router from django.db.models import signals +from django.db.models import Case, When, Value, IntegerField from django.db.models.fields.related import ( ManyToManyRel, OneToOneRel, @@ -77,13 +78,20 @@ def is_cached(self, instance): def get_queryset(self, extra_filters=None): try: - return self.instance._prefetched_objects_cache[self.prefetch_cache_name] + qs = self.instance._prefetched_objects_cache[self.prefetch_cache_name] except (AttributeError, KeyError): kwargs = extra_filters if extra_filters else {} - return self.through.tags_for(self.model, self.instance, **kwargs).order_by( + qs = self.through.tags_for(self.model, self.instance, **kwargs).order_by( *self.ordering ) + # distinct query doesn't work so we need to manually dedupe the query + kwargs = extra_filters if extra_filters else {} + pks = qs.values_list('pk', flat=True) + preserved = Case(*[When(pk=pk, then=Value(i)) for i, pk in enumerate(pks)], output_field=IntegerField()) + results = self.through.tag.field.related_model.objects.filter(pk__in=pks, **kwargs).annotate(ordering=preserved).order_by('ordering').distinct() + return results + def get_prefetch_queryset(self, instances, queryset=None): if queryset is None: return self.get_prefetch_querysets(instances) From 311c1e4a284a5de451c866176c16a744aa1abe99 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 26 Oct 2023 20:15:58 +0000 Subject: [PATCH 12/22] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- taggit/managers.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index 9d48c3e7..f195d31a 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -87,9 +87,17 @@ def get_queryset(self, extra_filters=None): # distinct query doesn't work so we need to manually dedupe the query kwargs = extra_filters if extra_filters else {} - pks = qs.values_list('pk', flat=True) - preserved = Case(*[When(pk=pk, then=Value(i)) for i, pk in enumerate(pks)], output_field=IntegerField()) - results = self.through.tag.field.related_model.objects.filter(pk__in=pks, **kwargs).annotate(ordering=preserved).order_by('ordering').distinct() + pks = qs.values_list("pk", flat=True) + preserved = Case( + *[When(pk=pk, then=Value(i)) for i, pk in enumerate(pks)], + output_field=IntegerField(), + ) + results = ( + self.through.tag.field.related_model.objects.filter(pk__in=pks, **kwargs) + .annotate(ordering=preserved) + .order_by("ordering") + .distinct() + ) return results def get_prefetch_queryset(self, instances, queryset=None): From 82922eb370de6ae381bf6a352b85a5435d84d8ca Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Tue, 31 Oct 2023 19:36:12 -0300 Subject: [PATCH 13/22] fix most_common test --- taggit/managers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/taggit/managers.py b/taggit/managers.py index f195d31a..d95f31a4 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -392,8 +392,9 @@ def clear(self): ) def most_common(self, min_count=None, extra_filters=None): + kwargs = extra_filters if extra_filters else {} queryset = ( - self.get_queryset(extra_filters) + self.through.tag_model().objects.filter(**kwargs) .annotate(num_times=models.Count(self.through.tag_relname())) .order_by("-num_times") ) From 8bfa7ed77f8551d837373a909f4b90482e3837b3 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Tue, 31 Oct 2023 20:04:45 -0300 Subject: [PATCH 14/22] a more elegant solution of imposing through table ordering only if filtering on a model instance --- taggit/managers.py | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index d95f31a4..5d5f4956 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -68,10 +68,12 @@ def __init__(self, through, model, instance, prefetch_cache_name, ordering=None) self.prefetch_cache_name = prefetch_cache_name if ordering is not None: self.ordering = ordering - else: + elif instance: # default to ordering by the pk of the through table entry related_name = self.through.tag.field.related_query_name() self.ordering = [f"{related_name}__pk"] + else: + self.ordering = [] def is_cached(self, instance): return self.prefetch_cache_name in instance._prefetched_objects_cache @@ -85,20 +87,7 @@ def get_queryset(self, extra_filters=None): *self.ordering ) - # distinct query doesn't work so we need to manually dedupe the query - kwargs = extra_filters if extra_filters else {} - pks = qs.values_list("pk", flat=True) - preserved = Case( - *[When(pk=pk, then=Value(i)) for i, pk in enumerate(pks)], - output_field=IntegerField(), - ) - results = ( - self.through.tag.field.related_model.objects.filter(pk__in=pks, **kwargs) - .annotate(ordering=preserved) - .order_by("ordering") - .distinct() - ) - return results + return qs def get_prefetch_queryset(self, instances, queryset=None): if queryset is None: @@ -392,9 +381,8 @@ def clear(self): ) def most_common(self, min_count=None, extra_filters=None): - kwargs = extra_filters if extra_filters else {} queryset = ( - self.through.tag_model().objects.filter(**kwargs) + self.get_queryset(extra_filters) .annotate(num_times=models.Count(self.through.tag_relname())) .order_by("-num_times") ) From acff592b54831915166f4531f6ef5464afe4e01d Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Tue, 31 Oct 2023 21:30:12 -0300 Subject: [PATCH 15/22] unused import --- taggit/managers.py | 1 - 1 file changed, 1 deletion(-) diff --git a/taggit/managers.py b/taggit/managers.py index 5d5f4956..df3dd2d0 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -6,7 +6,6 @@ from django.contrib.contenttypes.models import ContentType from django.db import connections, models, router from django.db.models import signals -from django.db.models import Case, When, Value, IntegerField from django.db.models.fields.related import ( ManyToManyRel, OneToOneRel, From b23715b10911d26c0227473d3299d1fd189516d6 Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Sun, 2 Jun 2024 21:51:32 +1000 Subject: [PATCH 16/22] Make sure to only query as much as needed --- taggit/managers.py | 106 +++++++++++++++++++++++++------------------ tests/test_models.py | 22 --------- tests/tests.py | 26 +++++++++++ 3 files changed, 89 insertions(+), 65 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index df3dd2d0..6e2dac15 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -65,10 +65,15 @@ def __init__(self, through, model, instance, prefetch_cache_name, ordering=None) self.model = model self.instance = instance self.prefetch_cache_name = prefetch_cache_name + if ordering is not None: self.ordering = ordering elif instance: - # default to ordering by the pk of the through table entry + # When working off an instance (i.e. instance.tags.all()) + # we default to ordering by the PK of the through table + # + # (This ordering does not apply for queries at the model + # level, i.e. Model.tags.all()) related_name = self.through.tag.field.related_query_name() self.ordering = [f"{related_name}__pk"] else: @@ -79,15 +84,13 @@ def is_cached(self, instance): def get_queryset(self, extra_filters=None): try: - qs = self.instance._prefetched_objects_cache[self.prefetch_cache_name] + return self.instance._prefetched_objects_cache[self.prefetch_cache_name] except (AttributeError, KeyError): kwargs = extra_filters if extra_filters else {} - qs = self.through.tags_for(self.model, self.instance, **kwargs).order_by( + return self.through.tags_for(self.model, self.instance, **kwargs).order_by( *self.ordering ) - return qs - def get_prefetch_queryset(self, instances, queryset=None): if queryset is None: return self.get_prefetch_querysets(instances) @@ -216,49 +219,66 @@ def _to_tag_model_instances(self, tags, tag_kwargs): """ db = router.db_for_write(self.through, instance=self.instance) - processed_tags = [] - tag_objs = [] - case_insensitive = getattr(settings, "TAGGIT_CASE_INSENSITIVE", False) manager = self.through.tag_model()._default_manager.using(db) + # tags can be instances of our through models, or strings + + tag_strs = [tag for tag in tags if isinstance(tag, str)] + existing_tags_for_str = {} + # we are going to first try and lookup existing tags (in a single query) + if case_insensitive: + # Some databases can do case-insensitive comparison with IN, which + # would be faster, but we can't rely on it or easily detect it + existing = [] + tags_to_create = [] + for name in tag_strs: + try: + tag = manager.get(name__iexact=name, **tag_kwargs) + existing_tags_for_str[name] = tag + except self.through.tag_model().DoesNotExist: + tags_to_create.append(name) + else: + # Django is smart enough to not actually query if tag_strs is empty + # but importantly, this is a single query for all potential tags + existing = manager.filter(name__in=tag_strs, **tag_kwargs) + # we're going to end up doing this query anyways, so here is fine + for t in existing: + existing_tags_for_str[t.name] = t + + result = [] + # this set is used for deduplicating tags + seen_tags = set() for t in tags: if isinstance(t, self.through.tag_model()): - if case_insensitive: - if t.name.lower() not in processed_tags: - tag_objs.append(t) - processed_tags.append(t.name.lower()) - else: - if t.name not in processed_tags: - tag_objs.append(t) - processed_tags.append(t.name) + if t in seen_tags: + continue + seen_tags.add(t) + result.append(t) elif isinstance(t, str): - if case_insensitive: - if t.lower() not in processed_tags: - try: - tag = manager.get(name__iexact=t, **tag_kwargs) - tag_objs.append(tag) - except self.through.tag_model().DoesNotExist: - lookup = {"name__iexact": t, **tag_kwargs} - tag, created = manager.get_or_create( - **lookup, defaults={"name": t} - ) - tag_objs.append(tag) - finally: - processed_tags.append(t.lower()) - else: - if t not in processed_tags: - try: - tag = manager.get(name=t, **tag_kwargs) - tag_objs.append(tag) - except self.through.tag_model().DoesNotExist: - lookup = {"name": t, **tag_kwargs} - tag, created = manager.get_or_create( - **lookup, defaults={"name": t} - ) - tag_objs.append(tag) - finally: - processed_tags.append(t) + # we are using a string, so either the tag exists (and we have the lookup) + # or we need to create the value + + existing_tag = existing_tags_for_str.get(t, None) + if existing_tag is None: + # we need to create a tag + # (we use get_or_create to handle potential races) + if case_insensitive: + lookup = {"name__iexact": t, **tag_kwargs} + else: + lookup = {"name": t, **tag_kwargs} + existing_tag, _ = manager.get_or_create( + **lookup, defaults={"name": t} + ) + # we now have an existing tag for this string + + # confirm if we've seen it or not (this is where case insensitivity comes + # into play) + if existing_tag in seen_tags: + continue + + seen_tags.add(existing_tag) + result.append(existing_tag) else: raise ValueError( "Cannot add {} ({}). Expected {} or str.".format( @@ -266,7 +286,7 @@ def _to_tag_model_instances(self, tags, tag_kwargs): ) ) - return tag_objs + return result @require_instance_manager def names(self): diff --git a/tests/test_models.py b/tests/test_models.py index 2228f1ee..e5ad6a7c 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -5,28 +5,6 @@ class TestTaggableManager(TestCase): - def test_set_ordering(self): - """ - Test that the tags are set and returned exactly - """ - str_tags = ["red", "green", "delicious"] - sample_obj = TestModel.objects.create() - - sample_obj.tags.set(str_tags) - self.assertEqual(str_tags, [tag.name for tag in sample_obj.tags.all()]) - - def test_set_mixed(self): - """ - Test with mixed str and obj tags - """ - tag_obj = Tag.objects.create(name="mcintosh") - str_tags = ["red", "green", "delicious"] - sample_obj = TestModel.objects.create() - - sample_obj.tags.set(str_tags + [tag_obj]) - results = str_tags + [tag_obj.name] - self.assertEqual(results, [tag.name for tag in sample_obj.tags.all()]) - def test_duplicates(self): sample_obj = TestModel.objects.create() sample_obj.tags.set(["green", "green"]) diff --git a/tests/tests.py b/tests/tests.py index cb0f1687..ef83ff88 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -59,6 +59,7 @@ TaggedTrackedFood, TenantModel, TenantTag, + TestModel, TrackedTag, UUIDFood, UUIDHousePet, @@ -67,6 +68,7 @@ UUIDTaggedItem, ) + if DJANGO_VERSION < (4, 2): TestCase.assertQuerySetEqual = TestCase.assertQuerysetEqual @@ -1362,6 +1364,30 @@ def test_added_tags_are_returned_ordered(self): list(obj.tags.values_list("name", flat=True)), ) + def test_ordered_by_creation_by_default(self): + """ + Test that the tags are set and returned exactly as they are provided in .set + """ + str_tags = ["red", "green", "delicious"] + sample_obj = TestModel.objects.create() + + sample_obj.tags.set(str_tags) + self.assertEqual(str_tags, [tag.name for tag in sample_obj.tags.all()]) + + def test_default_ordering_on_mixed_operation(self): + """ + Test that our default ordering is preserved even when mixing strings + and objects + """ + tag_obj = Tag.objects.create(name="mcintosh") + tag_obj_2 = Tag.objects.create(name="fuji") + str_tags = ["red", "green", "delicious"] + sample_obj = TestModel.objects.create() + + sample_obj.tags.set([tag_obj_2] + str_tags + [tag_obj]) + expected_results = [tag_obj_2.name] + str_tags + [tag_obj.name] + self.assertEqual(expected_results, [tag.name for tag in sample_obj.tags.all()]) + class PendingMigrationsTests(TestCase): def test_taggit_has_no_pending_migrations(self): From e203e07da973ad02dcc7a94195e6b50d6f502ac6 Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Sun, 2 Jun 2024 22:07:37 +1000 Subject: [PATCH 17/22] Add changelog entry --- CHANGELOG.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 11928849..985d133a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -4,6 +4,11 @@ Changelog (Unreleased) ~~~~~~~~~~~~ +* By default, order tag items on instances by the primary key. This generally means that they will be ordered by "creation date" for the tag item. + 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. + 5.0.1 (2023-10-26) ~~~~~~~~~~~~~~~~~~ From 5ba593457ccce2306dea27fa57a551bf31eaee94 Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Sun, 2 Jun 2024 22:12:41 +1000 Subject: [PATCH 18/22] Add note to tricky code --- taggit/managers.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/taggit/managers.py b/taggit/managers.py index 6e2dac15..023bf876 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -225,6 +225,10 @@ def _to_tag_model_instances(self, tags, tag_kwargs): # tags can be instances of our through models, or strings tag_strs = [tag for tag in tags if isinstance(tag, str)] + # This map from tag names to tags lets us handle deduplication + # without doing extra queries along the way, all while relying on + # data we were going to pull out of the database anyways + # existing_tags_for_str[tag_name] = tag existing_tags_for_str = {} # we are going to first try and lookup existing tags (in a single query) if case_insensitive: From 855def611d6756fb15c0389d709abd2ea990a209 Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Sun, 2 Jun 2024 22:16:17 +1000 Subject: [PATCH 19/22] fix style issues --- tests/test_models.py | 1 - tests/tests.py | 1 - 2 files changed, 2 deletions(-) diff --git a/tests/test_models.py b/tests/test_models.py index e5ad6a7c..4a9b748c 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,6 +1,5 @@ from django.test import TestCase, override_settings -from taggit.models import Tag from tests.models import TestModel diff --git a/tests/tests.py b/tests/tests.py index ef83ff88..79489598 100644 --- a/tests/tests.py +++ b/tests/tests.py @@ -68,7 +68,6 @@ UUIDTaggedItem, ) - if DJANGO_VERSION < (4, 2): TestCase.assertQuerySetEqual = TestCase.assertQuerysetEqual From 15d83bc20a4991584c83db00ca81918a54b90243 Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Sun, 2 Jun 2024 22:16:25 +1000 Subject: [PATCH 20/22] ignore .direnv for flake8 --- setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.cfg b/setup.cfg index 88504a01..3d777541 100644 --- a/setup.cfg +++ b/setup.cfg @@ -41,7 +41,7 @@ exclude = tests* [flake8] # E501: line too long ignore = E501 -exclude = .venv,.git,.tox +exclude = .venv,.git,.tox,.direnv [isort] profile = black From 1e702497d90caa4abde1a5989dfa214082e0e8c6 Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Tue, 18 Jun 2024 16:15:45 +0900 Subject: [PATCH 21/22] Add Steve to the AUTHORS --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index ad21915c..1165e340 100644 --- a/AUTHORS +++ b/AUTHORS @@ -19,3 +19,4 @@ Andrew Pryde John Whitlock Jon Dufresne Pablo Olmedo Dorado +Steve Reciao From e4baf499edf2f234649c9646caedd06aa7aa57e6 Mon Sep 17 00:00:00 2001 From: Raphael Gaschignard Date: Tue, 18 Jun 2024 16:16:38 +0900 Subject: [PATCH 22/22] remove continue usage --- taggit/managers.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/taggit/managers.py b/taggit/managers.py index 023bf876..0ff8e976 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -255,10 +255,9 @@ def _to_tag_model_instances(self, tags, tag_kwargs): seen_tags = set() for t in tags: if isinstance(t, self.through.tag_model()): - if t in seen_tags: - continue - seen_tags.add(t) - result.append(t) + if t not in seen_tags: + seen_tags.add(t) + result.append(t) elif isinstance(t, str): # we are using a string, so either the tag exists (and we have the lookup) # or we need to create the value @@ -278,11 +277,9 @@ def _to_tag_model_instances(self, tags, tag_kwargs): # confirm if we've seen it or not (this is where case insensitivity comes # into play) - if existing_tag in seen_tags: - continue - - seen_tags.add(existing_tag) - result.append(existing_tag) + if existing_tag not in seen_tags: + seen_tags.add(existing_tag) + result.append(existing_tag) else: raise ValueError( "Cannot add {} ({}). Expected {} or str.".format(