Opened 6 years ago

Last modified 6 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 Alasdair Nicol)

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 Tim Graham, 6 years ago

I'm not able to reproduce. I created this test file:

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.

comment:2 by Alasdair Nicol, 6 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 Tim Graham, 6 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Windson yang, 6 years ago

Owner: changed from nobody to Windson yang
Status: newassigned

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)

comment:5 by Windson yang, 6 years ago

Resolution: needsinfo
Status: assignedclosed

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)

in reply to:  5 comment:6 by Alasdair Nicol, 6 years ago

Resolution: needsinfo
Status: closednew

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.

Note: See TracTickets for help on using tickets.
Back to Top