radiac
commented
2 years ago Thanks for flagging this (and apologies for the delay getting back). You're right, it could have a very noticeable side effect, and it's something we should address.
I'm not that familiar with pickling in Django (personally I tend to avoid pickling). The use case that this change was trying to help (#109) was to pass a model instance into Celery, but now you mention it, when it's unpickled the manager should be there to query the tags anyway.
That means we should be fine to just skip the manager; the only change would be that the tags may have changed between pickling and unpickling, but if that can happen you have bigger problems with pickling anyway. I can't see where someone would benefit from having them pickled?
Unless there are any objections, I'll add some tests to confirm my understanding and then remove the lookup.
nsaje
First of all thanks for working on this library, it's great and very useful.
This change introduces a potential performance implication as the pickling of an object incurs additional queries to the database. This is a very large side effect for a serialization operation and contrary to what Django does and consequently what one would expect - that the model object is pickled as-is, without following related managers.
IMO the correct fix for pickling errors would be to simply skip the Tagulous manager since when an object is unpickled, tags should be queried from the database when the field is accessed anyway. This is Django's behaviour as well.
In some cases it might indeed be useful to have this pickled for ease-of-use. Maybe this could be made configurable and off by default?