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
Use correct schema mode for openAPI respones, fixes #1137 #1139
base: master
Are you sure you want to change the base?
Conversation
Hi @nofalx Could you also make a test case for this ? |
Hi @vitalik After Examining the issue one more time, it seems the issue is little bit deeper than what it appears to be. The issue is that currently there is no openAPI method to differentiate between requests requires and response required. Which means that the request and response schemas would conflict on I see solving the issue in 2 approaches. Approach A : Give the user the freedomThe user can decided to mark the defaults as required by relying on pydantic as below. The issue with this approach is that even when a schema is specified as type for request body it would be required, I guess this is the behavior in fastapi and other projects.
Approach B : Smart DetectionWe could detect if the schema is used as request or response with current code and generate the correct schema accordingly. However we risk having Schema conflict if the same one was used. We can handle this by passing a config open to Django Ninja to do one of the following:
|
I think the best approach would to follow fastapi steps, have 2 separate schemes but give the user the ability to retain current behavior https://fastapi.tiangolo.com/how-to/separate-openapi-schemas/ |
Hello @nofalx, why not always generate the schema from model with all fields as required (suitable for GET), as there is always a way to relax it with |
Hi @musanek , Im not sure I do understand what you mean very well. Originally we faced this issue when using modelSchema with a django model. As you can understand we can not change our django model to suit the openAPI schema generation. The fields we have issue with are under The issue is that the field is required to be non null but has default value. This means it should not be required when making a request as it will fall back to the default value (Current behavior), however when it is a response the field is guaranteed to be "required" as it will never be null (Currently missing). When relying on tools to automatically convert the openapi schema to typescript types for the frontend for example, this will generate incorrect possible nullable fields in response type and complicate things on the frontend. This issue extended beyond django and Currently the PR solves this issue and sets the correct This can be easily fixed and adjusted. We just need to warn the current library user about this "breaking change" in case they rely on automated tools to convert openAPI to types, as the name of the Schemas will be different. We can have an option to optout of this new Input/Output Schema name addition incase it is too much work for them to adjust all schemas names in their code. I just need the maintainers agreeing on the approach before I invest in adjusting this PR |
Hello @nofalx , thank you for getting detailed explanation. I have to admit I am new to Django and rest framework on top of it, so might be missing the foundation. But on what you wrote, wrongly generated TS types is exactly what brought me here. I am currently falling back to Required and it does the job. |
fixes #1137