Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have tags default to being in creation order #871

Closed
wants to merge 15 commits into from
Closed
73 changes: 33 additions & 40 deletions taggit/managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,12 @@ 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:
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
Expand Down Expand Up @@ -196,58 +198,49 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here this preserves the order of creation, right? But if someone has ordering set to name should this actually order by name instead?

Basically shouldn't we try to keep the ordering consistent across the board? This might be pretty straightforward to do in one query.

I'm not a super fan of this method already (kinda hard to get what it's trying to accomplish) so any sort of simplification would definitely be appreciated.

Copy link
Contributor Author

@steverecio steverecio Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here this preserves the order of creation, right? But if someone has ordering set to name should this actually order by name instead?

This processes the tags arg in the order its passed in. It attempts to get or create the Tag object associated with each t in tags while ignoring duplicates (and handling case insensitivity).

I would argue that name (and tag creation order) is much easier to order by on the retrieval end. By preserving the order of the original list in this function, we can order by any key we want when retrieving the model (tag name, tag pk, or through table entry pk).

"""
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_kwargs
)
tag_objs.append(tag)
else:
raise ValueError(
"Cannot add {} ({}). Expected {} or str.".format(
t, type(t), type(self.through.tag_model())
)
)

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
Expand Down
31 changes: 31 additions & 0 deletions tests/test_models.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,39 @@
from django.test import TestCase, override_settings

from taggit.models import Tag
from tests.models import TestModel


class TestTaggableManager(TestCase):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this suite of tests should be in this file or tests.py

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"])
desired_result = ["green"]
self.assertEqual(desired_result, [tag.name for tag in sample_obj.tags.all()])


class TestSlugification(TestCase):
def test_unicode_slugs(self):
"""
Expand Down