skorokithakis / django-annoying

A django application that tries to eliminate annoying things in the Django framework. ⛺
http://skorokithakis.github.io/django-annoying
BSD 3-Clause "New" or "Revised" License
921 stars 87 forks source link

fix: Use atomic as a context manager #105

Closed Eraldo closed 4 months ago

Eraldo commented 4 months ago

Removed the atomic decorator of from the AutoSingleRelatedObjectDescriptor.__get__ method and replaced it by using atomic as a context manager within the method when actually creating the new instance.

The previous behavior caused issues with other 3rd party packages. Notably: https://github.com/strawberry-graphql/strawberry-django

Further context: That field will contribute that descriptor to the class, which means ModelClass.<attr> will trigger that __get__ which does not need to be atomic unless an instance will actually get created. This caused introspection issues with the optimizer in the above mentioned package.

Credit goes to @bellini666 for helping me find the source of the issue as well as providing the solution patch. 🙏

skorokithakis commented 4 months ago

Hm, why is the decorator needed here? get_or_create is already atomic.

bellini666 commented 4 months ago

Hm, why is the decorator needed here? get_or_create is already atomic.

Not sure if atomic is required at all, but it was being used in the __get__ method, so we assumed it was because of the get_or_create, that's the reason I suggested moving it there. If it is not required, maybe it can be removed (the PR is keeping the old behavior)

The issue this is trying to fix is to avoid triggering a db query when doing something like SomeModel.some_column, which is a class access to the column itself (not the object), and not only that is opening a transaction unnecessarily, but it also causes sync issues when trying to do that outside of an async safe context (https://docs.djangoproject.com/en/5.0/topics/async/#async-safety), which is the case for the introspection https://github.com/strawberry-graphql/strawberry-django is doing.

skorokithakis commented 4 months ago

Hm yeah, you're right, let's not alter the behaviour then. Thank you.