Tags are not has any order...
jedie opened this issue · 6 comments
The tags has no order_by
... With SQLite you will get always the same order... But postgres returns the objects in "undefined" order. Here this makes problem in not reproducible tests ;) I can just use sorted()
in my tests to fix the problem, but what's about to sort the tags by default in tagulous?!?
e.g. Just replace list()
with sorted()
here:
django-tagulous/tagulous/models/managers.py
Lines 245 to 253 in 90c6ee5
Thanks for flagging, will investigate.
It's odd this hasn't shown before, there are a lot of tests with multiple tags where the order is checked, I'd have thought they would catch that - ci runs against sqlite, postgres and mysql to catch these things.
Perhaps it has to do with the order they're inserted, or the tests just happen to have values which don't trigger it.
In case it's not obvious when I get to it, do you have any examples for me to recreate?
Yes, SQlite retuns the tags in the order they are inserted. If this is the way it should goes, than a explicit position value is IMHO needed, if it's not SQlite.
Thanks, will investigate. It would make sense for the tag model to sort alphabetically by default, bit of an oversight it doesn't already!
Adding ordering = ['name']
to https://github.com/radiac/django-tagulous/blob/develop/tagulous/models/models.py#L142 should do the trick - I'll add some tests to check that works asap.
I would suggest to not add an ordering to model Meta. This may be slow down database queries (complicated joints etc)...
I suggest to replace list() with sorted() in the manager, see initial post.
Interesting. I'm concerned this would be a premature optimisation and suspect the benefit of having every type of tag query ordered predictably outweighs any performance issues for 99% of users. As I say, this feels like more of an oversight which should be rectified for all uses by default.
I also suspect production databases would be faster at sorting than python, and any complicated joins would also lead to more calls to sorted
in the python, offsetting any benefit. For the 1% of users who are looking to squeeze this sort of performance, they could always subclass and override the tag model to disable ordering.
Perhaps a compromise would be to add a tag option for the model which defaults to turning on database-level ordering as name ASC
but makes it convenient to override and disable for people who have concerns about the effect on db queries?
I've never seen a significant performance issue from tag lookups, but I suspect that at the point they are an issue, people would see a much greater performance boost elsewhere, eg by adding a de-normalised cache on the tagged object.
That said, I haven't run any tests to see if my suspicions are correct, so I'm open to it if you can give a scenario where we'd see a significant performance difference.
I've looked at this again, and was convinced by your point about introducing a performance regression - but I was pretty sure they were ordered already and that the tests assume they are, so I took another look at what is happening at the moment.
Although the common abstract base BaseTagModel
doesn't have Meta.ordering
, there already is Meta.ordering=["name"]
for its two subclasses used to generate the concrete models, TagModel
and TagTreeModel
.
I could well be wrong here, but my guess is that you're subclassing something other than TagModel
or TagTreeModel
- perhaps the BaseTagModel
? That would mean you didn't pick up any Meta.ordering
.
Because the documentation says to use these two subclasses, I think it's fair to assume that most people will be, so unless I've missed something, I think I'm going to call this a feature and close this issue for now - let power users who go deeper than the documentation decide how they want to manage ordering.
Swapping list
for sorted
would then go against that, and add an unnecessary O(N) for every tag lookup. My recommendation at this time would therefore be to add a Meta.ordering
to any subclasses that don't have it and need ordering, rather than introduce a potential performance regression for the majority of users which can't be easily overridden by people who don't want sorting.