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

Use correct schema mode for openAPI respones, fixes #1137 #1139

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nofalx
Copy link
Contributor

@nofalx nofalx commented Apr 24, 2024

fixes #1137

@nofalx nofalx changed the title Use correct schema mode for openAPI respones, Fixes #1137 Use correct schema mode for openAPI respones, fixes #1137 Apr 24, 2024
@vitalik
Copy link
Owner

vitalik commented Apr 25, 2024

Hi @nofalx
Thank you

Could you also make a test case for this ?

@nofalx
Copy link
Contributor Author

nofalx commented Apr 25, 2024

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 #/components/schemas/{model}

I see solving the issue in 2 approaches.

Approach A : Give the user the freedom

The 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.

class Exampke(Schema):
    i_default: int = Field(1)

    class Config(Schema.Config):
        json_schema_serialization_defaults_required = True

Approach B : Smart Detection

We 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:

  • Split the schema with prefix Input-{Schema} and Output-{Schema}
  • Raise an error of schema conflict by implementing some logic to detect if a schema was generated for request and response at same time

@nofalx
Copy link
Contributor Author

nofalx commented Apr 27, 2024

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/

pydantic/pydantic#7209

@musanek
Copy link

musanek commented May 2, 2024

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 fields_optional for UPDATE and PATCH (part of doc as well). There is no opposite way to make field as required (as if model has default value).

@nofalx
Copy link
Contributor Author

nofalx commented May 2, 2024

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 fields and not under fields_optional. However they still appear as optional in the schema.

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 ModelSchema. As any field with default value will suffer from the same issue above. As you can see this behavior is documented under pydantic docs under JsonSchemaMode

Currently the PR solves this issue and sets the correct JsonSchemaMode based on if the schema is a request or a response. However the remaining issue is that if we use the same schema for both request and response they will be generated under the same name and may overwrite each other. Therefore this can be fixed by specifying different names for Request and Response Schema.

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

@musanek
Copy link

musanek commented May 3, 2024

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.
I understand adjusting Django model for some other API purpose is not a thing and we talk here about automatically generated schema from those models. But all I need is a schema from my Django model that does not have any optional fields. If I need some fields optional, for say update or patch I can always create new schema based on the default one using fields_optional . (exactly as described here https://django-ninja.dev/guides/response/django-pydantic/ Make fields optional). Picking schema based on Request/Response seems maybe nice, but having auto generated schema that does not makes fields optional if the model has default value will give me full control.

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.

[BUG] ModelSchema does not mark field as required if it has a default value
3 participants