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.

Sign up for GitHub

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

Jump to bottom

don't validate uniqueness if there is a prior error #3353

Merged
merged 2 commits into from
Jul 3, 2020
Merged

don't validate uniqueness if there is a prior error #3353

merged 2 commits into from
Jul 3, 2020

Conversation

slashmili
Copy link
Contributor

@slashmili slashmili commented Jul 2, 2020 •
edited
Loading

There is no need to validate uniqueness when there is an error on the expected unique fields

Example:

user
|> cast(attrs, [:name, :email])
|> validate_format(:email, ~r/@/)
|> unsafe_validate_unique([:email], MyApp.Repo)

if email doesn't contain at sign, we can save few database query until the email is in the correct format.

@slashmili slashmili changed the title don't validate uniqueness if there's a prior error don't validate uniqueness if there is a prior error Jul 2, 2020
@slashmili
dont validate uniqueness if there's a prior error
080097f 
There is no need to validate uniqueness when there is an error
on the expected unique fields

Example:

    user
    |> cast(attrs, [:name, :email])
    |> validate_format(:email, ~r/@/)
    |> unsafe_validate_unique([:email], MyApp.Repo)

if email doesn't contain at sign, we can save few database query
until the email is in the correct format.
@slashmili slashmili force-pushed the dont-validate-uniqueness-prior-error branch from 7f8c1a2 to 080097f Compare July 2, 2020 22:08
@josevalim josevalim merged commit b4a3a3c into elixir-ecto:master Jul 3, 2020
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@slashmili slashmili deleted the dont-validate-uniqueness-prior-error branch July 4, 2020 04:56
@nathanl
Copy link
Contributor

nathanl commented Jul 6, 2020

I thought I had implemented this, but I remembered wrong - I was thinking of #2905

Nice catch and very nice PR! 👏 😄 💯

@michalmuskala
Copy link
Member

michalmuskala commented Jul 6, 2020 •
edited
Loading

I'm not convinced this is a correct change. In general validators try to add as many errors as possible at one go - e.g. we run subsequent validations even in a previous one failed.

This has significant impact on the final UI presented to the user. Imagine a registration situation. The user will either get information that the password they chose is not long enough and the username is already taken at the same time, or they have to submit the form twice.

This change will lead to the poorer user experience of having to submit the form multiple times and getting a trickle of errors one-by-one. Avoiding this issue of one-by-one errors is the main advantage of unsafe_validate_unique over constraints. With this change I don't really see a reason to use this function over constraints.

Please disregard above. I just noticed it only checks for errors on fields that are being validated. This makes sense. Sorry for sounding the alarms without reading through everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josevalim josevalim josevalim left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@slashmili @josevalim @nathanl @michalmuskala