vitalik / django-ninja

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

[Bug] pydantic json_encoder is ignored (is it bug or feature?) #247

Open KimSoungRyoul opened 3 years ago

KimSoungRyoul commented 3 years ago

example

override json_encoders

class HelloResponseSchema(NinjaSchema):
    aa_datetime : datetime

    class Config(NinjaSchema.Config):
        json_encoders = {
            datetime: lambda v: datetime_to_utc_isoformat(v),
        }

...

@app.get("/aaa")
def api_aaa():
     return 200 , HelloResponseSchema(...)

api_aaa API Response's datetime field should not be isoformat because NinjaJsonEncoder does not use pydantic.json()


# ninja   responses.py

class NinjaJSONEncoder(DjangoJSONEncoder):
    def default(self, o: Any) -> Any:
        if isinstance(o, BaseModel):
            return o.dict()
        return super().default(o)

class Response(JsonResponse):
    def __init__(self, data: Any, **kwargs: Any) -> None:
        super().__init__(data, encoder=NinjaJSONEncoder, safe=False, **kwargs)

is it bug or feature?

shughes-uk commented 2 years ago

I'm trying to use SecretStr in a response schema and also got hit by this. Im curious why ninja is using .dict() instead of .json()

vitalik commented 2 years ago

Im curious why ninja is using .dict() instead of .json()

well.. Django-ninja can return other content types (yaml, xml, etc)

@shughes-uk @KimSoungRyoul you can extend default json renderer with extra encoders like this:

from ninja.responses import NinjaJSONEncoder
from ninja.renderers import JSONRenderer

class SecretsJSONEncoder(NinjaJSONEncoder):
    def default(self, o):
        if isinstance(o, (SecretStr, SecretBytes)):
            return o.get_secret_value()
        return super().default(o)

class MyJsonRenderer(JSONRenderer):
    encoder_class = SecretsJSONEncoder

api = NinjaAPI(renderer=MyJsonRenderer())
shughes-uk commented 2 years ago

Have not tested this, but this is closer to my desired behavior I think. Given that response schema's are always pydantic models, having ninja be able to support all the inbuilt pydantic fields by default make sense (not sure how to replicate that for xml or yaml)

from ninja.responses import NinjaJSONEncoder
from ninja.renderers import JSONRenderer
from pydantic.json import pydantic_encoder

class PydanticJSONEncoder(NinjaJSONEncoder):
    def default(self, o):
        return pydantic_encoder(o)

class MyJsonRenderer(JSONRenderer):
    encoder_class = PydanticJSONEncoder

api = NinjaAPI(renderer=MyJsonRenderer())
stinovlas commented 2 years ago

The solution that @shughes-uk suggested seems to work fine for pydantic builtins. However, you can also specify json_encoders in model Config and those are ignored in django-ninja right now. I could do something like this:

import json
from pydantic import BaseModel
from ninja.responses import NinjaJSONEncoder
from ninja.renderers import JSONRenderer

class PydanticJSONEncoder(NinjaJSONEncoder):
    def default(self, o):
        if isinstance(o, BaseModel):
            return json.loads(o.json())
        return super().default(o)

class MyJsonRenderer(JSONRenderer):
    encoder_class = PydanticJSONEncoder

api = NinjaAPI(renderer=MyJsonRenderer())

This would actually use all json-related features of pydantic, but it also includes repetitive json encoding, which I find problematic because for performance reasons. Maybe we could write entirely different JSONRenderer, that would use model.json() directly?

stinovlas commented 2 years ago

I'm thinking something along the lines of:

from pydantic import BaseModel
from ninja.renderers import JSONRenderer

class PydanticJsonRenderer(JSONRenderer):
    def render(self, request: HttpRequest, data: Any, *, response_status: int) -> Any:
        if isinstance(data, BaseModel):
            return data.json()
        return super().render(request, data, response_status=response_status)
stinovlas commented 2 years ago

I tried to do something similar to what I suggested above, but I ran into another problem – ninja.operations actually calls .dict() on my returned data, so custom decoding of pydantic models doesn't actually help. When the data get to the renderer, there are no more pydantic models. Operations kills it all.

See https://github.com/vitalik/django-ninja/blob/master/ninja/operation.py#L196

This is unfortunate, because it makes working with custom json encoders on pydantic models really hard. I can always return Response directly, but that seems to go against the primary purpose. And also, I'd need to use my own sublass of Response that doesn't use NinjaJSONEncoder.

I'm interested in why is django-ninja designed this way. It seems logical to leave decoding of the data and rendering to the renderers, yet they're changed directly in operation.py.