tortoise / tortoise-orm

Familiar asyncio ORM for python, built with relations in mind
https://tortoise.github.io
Apache License 2.0
4.68k stars 390 forks source link

Better validation errors #1460

Open georges-g opened 1 year ago

georges-g commented 1 year ago

Is your feature request related to a problem? Please describe. At the moment, validation errors are emitted with this code:

    for v in self.validators:
            if self.null and value is None:
                continue
            try:
                if isinstance(value, Enum):
                    v(value.value)
                else:
                    v(value)
            except ValidationError as exc:
                raise ValidationError(f"{self.model_field_name}: {exc}")

I usually want to get the value of the field that caused the error so that I can inform the frontend of the problematic field.

The problem is that, to get the value of the field that cause the error: self.model_field_name, I need to use a regular expression on the string.

Describe the solution you'd like I would like to have an error class that has specific field and msg instance attributes. The msg attribute would not contain the field value.

Describe alternatives you've considered Using a regex to isolate the field / msg.

Additional context None.

long2ice commented 1 year ago

Please check that.

georges-g commented 1 year ago

Close, but I still need the original msg (the {exc} part in the string).

You could add another optional attribute field_msg or something like that.

Thanks!

EDIT: field_name and field_msg sounds like a good option IMO

long2ice commented 1 year ago

See https://github.com/tortoise/tortoise-orm/pull/1461

georges-g commented 1 year ago

It still does not work.

I still need to parse the FieldValidationError.msg to remove the '{self.model_field_name}: ' part and access the {exc} part.

What I would like is to have a FieldValidationError.field_msg with only the {exc} part.

That being said, if you want to go for two separate classes, that would be a good occasion to validate all the fields instead of stopping at the first one failed. That would be useful for forms to indicate all the problematic fields at once.

Instead of having this ValidationError at field level and FieldValidationError at object level, I would have a FieldValidationError at field level, and ObjectValidationError at object level.

The ObjectValidationError could have a errors attribute that would be something like that:

errors: {
    field_name: FieldValidationError,
    …
}

It would be filled progressively during the validate() method, then the ObjectValidationError would be raised if errors is not empty.

The message of the ObjectValidationError could be a concatenation of the field errors with their {field_name}: at the beginning and \n at the end.

long2ice commented 1 year ago

I know your need. But I think which is better handled in frontend.

georges-g commented 1 year ago

I don't think so.

Have you checked how django does model validation? They allow multiple errors on a field and checking all the fields. That's something very much worth doing IMO.

georges-g commented 1 year ago

And the problem with doing the validation in frontend is that it is not safe, and you need to duplicate the validation in the backend. I would rather do it once backend, and send the errors to the frontend. It's safer and there is no duplication.

georges-g commented 1 year ago

I thought about this issue and something that could be done would be to keep the current validate() method, but add a validate_full() method to fields and objects doing a full check and raising the more detailed errors.

This way, you don't have to implement breaking changes.

I don't have much time right now, but I might work on this in the future and do a PR.