Use correct schema mode for openAPI respones, fixes #1137

[nofalx]

fixes #1137

[vitalik]

Hi @nofalx Thank you

Could you also make a test case for this ?

[nofalx]

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]

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/

https://github.com/pydantic/pydantic/issues/7209

[musanek]

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]

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]

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

[Mouhand-Kaddo]

@vitalik Can we please have any updates on this? it will be great help to us

[jceipek]

I would also like this to match what FastAPI does, but I'm not sure how to best achieve that. FastAPI doesn't implement schema splitting itself but defers to pydantic to generate appropriate schema names.

My understanding is that FastAPI's code is here: https://github.com/tiangolo/fastapi/blob/653315c496304be928b46c34d35097d0ce847646/fastapi/openapi/utils.py#L475 It seems to aggregate all models before passing them collectively to pydantic to select appropriate names, which results in the simplest of many possible names being chosen (-Input and -Output) suffixes are added only in the case of duplicates. See https://github.com/pydantic/pydantic/blob/478ed07d8e92cf2a055c94122ca5a5faefd3c2ac/pydantic/json_schema.py#L1946-L1971 and https://github.com/pydantic/pydantic/blob/478ed07d8e92cf2a055c94122ca5a5faefd3c2ac/pydantic/json_schema.py#L2209-L2211

Is there a way to achieve a similar result with the current architecture of the OpenAPI generator (OpenAPISchema) in django-ninja? The existing generator generates schemas for each model separately via model_json_schema, which makes it difficult to reason about things like schema duplication. In fact, https://github.com/vitalik/django-ninja/blob/master/ninja/openapi/schema.py#L314-L320 seems to have open TODOs for handling schema duplication.

@nofalx How were you imagining implementing the name suffixes? You said:

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.

Were you planning to do this by changing the interface of OpenAPISchema, or do you have an idea of how to preserve it?

[nofalx]

@jceipek I mean in the end its all controlled by

REF_TEMPLATE: str = "#/components/schemas/{model}"

Which can be adjust to have suffix of input or output based on already in place logic that create schemas for req or resp . You can see that _create_schema_from_model is being called by either responses(..) or responses(..)

[jceipek]

@jceipek I mean in the end its all controlled by

REF_TEMPLATE: str = "#/components/schemas/{model}"

Which can be adjust to have suffix of input or output based on already in place logic that create schemas for req or resp . You can see that _create_schema_from_model is being called by either responses(..) or responses(..)

@nofalx This was one of the first things I tried. REF_TEMPLATE: str = "#/components/schemas/{model}_TEST" causes Redoc to complain that there is an "Invalid reference token", even without splitting the schema for inputs and outputs.

[vitalik]

still thinking about this - will move it to next release (as current fixes urgent python compatibility)