Skip to content

Conversation

@kytta
Copy link

@kytta kytta commented Jan 8, 2025

This PR allows requestBody in "safe" methods (GET, HEAD, DELETE, etc.) when using OpenAPI spec version 3.1.x, where it is valid.

Closes #809

(also related to #218, #1014, and #1133)

@kytta kytta force-pushed the 809-request-body branch from 2c8ce14 to 7fe1272 Compare April 13, 2025 12:36
@kytta
Copy link
Author

kytta commented Apr 13, 2025

@tfranzel sorry to tag you, but I wanted to ask if there is something preventing this MR to be merged and if one can help you with it?

@tfranzel
Copy link
Owner

Hi @kytta, although I am still not a fan of this, I admit this should be allowed with 3.1.

Have you run the tests on this? I have 5 failing tests running this locally. Generally, I think this change should not affect existing projects. I don't think we should add a body to every "safe" method by default, but should rather have this as a opt-in. This is still strongly discouraged and an edge-case.

Can you rebase your PR to the current master please so that the tests do run? Unfortunately, they retired the GH worker image we use yesterday and rerunning this action does not automatically use the new image.

@kytta
Copy link
Author

kytta commented Apr 18, 2025

Have you run the tests on this? I have 5 failing tests running this locally.

I could've sworn I had run them, but you are right, I see the failing tests as well, and from what I can see, my implementation is rather problematic 🙈

Can you rebase your PR to the current master please so that the tests do run?

I will after I have fixed the tests :)

Generally, I think this change should not affect existing projects. I don't think we should add a body to every "safe" method by default, but should rather have this as a opt-in.

(depending on your definition of opt-in) This is exactly what I was imagining, too. We want only those unsafe methods to have a body which actually make use of it and declare that. Meaning, on simple ViewSets, there should be no change introduced with this PR.

@kytta kytta marked this pull request as draft April 18, 2025 07:00
@tfranzel
Copy link
Owner

sounds good @kytta!

One of the failing tests I observed was a added DELETE body where there was none explicitly given, e.g. a ModelViewSet with a class-wide serializer_class should not take take that "generic" serializer and advertise it as the DELETE body. I haven't looked into it much, but it might be non-trivial to do.

opt-in in the sense that DELETE bodies should not show up for everybody out of nowhere just because they have a default serializer_class on a regular ViewSet. We might need to detect wether delete was modified from the default implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Relax "only unsafe methods can have a requestBody" constraint

2 participants