AutoSchema fails when set on detail_route

[axnsan12]

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

1. Create a view with a detail_route with a schema kwarg

        class AViewSet(GenericViewSet):
            @detail_route(schema=AutoSchema(manual_fields=[
                coreapi.Field(
                    "my_extra_field",
                    required=True,
                    location="path",
                    schema=coreschema.String()
                ),
            ]))
            def a_detail_route(self, request, my_normal_field):
                pass

2. Register the view with a router

        router = routers.SimpleRouter()
        router.register(r'detail', AViewSet, base_name='detail')
        routes = router.urls

3. Attempt to generate a schema from the view

        callback = routes[0].callback
        generator = SchemaGenerator()
        view = generator.create_view(callback, 'GET')
        link = view.schema.get_link('/a/url/{id}/', 'GET', '')

Expected behavior

A schema is generated for the detail route using the given AutoSchema.

Actual behavior

Crashes:
AttributeError: 'AutoSchema' object has no attribute '_view'

[carltongibson]

Hi @axnsan12. Thanks for the report (and the test-case).

This use-case isn't currently supported. It's not documented that it should work. But it does seem like a good enhancement to be able to customise the schema for the extra action decorators declaratively.

Current workaround will be to subclass AutoSchema, override get_link to check self.view.action and adjust fields for your detail route.

Related #5621 (comment): We should Extract Method on the logic to adjust the generated fields list...

[axnsan12]

Most class options work when set on a detail_route, so intuition would guide one to think this should also work.

Upon looking closer, this seems to be related to ViewInspector wanting to bind itself as a descriptor. Making this work would require ViewInspector to act as a data descriptor instead of a method descriptor (i.e. also implement __set__, in essence becoming a kind of @property on the view class).

A more concrete description of what I mean:

class ViewInspector(object):
    """
    Descriptor class on APIView.

    Provide subclass for per-view schema generation
    """
    def __get__(self, instance, owner):
        """
        Enables `ViewInspector` as a Python _Descriptor_.

        This is how `view.schema` knows about `view`.

        `__get__` is called when the descriptor is accessed on the owner.
        (That will be when view.schema is called in our case.)

        `owner` is always the owner class. (An APIView, or subclass for us.)
        `instance` is the view instance or `None` if accessed from the class,
        rather than an instance.

        See: https://docs.python.org/3/howto/descriptor.html for info on
        descriptor usage.
        """
        real_schema = getattr(instance, '_real_schema', self)
        real_schema.view = instance
        return real_schema

    def __set__(self, instance, other):
        """
        `__set__` is called when the descriptor is set on the owner.
        (That will be when `view.schema = other` is called in our case.)

        `other` is always the new value being set (most likely a ViewInspector,
        or subclass for us.)
        `instance` is the view instance or `None` if accessed from the class,
        rather than an instance.

        If __get__ is defined without defining __set__, this object becomes a
        `non-data descriptor`, which has lower lookup priority than an object's
        __dict__. This means that calls to setattr(instance, 'schema', other)
        would cause the descriptor logic to be bypassed altogether. It is
        necessary to also implement __set__ to become a `data descriptor` and
        prevent this.
        From the python documentation link above:
            * data descriptors always override instance dictionaries.
            * non-data descriptors may be overridden by instance dictionaries.

        This essentially enables the replacing of a view's schema by setting
        it on an instance, and is necessary to support e.g. setting the schema
        on a detail_route declaration.
        """
        if isinstance(other, ViewInspector):
            instance._real_schema = other

This works and passes the test, but feels a bit hackish and confusing.

[carltongibson]

Hey @axnsan12.

Yep. I can see why you'd think this may work. (Agreed it would be a nice enhancement.)

No problem, in theory, adding a __set__. (As ever it'll be complexity vs benefit.)

If you want to expand #5631 to include a more featured test case where the user is setting manual fields via the view's schema attribute as well as using detail_route as per here (as well as similar combinations if you can — the more varied the better) that would be great.

We can then think about an implementation.

Just to note now: it may be that we turn down adding this — sticking with the subclassing approach — depending on whether it turns out suitably nice™.

Similar to that is per-method custom fields, as per #5621. There the current best option is to allow people to handle their own case in a subclass without trying to do everything out-of-the-box. The subclasses are simple; the general case has many edge-cases to consider; thus the cost-benefit is on the side on not providing that.

Obviously all such judgements are open to re-assesment in the face of a nice implementation. 🙂

[axnsan12]

Hello,

I see now that this is a bit more complicated than I imagined, from a design point of view.

I will come back to this after I play with views/schema generation and understand it better.

[carltongibson]

Note for later: Make sure to see the test case in #5631

[axnsan12]

After further thought I think you may be right that special casing this might not be worth it, but there are arguments both for and against. Here are some of my thoughts on this:

• a detail_route in a ViewSet behaves a bit like a nested APIView (or function based api_view), so it would make sense to be able to be able to set all the variables you can set on an APIView
  • as it is now, to check for a detail_route or list_route from the AutoSchema of a containing viewset you have to switch on both the action and the HTTP method (because list/detail routes can accept more than one method) - I would argue this is a bit of a design flaw in detail/list routes, as they break the general viewset paradigm that 1 action == 1 class method == 1 (path + HTTP method) combination that applies to the default actions
• the only reason this does not work is because of the slightly awkward implementation of ViewInspector as a descriptor, but I think it could be made to work

The deeper problem, though, is that a ViewInspector as a whole is responsible solely for generating, in coreapi terminology, Link objects, which correspond to a (path, HTTP method) combination. It makes no sense to me to have a View/ViewSet-wide object which is reponsible for inspecting all its Links, in a perhaps if-else-if-else manner in the get_link method. IMO, it would be cleaner/more consistent to have a sort of get_schema method and schema_class attribute, à la get_serializer/serializer_class. Like that you could return a different AutoSchema subclass/instance for specific actions/methods.

So, overall I feel like making AutoSchema work on detail_route would not solve much, since it simplifies a narrow use case while not solving problems in broader use cases.

[carltongibson]

Hi @axnsan12.

Thanks for your comments. Very useful.

Part of this is down to view sets... — you're right that we need to switch on action + method, and that's a bit funcky. However, that is just view sets. (If you pushed hard enough you might find me saying "Don't use them — yeah they're great for getting up and running but..." — part of that is related to this kind of thing...) (Something about explicit is better than... 🙂)

I don't see a barrier to having ViewInspector return a differently configured class for different actions. It's not something we do yet but...

The question for me would be how best to provide the hooks to make that extensible because...

We deliberately avoided adding more API to APIView. The single schema attribute was the smallest footprint we could get away with. Our view — the design decision — was that API on APIView is already complex enough, and we didn't want to add more here. We wanted to keep all the schema related stuff in a separate class.

We'll keep this open. I'll see what I can think up for v3.8 in the new year.

...no sense to me to have a View/ViewSet-wide object which is reponsible for inspecting all its Links, in a perhaps if-else-if-else manner in the get_link method.

I hear where you're coming from: It looks a lot nicer if you're using separate views instead of viewsets (see opening points.)

Prior to v3.7 we had a single API-wide object doing if-else-if-else on the whole API. That's why it was impossible to customise. Bar these points about Viewsets, I think single object inspecting single view is about right. (Again, we really didn't want to bundle the reflection logic into APIView.)

[axnsan12]

Hello again,

And what about

IMO, it would be cleaner/more consistent to have a sort of get_schema method and schema_class attribute, à la get_serializer/serializer_class. Like that you could return a different AutoSchema subclass/instance for specific actions/methods.

Do you think that would be too much API footprint? (as in, 1 attribute and 1 method vs just 1 attribute)... for the common case you would not have to override both, just like a lot of the time you can get away with serializer_class

[hzj629206]

Hello, any update? still get this error.

Currently I have to use this:

class CustomGenerator(generators.SchemaGenerator):
	def create_view(self, callback, method, request=None):
		view = super(CustomGenerator, self).create_view(callback, method, request)
		view.schema.view = view
		return view

[carltongibson]

This is currently a known limitation. "PRs welcome" 🙂

[rpkilby]

I'll take a stab at this in 5605. In general, that PR aims for greater framework support for extra viewset actions. I'm not incredibly familiar with core-api, but at the very least, 5606 should provide the methods necessary to implement schema support.

[rpkilby]

So this isn't within the scope of 5605, but I did take a few minutes to look at this.

Worth noting, the below fails at the assert self._view is not None check:

class MyViewSet(GenericViewSet):
    detail_schema = AutoSchema(...)

    @detail_route(schema=detail_schema)
    def a_detail_route(self, request, pk, **kwargs)
         ...

The problem with the create_view approach above is that it's not thread-safe, given that you're mutating a class-level variable per-instance. The best bet is probably to change the API schema_class/get_schema and set the schema.view there.

It wouldn't be too difficult to deprecate/change this:

• If schema is present (e.g., set by user on a subclass), the inspector/generator warns and uses the provided schema.
• Otherwise, use get_schema.

[rpkilby]

btw, I'm happy to whip this together if the schema deprecation and move to schema_class/get_schema is okayed.

[carltongibson]

OK, we should at least look at it. (The same idea has come up a course le of times.)

Minimal, what does that look like? Followed by a fix for detail route.

[carltongibson]

@rpkilby Thinking about this quickly, I think my favoured option would be to just add a get_schema(), which by default went like this:

def get_schema(self):
    return self.schema

(Do we need params? We have self.request)

I'll leave it with you. We can discuss further on a PR. 👍

[rpkilby]

Hi @axnsan12 - you may be interested in #5992. If you have the time, it would be helpful if you could verify that the changes behave as expected.

[axnsan12]

Hello!

Tried it and yeah, seems to solve the issue. Great to see this implemented! 😄