From ba771cc5e513b4cdf59b30b61eeb72962f9696a8 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Tue, 10 Oct 2023 11:52:26 -0600 Subject: [PATCH 01/15] 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 b2274954a15ef21faf2ddec07ccf493821ffebbd Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 11 Oct 2023 11:10:17 -0600 Subject: [PATCH 02/15] 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 85542c90..20f85f0d 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -196,18 +196,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( @@ -215,41 +235,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 9bb0827f6b7ed4b276572ec80c7213013f9fb76d Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 11 Oct 2023 11:54:26 -0600 Subject: [PATCH 03/15] 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 20f85f0d..0fe1c8fa 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -69,7 +69,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 2461afb976c408f4fc4f1d1d58aaa0144128d7f8 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/15] [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 0fe1c8fa..d2620fb8 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -221,14 +221,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( @@ -239,7 +243,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 a6dc1e27488f168555fd3701c68360e5a7a1cea4 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 16:38:41 -0400 Subject: [PATCH 05/15] missing arg --- taggit/managers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/taggit/managers.py b/taggit/managers.py index d2620fb8..d8c89dc8 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -231,7 +231,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 62e5cfb125fade92138fe5bfdb125bc6f0abb386 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 16:42:05 -0400 Subject: [PATCH 06/15] 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 7b82c285439b2a0e44e99762dbee6280570bedea Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 16:45:36 -0400 Subject: [PATCH 07/15] 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 d8c89dc8..9b114583 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -66,7 +66,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 3469e6107494567ecc678b7358291a0445dcaed2 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 16:47:45 -0400 Subject: [PATCH 08/15] 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 8c8ee13ab5fa23ea6c7a5dd55aa0b0a787596197 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 17:16:15 -0400 Subject: [PATCH 09/15] 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 9b114583..0016a2c6 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -214,7 +214,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) @@ -225,15 +224,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 71635d523b757785b657166ec1921ea80f18d4c2 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Wed, 25 Oct 2023 17:31:19 -0400 Subject: [PATCH 10/15] 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 0016a2c6..6a21831d 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -210,11 +210,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) @@ -226,7 +232,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 e3afa7d3cca960899df48de4589cf79789b5f325 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Thu, 26 Oct 2023 16:15:44 -0400 Subject: [PATCH 11/15] 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 6a21831d..4b0bc8db 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -7,6 +7,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, @@ -78,13 +79,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 not None: raise ValueError("Custom queryset can't be used for this lookup.") From 53e566defcb4496ae3f0f35a3915d3925a7e5413 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/15] [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 4b0bc8db..324ea907 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -88,9 +88,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 418fa0100ade8fd9615e553d8a0454c754c998ad Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Tue, 31 Oct 2023 19:36:12 -0300 Subject: [PATCH 13/15] 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 324ea907..ff631648 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -382,8 +382,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 121ed6078a8f167568465bcd6d33d10df51d9029 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Tue, 31 Oct 2023 20:04:45 -0300 Subject: [PATCH 14/15] 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 ff631648..c43dd122 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -69,10 +69,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 @@ -86,20 +88,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 not None: @@ -382,9 +371,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 73dc9d8ccda142a7faaa3569f481c16765bb6f47 Mon Sep 17 00:00:00 2001 From: Steve Recio Date: Tue, 31 Oct 2023 21:30:12 -0300 Subject: [PATCH 15/15] unused import --- taggit/managers.py | 1 - 1 file changed, 1 deletion(-) diff --git a/taggit/managers.py b/taggit/managers.py index c43dd122..19b6cfb1 100644 --- a/taggit/managers.py +++ b/taggit/managers.py @@ -7,7 +7,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,