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

Django ?format doesn't work correctly in some endpoints #4450

Closed
jerabekjiri opened this issue Sep 19, 2023 · 6 comments · Fixed by #4506
Closed

Django ?format doesn't work correctly in some endpoints #4450

jerabekjiri opened this issue Sep 19, 2023 · 6 comments · Fixed by #4506
Assignees
Labels

Comments

@jerabekjiri
Copy link
Contributor

Version

pip3 list | grep -E "pulpcore|pulp-ansible|galaxy-ng"
galaxy-ng                                4.9.0.dev0
pulp-ansible                             0.19.0
pulpcore                                 3.28.14

Describe the bug
Pulp incorrectly assumes that ?format=api|json is a filter parameter instead of the django rendering format type. This issue only occurs in some endpoints.

To Reproduce
Steps to reproduce the behavior:
Visit endpoints
/pulp/api/v3/repositories/?format=api or
/pulp/api/v3/repositories/ansible/ansible/?format=json:

{
  "errors": [
    "Invalid Filter: 'format'"
  ]
}

/pulp/api/v3/repositories/?format=test:

{
  "detail": "Not found."
}

Screenshot from 2023-09-18 22-49-54
Screenshot from 2023-09-18 22-49-41
Screenshot from 2023-09-19 18-34-11

Expected behavior
Should switch between raw format and Django's pretty format, and display the data as usual.

Additional context
Discovered in ansible/galaxy_ng#1891

@jerabekjiri
Copy link
Contributor Author

I think this can be fixed by adding format to DEFAULT_FILTERS in BaseFilterSet

def is_valid(self, *args, **kwargs):
is_valid = super().is_valid(*args, **kwargs)
DEFAULT_FILTERS = [
"exclude_fields",
"fields",
"limit",
"minimal",
"offset",
"page_size",
"ordering",
]

@mdellweg
Copy link
Member

That's not even supposed to be a filter. You should user the encoding header to determine the used format. Also i'm unsure we support anything but json anyway.

@mdellweg
Copy link
Member

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept

@jerabekjiri
Copy link
Contributor Author

@mdellweg Okay, it certainly works with accept headers, but it's still inconsistent with the format parameters. So, if pulp doesn't support text/html (?format=api), should the format parameter be completely forbidden?

Also, for https://issues.redhat.com/browse/AAH-2148, we want to use the 'pretty' format ('format=api') for the galaxy-beta. Currently, it doesn't work, and I'm wondering if this issue could be related to Pulp not supporting it and not displaying it in the production image.

@mdellweg
Copy link
Member

Looks like both methods are supported after all:
https://www.django-rest-framework.org/topics/browsable-api/#formats

@mdellweg
Copy link
Member

@jerabekjiri can you file your solution as a PR?
On a first glance, it looks sane to me.

jerabekjiri added a commit to jerabekjiri/pulpcore that referenced this issue Sep 27, 2023
@jerabekjiri jerabekjiri self-assigned this Sep 27, 2023
dkliban pushed a commit that referenced this issue Oct 3, 2023
patchback bot pushed a commit that referenced this issue Oct 3, 2023
fixes: #4450
(cherry picked from commit 600a8ea)
patchback bot pushed a commit that referenced this issue Oct 3, 2023
fixes: #4450
(cherry picked from commit 600a8ea)
patchback bot pushed a commit that referenced this issue Oct 3, 2023
fixes: #4450
(cherry picked from commit 600a8ea)
gerrod3 pushed a commit that referenced this issue Oct 3, 2023
fixes: #4450
(cherry picked from commit 600a8ea)
gerrod3 pushed a commit that referenced this issue Oct 3, 2023
fixes: #4450
(cherry picked from commit 600a8ea)
gerrod3 pushed a commit that referenced this issue Oct 3, 2023
fixes: #4450
(cherry picked from commit 600a8ea)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants