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] ninja.errors.ValidationError contains incorrect loc and discards important context #1381

Open
jceipek opened this issue Jan 3, 2025 · 0 comments

Comments

@jceipek
Copy link
Contributor

jceipek commented Jan 3, 2025

Describe the bug
A good error message should provide enough information for users to resolve the problem, in terms that the users understand.
In Django Ninja, the intended way to format request validation errors for user presentation is to register an exception handler for ninja.errors.ValidationError.
Unfortunately, when Django Ninja converts pydantic.ValidationErrors into ninja.errors.ValidationErrors, it...

  1. Sometimes emits incorrect loc values
  2. Always discards Pydantic and Django Ninja context that's needed to present error locations in a way that's meaningful to users

This means APIs developed with Django Ninja cannot return user-friendly request validation error messages that would be possible using Pydantic directly. Based on the discussion in #903, I think ninja.errors.ValidationErrors should support any error formatting you could do with Pydantic.

I created #1380, which gives users access to the necessary Pydantic context without breaking backwards compatibility.

I'm filing this issue to describe why it's needed (problem 1 below) and to explain the other problems (2-4 below) it doesn't solve. @vitalik, I'd love your help figuring out how to solve these other problems.

Versions:

  • Python version: 3.11
  • Django version: 5.1.4
  • Django-Ninja version: 1.3.0
  • Pydantic version: 2.10.3

Understanding the problems

When a request fails validation, Django Ninja catches the resulting pydantic.ValidationErrors and transforms them into a ninja.errors.ValidationError in ninja.operation.Operation's _get_values (see here). This approach has several problems.

Problem 1: discarding model.__pydantic_core_schema__

The loc part of a Pydantic 2 pydantic.ValidationError describes a path through the model.__pydantic_core_schema__ of the model that failed validation. Django Ninja does not provide access to model.__pydantic_core_schema__, so developers can't write code that understands what loc means. Developers might want to do so for multiple reasons, including:

This problem would be addressed via #1380

Problem 2: transforming loc

In ninja.operation.Operation's _get_values, loc is overwritten for ninja.errors.ValidationErrors via:

i["loc"] = (
model.__ninja_param_source__,
) + model.__ninja_flatten_map_reverse__.get(i["loc"], i["loc"])

Overwriting the loc means that developers can't understand what the elements of loc mean in the context of model.__pydantic_core_schema__.

There's 2 parts to this:

  1. Prepend model.__ninja_param_source__, which is a string indicating a parameter type ("path", "query", "body", "header", "cookie", "body", "form", "file")
  2. model.__ninja_flatten_map_reverse__.get(i["loc"], i["loc"]). This can easily do the wrong thing and is destructive. I think it's intended to remove framework internals from the loc, but it doesn't and can't work, because model.__ninja_flatten_map_reverse__ incorrectly assumes that every element of loc is a field name. Expand the Problem Demonstration below to see why this is an issue.
Problem Demonstration

The code below demonstrates the inconsistency resulting from use of model.__ninja_flatten_map_reverse__.get(i["loc"], i["loc"]). Note that the function parameter name sometimes makes it into the output and sometimes doesn't.
The UntransformedErrorAPI part of the code relies on #1380 to illustrate what the underlying pydantic errors look like, for comparison.

from typing import Any, Dict, List, Union

import django.http
import pydantic
import pytest

import ninja
import ninja.errors
from ninja.testing import TestClient

api = ninja.NinjaAPI()

class SimpleIdentityParam(ninja.Schema):
  identity: pydantic.Base64Str


class UnionIdentityParam(ninja.Schema):
  identity: Union[int, pydantic.Base64Str]


@api.post("/problem2/flat/simple/{identity}")
def problem2_flat_simple(request: django.http.HttpRequest, identity: pydantic.Base64Str) -> str:
  return "ok"

@api.post("/problem2/flat/union/{identity}")
def problem2_flat_union(request: django.http.HttpRequest, identity: Union[int, pydantic.Base64Str]) -> str:
  return "ok"

@api.post("/problem2/model/simple/{identity}")
def problem2_model_simple(request: django.http.HttpRequest, identity: SimpleIdentityParam) -> str:
  return "ok"

@api.post("/problem2/model/union/{identity}")
def problem2_model_union(request: django.http.HttpRequest, identity: UnionIdentityParam) -> str:
  return "ok"

@api.post("/problem2/path/simple/{identity}")
def problem2_path_simple(request: django.http.HttpRequest, data: ninja.Path[SimpleIdentityParam]) -> str:
  return "ok"

@api.post("/problem2/path/union/{identity}")
def problem2_path_union(request: django.http.HttpRequest, data: ninja.Path[UnionIdentityParam]) -> str:
  return "ok"

client = TestClient(api)

def test_problem2() -> None:
  resp_flat_simple_invalid = client.post("/problem2/flat/simple/invalid")
  resp_flat_union_invalid = client.post("/problem2/flat/union/invalid")
  resp_model_simple_invalid = client.post("/problem2/model/simple/invalid")
  resp_model_union_invalid = client.post("/problem2/model/union/invalid")
  resp_path_simple_invalid = client.post("/problem2/path/simple/invalid")
  resp_path_union_invalid = client.post("/problem2/path/union/invalid")
  assert resp_flat_simple_invalid.status_code == 422
  assert resp_flat_union_invalid.status_code == 422
  assert resp_path_simple_invalid.status_code == 422
  assert resp_path_union_invalid.status_code == 422

  assert resp_flat_simple_invalid.json()["detail"] == [
      {
          "ctx": {
              "error": "Incorrect padding",
          },
          "loc": [
              "path",
              "identity",
          ],
          "msg": "Base64 decoding error: 'Incorrect padding'",
          "type": "base64_decode",
      },
  ]
  assert resp_flat_union_invalid.json()["detail"] == [
      {
          "loc": [
              "path",
              "identity",
              "int",
          ],
          "msg": "Input should be a valid integer, unable to parse string as an integer",
          "type": "int_parsing",
      },
      {
          "ctx": {
              "error": "Incorrect padding",
          },
          "loc": [
              "path",
              "identity",
              "function-after[decode_str(), str]",
          ],
          "msg": "Base64 decoding error: 'Incorrect padding'",
          "type": "base64_decode",
      },
  ]
  assert resp_model_simple_invalid.json()["detail"] == [
      {
          "ctx": {
              "error": "Incorrect padding",
          },
          "loc": [
              "path",
              "identity", # Why is there one identity here?
          ],
          "msg": "Base64 decoding error: 'Incorrect padding'",
          "type": "base64_decode",
      },
  ]
  assert resp_model_union_invalid.json()["detail"] == [
      {
          "loc": [
              "path",
              "identity",
              "identity", # Why are there two `identity`s here?
              "int",
          ],
          "msg": "Input should be a valid integer, unable to parse string as an integer",
          "type": "int_parsing",
      },
      {
          "ctx": {
              "error": "Incorrect padding",
          },
          "loc": [
              "path",
              "identity",
              "identity", # Why are there two `identity`s here?
              "function-after[decode_str(), str]",
          ],
          "msg": "Base64 decoding error: 'Incorrect padding'",
          "type": "base64_decode",
      },
  ]
  assert resp_path_simple_invalid.json()["detail"] == [
      {
          "ctx": {
              "error": "Incorrect padding",
          },
          "loc": [
              "path",
              "identity",
          ],
          "msg": "Base64 decoding error: 'Incorrect padding'",
          "type": "base64_decode",
      },
  ]
  assert resp_path_union_invalid.json()["detail"] == [
      {
          "loc": [
              "path",
              "data", # This function param name shouldn't be here
              "identity",
              "int",
          ],
          "msg": "Input should be a valid integer, unable to parse string as an integer",
          "type": "int_parsing",
      },
      {
          "ctx": {
              "error": "Incorrect padding",
          },
          "loc": [
              "path",
              "data", # This function param name shouldn't be here
              "identity",
              "function-after[decode_str(), str]",
          ],
          "msg": "Base64 decoding error: 'Incorrect padding'",
          "type": "base64_decode",
      },
  ]

# NOTE: The following will only work with https://github.com/vitalik/django-ninja/pull/1380
class UntransformedErrorAPI(ninja.NinjaAPI):
  def validation_error_from_error_contexts(
      self,
      error_contexts: List[ninja.errors.ValidationErrorContext],
  ) -> ninja.errors.ValidationError:
      errors: List[Dict[str, Any]] = []
      for context in error_contexts:
          for e in context.pydantic_validation_error.errors(include_url=False):
              errors.append(dict(e))
      return ninja.errors.ValidationError(errors)

untransformed_api = UntransformedErrorAPI()

@untransformed_api.post("/problem2/flat/simple/{identity}")
def untransformed_problem2_flat_simple(request: django.http.HttpRequest, identity: pydantic.Base64Str) -> str:
  return "ok"

@untransformed_api.post("/problem2/flat/union/{identity}")
def untransformed_problem2_flat_union(request: django.http.HttpRequest, identity: Union[int, pydantic.Base64Str]) -> str:
  return "ok"

@untransformed_api.post("/problem2/model/simple/{identity}")
def untransformed_problem2_model_simple(request: django.http.HttpRequest, identity: SimpleIdentityParam) -> str:
  return "ok"

@untransformed_api.post("/problem2/model/union/{identity}")
def untransformed_problem2_model_union(request: django.http.HttpRequest, identity: UnionIdentityParam) -> str:
  return "ok"

@untransformed_api.post("/problem2/path/simple/{identity}")
def untransformed_problem2_path_simple(request: django.http.HttpRequest, data: ninja.Path[SimpleIdentityParam]) -> str:
  return "ok"

@untransformed_api.post("/problem2/path/union/{identity}")
def untransformed_problem2_path_union(request: django.http.HttpRequest, data: ninja.Path[UnionIdentityParam]) -> str:
  return "ok"

untransformed_client = TestClient(untransformed_api)

def test_untransformed_problem2() -> None:
  resp_flat_simple_invalid = untransformed_client.post("/problem2/flat/simple/invalid")
  resp_flat_union_invalid = untransformed_client.post("/problem2/flat/union/invalid")
  resp_model_simple_invalid = untransformed_client.post("/problem2/model/simple/invalid")
  resp_model_union_invalid = untransformed_client.post("/problem2/model/union/invalid")
  resp_path_simple_invalid = untransformed_client.post("/problem2/path/simple/invalid")
  resp_path_union_invalid = untransformed_client.post("/problem2/path/union/invalid")
  assert resp_flat_simple_invalid.status_code == 422
  assert resp_flat_union_invalid.status_code == 422
  assert resp_path_simple_invalid.status_code == 422
  assert resp_path_union_invalid.status_code == 422

  assert resp_flat_simple_invalid.json()["detail"] == [
      {
          'ctx': {
              'error': 'Incorrect padding',
          },
         'input': 'invalid',
          'loc': [
              'identity',
          ],
          'msg': "Base64 decoding error: 'Incorrect padding'",
          'type': 'base64_decode',
      },
  ]
  assert resp_flat_union_invalid.json()["detail"] == [
      {
         'input': 'invalid',
          'loc': [
              'identity',
              'int',
          ],
          'msg': 'Input should be a valid integer, unable to parse string as an integer',
          'type': 'int_parsing',
      },
      {
          'ctx': {
              'error': 'Incorrect padding',
          },
         'input': 'invalid',
          'loc': [
              'identity',
              'function-after[decode_str(), str]',
          ],
          'msg': "Base64 decoding error: 'Incorrect padding'",
          'type': 'base64_decode',
      },
  ]
  assert resp_model_simple_invalid.json()["detail"] == [
      {
          'ctx': {
              'error': 'Incorrect padding',
          },
         'input': 'invalid',
          'loc': [
              'identity', # This is removed by Django Ninja
              'identity',
          ],
          'msg': "Base64 decoding error: 'Incorrect padding'",
          'type': 'base64_decode',
      },
  ]
  assert resp_model_union_invalid.json()["detail"] == [
      {
         'input': 'invalid',
          'loc': [
              'identity',
              'identity',
              'int',
          ],
          'msg': 'Input should be a valid integer, unable to parse string as an integer',
          'type': 'int_parsing',
      },
      {
          'ctx': {
              'error': 'Incorrect padding',
          },
         'input': 'invalid',
          'loc': [
              'identity',
              'identity',
              'function-after[decode_str(), str]',
          ],
          'msg': "Base64 decoding error: 'Incorrect padding'",
          'type': 'base64_decode',
      },
  ]
  assert resp_path_simple_invalid.json()["detail"] == [
      {
          "ctx": {
              "error": "Incorrect padding",
          },
          'input': 'invalid',
          "loc": [
              "data", # This is removed by Django Ninja
              "identity",
          ],
          "msg": "Base64 decoding error: 'Incorrect padding'",
          "type": "base64_decode",
      },
  ]
  assert resp_path_union_invalid.json()["detail"] == [
      {
          'input': 'invalid',
          "loc": [
              "data",
              "identity",
              "int",
          ],
          "msg": "Input should be a valid integer, unable to parse string as an integer",
          "type": "int_parsing",
      },
      {
          "ctx": {
              "error": "Incorrect padding",
          },
          'input': 'invalid',
          "loc": [
              "data",
              "identity",
              "function-after[decode_str(), str]",
          ],
          "msg": "Base64 decoding error: 'Incorrect padding'",
          "type": "base64_decode",
      },
  ]

A proper solution to Problem 2 needs to give developers the tools to understand what elements of a loc correspond to framework internals like function parameter names (which usually shouldn't be exposed to end-users) without impeding developers' ability to traverse model.__pydantic_core_schema__.
There is one edge case where exposing function parameter names to end users is desirable: in situations where a function uses multiple parameters for the request body, the caller needs to supply the function parameter names as JSON keys and so must be aware of them.

Problem 2 can be partially worked around via #1380 because it allows developers to change if/how loc is transformed when creating ninja.errors.ValidationErrors. However, it doesn't allow developers to know which parts of the loc are internal to Django Ninja.

Problem 3: creating ambiguous union discriminators

Django Ninja requires developers to create schemas that derive from ninja.Schema. This has a @model_validator(mode="wrap") @classmethod called _run_root_validator. As a result, Django Ninja schemas in unions show up as "function-wrap[_run_root_validator()]" in error locs. A union of two Django Ninja schemas will show "function-wrap[_run_root_validator()]" as the discriminator for both variants, which makes it impossible to walk model.__ninja_param_source__ using loc in such cases.

There's more context, examples, and an awkward workaround in this pydantic feature request: pydantic/pydantic#10850

Problem 4: removing input

The "input" field of Pydantic errors shows the specific part of the input that failed validation, which is very useful when rendering error messages that show users what went wrong.
Django Ninja discards the input field:

# removing pydantic hints
del i["input"] # type: ignore

As a result, developers can't use it to know and display the input associated with an error. Its contents sometimes use an opaque Django Ninja class, DjangoGetter, so exposing it isn't sufficient to address the problem.

Related issues

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

1 participant