Project

Profile

Help

Issue #3537

Filters are applied to detail endpoints

Added by daviddavis over 1 year ago. Updated 6 months ago.

Status:
CLOSED - WONTFIX
Priority:
Normal
Assignee:
-
Category:
-
Sprint/Milestone:
Start date:
Due date:
Severity:
2. Medium
Version:
Platform Release:
Blocks Release:
OS:
Backwards Incompatible:
No
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
QA Contact:
Complexity:
Smash Test:
Verified:
No
Verification Required:
No
Sprint:

Description

In our api schema, it looks like we have filters on both /api/v3/repositories/uuid/versions/ and /api/v3/repositories/uuid/versions/version/.


Checklist

History

#1 Updated by daviddavis over 1 year ago

  • Subject changed from Filters should not exist for /api/v3/repositories/uuid/versions/<version>/ to Filters should not exist for /api/v3/repositories/<uuid>/versions/<version>/
  • Description updated (diff)

#2 Updated by daviddavis over 1 year ago

  • Tags Pulp 3, Pulp 3 MVP added

We should also confirm that this problem doesn't exist elsewhere in Pulp.

#3 Updated by daviddavis over 1 year ago

  • Sprint set to Sprint 35

#4 Updated by dalley over 1 year ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to dalley
  • Triaged changed from No to Yes

#5 Updated by dalley over 1 year ago

Also applies to Task and Worker detail routes, so I assume it applies to all of them.

Worth pointing out that when filtering on a detail route, a 404 is returned if the resource doesn't match the filter, instead of the 200 with an empty list returned when filtering on a list route.

#6 Updated by dalley over 1 year ago

So, according to one of my ex-coworkers who is the top contributor to django-filters by a solid margin:

Yeah, so this is more of a DRF issue and not a django-filters issue
basically, django-filters implements the DRF filter backend spec, but it has no control over when DRF invokes it
Your problem spot is here: https://github.com/encode/django-rest-framework/blob/3.8.2/rest_framework/generics.py#L85
`get_object` calls `filter_queryset`
Your best bet is to override filter_queryset
and to noop on `retrieve` actions
so...

def filter_queryset(self, queryset):
    if self.action != 'list':
        return queryset
    return super().filter_queryset(queryset)
 

All filter_queryset does is invoke the filter backends
Just make sure that your permissions are not implemented as filters
or something like that
which would be weird
and probably bad

So, this is fixable, but it does involve overriding basic DRF behavior. Is it still something we want to do? I would say, you probably just shouldn't attempt to filter on a detail endpoint to begin with.

#7 Updated by daviddavis over 1 year ago

Is opening an issue or PR against DRF an option?

#8 Updated by dalley over 1 year ago

Already been done - the issue was closed

https://github.com/encode/django-rest-framework/issues/5412


This will be more clear if you consider the filter_queryset removes rows a user shouldn't be able to see. If we remove the call to filter_queryset then anyone will be able to see any object.

NB: I agree in your use case that's counter intuitive although in other it's totally valid.
Maybe we can improve that by adding a word on the documentation about the instances calling the filtering.

#9 Updated by daviddavis over 1 year ago

I see.

I'm less worried about users trying to filter on a detail or retrieve endpoint and more worried about how our schema doc looks. I think any user who looks at one of these endpoints is going to wonder why these fields exist.

I don't like the idea of overriding filter_queryset either though. Maybe we can just close this out for now as WONTFIX and see if other users complain?

#10 Updated by CodeHeeler over 1 year ago

I agree that it would be better to not override filter_queryset. Perhaps we could add a note in the docs somewhere at least if we opt for WONTFIX?

#11 Updated by dalley over 1 year ago

FYI I looked at the generated schema and the filter options do not show up on detail endpoints, just list endpoints as you would expect.

They "work" on detail endpoints but (whatever thing is responsible for the schema) is smart enough to know that you shouldn't do it.

#12 Updated by daviddavis over 1 year ago

@dalley, I am seeing something different. Here is the repo versions read endpoint:

                    "read": {
                        "_type": "link",
                        "action": "get",
                        "fields": [
                            {
                                "location": "path",
                                "name": "number",
                                "required": true,
                                "schema": {
                                    "_type": "string",
                                    "description": "",
                                    "title": "number" 
                                }
                            },
                            {
                                "location": "path",
                                "name": "repository_pk",
                                "required": true,
                                "schema": {
                                    "_type": "string",
                                    "description": "",
                                    "title": "" 
                                }
                            },
                            {
                                "location": "query",
                                "name": "version_min",
                                "schema": {
                                    "_type": "number",
                                    "description": "",
                                    "title": "" 
                                }
                            },
                            {
                                "location": "query",
                                "name": "version_max",
                                "schema": {
                                    "_type": "number",
                                    "description": "",
                                    "title": "" 
                                }
                            },
                            {
                                "location": "query",
                                "name": "created_after",
                                "schema": {
                                    "_type": "string",
                                    "description": "",
                                    "title": "" 
                                }
                            },
                            {
                                "location": "query",
                                "name": "created_before",
                                "schema": {
                                    "_type": "string",
                                    "description": "",
                                    "title": "" 
                                }
                            },
                            {
                                "location": "query",
                                "name": "content",
                                "schema": {
                                    "_type": "string",
                                    "description": "",
                                    "title": "" 
                                }
                            }
                        ],
                        "url": "/api/v3/repositories/{repository_pk}/versions/{number}/" 
                    },

#13 Updated by dalley over 1 year ago

@daviddavis

Granted, I'm doing this from the branch that switches to using drf_yasg for docs, instead of drf_openapi. I've noticed that the two have some divergent behavior and that the output from drf_openapi actually doesn't validate properly against the openapi schema, while drf_yasg does.

#14 Updated by daviddavis over 1 year ago

@dalley ah maybe drf_yasg knows somehow to hide the filters. If that's the case, then +1 to WONTFIX on this from me.

#15 Updated by dalley over 1 year ago

Yeah, this is what yasg outputs (yaml format)

/repositories/{repository_pk}/versions/{number}/:
    get:
      operationId: repositories_versions_read
      description: ''
      parameters: []
      responses:
        '200':
          description: ''
          schema:
            $ref: '#/definitions/RepositoryVersion'
      tags:
        - repositories

#16 Updated by dalley over 1 year ago

I'm +1 on WONTFIX as well but I'll leave the issue open for the rest of the day in case the list brings in more discussion.

#17 Updated by amacdona@redhat.com over 1 year ago

I am not very concerned about users actually using the filters. For me, the only important reason to change this is to remove extraneous docs.

Would override of filter_queryset remove those filters from the docs?
  • My instinct is no, which would mean that we still document those filters, but that users couldn't use them.
  • If no, -1 to override
  • if yes (and drf-yasg doesnt work out) I am not too concerned with overriding this in a base class. If some plugin needs filters for a detail route (I cant imagine why they would) they can easily override.

If these filters are "legal" in the code, but not mentioned in the docs, I am fine with that situation and we can close.

If these filters are mentioned in the docs, +0 leave open as a documentation bug. (Remove Pulp 3 MVP tag)

#18 Updated by dalley over 1 year ago

At least on my drf_yasg branch (and again I haven't looked at the current openapi branch), the filters definitely do not show up in the docs for the repoversion detail endpoint regardless. I just checked via the live docs webpage.

And since I do think we're planning to use drf_yasg as soon as the Travis issues get worked out so that the tests can pass, personally that says "wontfix" to me.

#19 Updated by dalley over 1 year ago

  • Status changed from ASSIGNED to CLOSED - WONTFIX
  • Assignee deleted (dalley)
  • Tags deleted (Pulp 3 MVP)

#20 Updated by dalley over 1 year ago

  • Sprint deleted (Sprint 35)

#21 Updated by dalley over 1 year ago

  • Subject changed from Filters should not exist for /api/v3/repositories/<uuid>/versions/<version>/ to Filters are applied to detail endpoints

#22 Updated by daviddavis 6 months ago

  • Sprint/Milestone set to 3.0

#23 Updated by bmbouter 6 months ago

  • Tags deleted (Pulp 3)

Please register to edit this issue

Also available in: Atom PDF