tortoise-orm icon indicating copy to clipboard operation
tortoise-orm copied to clipboard

Better validation errors

Open georges-g opened this issue 1 year ago • 8 comments

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.

georges-g avatar Aug 22 '23 09:08 georges-g

Please check that.

long2ice avatar Aug 22 '23 12:08 long2ice

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

georges-g avatar Aug 22 '23 14:08 georges-g

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

long2ice avatar Aug 23 '23 02:08 long2ice

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.

georges-g avatar Aug 23 '23 06:08 georges-g

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

long2ice avatar Aug 23 '23 06:08 long2ice

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 avatar Aug 23 '23 07:08 georges-g

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 avatar Aug 23 '23 07:08 georges-g

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.

georges-g avatar Aug 30 '23 09:08 georges-g