Fixing support for subresources and non-Django backends #670

Closed
wants to merge 3 commits into from

2 participants

@mitar

Because ResourceOptions defines queryset as None, hasattr is always true. A consequence is that even if queryset is not define/applicable (for example, for subresources or non-Django backends trying to override this class), Tastypie tries to extract self._meta.queryset.query.query_terms and it blows up.

This is why this check should be done differently.

Furthermore, it would be also much better if self._meta.queryset.query.query_terms would be accessed part by part, because in my case (https://github.com/mitar/django-tastypie-mongoengine) for example I have a am providing a queryset-like object there, which does not have all internals, just external API the same. And so there is no query. Of course I could add it (and I am doing that now as a workaround), but it is ugly. It should try a bit more gently (or simply wrap it into the exception handler).

@mitar

Please pull this in. It is preventing https://github.com/mitar/django-tastypie-mongoengine from working with unpatched Tastypie and I do not find any smart workaround.

@issackelly

If you can provide a test that is broken in master and that this patch fixes, I'll merge it for you. I get that it's straightforward, and I understand your reasoning, just trying to maintain our contribution guidelines.

@mitar

It is not really possible to provide a test case because this happens only when you try to subclass ModelResource with a resource type which might not have queryset defined in Meta (because maybe subresource parent has queryset defined). If there would be existing tests for extended resources in Tastypie, it would be easier.

But to ask you otherwise: is there any way for if hasattr(self._meta, 'queryset'): to be false? If not, then why is there this if there? If yes, probably it was meant exactly for situation to be subclassed. And it does not really work because ResourceOptions has queryset always defined (but set to None) for default. So hasattr(self._meta, 'queryset') is always true.

So this is just code which makes subclassing easier/possible. Not really bug in Tastypie, but making Tastypie playing nice with others.

So, or remove hasattr(self._meta, 'queryset') (because it is useless) or make it useful. And this is just program logic thing.

@issackelly

Ok. I'm going to leave this on the backburner until a test gets written, by me or otherwise

@issackelly

It's possible to have a ModelResource that just uses object_class & not queryset. queryset is actually just a convenient shortcut.

class TheTestResource(ModelResource):
    class Meta:
        object_class = Note
        filtering = {
            'name': ALL,
        }

ttr = TheTestResource()
self.assertEqual(ttr.build_filters(), {...})
@mitar

Are you sure? code here directly access queryset.

@issackelly

I guess I was assuming if you're overriding ModelResource and the thing that you want as your backend doesn't have a queryset you'll also override get_object_list.

Resource is the recommended thing to subclass if you're not working with a django model

@mitar

I am overriding get_object_list. But I am saying that there is no existing test resource implementation, which would do something like that in current test. So it is OK if I create a custom resource, which overrides get_object_list, not defines queryset, and shows that GET fails because build_filters fails?

@issackelly

Yeah, that'd be fine, You don't even have to define get_object_list for this to fail.

@mitar

Added test. Also fixed on Django 1.5 incompatibility.

@issackelly

This looks good, I'll test and merge as soon as I can. Thanks.

@issackelly issackelly added a commit that closed this pull request Oct 20, 2012
@issackelly issackelly Fixed an issue where queryset needed to be defined for build_filters …
…to succeed on a ModelResource. Affected alternate implementations of ModelResource.

Thanks Mitar for the patch, test, and follow-up
Closes #670
342076c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment