Bug in get_schema_fields

[pySilver]

There is an issue at get_schema_fields which prevents nested views to be processed. The problem is that url named argument contains a data that is consumed by queryset is always None since you manually setup a view.

Example

    # some view
    def get_project(self):
       return Project.objects.get(id=self.kwargs.get('project_id'))

    def get_queryset(self):
        project = self.get_project()
        return self.queryset.filter(project=project)

Root of a problem lays here:

# django_filters.rest_framework.backends.DjangoFilterBackend#get_schema_fields
filter_class = self.get_filter_class(view, view.get_queryset()) # <---- :(

[carltongibson]

@pySilver Thanks for the report. Any chance you can reduce this down to a test case so we can see what's happening?

[pySilver]

@carltongibson sorry, I'm kinda overloaded right now :( Just consider the fact that view.get_queryset() is not a best option since you have no guarantee that view does not use anything external in that method (self.request.user for instance). Since this queryset has no influence on introspection results we might use something like that:

    def get_schema_fields(self, view):
        # This is not compatible with widgets where the query param differs from the
        # filter's attribute name. Notably, this includes `MultiWidget`, where query
        # params will be of the format `<name>_0`, `<name>_1`, etc...
        filter_class = None
        if hasattr(view, 'filter_class') and view.filter_class is not None:
            queryset = view.filter_class.Meta.model._default_manager.all()
            try:
                view.filter_class._meta.model._default_manager
            except AttributeError:
                pass

            filter_class = self.get_filter_class(view, queryset)

        return [] if not filter_class else [
            coreapi.Field(name=field_name, required=False, location='query')
            for field_name in filter_class().filters.keys()
        ]

[rpkilby]

Correct me if I'm wrong, but I don't think nested views are a supported feature of DRF? Without additional details, it seems like the problem is that the nested view hasn't been properly initialized (ie, the view not having a request object).

[pySilver]

@rpkilby nested views are supported since 2.x or even earlier. It's simply about the urlconfig and has nothing do to with DRF itself.

Having urlconfig like this /(?P<project_id>\d+)/issues/(?P<issue_pk>\d+) will give you project_id in your IssuesViewSet and its value will never be None, since reverse_lazy won't generate such url. So making fake request object won't solve the issue since you have no idea what might be inside view.queryset method and this method is overloaded very often.

[rpkilby]

It's simply about the urlconfig and has nothing do to with DRF itself.

Gotcha - the viewset isn't nested in another viewset, it's a multi-argument/nested url pattern.

The problem is that url named argument contains a data that is consumed by queryset is always None since you manually setup a view.

Do you mean the view initialized by the schema generation? The view instance is directly passed to get_schema_fields() and isn't manually created by the filter backend.

So making fake request object won't solve the issue

I'm not advocating creating a fake request object. I'm saying that the view should have been properly initialized with the request. This might have been a bug that was fixed in a more recent release. See encode/django-rest-framework#4383.

Also, keep in mind that this would break the ViewSet.filter_fields argument. Without a valid queryset (provided by view.get_queryset()), there is no model to generate the filterset with.

[pySilver]

@rpkilby yeah I'm aware that solution I've posted earlier won't work for filter_fields I'm just in a rush to upgrade our app to latest versions of drf, django-filters and, drf-swagger so I intentionally omitted filter_fields support since we never use this feature.

This might have been a bug that was fixed in a more recent release. See encode/django-rest-framework#4383.

It's still an issue, im using latest 3.5.2 release.

@tomchristie what you think about that? Whole issue looks like method signature problem since we setup queryset just because it's required and this has no real influence on fields schema extraction itself.

[rpkilby]

this has no real influence on fields schema extraction itself.

No, but it may be used during filterset creation. Without the queryset/model, you have no filterset to get fields from. The queryset model is also used as part of a sanity check.

Either way, I'm not convinced that the real issue is with get_schema_fields(). The referenced issue indicates that the view instance should have a valid request object. However, you're reporting that this is not the case.

[pySilver]

The referenced issue indicates that the view instance should have a valid request object

Ok, how one would create a valid request object in that case? I can't see how we can create required imaginary project_id since it should be a valid one, because it might be used inside view.get_queryset ...

[rpkilby]

Ok, how one would create a valid request object in that case?

It's not clear to me why you're manually creating a request here. This should be DRF's responsibility when initializing the view.

[pySilver]

thats what I ment – how DRF could do that, it sounds like "impossible"

[rpkilby]

Alright, did a little poking around the schema generation. The problem is not that the request object doesn't exist. The problem is that the view is initialized without args/kwargs (see SchemaGenerator.create_view).

Without the kwargs, you don't have a valid project_id. This makes sense now.

I would do the following:

    def get_project(self):
        try:
            return Project.objects.get(id=self.kwargs.get('project_id'))
        except Project.DoesNotExist:
            return None

    def get_queryset(self):
        project = self.get_project()
        if project is None:
            return self.queryset.none()
        return self.queryset.filter(project=project)

[pySilver]

yeah this is basilcally what im doing now, and it looks bad. I mean you fix working code in order to make it work with schema generator.

[rpkilby]

I mean, what do you do now if a user inputs an invalid project_id? Wouldn't your server 500 b/c of the uncaught Project.DoesNotExist?

[pySilver]

it would raise 404 on get_project lvl. Okay, what if one uses request.user inside view.get_queryset ? Are we supposed to:

def get_queryset(self):
        project = self.get_project()

        if project is None:
            return self.queryset.none()

        if self.request and self.request.user:
            self.queryset =  self.queryset.filter(author=self.request.user)

        return self.queryset.filter(project=project)

And that would lead to useless checks all over again and again across large codebase just in order to make schema generation work... It sounds insane, taking into account that queryset has no influence on fields setup at filterset class lvl. At least not now.

[rpkilby]

what if one uses request.user

There should be no problem here. The view is initialized with the request. The request user, headers, etc... should all be there.

And that would lead to useless checks all over again and again across large codebase just in order to make schema generation work... It sounds insane

Checking for the existence of the parent object doesn't seem insane to me. This really seems to be the only case where this kind of check would be necessary.

Additionally, you may not actually need to fetch the project instance. Wy not:

    def get_queryset(self):
        return self.queryset.filter(project_id=self.kwargs.get('project_id'))

taking into account that queryset has no influence on fields setup at filterset class lvl. At least not now.

Except this is not correct. If using filter_fields, you need a queryset/model in order to generate filters.

[tomchristie]

There's always going to be some limits to what we'll be able to do with view introspection for schema generation. Broadly I'd be in favour of us making sure we document those cases and describe how you can best handle parts of it manually when required.

[rpkilby]

Yep - this is an issue of expectations. Users (reasonably so) expect that their views will be initialized with the request, args, and kwargs. With schema generation, the view is artificially constructed (as the incoming request does not target the view), so the args and kwargs are empty. It might be useful to add a note to the schema docs that this is usually a non-issue but that users may need to account for this.

The django-filter docs should probably have a note that the backend specifically calls get_queryset() on the view.

[rpkilby]

Hi @tomchristie @pySilver. See #553, which adds a section to the docs about the caveat.

[pySilver]

@tomchristie, @rpkilby what if we some manual reflection helper that might override or even disable some inspection when user wants it?

We've build really large application with DRF and we've used DRF swagger from early days and we've faced a situation that 10% of views cannot be inspected on any circumstances, they are just not fit into tooling. So at some point we end up with almost good API docs. This could be fixed if there will be some method on view/viewset level that can provide required signatures manually. Smth like:

def openapi(self, *args, **kwargs):
    return {...} # dict with manually provided signatures of this view(set). "GET" -> "resource" -> "structures"

It looks like a hack but on the other hand it allows you to have 90% of api docs build from source and just 10% provided manually. This is why I've created YAML support for drf-swagger in early days.

[carltongibson]

@pySilver:

what if we some manual reflection helper that might override or even disable some inspection when user wants it?

How is this different from the approach outlined in the Explicit schema definition section of the DRF docs? For a FilterSet you can just manually implement get_schema_fields. Do we really need extra API?

[carltongibson]

Closed by #553

Schema Generation is pretty new. Both Limitations and Patterns will emerge over time. I'm happy to look at problems and concrete suggestions but don't consider this an active issue at the moment.

General strategy will be (as ever) to pull the logic back into the view, bypassing the framework code, and implementing just for the case at hand. When people have done that a few times we can see where to generalise, or, if it's just edge cases, where to document.

[carltongibson]

I'm reopening this on the back of encode/django-rest-framework#4960

When this came up previously, we had this exchange:

what if one uses request.user

There should be no problem here. The view is initialized with the request. The request user, headers, etc... should all be there.

i.e. We could live with conditionally checking for (e.g.) kwargs in get_queryset but not request and request.user — that would be too much.

Well, from encode/django-rest-framework#4960, with the new API docs, request is None so I we need to think again.

First As per @tomchristie's comment on the issue we can catch the exception and set filter_class=None. (Here we should raise a warning, to the effect "View class ___ is not compatible with FilterSet class ___ generation in non-request context" — or such.)

Then We should review the need for the get_queryset call.

When filter_class is defined the queryset is unused. (In principle it could be passed when instantiating the filter set but it is not) — so we could just avoid it (if we handle the sanity check in get_filter_class)

If people are using filter_fields AND want schema generation AND can't adjust get_queryset to not raise then... — well, that might just be a limitation.

Finally Is there anything to do to help people with explicitly defining the schema?

[carltongibson]

#659 is the sort of thing I have in mind