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

Improve belongs_to and validates_uniqueness_of from database_validations gem #34650

Closed
djezzzl opened this issue Dec 7, 2018 · 6 comments
Closed

Comments

@djezzzl
Copy link

djezzzl commented Dec 7, 2018

Hey guys, we're using database_validations gem. It provides a better experience with belongs_to and validates_uniquness_of.

Long story short, those improved methods:

  • at least 2 times faster;
  • provide a compatibility with existing corresponding analogs;
  • provide a true database consistency because they use constraints in the database;

Check out the repo to get more details. There are also few articles about it: here and here.

The gem becomes more popular every day and some guys support an idea to provide a PR to merge it to the core.

Because @dhh introduced create_or_find_by we can create a PR to merge the logic from the gem to the ActiveRecord.

WDYT about it? Any feedback is appriciated!

@rafaelfranca
Copy link
Member

Not sure if we can call that better experience. The gem itself already show some disavantages over the in-memory validation. One that is really a deal breaker to me is "Cannot handle multiple validations at once because database raises only one error per query.". This degrades the error message you show to the users and give really bad user experience.

The validations in Rails are not only to give developers ways to protect they database but also to give users of the application good usability. While they can be slower than no running any validation in memory and rely on the database to set constraints we believe in-memory validation improves the user experience, and this was always the goal of the framework regrading validations since the beginning.

That being said while I think some people can use this I prefer to not introduce this behavior in Rails. I'm happy that the plugin exists and people can take advantage of this if they want.

Thank you for the issue.

@djezzzl
Copy link
Author

djezzzl commented Dec 7, 2018

Hi @rafaelfranca , thank you for such a quick response!

This degrades the error message you show to the users and give really bad user experience.

Yes, I agree, but at the same time:

  • Right now, validates_uniqueness_of doesn't ensure uniqueness (because of race conditions)
  • I think most models have no more than a few uniqueness validations, so the case where few of them fail at the same time is really rare and I doubt we have to show both errors.

this was always the goal of the framework regrading validations since the beginning.

Okay, I undestand that point. Sounds right. But what if I suggest you to combine both approach. (I can provide a PR for that).

Idea: Use old way of those validations and run them with additional SQL queries and etc as it is now but at the same time if we get some constraint error (after we pass all validations), e.g. ActiveRecord::RecordNotUnique and we have the uniqueness validation on this field (group of fields) we rescue the error and return proper errors on the instance and correct behavior as in case validations were failed. Like we do here.

I think this idea covers your thoughts and also gives our users better experience with the framework because it handles corner cases (race condition, etc) properly.

WDYT?

@rafaelfranca
Copy link
Member

That makes sense. We do that in https://github.com/Shopify/activerecord-rescue_from_duplicate. This behavior can be merged with the current validator.

@djezzzl
Copy link
Author

djezzzl commented Dec 7, 2018

That's very similar to what we did in database_validations. Didn't see it before.

This behavior can be merged with the current validator.

So if you're fine to have that inside ActiveRecord, will you approve my PR if I provide it?

@rafaelfranca
Copy link
Member

yes. I'm fine with it.

@djezzzl
Copy link
Author

djezzzl commented Dec 17, 2018

Hi @rafaelfranca, I have just provided a PR: #34725. Please have a look.

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

No branches or pull requests

2 participants