litestar-org / litestar

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs
https://litestar.dev/
MIT License
5.66k stars 384 forks source link

enhancement: OpenAPI re-use existing schemas of same name #2471

Closed chris-telemetry closed 1 year ago

chris-telemetry commented 1 year ago

Description

We're getting an unexpected ImproperlyConfiguredException in litestar/_openapi/schema_generation/schema.py", line 594, in process_schema_result.

In our app, we defined a basePlayer model using Pydantic. This model is returned on its own and also as a subdocument in other endpoints.

Schema generation fails recognizes two different schemas with the same title. However, the schemas only differ in their description. The description itself is set as a field-level annotation when Player is used as a subdocument.

This makes setting the description at the field level much less useful.

I may be missing something. Looking at the code it hashes the schemas to ensure a 1-1 relationship between titles and schemas. I don't believe this was previously a constraint and schemas were produced based on the module path. I may not understand why this is now a requirement.

I've provided an example showing this behavior for both msgspec and Pydantic.

This behavior is new to Litestar 2.0 and did not exist in Litestar 1.51

URL to code causing the issue

No response

MCVE

### Example showing the issue with Pydantic

import traceback
from litestar import Litestar, get
from litestar.params import Parameter
from pydantic import BaseModel, Field

from litestar.types.asgi_types import Scope

class Player(BaseModel):
    id: str
    name: str
    position: str
    jersey: str

class Play(BaseModel):
    id: str
    ball_carrier: Player = Field(description="Sub document representing player who carried the ball")

@get("/player", sync_to_thread=True)
def get_player_by_id(id: str = Parameter(str)) -> Player:
    return Player(id=id, name="Chris", position="RB", jersey="00")

@get("/play", sync_to_thread=True, include_in_schema=True)
def get_play_by_id(id: str = Parameter(str)) -> Play:
    return Play(id=id, ball_carrier=Player(id="asdasd", name="chris", position="rb", jersey="00"))

async def after_exception_hook(exc: Exception, scope: "Scope") -> None:
    traceback.print_exc()

app = Litestar(route_handlers=[get_player_by_id, get_play_by_id], after_exception=[after_exception_hook])

if __name__ == "__main__":
    import uvicorn

    uvicorn.run(app)

Example using msgspec

import traceback
from litestar import Litestar, get
from litestar.params import Parameter

from litestar.types.asgi_types import Scope

from msgspec import Meta, Struct
from typing import Annotated

class Player(Struct):
    id: str
    name: str
    position: str
    jersey: str

class Play(Struct):
    id: str
    ball_carrier: Annotated[Player, Meta(description="Sub document representing player who carried the ball")]

@get("/player", sync_to_thread=True)
def get_player_by_id(id: str = Parameter(str)) -> Player:
    return Player(id=id, name="Chris", position="RB", jersey="00")

@get("/play", sync_to_thread=True, include_in_schema=True)
def get_play_by_id(id: str = Parameter(str)) -> Play:
    return Play(id=id, ball_carrier=Player(id="asdasd", name="chris", position="rb", jersey="00"))

async def after_exception_hook(exc: Exception, scope: "Scope") -> None:
    traceback.print_exc()

app = Litestar(route_handlers=[get_player_by_id, get_play_by_id], after_exception=[after_exception_hook])

if __name__ == "__main__":
    import uvicorn

    uvicorn.run(app)

### Steps to reproduce

```bash
1. Create a base app
2. Define a model and set it as the return type of an endpoint
3. Define a second model, containing a field annotated with the model defined in step 2. Set a `description` on the field (i.e. Pydantic `Field(description="{desc}"` or msgspec  Annotated[Player, Meta(description="{desc}")]). Create a second endpoint returning this model
4. See error

Screenshots

No response

Logs

litestar.exceptions.http_exceptions.ImproperlyConfiguredException: 500: Two different schemas with the title Player have been defined.

first: {"properties":{"id":{"type":"string"},"name":{"type":"string"},"position":{"type":"string"},"jersey":{"type":"string"}},"type":"object","required":["id","jersey","name","position"],"title":"Player"}
second: {"properties":{"id":{"type":"string"},"name":{"type":"string"},"position":{"type":"string"},"jersey":{"type":"string"}},"type":"object","required":["id","jersey","name","position"],"title":"Player","description":"Sub document representing player who carried the ball"}

Litestar Version

Python 3.11 litestar[full] @ 2.2.1 pydantic @ 2.4.2

Platform


[!NOTE]
While we are open for sponsoring on GitHub Sponsors and OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh Litestar dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.

Fund with Polar

chris-telemetry commented 1 year ago

After poking around for a bit I guess I'm somewhat confused as to why we care so much about the title being unique.

In litestar._openapi.schema_generation.schema.

Revising this:

 if schema.title and schema.type in (OpenAPIType.OBJECT, OpenAPIType.ARRAY):
            if schema.title in self.schemas and hash(self.schemas[schema.title]) != hash(schema):
                raise ImproperlyConfiguredException(
                    f"Two different schemas with the title {schema.title} have been defined.\n\n"
                    f"first: {encode_json(self.schemas[schema.title].to_schema()).decode()}\n"
                    f"second: {encode_json(schema.to_schema()).decode()}\n\n"
                    f"To fix this issue, either rename the base classes from which these titles are derived or manually"
                    f"set a 'title' kwarg in the route handler."
                )

            self.schemas[schema.title] = schema
            return Reference(ref=f"#/components/schemas/{schema.title}")
 return schema

To this:

if schema.title and schema.type in (OpenAPIType.OBJECT, OpenAPIType.ARRAY):
            class_name = str(get_text_in_single_quotes(str(field.annotation)).replace(".", ""))

            existing = self.schemas.get(class_name)
            if existing:
                return schema

            self.schemas[name_title] = schema
            return Reference(ref=f"#/components/schemas/{class_name}")
return schema

Seems to work. I'm sure I'm missing something here. Open to any input on it.

peterschutt commented 1 year ago

Thanks for the excellent report! Hopefully someone with more knowledge on this than me can chime in. Git blame is no help.

Do all tests pass with your change?

chris-telemetry commented 1 year ago

No, they would need to be updated to reflect the new naming scheme as they're currently hardcoded to the old one.

I'd make a PR for this but I'm afraid I'm not particularly familiar with the OpenAPI spec and I'm not quite sure if this was changed for compliance purposes?

chris-telemetry commented 1 year ago

@peterschutt My research leads me to believe my implementation would not be a violation of OpenAPI's spec. I've made an early PR pending feed back.

https://github.com/litestar-org/litestar/pull/2475

peterschutt commented 1 year ago

Thanks @chris-telemetry

LonelyVikingMichael commented 1 year ago

Is there a workaround for this issue in the meantime?

chris-telemetry commented 1 year ago

Is there a workaround for this issue in the meantime?

@LonelyVikingMichael The best workaround I've found is fudging the names of the schema using either the 'title' keyword on the Field or by creating a dummy class:

class Player(BaseModel):
    id: str
    name: str
    position: str
    jersey: str

class PlayerWithDescription(Player):
    pass

class Play(BaseModel):
    id: str
    ball_carrier: PlayerWithDescription = Field(description="Sub document representing player who carried the ball")