search

radiac / django-tagulous

Fabulous Tagging for Django
http://radiac.net/projects/django-tagulous/
Other
332 stars 65 forks source link

Tags are not has any order... #155

Open jedie opened 2 years ago

jedie commented 2 years ago

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: https://github.com/radiac/django-tagulous/blob/90c6ee5ac54ef1127f2edcfac10c5ef3ab09e255/tagulous/models/managers.py#L245-L253

radiac commented 2 years ago

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?

jedie commented 2 years ago

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.

radiac commented 2 years ago

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.

jedie commented 2 years ago

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.

radiac commented 2 years ago

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.