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

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

Open
KimSoungRyoul opened this issue Oct 14, 2021 · 6 comments
Open

Comments

@KimSoungRyoul
Copy link

KimSoungRyoul commented Oct 14, 2021

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?

@KimSoungRyoul KimSoungRyoul changed the title [Bug] pydantic json_encoder ignored (is it bug or feature?) [Bug] pydantic json_encoder is ignored (is it bug or feature?) Oct 14, 2021
@shughes-uk
Copy link

shughes-uk commented Jan 24, 2022

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
Copy link
Owner

vitalik commented Jan 24, 2022

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
Copy link

shughes-uk commented Jan 25, 2022

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
Copy link

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
Copy link

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
Copy link

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.

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

No branches or pull requests

4 participants