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

Unique validator does not raises 500 when it requires a type coercion and it raises an exception #140

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

TrilceAC
Copy link
Contributor

On issue #139 I presented an scenario where unique causes 500 errors because the query to check for uniquenes cannot be performed due to a failed type coercion with PhoneNumberType and phonenumbers.

This pull request with its associated test solves this issue using a reviewed version of the restrictive approach that I proposed. Though it is not perfect, nothing better comes to my mind. If the unique validation cannot be performed, no matter the reason, there is a ValidationError with a customizable message through a new parameter of the Unique validator called message_uncheck. By default, it is set to show the message "Unable to check uniqueness." This causes validation errors instead of 500 errors when invalid data is set on unique fields that require a typer coercion that might raise exceptions.

@kvesteri
Copy link
Owner

I think a good way to implement this would be to:

  1. Use python_type property of the associated TypeEngine / TypeDecorator object and try to coerce the value using the class returned by the python_type and catching errors.
  2. You could also check that the associated TypeEngine / TypeDecorator has this python_type property defined and if not then the Unique validator can not be used for that specific type.

By catching all errors when executing the uniqueness check query is bad for a whole bunch of reasons. The error might occur for example if the database connection is lost, if something else in the query fails, if there is bug in the underlying database driver...

@TrilceAC
Copy link
Contributor Author

Hi,

As suggested, I have tried to implemented a general way to coerce the data when checking for uniqueness, but I couldn't. It seems to me that each TypeDecorator might implement different approaches to coerce data, at least when data comes from a form.

What I have done is to perform a check for PhoneNumberType, trying the type coercion, by means of using a private method, i.e. not the best solution. I no longer try to catch any exception when the query is performed. Instead, I try to do the _coerce that is going to be performed, and I catch the exception PhoneNumberParseException. This works for PhoneNumberType, but is not a general solution for any type.

Advantages:

  • No generic except. Only PhoneNumberParseException is catched.

Drawbacks:

  • Not a general solution. Other types will require different approaches.

By catching all errors when executing the uniqueness check query is bad for a whole bunch of reasons. The error might occur for example if the database connection is lost, if something else in the query fails, if there is bug in the underlying database driver...

I agree that in general, catching any exception is not the best practice. In this case, I came to the conclusion that any exception, no matter which is the cause, results in a failed validation. In this case, I don't see harmful to catch any exception and raise a ValueError. In the end, the validation is failing and in cases of connection lost or other causes, you won't be able to insert the data.

Having said this, I have removed the except Exception. At the moment of writting, only PhoneNumberParseException is catched.

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

Successfully merging this pull request may close these issues.

2 participants