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

When having a discriminated union as request body schema the schema validation error should not show all the union components #903

Closed
Vulwsztyn opened this issue Nov 4, 2023 · 4 comments

Comments

@Vulwsztyn
Copy link

Vulwsztyn commented Nov 4, 2023

class A(ninja.Schema):
    type: typing.Literal['A']

class B(ninja.Schema):
    type: typing.Literal['B']

class C(ninja.Schema):
    type: typing.Literal['C']

class D(ninja.Schema):
    type: typing.Literal['D']
    something: conlist(str, min_items=2)
    
def handler(
    request: http.HttpRequest,
    body: A | B | C | D,
):
  pass

the reposnse for request with type D and only 1 item in something is:

{
  "detail": [
    {
      "loc": [
        "body",
        "body",
        "type"
      ],
      "msg": "unexpected value; permitted: 'A'",
      "type": "value_error.const",
      "ctx": {
        "given": "D",
        "permitted": [
          "A"
        ]
      }
    },
    {
      "loc": [
        "body",
        "body",
        "type"
      ],
      "msg": "unexpected value; permitted: 'B'",
      "type": "value_error.const",
      "ctx": {
        "given": "D",
        "permitted": [
          "B"
        ]
      }
    },
    {
      "loc": [
        "body",
        "body",
        "type"
      ],
      "msg": "unexpected value; permitted: 'C'",
      "type": "value_error.const",
      "ctx": {
        "given": "D",
        "permitted": [
          "C"
        ]
      }
    },
    {
      "loc": [
        "body",
        "body",
        "something"
      ],
      "msg": "ensure this value has at least 2 items",
      "type": "value_error.list.min_items",
      "ctx": {
        "limit_value": 2
      }
    }
  ]
}

I'd like it to be:

{
  "detail": [
    {
      "loc": [
        "body",
        "body",
        "something"
      ],
      "msg": "ensure this value has at least 2 items",
      "type": "value_error.list.min_items",
      "ctx": {
        "limit_value": 2
      }
    }
  ]
}
@vitalik
Copy link
Owner

vitalik commented Nov 5, 2023

@Vulwsztyn well at some point the full output actually makes sense - if you defining that a complex union and nothing matched - it's actually good that it reports all issues during the match process

Anyway - this is not something that is in control of django-ninja - you might look into pydantic to find the solution to this validation

Maybe what you need is actually one class like this (note this is example for ninja v1b2 and pydantic2) :

from pydantic import model_validator

class Data(Schema):
    type: typing.Literal["A", "B", "C", "D"]
    something: str = ""

    @model_validator(mode="after")
    def validate_something(self):
        if self.type == "D" and (not self.something or len(self.something) < 3):
            raise ValueError(
                "something is required for type D and must be at least 2 chars long"
            )
        return self

@api.post("/test")
def handler(request: HttpRequest, body: Data):
    return body.dict()

@Vulwsztyn
Copy link
Author

Thank you. You can close the issue if you want.

Regarding the example, I'd hate to lose the different schemas in redoc/swagger and it counters the whole point of having unions. When you want:

class A(Schema):
    a: int
class B(Schema):
    b: list[int]
    bb: str
class C(Schema):
    a: list[int]
    b: str

you do not actaully want:

class D(Schema):
    a: list[int] | int | None
    b: str | list[int] | None
    bb: str | None

I know you know it, I'm just explaining my reasoning if anyone encounters this issue in the future.

@vitalik
Copy link
Owner

vitalik commented Nov 8, 2023

@Vulwsztyn yep, make sense

but this is not something django-ninja has control of (it simply outptputs pydantic validation error) - you should ask this at pydantic - I am sure there must be solution that covers this case (and if you find any please share here as well)

@jceipek
Copy link
Contributor

jceipek commented Jan 3, 2025

@Vulwsztyn yep, make sense

but this is not something django-ninja has control of (it simply outptputs pydantic validation error) - you should ask this at pydantic - I am sure there must be solution that covers this case (and if you find any please share here as well)

I've been looking into request validation error formatting for a few weeks, and this is mostly achievable in Django Ninja at the moment because of the explicit type discriminator, but...

  1. It requires an awkward amount of boilerplate
  2. The loc is confusing and can't be improved without addressing some of the problems in [BUG] ninja.errors.ValidationError contains incorrect loc and discards important context #1381
from typing import List, Literal, Union

import django.http
import pydantic
import pytest
from typing_extensions import Annotated

import ninja
from ninja.params import Body
from ninja.testing import TestClient

api = ninja.NinjaAPI()

class A(ninja.Schema):
    type: Literal['A']

class B(ninja.Schema):
    type: Literal['B']

class C(ninja.Schema):
    type: Literal['C']

class D(ninja.Schema):
    type: Literal['D']
    something: Annotated[List[str], pydantic.Field(min_length=2)]

# This doesn't inherit from `ninja.Schema` because Django Ninja doesn't support `RootModel` directly.
# However, we can't use `Union[A,B,C,D] = pydantic.Field(discriminator='type')` in the handler directly either, because Django Ninja doesn't support `Field` in that context.
class MyBody(pydantic.RootModel[Union[A,B,C,D]]):
    root: Union[A,B,C,D] = pydantic.Field(discriminator='type')

@api.post("/demo")
def demo(
    request: django.http.HttpRequest,
    body: Body[MyBody]
) -> None:
  pass

client = TestClient(api)

def test_discriminator() -> None:
    resp = client.post("/demo", json={"type": "D", "something": ["hello"]})
    assert resp.json()["detail"] == [
           {
               'ctx': {
                   'actual_length': 1,
                   'field_type': 'List',
                   'min_length': 2,
               },
               'loc': [
                   'body',
                   'body', # The name of the function parameter, which users can't see. Makes the error look like the request had a "body" field. I'm convinced this is a bug, because Django Ninja sometimes tries to remove function parameter names, there is only one body parameter in the function, and users shouldn't be encouraged to use "body" as a field in their requests.
                   'D', # The union discriminator. Because of `pydantic.Field(discriminator='type')`, it's not 'function-wrap[_run_root_validator()]', which it would be without an explicit discriminator or a `pydantic.Tag` due to Problem 3 in https://github.com/vitalik/django-ninja/issues/1381
                   'something',
               ],
               'msg': 'List should have at least 2 items after validation, not 1',
               'type': 'too_short',
           },
       ]

Without an explicit discriminator, the best way I've found to summarize errors involving unions is to traverse model.__pydantic_core_schema__ with the pydantic loc from multiple errors. This is not currently possible in Django Ninja but would be with #1380

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

3 participants