vitalik / django-ninja

đź’¨ Fast, Async-ready, Openapi, type hints based framework for building APIs
https://django-ninja.dev
MIT License
7.01k stars 421 forks source link

Question: How to handle custom input validation errors in @field_validator #825

Closed SRautila closed 1 year ago

SRautila commented 1 year ago

Hey. I'm a long-time DRF user trying out django-ninja 1.0a2 with pydantic 2.1 and I'm curious about some best practises.

What I'm doing now is writing registration. Code example:

import uuid

from django.contrib.auth.password_validation import validate_password
from django.core.validators import validate_email
from ninja import Router, Schema
from pydantic import constr, field_validator
from django.core.exceptions import ValidationError as DjangoValidationError

from users import models

router = Router()

class UserMeInSchema(Schema):
    email: constr(to_lower=True) = None
    first_name: str
    last_name: str
    password: str

    @field_validator("email")
    @classmethod
    def email_validator(cls, v: str):
        try:
            validate_email(v)
        except DjangoValidationError as e:
            raise ValueError(e.messages)

        if models.User.objects.filter(email=v).exists():
            raise ValueError(["A user with this email already exists"])
        return v

    @field_validator("password")
    @classmethod
    def password_validator(cls, v: str):
        try:
            validate_password(v)
        except DjangoValidationError as e:
            raise ValueError(e.messages)
        return v

class UserMeOutSchema(Schema):
    id: uuid.UUID
    email: str
    first_name: str
    last_name: str

@router.post("/register", response=UserMeOutSchema)
def register(request, data: UserMeInSchema):
    user = models.User.objects.create_user(
        first_name=data.first_name,
        last_name=data.last_name,
        email=data.email,
        password=data.password,
    )
    return user

with this I get exception TypeError: Object of type ValueError is not JSON serializable

traceback:

Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/ninja/operation.py", line 106, in run
    values = self._get_values(request, kw, temporal_response)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ninja/operation.py", line 239, in _get_values
    raise ValidationError(errors)
ninja.errors.ValidationError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/sentry_sdk/integrations/django/views.py", line 84, in sentry_wrapped_callback
    return callback(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ninja/operation.py", line 390, in _sync_view
    return operation.run(request, *a, **kw)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ninja/operation.py", line 114, in run
    return self.api.on_exception(request, e)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ninja/main.py", line 471, in on_exception
    return handler(request, exc)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ninja/errors.py", line 91, in _default_validation_error
    return api.create_response(request, {"detail": exc.errors}, status=422)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ninja/main.py", line 416, in create_response
    content = self.renderer.render(request, data, response_status=status)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ninja/renderers.py", line 25, in render
    return json.dumps(data, cls=self.encoder_class, **self.json_dumps_params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/json/__init__.py", line 238, in dumps
    **kw).encode(obj)
          ^^^^^^^^^^^
  File "/usr/local/lib/python3.11/json/encoder.py", line 200, in encode
    chunks = self.iterencode(o, _one_shot=True)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/json/encoder.py", line 258, in iterencode
    return _iterencode(o, 0)
           ^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ninja/responses.py", line 25, in default
    return super().default(o)
           ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/django/core/serializers/json.py", line 106, in default
    return super().default(o)
           ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/json/encoder.py", line 180, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '

I noticed I can raise from ninja.errors import ValidationError instead which works but it feels wrong since pydantic docs says "validators should either return the parsed value or raise a ValueError or AssertionError (assert statements may be used)." . Also this leads to errors having a different structure than the other pydantic errors. https://docs.pydantic.dev/2.1/usage/validators/#field-validators

How do I do this in the right way? Thanks!

zachmullen commented 1 year ago

I'm running into this same issue on 1.0a2, and dug into it a bit. The issue is that the value of exc.errors being passed into api.create_response looks like this:

[{'type': 'value_error', 'loc': ('body', 'payload', 'query'), 'msg': "Value error, Couldn't parse search query.", 'ctx': {'error': ValueError("Couldn't parse search query.")}}]

It seems if the ctx value is supposed to be emitted, it would need to be specially handled in the JSON encoder.

SRautila commented 1 year ago

Yeah I dug into it a bit as well. Either the JSON encoder or in Operation _get_values() method. I feel like encoding ValueError in general might be a bit too broad considering there are possibly other reasons for ValueError getting raised so I think handling ctx specifically in _get_values() is a better way forward.

I guess doing a del as with the input and url parts would solve the issue. Personally I'm not a big fan of forwarding the pydantic msgstarting with "Value error, lore ipsum" to the client though. Every error message of this type starting with Value error, feels quite ugly :/

Also, wouldn't it be a bit more safe to be explicit and pick out relevant keys instead of doing del?

Source: https://github.com/vitalik/django-ninja/blob/master/ninja/operation.py#L226-L237

SRautila commented 1 year ago

This is how I decided to implement this for now:

import uuid

from django.contrib.auth.password_validation import validate_password
from django.core.validators import validate_email
from ninja import Router, Schema
from pydantic import constr, field_validator
from django.core.exceptions import ValidationError as DjangoValidationError
from pydantic_core import PydanticCustomError

from users import models

from django.utils.translation import gettext as _

router = Router()

class UserMeInSchema(Schema):
    email: constr(to_lower=True) = None
    first_name: str
    last_name: str
    password: str

    @field_validator("email")
    @classmethod
    def email_validator(cls, v: str):
        try:
            validate_email(v)
        except DjangoValidationError as e:
            raise PydanticCustomError(
                e.code,
                e.message,
            )

        if models.User.objects.filter(email=v).exists():
            raise PydanticCustomError(
                "unique",
                _("A user with this email already exists"),
            )
        return v

    @field_validator("password")
    @classmethod
    def password_validator(cls, v: str):
        try:
            validate_password(v)
        except DjangoValidationError as e:
            raise PydanticCustomError(
                "too_easy_password",
                _("This password is too easy to guess. Please create a new one."),
                {"errors": e.messages},
            )
        return v

class UserMeOutSchema(Schema):
    id: uuid.UUID
    email: str
    first_name: str
    last_name: str

@router.post("/register", response=UserMeOutSchema)
def register(request, data: UserMeInSchema):
    user = models.User.objects.create_user(
        first_name=data.first_name,
        last_name=data.last_name,
        email=data.email,
        password=data.password,
    )
    return user

Sample output:

{
  "detail": [
    {
      "type": "unique",
      "loc": [
        "body",
        "data",
        "email"
      ],
      "msg": "A user with this email already exists"
    },
    {
      "type": "string_type",
      "loc": [
        "body",
        "data",
        "last_name"
      ],
      "msg": "Input should be a valid string"
    },
    {
      "type": "too_easy_password",
      "loc": [
        "body",
        "data",
        "password"
      ],
      "msg": "This password is too easy to guess. Please create a new one.",
      "ctx": {
        "errors": [
          "This password is too short. It must contain at least 8 characters.",
          "This password is too common.",
          "This password is entirely numeric."
        ]
      }
    }
  ]
}

For clarity: validate_email() runs one Django validator and validate_password() runs a list of validators.

I think a single error in msg for frontend to display if needed (thinking specifically about _("A user with this email already exists"),) and ctx containing eventual extra info for debugging is good enough. Above this frontend will of course run their own validation.

The discussion about handling ValueError is still relevant though I guess.

vitalik commented 1 year ago

@SRautila @zachmullen

could you share all your versions (django, python, pydantic, ninja) - cannot reproduce the error on my enviroment

generally speaking you throw either assert or ValueError in validators (or PydanticCustomError with moredetailed context)

keep in mind that you should pass string and not list to value error :

class UserMeInSchema(Schema):
    email: constr(to_lower=True) = None
    first_name: str
    last_name: str
    password: str

    @field_validator("email")
    @classmethod
    def email_validator(cls, v: str):
        try:
            validate_email(v)
        except DjangoValidationError as e:
            raise ValueError(', '.join(e.messages)). # <--------------- join 

        if xxxx:
            raise ValueError("A user with this email already exists")
        return v

    @field_validator("password")
    @classmethod
    def password_validator(cls, v: str):
        try:
            validate_password(v)
        except DjangoValidationError as e:
            raise ValueError(', '.join(e.messages)) # <--------------- join 
        return v
zachmullen commented 1 year ago
django-ninja==1.0a2
Django==4.1.10
pydantic==2.1.1
pydantic_core==2.4.0

Python 3.10.4

In my case, I saw the same error when passing just a string e.g. raise ValueError('My error message') inside of a @field_validator on an input payload schema.

Thanks, I will try @SRautila 's suggestion for now.

SRautila commented 1 year ago

Python 3.11.4

Django==4.2.4
django-ninja==1.0a2
pydantic==2.1.1
pydantic_core==2.4.0
zachmullen commented 1 year ago

@vitalik I'm not sure when this changed, but this behavior of pydantic is documented here: https://docs.pydantic.dev/latest/errors/errors/#custom-errors

Note the 'ctx' value in e.errors

vitalik commented 1 year ago

@zachmullen @SRautila

this ctx behaviour introduced in pydantic==2.1+

fixed try with django-ninja==1.0a3

SRautila commented 1 year ago

That was fast! Thanks!

Regarding best practise, I'm till considering sticking to PydanticCustomError since the following code with Django's translations enabled would lead to some not so nice output. But this is a pydantic issue and not a django-ninja issue imo.

from django.utils.translation import gettext as _

...

    @field_validator("email")
    @classmethod
    def email_validator(cls, v: str):
        if models.User.objects.filter(email=v).exists():
            raise ValueError(_("A user with this email already exists"))
        return v

Swedish output:

     {
      "type": "value_error",
      "loc": [
        "body",
        "data",
        "email"
      ],
      "msg": "Value error, En användare med den här eposten finns redan",
      "ctx": {
        "error": "En användare med den här eposten finns redan"
      }
    },

So now frontend either needs to use ctx["error"] if exists or display not so nice error message.

But issue closed. Thanks!