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

Set json template for DELETE requests #1090

Merged
merged 2 commits into from
Dec 7, 2020

Conversation

tobias-urdin
Copy link
Contributor

We hardcode the content-type on all DELETE requests
otherwise Pecan will throw NotAcceptable errors when
doing requests to the APIs without any content-type set
and will default to text/html.

@tobias-urdin
Copy link
Contributor Author

This fixes python-gnocchiclient issues.

@tobias-urdin tobias-urdin requested a review from jd December 4, 2020 08:49
Copy link
Member

@jd jd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM that should be tested though :/

@tobias-urdin tobias-urdin changed the title Set application/json content-type for DELETE requests Set json template for DELETE requests Dec 4, 2020
@tobias-urdin
Copy link
Contributor Author

tobias-urdin commented Dec 4, 2020

Setting template to expose() to json is cleaner than hardcoding application/json (pecan will set that content type if template is json [1]). It defaults to using text/html if nothing is given [2].

I don't know any good way to test it since it doesn't change the actual response but only the internal logic that Pecan doesn't error out [3] when it doesn't know the content-type is should use (since we are setting guess_content_type_from_ext=False in pecan config).

[1] https://github.com/pecan/pecan/blob/master/pecan/decorators.py#L53
[2] https://github.com/pecan/pecan/blob/master/pecan/decorators.py#L51
[3] https://github.com/pecan/pecan/blob/78a1ff5f6db3b76afe2ca29a27fa06aa3bc48b64/pecan/core.py#L518

Copy link
Member

@jd jd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be tested with Gabbi?

@tobias-urdin
Copy link
Contributor Author

Let me check.

Setting expose to json will cause content-type
internally for Pecan to be set to application/json.

If this is not set Pecan will throw a NotAcceptable
error if it cannot parse or understand what type
of request it is.

This does not affect the content-type in the response
for DELETE calls.
@tobias-urdin
Copy link
Contributor Author

@jd something like this? I verified the tests passes before the expose() change.

@jd
Copy link
Member

jd commented Dec 4, 2020

Yes, something like that :)

@tobias-urdin
Copy link
Contributor Author

Adding oslo.policy pin here, might be the one causing postgresql job to have issues.

@tobias-urdin
Copy link
Contributor Author

@Mergifyio backport stable/4.4

@mergify
Copy link
Contributor

mergify bot commented Sep 18, 2023

backport stable/4.4

✅ Backports have been created

tobias-urdin added a commit that referenced this pull request Sep 18, 2023
Set json template for DELETE requests (backport #1090)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants