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

Should either include a queryset attribute, or override the get_queryset() method. #1

Closed
chogarcia opened this issue Nov 11, 2015 · 17 comments

Comments

@chogarcia
Copy link

djangorestframework==3.3.1

@chogarcia chogarcia changed the title 'SlideList' should either include a queryset attribute, or override the get_queryset() method. Should either include a queryset attribute, or override the get_queryset() method. Nov 11, 2015
@chogarcia
Copy link
Author

adding

queryset = ''

works

@MattBroach
Copy link
Owner

Thanks for catching this problem, @cho-is -- I saw it a couple days ago myself. This problem crept in with DRF 3.3, and I'm gonna release an update today or tomorrow with a very similar patch to the one you proposed

@MattBroach
Copy link
Owner

Just uploaded released a patch that fixes this error (and created a test to confirm the problem and fix). I did not alter the mixin, because I didn't want to make any assumptions about use. The new version is up on pypi or can be downloaded from the releases here.

@chogarcia
Copy link
Author

It seems to be an issue with DjangoRestMultipleModels and cache.

It does not invalidate the cache when you change a model.

I think it might be related with

def get_queryset(self):
        return

I will take a deeper look.

@chogarcia
Copy link
Author

The only different thing I found from DRF that your Mixin does not have is:

        if isinstance(queryset, QuerySet):
            # Ensure queryset is re-evaluated on each request.
            queryset = queryset.all()

Do you think this might be why it does not reflect the model changes through the API View when you change a model in the backend?

@chogarcia
Copy link
Author

Any help would be much appreciated.

@MattBroach
Copy link
Owner

I think that line you found in DRF might be the key, @cho-is -- nice sleuthing. The first thing I would try would be replacing these lines in the multiple model mixin:

for pair in queryList:
    # Run the queryset through Django Rest Framework filters
    queryset = self.filter_queryset(pair[0])

with this:

for pair in queryList:
    queryset = pair[0].all()
    # Run the queryset through Django Rest Framework filters
    queryset = self.filter_queryset(queryset)

I won't have time to test this in the next few days, but if you try it out, let me know what happens.

@chogarcia
Copy link
Author

It seems to be working fine now.

Many thanks! much appreciated.

@MattBroach
Copy link
Owner

Glad it's fixed. I'll try to update the repo with the changes in the next week or so.

@tony47
Copy link

tony47 commented Jun 10, 2016

Hello, I am experiencing caching issues even with this fix.

The only way to receive updated results is by changing line 73 in mixins.py:

queryset = self.filter_queryset(query.queryset)

to this one:

queryset = query.queryset.all()

Please, let me know if you need more details from me.

@MattBroach
Copy link
Owner

Hmm, the mixin has gone through some refactoring, and I guess the caching problem snuck back in. Rather than replacing line 73 entirely, does the caching work if you insert your suggested line before, like:

queryset = query.queryset.all()
queryset = self.filter_queryset(queryset)

I'd still like to run the queryset through the filter backend, so I don't want to remove that line entirely. Let me know if the above suggestion still fixes the caching issue.

@MattBroach MattBroach reopened this Jun 10, 2016
@tony47
Copy link

tony47 commented Jun 10, 2016

Thanks for your response.

I confirm that doing so still fixes the caching issue.

@MattBroach
Copy link
Owner

Thanks for confirming, @tony47. Fix is now in PyPI.

@mbox
Copy link
Contributor

mbox commented Oct 19, 2016

We've run into a problem with this - it breaks the ability to cache the querysets and return them from the get_queryList() method.

Code like this used to work:

def get_queryList(self):
    querylist = cache.get(CACHE_KEY)
    if querylist is None:
        # Calculate querylist and cache here...
        ....
    return querylist

And mean that subsequent calls to the API would not result in a new DB query.
But now this doesn't work because each queryset is forced to be re-evalulated by query.all()

Equivalent code works fine in DRF because the re-evaluation is forced in get_queryset, so if get_queryset is overridden it's assumed the user knows what they're doing.

So the right fix is to move this to the get_queryList() function. I'll prepare a PR.

@RobertoKennedy
Copy link

adding

queryset = ''

works

holyshit, thanks

@python997
Copy link

adding

queryset = ''

works

Genius!

@islombek-cs
Copy link

The solution is adding ' ' this in views file. After you have called serializer class like this:
serializer_class = ExampleSerializers
queryset = ' '

that is a solution of our bug!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants