Opened 7 years ago
Last modified 7 years ago
#29303 new Bug
non_atomic_requests decorator alters _non_atomic_requests attribute of original function
Reported by: | Alasdair Nicol | Owned by: | Windson yang |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
When calling non_atomic_requests
with a function, it alters the _non_atomic_requests
attribute of the original function.
Here's an example:
from django.test import TestCase # Create your tests here. from django.test import TestCase from django.db import transaction @transaction.non_atomic_requests(using='default') def my_view(request): return HttpResponse('') class TestViews(TestCase): def test(self): assert my_view._non_atomic_requests == {'default'} # passes wrapped_view = transaction.non_atomic_requests(using='other')(my_view) assert wrapped_view._non_atomic_requests == {'default', 'other'} # passes assert my_view._non_atomic_requests == {'default'} # fails
I realise that this is a contrived example. It isn't an issue when non_atomic_requests
is used as a decorator:
@transaction.non_atomic_requests(using='default') @transaction.non_atomic_requests(using='other') def my_view(request) return HttpResponse('')
Change History (6)
comment:1 by , 7 years ago
comment:2 by , 7 years ago
Description: | modified (diff) |
---|
Sorry, there was a mistake in the wrapped_view
line, I didn't apply it to the view. It should be:
wrapped_view = transaction.non_atomic_requests(using='other')(my_view)
I've updated the example in the ticket description.
comment:3 by , 7 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I'm not sure how to fix this, as we can see, in _non_atomic_requests method we add 'using' in the original view then return it. That is why my_view._non_atomic_requests changed. The first solution will copy a new view from the old one and return the new view. The second will be we make the document more clear about this behavior.
def _non_atomic_requests(view, using): try: view._non_atomic_requests.add(using) except AttributeError: view._non_atomic_requests = {using} return view def non_atomic_requests(using=None): if callable(using): return _non_atomic_requests(using, DEFAULT_DB_ALIAS) else: if using is None: using = DEFAULT_DB_ALIAS return lambda view: _non_atomic_requests(view, using)
follow-up: 6 comment:5 by , 7 years ago
Resolution: | → needsinfo |
---|---|
Status: | assigned → closed |
I don't think this is a bug, in your example wrapped_view is my_view. You can use id() to find out.
class TestViews(TestCase): def test(self): assert my_view._non_atomic_requests == {'default'} # passes wrapped_view = transaction.non_atomic_requests(using='other')(my_view) print(id(wrapped_view)) # 4510701904 print(id(my_view)) # 4510701904 assert wrapped_view._non_atomic_requests == {'default', 'other'} # passes assert my_view._non_atomic_requests == {'default'} # fails
If you want to define a new view, you should use
wrapped_view = transaction.non_atomic_requests(using='other')(wrapped_view)
comment:6 by , 7 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
Replying to Windson yang:
I don't think this is a bug, in your example wrapped_view is my_view. You can use id() to find out.
That explains why the test case is failing, but it's not clear that that should be the behaviour. Why should wrapped_view
be my_view
?
If you wrap a view with transaction.atomic()
, you get a new view:
def my_view(request): return HttpResponse('Hello, world') wrapped_view = transaction.atomic(my_view) assert wrapped_view is not my_view
But when you wrap a view with transaction.non_atomic_requests
, it modifies the original view.
def my_view(request): return HttpResponse('Hello, world') wrapped_view = transaction.non_atomic_requests(my_view) assert wrapped_view is my_view
This behaviour is inconsistent.
As I said in the original ticket, I'm not sure that this is a problem in practice. I assume that most of the time, non_atomic_requests
is used as a decorator, so you don't need the original unwrapped view.
@transaction.non_atomic_requests(using='default') @transaction.non_atomic_requests(using='other') def my_view(request) return HttpResponse('')
I'd understand if we closed this ticket as WONTFIX or simply added a note to the documentation, but I think that closing as NEEDSINFO is incorrect.
I'm not able to reproduce. I created this test file:
I get:
and if I remove the assertion, the next assertion passes.