Project

Profile

Help

Issue #3093

api schema includes the same params for all request types

Added by daviddavis almost 2 years ago. Updated 6 months ago.

Status:
MODIFIED
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:
Sprint 32

Description

Hit the api schema (i.e. http :8000/api/v3/) and you'll see tons of extra params on links.

A couple examples:

1. Publish seems to take an 'auto_publish' param
2. Every delete endpoint takes the model's full set of parameters. Artifact's delete endpoint takes md5, sha1, etc.
3. The importer sync endpoint takes all the repo params including feed_url, download_policy, etc

I think whenever we specify a detail_route it's automatically using the model's serializer to determine what params to show in the API schema.

Associated revisions

Revision e25a99d0 View on GitHub
Added by ttereshc over 1 year ago

Include filter fields into schema for read actions only

closes #3093
https://pulp.plan.io/issues/3093

Revision e25a99d0 View on GitHub
Added by ttereshc over 1 year ago

Include filter fields into schema for read actions only

closes #3093
https://pulp.plan.io/issues/3093

Revision e25a99d0 View on GitHub
Added by ttereshc over 1 year ago

Include filter fields into schema for read actions only

closes #3093
https://pulp.plan.io/issues/3093

History

#1 Updated by daviddavis almost 2 years ago

  • Description updated (diff)

#2 Updated by bmbouter almost 2 years ago

+1 to deleting auto_publish even if we bring it back later because it doen't do anything. We can bring it back when that features is ready to be implemented.

#3 Updated by amacdona@redhat.com almost 2 years ago

+1 to removing extra fields, but lets not leave it up to one person to figure out what they all are.

Before this is groomed, I'd like to see:
# change to a task "Remove unused fields"
# list all the fields to remove (by model)
^ this work as been moved to a separate issue: https://pulp.plan.io/issues/3097

Removing auto_publish and other unused fields should be a separate task. I think this issue is that all serializer parameters are accepted for every request type.

#4 Updated by amacdona@redhat.com almost 2 years ago

  • Subject changed from Tons of extra params appear in the api schema to api schema includes the same params for all request types

This is baked into the django rest framework. Unless we specify otherwise, a ViewSet shares a single serializer. That means that the schema will include the same params for PUT and DELETE, when DELETE should not need any.

The DRF way to change this is to use a different serializer for some views. We could make a destroy view that uses a destroy serializer that accepts no parameters.... But DRF doesn't raise a 400 when extra parameters are passed, so this isn't a behavior problem, but it is a documentation problem, and it would be a shame to add so much code when it wouldnt change behavior.

I hope that this can be fixed at the api auto-documentation level, or in the schema view.

#5 Updated by dalley almost 2 years ago

  • Sprint/Milestone set to 46
  • Triaged changed from No to Yes

#6 Updated by bizhang almost 2 years ago

I think this is an issue introduced in drf 3.7

With djangorestframework==3.6.4 installed the Artifact DELETE schema looks like:

"delete": {
            "_type": "link",
            "url": "/api/v3/artifacts/{id}/",
            "action": "delete",
            "fields": [
                {
                    "name": "id",
                    "required": true,
                    "location": "path",
                    "schema": {
                        "title": "id",
                        "description": "A UUID string identifying this artifact.",
                        "_type": "string" 
                    }
                }
            ]
        }

#7 Updated by mhrivnak almost 2 years ago

  • Sprint/Milestone changed from 46 to 47

#8 Updated by ttereshc almost 2 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to ttereshc

#9 Updated by ttereshc almost 2 years ago

@bizhang, +1
It seems like there was a major change0 in schema generation to give devs more control over it.
/me is looking into changes and for a right way to change our schema.

0 https://github.com/encode/django-rest-framework/pull/5354

#10 Updated by daviddavis almost 2 years ago

FWIW, the user delete endpoint only has the id parameter and none of the other params like username or is_superuser. Not sure why it's working there.

#11 Updated by rchan almost 2 years ago

  • Sprint/Milestone changed from 47 to 48

#12 Updated by daviddavis almost 2 years ago

  • Tags Pulp 3 MVP added

#13 Updated by ttereshc almost 2 years ago

  • Status changed from ASSIGNED to POST

#14 Updated by ttereshc almost 2 years ago

FWIW, this change https://github.com/encode/django-rest-framework/pull/5454 in DRF changed behaviour between 3.6 and 3.7.

#15 Updated by rchan almost 2 years ago

  • Sprint/Milestone changed from 48 to 52

#16 Updated by rchan almost 2 years ago

  • Sprint/Milestone changed from 52 to 53

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

  • Sprint/Milestone changed from 53 to 54

#18 Updated by bmbouter over 1 year ago

  • Sprint set to Sprint 32

#19 Updated by bmbouter over 1 year ago

  • Sprint/Milestone deleted (54)

#20 Updated by ttereshc over 1 year ago

  • Status changed from POST to MODIFIED

#21 Updated by dkliban@redhat.com over 1 year ago

  • Sprint/Milestone set to 3.0

#22 Updated by bmbouter 6 months ago

  • Tags deleted (Pulp 3, Pulp 3 MVP)

Please register to edit this issue

Also available in: Atom PDF