[Django] #29303: non_atomic_requests decorator alters _non_atomic_requests attribute of original function
Django
original function
-------------------------------------+-------------------------------------
Reporter: Alasdair | Owner: nobody
Nicol |
Type: Bug | Status: new
Component: Database | Version: master
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.db import transaction
@transaction.non_atomic_requests(using='default')
def my_view(request):
return HttpResponse('')
assert my_view._non_atomic_requests == {'default'} # passes
wrapped_view = transaction.non_atomic_requests(using='other')
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('')
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29303>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
original function
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
I'm not able to reproduce. I created this test file:
{{{ #!python
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')
assert wrapped_view._non_atomic_requests == {'default', 'other'}
# passes
assert my_view._non_atomic_requests == {'default'} # fails
}}}
I get:
{{{
Traceback (most recent call last):
File "/home/tim/code/mysite/polls/test_29303.py", line 15, in test
assert wrapped_view._non_atomic_requests == {'default', 'other'} #
passes
AttributeError: 'function' object has no attribute '_non_atomic_requests'
}}}
and if I remove the assertion, the next assertion passes.
--
Ticket URL: <https://code.djangoproject.com/ticket/29303#comment:1>
Django
original function
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> 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.db import transaction
>
> @transaction.non_atomic_requests(using='default')
> def my_view(request):
> return HttpResponse('')
>
> assert my_view._non_atomic_requests == {'default'} # passes
>
> wrapped_view = transaction.non_atomic_requests(using='other')
>
> 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('')
> }}}
New description:
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('')
}}}
--
Comment (by Alasdair Nicol):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29303#comment:2>
Django
original function
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/29303#comment:3>
Django
original function
-------------------------------------+-------------------------------------
| yang
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Windson yang
Comment:
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29303#comment:4>
Django
original function
-------------------------------------+-------------------------------------
| yang
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => needsinfo
Comment:
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29303#comment:5>
Django
original function
-------------------------------------+-------------------------------------
| yang
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: needsinfo =>
Comment:
Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29303#comment:6>