-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add validate_unique_tentatively/3 #2162
Add validate_unique_tentatively/3 #2162
Conversation
test/ecto/changeset_test.exs
Outdated
@@ -1044,6 1045,58 @@ defmodule Ecto.ChangesetTest do | |||
assert changeset.errors == [terms_of_service: {"must be abided", [validation: :acceptance]}] | |||
end | |||
|
|||
test "validate_unique_tentatively/3" do | |||
defmodule FakeRepoWithDup do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josevalim I couldn't find (or maybe didn't recognize) what you were referring to here:
One way to solve it is to use the process inbox or the process dictionary to instruct the adapter to return a certain value. We do so currently only on the migration tests.
... so this is my somewhat dumb solution. 😊 I'm happy to do it differently if someone has a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @josevalim had an idea to modify the test adapter. In particular the function here: https://github.com/elixir-ecto/ecto/blob/master/test/support/test_repo.exs#L43-L45
We could make it use something like:
Process.get(:test_repo_all_result) || {1, [[1]]}
And then in the test case, we could set the result with Process.put(:test_repo_all_result, ...)
.
lib/ecto/changeset.ex
Outdated
end | ||
end | ||
|
||
defp primary_key(changeset = %Changeset{}) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File has the variable name before the pattern while most of the files have the variable name after the pattern when naming parameter pattern matches
I wonder about the name. Often something that bypasses some rules is called "dirty". Would it make sense to call the function |
@michalmuskala "dirty" implies dirty tracking (i.e. check if an object was touched) I prefer the name "validate_optimistic_uniqueness". This is after all an "optimistic" check. |
@michalmuskala I tried the process dictionary approach - see latest commit. Personally I like this approach less than the previous one. I had to guess to discover the values Regarding the name: all the existing validation names are of the pattern
|
I should say - I'm not adamant about the name. Happy to call it whatever y'all think best. |
My primary concern with "tentatively" is that it's not a very common word and might be confusing for non-native speakers. |
Perhaps |
That could work. But it might convey "this approach isn't very sophisticated" as opposed to "this is intended to be the first check, to be followed up somehow with a final check." "Tentative" has the built-in idea of "needs confirmation".
Ah. Maybe so. It's also not super easy to type. 😆 "Preliminary" means something similar to "tentative", but "preliminarily" is awful. 🤣 |
|
||
# If we don't have values for all fields, we can't query for uniqueness | ||
if Enum.any?(where_clause, fn (tuple) -> is_nil(elem(tuple, 1)) end) do | ||
changeset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should still add something to the validations
key here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michalmuskala This PR was merged, but I can do another to add to validations
. I was looking for an example of a validation that explicitly does that, but had to stop working before I found one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, I have added this to master and also renamed it to unsafe_validate_unique. :)
I still believe "optimistic" is the correct word in the computer engineering context ( https://en.wikipedia.org/wiki/Optimistic_concurrency_control Conversely, a proper "pessimistic" uniqueness validation at insert time is one that locks the database so that no other transaction can modify the data (e.g. create a duplicate value) while the insert is occurring. |
@johnnyshields I see what you're saying, but I think a unique constraint in the db is the optimistic check here - we don't lock the table before inserting, but if, at the last moment, it turns out that there's a duplicate, we roll back the transaction. The check is optimistic (no up-front locks), but it's reliable. Validating uniqueness doesn't offer any guarantees, just a better user experience in the 99.99% case where the duplicate already exists at the time of validation. |
❤️ 💚 💙 💛 💜 |
Provide a way to check for uniqueness in the validation phase.
This PR comes after much discussion - see #2006 - but that doesn't mean that the implementation (or the tests) are perfect.