Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add distinction between request and response serializers for OpenAPI #7424

Merged

Conversation

denisorehovsky
Copy link
Contributor

Description

In my custom implementation of DRF views, instead of using get_serializer method, I have two methods: get_request_serializer and get_response_serializer.

It allows me to use one serializer for handling request data, and another serializer for generating response data.

Additionally, I try to generate OpenAPI. But right now there is no distinction between request and response serializers during schema generation. In both methods, get_request_body and get_responses, we call the same method:
def get_serializer(self, path, method)

This PR adds a distinction between request and response serializers. It doesn't change anything under the hood. All it does is it adds two methods:
get_request_serializer and get_response_serializer.

We can override those methods and point to different serializers.


Can be related to #1563

@akx
Copy link
Contributor

akx commented Jul 23, 2020

👍

What a coincidence that we came across the same issue only 1.5 hours apart :)

@denisorehovsky
Copy link
Contributor Author

@akx Hah, yeah. Can't believe it.

@carltongibson
Copy link
Collaborator

@apirobot Can we add short docs for the new methods?

If your view uses different serialisers for request and response you can ...

@denisorehovsky
Copy link
Contributor Author

@carltongibson Yeah, sure. I've pushed new commit.

@gaara4896
Copy link

Can't wait for the release, facing the same issue here too

@akx
Copy link
Contributor

akx commented Aug 25, 2020

Agreed – hoping for a release with this and #7208...

@akx
Copy link
Contributor

akx commented Oct 30, 2020

@carltongibson Can we get this merged? 😄

@yardensachs
Copy link

Oh wow, we really need this at where I work!

@xordoquy
Copy link
Collaborator

Naive question, why don't you use a single serializer with read_only / write_only on fields ?

@carltongibson
Copy link
Collaborator

I don't want to jump off the deep-end of complexity but, I have no problem adding this kind of thing in principle. This patch still needs tests and docs at the least though.

@yardensachs
Copy link

@apirobot Can you add tests and docs?

@elonzh
Copy link

elonzh commented Nov 16, 2020

what about query parameters? relate #7163

@shaun-emburse
Copy link
Contributor

We're hoping to use this at my work and would prefer to be able to just update to latest Django to do so rather than having an internally patched version, so I intend to add documentation and tests for this change at some point in February so this PR can hopefully be merged in March.

@shaun-emburse
Copy link
Contributor

@carltongibson Hi, I added a little bit of documentation of the new functions and a basic test case. I'm glad to add more if desired, just wanted to put something up to start and then ask for feedback on any specific examples of desired test cases to cover or items to cover in documentation or additional areas to document.

I'm really confused by this CI build failure though. It's talking about trying to import a module named six in some pytest_django code. I didn't hit this issue locally. I'm wondering if possibly it's a changed environment on Travis CI having an issue? Maybe my test case is somehow causing this but I don't see how.

@auvipy
Copy link
Member

auvipy commented Feb 16, 2021

you can pull on master and build should pass now

@carltongibson
Copy link
Collaborator

Hey @tomchristie i don't think this is far off, but I didn't get the cycle to swing back to it yet.

@tomchristie
Copy link
Member

I don't have any objection to this.

I'd be interested to see a proper example of this being used in the wild, to get a better idea of how folks are expecting to hook this up in practice. But otherwise, sure let's get this in.

@tomchristie tomchristie merged commit 8812394 into encode:master Apr 20, 2021
mgdaily added a commit to observatorycontrolsystem/observation-portal that referenced this pull request Aug 27, 2021
This code is based off of a PR that has been merged to master in the DRF repo, but has not yet been included in a release. See encode/django-rest-framework#7424
@khamaileon
Copy link

@apirobot Can you provide a full example of how this works? Even with the documentation provided, the view part is still very obscure to me.

@royhzq
Copy link

royhzq commented Nov 3, 2021

Any idea why the current docs have already included the new methods get_request_serializer, get_response_serializer? I find this quite misleading since they don't exist in the latest 3.12 release. Spent quite a bit of time figuring out why these methods aren't being called.

https://www.django-rest-framework.org/api-guide/schemas/#get_request_serializer

Either way, looking forward to seeing this released soon 👍

@shaun-emburse
Copy link
Contributor

It looks like it's on the master branch, which must be what the documentation is generating from, but not in the latest tagged release, which is currently 3.12.4 and dated 2021 May 31st.

I think the dating issue must be because Github is showing the version release date, and that release was actually forked a couple months prior and had final testing, so these changes must have just missed that last release.

Because this change was merged prior to that tagged release date, but it's close, and I do see these code changes listed in the diff between master branch and that tagged release.

sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
…ncode#7424)

* Add distinction between request and response serializers

* Add docs

* document new functions in schemas.md

* add a test case for different request vs response objects

* Correct formatting for flake8

Co-authored-by: Shaun Gosse <shaun.gosse@emburse.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.