Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DjangoFilterBackend deprecation breaks mro #5089

Closed
5 of 6 tasks
mattjmorrison opened this issue Apr 21, 2017 · 9 comments
Closed
5 of 6 tasks

DjangoFilterBackend deprecation breaks mro #5089

mattjmorrison opened this issue Apr 21, 2017 · 9 comments
Labels
Milestone

Comments

@mattjmorrison
Copy link
Contributor

mattjmorrison commented Apr 21, 2017

Checklist

  • I have verified that that issue exists against the master branch of Django REST framework.
  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • This is not a usage question. (Those should be directed to the discussion group instead.)
  • This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
  • I have reduced the issue to the simplest possible case.
  • I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

I ran into a really nasty bug today because of how we are using DjangoFilterBackend. Here is our scenario... we have a FilterBackend as well as a parent filter backend (that is really a mixin). Our backend extends our mixin as well as the rest_framework.filters.DjangoFilterBackend.... so it looks something like this

class NewDjangoFilterBackend:

    def filter_queryset(self):
        print("Inside django-filters")


class DefaultDRFBackwardsCompat:

    def __new__(*args, **kwargs):
        return NewDjangoFilterBackend()


class OurCustomerFilterBackendParent:

    def filter_queryset(self):
        print("Inside Our Parent Object")


class OurFilterBackend(OurCustomerFilterBackendParent, DefaultDRFBackwardsCompat):
    pass

OurFilterBackend().filter_queryset()

Expected behavior

(this is in Python 3.4.6 btw)

I expect, because of Python's normal MRO that filter_queryset on OurCustomerFilterBackendParent will be called.

Actual behavior

Due to the __new__ in DefaultDRFBackwardsCompat the filter_queryset inside of django-filters is called instead.

It is not very clear to my why this would happen... but there has to be a way to deprecate a class without breaking Python's MRO.

In our case, the fix to our bug was to just use the DjangoFilterBackend from django-filters instead of the one in rest_framework... but this just seems like a potentially dangerous way to make something be backwards compatible.

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 21, 2017

Interesting.

@xordoquy xordoquy added the Bug label Apr 21, 2017
@mattjmorrison
Copy link
Contributor Author

mattjmorrison commented Apr 21, 2017

Here is another example that illuminates what is actually happening...

class NewDjangoFilterBackend:

    def filter_queryset(self):
        print("Inside django-filters")


class DefaultDRFBackwardsCompat:

    def __new__(*args, **kwargs):
        return NewDjangoFilterBackend()


class OurCustomerFilterBackendParent:

    def filter_queryset(self):
        print("Inside Our Parent Object")


class OurFilterBackend(OurCustomerFilterBackendParent, DefaultDRFBackwardsCompat):

    def x(self):
        print("in x")

OurFilterBackend().x()

>>> Traceback (most recent call last):
>>>   File "<stdin>", line 25, in <module>
>>> AttributeError: 'NewDjangoFilterBackend' object has no attribute 'x'

It looks like, because of the way __new__ works OurFilterBackend is just NewDjangoFilterBackend instead.

@carltongibson
Copy link
Collaborator

carltongibson commented Apr 22, 2017

@mattjmorrison Can I just check, if you update to use django_filters.rest_framework.DjangoFilterBackend this issue goes away right? i.e. it's an issue with the deprecation wrapper.

@carltongibson
Copy link
Collaborator

carltongibson commented Apr 22, 2017

Sorry, didn't get to the end:

In our case, the fix to our bug was to just use the DjangoFilterBackend from django-filters instead of the one in rest_framework... but this just seems like a potentially dangerous way to make something be backwards compatible.

The issue is here. There's no effort at all to handle a custom backend class.

I'm not sure of the cost/benefit of bullet-proofing this at point. Obviously a PR would be worth merging but it's a bit of an edge-case that'll drop out shortly. (It's quite interesting though)

@rpkilby
Copy link
Member

rpkilby commented Apr 22, 2017

It should be straightforward to fix this:

  • The deprecation backend should inherit from django_filters.rest_framework.DjangoFilterBackend
  • The __new__ implementation should raise the warnings and then just call super().__new__(...)

@xordoquy
Copy link
Collaborator

xordoquy commented May 2, 2017

The deprecation backend should inherit from django_filters.rest_framework.DjangoFilterBackend

not going to work as it'll require django_filters to be installed in any case.

@rpkilby
Copy link
Member

rpkilby commented May 3, 2017

This could work with

if django_filters:
    DFBase = django_filters.rest_framework.DjangoFilterBackend
else:
    DFBase = BaseFilterBackend

class DjangoFilterBackend(DFBase):

@xordoquy
Copy link
Collaborator

xordoquy commented May 3, 2017

yup could do.

@tomchristie
Copy link
Member

tomchristie commented May 4, 2017

Closed via #5117.

@tomchristie tomchristie added this to the 3.6.3 Release milestone May 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants