ValidationError includes other keys apart from the actual key

[blazing-gig]

Hey @vitalik ,

Something that I noticed with the ValidationError being thrown is that there are additional fields present as opposed to the fields that are expected if the same ValidationError gets thrown from pydantic. The major problem with this is that when the client has to map this data to display the error to the user, it has to deliberately strip away a few fields all the time which doesn't make sense.

For eg: The ValidationError that django-ninja gives for an invalid input:

{

    "detail": [
      {
        "loc": [
          "body", <---- whats this ? 
          "org", <------ and this  (the input param name I believe)?
          "name"
        ],
        "msg": "none is not an allowed value",
        "type": "type_error.none.not_allowed"
      },
      {
        "loc": [
          "body",
          "org",
          "address"
        ],
        "msg": "str type expected",
        "type": "type_error.str"
      },
      {
        "loc": [
          "body",
          "org",
          "org_type"
        ],
        "msg": "str type expected",
        "type": "type_error.str"
      }
   ]
}

The ValidationError that Pydantic gives for invalid input:

{
    "detail": [
      {
        "loc": [
          "name"
        ],
        "msg": "none is not an allowed value",
        "type": "type_error.none.not_allowed"
      },
      {
        "loc": [
          "address"
        ],
        "msg": "str type expected",
        "type": "type_error.str"
      },
      {
        "loc": [
          "org_type"
        ],
        "msg": "str type expected",
        "type": "type_error.str"
      }
   ]
 }

The latter is the one I expect django-ninja to return as well. I'd like to know your thoughts on this.

[vitalik]

Hi @blazing-gig

can you show your operation definition ?

@api.post(...
def operation(request, ...

[blazing-gig]

Hey @vitalik ,

Here's the api def

@org_api_router.post('/')
def org_create_view(request, org: OrgSchemaIn):
    created_org = Org.objects.create(**org.dict())
    return CustomResponseObj(
        status=StatusConstants.SUCCESS,
        data=OrgSchemaOut.from_orm(created_org)
    ).dict()

[rngallen]

Am on the same page with you @blazing-gig this is the response

  "detail": [
    {
      "loc": [
        "body",
        "payload",
        "truck_plate_number"
      ],
      "msg": "plate number is in use",
      "type": "value_error"
    }
  ]
}

from this validation

class VehicleInSchema(Schema):
    truck_plate_number: constr(strip_whitespace=True, max_length=9)
    tank_plate_number: constr(strip_whitespace=True, max_length=9)
    capacity: Optional[Decimal] = Decimal("0")

    @validator("truck_plate_number")
    # unique plate number for either a truck or tank
    def unique_plate_number(cls, v):
        if Vehicle.objects.filter(Q(truck_plate_number=v)).exists():
            raise ValueError(_("plate number is in use"))
        return v

with this router

@transaction.atomic
def create_vehicle(request, payload: VehicleInSchema):
    data = payload.dict()
    data["created_by"] = request.user
    try:
        with transaction.atomic():
            vehicle = Vehicle.objects.create(**data)
            return 201, {"status": 201, "detail": f"vehicle created {vehicle.id}"}
    except IntegrityError as e:
        return 409, {"status": 409, "detail": f"{e}"}

from this I can know that on the request "body", "payload" is the one reference on "loc" The major issue I face is on exception. How can I make exception error presentation looks like pydantic presentation? I have tried to raise ValidationError from ninja.errors import ValidationError but the problem persist

Have tried this

from ninja.errors import ValidationError

@api.exception_handler(ValidationError)
def validation_errors(request, exc):
    return HttpResponse("Invalid input", status_code=422)

but it override all errors with one single message

Have tried to modify my custom validator to this

@validator("truck_plate_number")
    # unique plate number for either a truck or tank
    def unique_plate_number(cls, v):
        if Vehicle.objects.filter(Q(truck_plate_number=v)).exists():
            raise ValidationError({"truck_plate_number": _("plate number is in use")})
        return v

and the error representation changed to this

{
  "detail": {
    "truck_plate_number": "plate number is in use"
  }
}

but the problem pydantic default validation is not changing which make confusing on error representation

[vitalik]

@blazing-gig @rngallen

I think the misunderstanding here is that ninja.ValidationError is not the same as pydantic.ValidationErorr

1) as for pydantic - I think you do not need to throw exactly ValidationError in your validators

it' just enough to do assert with a message:

    @validator("truck_plate_number")
    # unique plate number for either a truck or tank
    def unique_plate_number(cls, v):
        exist = Vehicle.objects.filter(Q(truck_plate_number=v)).exists()
        assert not exist, "plate number is in use"    # <-----
        return v

2) ninja.ValiadtionError - is raised by django ninja - and it performs validation on all pydantic models defined by operation (body params, GET params, path params, etc)

Note: you will never catch pydantic.ValidationError in @api.exception_handler as it handled by ninja and then incapsulated into own class with all the details

and indeed these new details also has information about operation argument that was not able to validate (like body.org or body.payload your respective cases)

It is there so that in your exception_handler you can understand which model raised validation error

for both your cases I guess if you want to skip variable in your http response you can do the following :

from ninja.errors import ValidationError    # not pydantic !!!   

@api.exception_handler(ValidationError)
def validation_errors(request, exc):
    for err in exc.errors:
         err["loc"] = [err["loc"][0], err["loc"][-1]]   # skip argument <------- !!
    return api.create_response(request, {"detail": errors}, status=422)

[rngallen]

I can skip overriding @api.exception_handler() If I'll make my router func like this does it make sense?

def update_vehicle(request, truck_id: int, payload: VehicleUpdateSchema):
    data = payload.dict()
    data["created_by"] = request.user
    try:
        truck = Vehicle.objects.get(id=truck_id)
        for attr, values in data.items():
            setattr(truck, attr, values)
        truck.save()
    except Vehicle.DoesNotExist:
        return 404, {
            "detail": [{"loc": ("truck_plate_number"), "msg": "plate number is in use"},]
        }

[vitalik]

@rngallen

for 404 you can use django's get_object_or_404

from django.shortcuts import get_object_or_404

... 

def update_vehicle(request, truck_id: int, payload: VehicleUpdateSchema):
    truck = get_object_or_404(Vehicle., id=truck_id))  # <---- this will throw default 404 JSON response (that you can overrie) 
    data = payload.dict()
    data["created_by"] = request.user
    for attr, values in data.items():
         setattr(truck, attr, values)
..

[blazing-gig]

Hey @vitalik

Firstly, thanks for responding. I already had in mind the technique which you mentioned about handling ninja.ValidationError using api.exception_handler. Also I read through code and found that you were deliberately parsing the pydantic.ValidationError and injecting additional contextual info.

The only problem I face here is that 99% of the time, I'd just like to directly return the ValidationError that pydantic throws without having any additional contextual information about whether this was part of the payload/query params because the client sending the request would already have this info. So parsing the ValidationError twice (once within the library and once outside) seems like a doubly wasted effort.

On the other hand, I do agree that this info might come in handy if the api is doing some kind of request logging and needs to identify the source of the error.

To take a mean path, would it be possible for a switch to be exposed at the NinjaAPI level that can inform whether the additional contextual info has to be injected into the ValidationError instance? This switch can be overridden at the router or the api level.

api = NinjaAPI(..., validation_err_with_ctx=True/False)
# the same can be overridden at the router and api level

Another approach could be to take a custom class that can subclass ninja.ValidationError and have an attribute set on it based on which this decision can be made.

# the dotted path to this class can be gotten from `settings.py` or anywhere else and imported at runtime.
class CustomValidationError(ninja.ValidationError):
    inject_request_context = True / False

Let me know your thoughts on the same. Also, I made a comment on one of the open proposals as well. Feel free to check it out.

[blazing-gig]

Hey @vitalik ,

Any update on this ? Just trying to keep the discussion alive.

[vitalik]

@blazing-gig

still thinking... maybe the exception will contain references to original exception...

but on the other hand it's almost identical except this patch

err["loc"] = [err["loc"][0], err["loc"][-1]]   # skip argument <------- !!

[blazing-gig]

Hey @vitalik ,

I understand your confusion. So I believe there are two paths here that can potentially be taken:

1. Having a reference to the original exception. Pros: Probably the two exception objects are quite similar now as you pointed out but if they diverge in the future due to some reason, then this scheme accommodates for it. Cons: Not really sure if such a use-case due to which they diverge, will occur in the near future.

2. Without having a reference to the original exception: a. Probably one of the solutions I've mentioned above b. Or some other new strategy.

   Pros: Allows developers to control what additional context from the request goes into the exception. For now, it's just one flag. This could grow in the future.

   Cons: No way to access the original Pydantic exception even if desired. But if fine grained control is provided, I'm not really sure if this might be a problem.

Based on the above, I feel going for the reference to the original exception is a safer bet.

Let me know your thoughts on the same.

[vitalik]

but i'm not yet sure how reference will improve here...

let's take a case like this:

class Payload(Schema):
     x: int
     y: int

@api.post('/task/{id}')
def do_some_with_task(request, id: int, payload: Payload):
      pass

now imagine you are posing invalid request where id is not integer and x is missing:

http POST http://site.com/api/task/notint  x=1

this will result ninja.ValidationError

that will contain two errors:

1) loc = path/id , message: invalid integer 2) loc = body/payload/y missing y variable

I mean you already have all the information and can report to the client this response or patch it and report your own

[blazing-gig]

Hey @vitalik ,

Thanks again for responding. I see what you're saying. Let me give it a short think and get back.

[vitalik]

I guess if you only intereseted in last attribute you can just patch loc with

deatais['loc'] = [deatais['loc'][-1]]

[robinsandstrom]

Here is a simple helper function that can be used to parse the errors into a json on the form:

### returns errors on the form {'field': ['error1', 'error2', ... ]}

def parse_pydantic_errors(errors):
    parsed_errors = defaultdict(list)
    if isinstance(errors, dict):
        if 'loc' in errors:
            parsed_errors[errors.get('loc')[-1]].append(
                errors.get('msg')
            )
    elif isinstance(errors, list):
        for error in errors:
            if 'loc' in error:
                parsed_errors[error.get('loc')[-1]].append(
                    error.get('msg')
                )
    else:
        parsed_errors.append(errors)
    return parsed_errors

And then override:

@api.exception_handler(ValidationError)
def validation_errors(request, exc):
    errors = vars(exc).get('errors')
    parsed_errors = parse_pydantic_errors(errors)
    return JsonResponse({'errors': parsed_errors}, status=422)

Works like a charm

[lundberg]

Totally agree @blazing-gig that loc wrongly contains field names that's not part of the schema, i.e. can't be sent or known from a client's perspective.

The second item, org in your example, is the one that shouldn't be exposed in loc by ninja, since that is only a container for the posted body, and not a name/field that the client has sent or is visible in the OpenAPI schema.

Though, if a view have two body arguments, like the following example, then they should be part of loc IMO ...

@api_router.post('/')
def foobar(request, a: SchemaA, b: SchemaB):
    ...

... since in this case the client would actually post data on fields a and b, and should therefor be part of loc.

About the first item in loc, body, I don't agree. This is correct and should be present in both cases since the error is for a body param, i.e. the error's location should tell the about its source; path, query or body.

FastAPI have had the same issue, but it was solved in this PR, omitting names that isn't part of the client's seen schema.

To solve this here, I think this might be the place to craft loc ... we have full context, e.g. errors, view, request etc.

Thoughts or better placed fix @vitalik ?