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

Filter fields are technically allowed for all DRF view actions. This messes with CoreAPI schema. #908

Closed
Sonictherocketman opened this issue May 16, 2018 · 3 comments

Comments

@Sonictherocketman
Copy link

When using CoreAPI with DRF and DjangoFilterBackend enabled, the client attempts to map all fields for a create, update or partial_update actions to the query params as well. This breaks the client's ability to use these methods.

Example

If we have a request that attempts to create a book with a name, and name is also a filterable field on our model:

# book views
class BookViewSet():
    filter_fields = ('name',)
   # ...
// schema
client.action('book', 'create', { 'name': 'My Awesome Book' });

This will create a request with both name in the URL param as well as the request body. This is not expected behavior.

POST /api/book?name=My%20Awesome%20Book

name=My Awesome Book

Proposed Solution

We've fixed this in our code by instead pointing our DRF filter backend to a custom override with the following code:

class OnlyFilterOnReadDjangoFilterBackend(DjangoFilterBackend):
    """ A filter backend that only allows for filtering by properties on
    actions deemed as safe. This means that create, update, and partial_update
    actions will not provide filter options.
    """
    SAFE_ACTIONS = ('list', 'retrieve')

    def get_schema_fields(self, view):
        if view.action in self.SAFE_ACTIONS:
            return super().get_schema_fields(view)
        return []
@carltongibson
Copy link
Owner

carltongibson commented May 16, 2018

Hi @Sonictherocketman. Thanks for the report.

My first thought is that this is a bug with the client API — it's not suitably distinguishing between query string parameters and request body when constructing the request.

In general, just disabling filtering for non-safe actions isn't correct. Filtering occurs when establishing the base queryset that an update will be made against.

I'll leave this open for now, just to have a think about it.

@Sonictherocketman
Copy link
Author

I'm also kinda think that it's a client issue, but the more I thought about it, I feel like the actions don't really make any sense on write queries (especially writing to a specific resource) and imo the schema shouldn't have those fields if they shouldn't be used.

Thanks for the quick reply and I'm super interested in what your thoughts are on this.

@carltongibson
Copy link
Owner

More or less, I think we're limited here by our desires for automagic code generation outstripping the tech (in it's current state).

There's lots we can't (yet) do with schemas (in any of the available formats) that we'd like to — for example, with CoreAPI, we're not yet able to group parameters together for a date range filter to show that they're related. (There's a lot more we can do writing schemas by hand, but no one wants to do that, of course.)

But it evolves, and as it does so does the tooling. It'll improve.

That this particular line:

client.action('book', 'create', { 'name': 'My Awesome Book' });

uses the same dict for both query string and the request body isn't great. I guess it's the abstraction leaking. You really want to be building this request by-hand, specifying query params and body in separate places.

This isn't currently anything we can address here. Thanks again for the report though.

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

2 participants