vitalik / django-ninja

💨 Fast, Async-ready, Openapi, type hints based framework for building APIs
https://django-ninja.dev
MIT License
7.25k stars 431 forks source link

[BUG] Path and Query parameter Annotated description + title are removed from OpenAPI docs #1220

Open jceipek opened 4 months ago

jceipek commented 4 months ago

Describe the bug Given:

@api.get("/items/{item_id}")
def read_item(request, item_id: Annotated[str, Field(examples=["an example"], title="Some Title", description="A description")]):
    return {"item_id": item_id}

I expect "Some Title" and "A description" show up in the generated OpenAPI docs. However, only "an example" shows up in the generated OpenAPI docs.

Versions (please complete the following information):

Root Cause

ninja's class Param(FieldInfo) in ninja.params.models calls:

        super().__init__(
            default=default,
            alias=alias,
            title=title,
            description=description,
            gt=gt,
            ge=ge,
            lt=lt,
            le=le,
            min_length=min_length,
            max_length=max_length,
            json_schema_extra=json_schema_extra,
            **extra,
        )

This ultimately causes a Path instance to be created with these _attributes_set values:

{'default': Ellipsis, 'alias': None, 'title': None, 'description': None, 'gt': None, 'ge': None, 'lt': None, 'le': None, 'min_length': None, 'max_length': None, 'json_schema_extra': {}}

Note that the values of title and description are None.

Direct Cause

When ninja constructs an Operation, it creates a ViewSignature, which calls _create_models. The end of the _create_models function calls model_cls = type(cls_name, (base_cls,), attrs). This causes Pydantic's ModelMetaclass __new__ method to be called, which results in calls to set_model_fields, collect_model_fields, and from_annotated_attribute. from_annotated_attribute calls merge_field_infos on a tuple of three elements (I'm not sure why the first two elements are identical):

FieldInfo(annotation=NoneType, required=True, title='Some Title', description='A description', examples=['an example'])
FieldInfo(annotation=NoneType, required=True, title='Some Title', description='A description', examples=['an example'])
Path(annotation=str, required=True, json_schema_extra={}, metadata=[FieldInfo(annotation=NoneType, required=True, title='Some Title', description='A description', examples=['an example'])])

The Path element in the tuple has an _attributes_set attribute containing this dictionary:

{'default': Ellipsis, 'alias': None, 'title': None, 'description': None, 'gt': None, 'ge': None, 'lt': None, 'le': None, 'min_length': None, 'max_length': None, 'json_schema_extra': {}}

Note that title and description are set to None, so merge_field_infos overwrites the desired description and title with None.

Potential Solution

This bug disappears if FieldInfo.__init__ in class Param(FieldInfo) does not receive None values in the default case. For example, adding this to the bottom of __init__ in class Param(FieldInfo) addresses the immediate problem:

    initializer: dict[str, Any] = {}
    if alias is not None:
        initializer["alias"] = alias
    if title is not None:
        initializer["title"] = title
    if description is not None:
        initializer["description"] = description
    if gt is not None:
        initializer["gt"] = gt
    if ge is not None:
        initializer["ge"] = ge
    if lt is not None:
        initializer["lt"] = lt
    if le is not None:
        initializer["le"] = le
    if min_length is not None:
        initializer["min_length"] = min_length
    if max_length is not None:
        initializer["max_length"] = max_length

    FieldInfo.__init__(
        self,
        default=default,
        json_schema_extra=json_schema_extra,
        **initializer,
        **extra,
    )

I'd be happy to make a pull request making this change, but first I'd like to know if this is an appropriate solution. Is there an easier way to do this?

dmartin commented 4 months ago

I think the intended way to do this is to use param: type = Path(...) directly in the function signature as shown here.

vitalik commented 4 months ago

Hi @jceipek

Basically you need to use Path marker (and PathEx to pass EXtras + P param):

from ninja import NinjaAPI, PathEx, P
api = NinjaAPI()

@api.get("/items/{item_id}")
def read_item(request, item_id: PathEx[str, P(example="an example", title="Some Title", description="A description")]):
    return {"item_id": item_id}

Note: swagger supports only 1 example (not multiple examples) for path params

jceipek commented 4 months ago

I think the intended way to do this is to use param: type = Path(...) directly in the function signature as shown here.

@dmartin That doesn't quite work; Path from from ninja import Path expects a generic parameter and mypy warns that one is missing. It does work with param_functions.Path from from ninja.params import functions as param_functions.

Basically you need to use Path marker (and PathEx to pass EXtras + P param):

from ninja import NinjaAPI, PathEx, P
api = NinjaAPI()

@api.get("/items/{item_id}")
def read_item(request, item_id: PathEx[str, P(example="an example", title="Some Title", description="A description")]):
    return {"item_id": item_id}

@vitalik That works as a workaround; thank you! Is the problem I reported with using Annotated expected behavior, then? It's especially confusing when usingAnnotated indirectly; for example:

ItemType = Annotated[str, Field(examples=["an example"], title="Some Title", description="A description")]

...

@api.get("/items/{item_id}")
def read_item(request, item_id: ItemType):
    return {"item_id": item_id}

In that case, it seems odd to need to repeat the information like this:

ItemType = Annotated[str, Field(examples=["an example"], title="Some Title", description="A description")]

...

@api.get("/items/{item_id}")
def read_item(request, item_id: PathEx[ItemType, P(example="an example", title="Some Title", description="A description")]):
    return {"item_id": item_id}

Note: swagger supports only 1 example (not multiple examples) for path params

Good to know; thanks!

dmartin commented 4 months ago

@dmartin That doesn't quite work; Path from from ninja import Path expects a generic parameter. It does work with param_functions.Path from from ninja.params import functions as param_functions.

Ah, sorry for the misinformation there. I have some functions like

def submissions(
    request: Request,
    id: int = Path(
        ...,
        description="Must be a member of a classroom in which the user is a leader.",
    )
):

which seem to be working (the description appears in the OpenAPI spec), but maybe that is simply coincidental.

Is there any documentation about when and how to use different parameter constructs like: params: MyParams = Query(...) vs params: Query[MyParams] vs QueryEx?

vitalik commented 4 months ago

Is there any documentation about when and how to use different parameter constructs like: params: MyParams = Query(...) vs params: Query[MyParams] vs QueryEx?

Query - QueryEx - is basically the same thing in runtime

it just because it's a special Annotated alias - mypy does not like it with parameters