Project

Profile

Help

Issue #3093

api schema includes the same params for all request types

Added by daviddavis over 2 years ago. Updated 5 months ago.

Status:
CLOSED - CURRENTRELEASE
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 about 2 years 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 about 2 years ago

Include filter fields into schema for read actions only

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

History

#1 Updated by daviddavis over 2 years ago

  • Description updated (diff)

#2 Updated by bmbouter over 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 over 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 over 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 over 2 years ago

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

#6 Updated by bizhang over 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 over 2 years ago

  • Sprint/Milestone changed from 46 to 47

#8 Updated by ttereshc over 2 years ago

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

#9 Updated by ttereshc over 2 years ago

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

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

#10 Updated by daviddavis over 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 over 2 years ago

  • Sprint/Milestone changed from 47 to 48

#12 Updated by daviddavis over 2 years ago

  • Tags Pulp 3 MVP added

#13 Updated by ttereshc over 2 years ago

  • Status changed from ASSIGNED to POST

#14 Updated by ttereshc over 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 over 2 years ago

  • Sprint/Milestone changed from 48 to 52

#16 Updated by rchan over 2 years ago

  • Sprint/Milestone changed from 52 to 53

#17 Updated by jortel@redhat.com over 2 years ago

  • Sprint/Milestone changed from 53 to 54

#18 Updated by bmbouter about 2 years ago

  • Sprint set to Sprint 32

#19 Updated by bmbouter about 2 years ago

  • Sprint/Milestone deleted (54)

#20 Updated by ttereshc about 2 years ago

  • Status changed from POST to MODIFIED

#21 Updated by dkliban@redhat.com about 2 years ago

  • Sprint/Milestone set to 3.0.0

#22 Updated by bmbouter about 1 year ago

  • Tags deleted (Pulp 3, Pulp 3 MVP)

#23 Updated by bmbouter 5 months ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Please register to edit this issue

Also available in: Atom PDF