search

toptal / database_validations

Database validations for ActiveRecord
MIT License
530 stars 13 forks source link

Doesn't work with nested attributes / autosave associations. #55

Closed jduff closed 2 years ago

jduff commented 3 years ago

When using nested attributes the associated objects are autosaved, which either does a direct insert or saves with validate: false as you you can see here. This means when we get to the create_or_update defined in this gem here validate is false and the exceptions are not handled.

This is likely true of any collection associations that are autosaved, I only noticed it when using nested attributes though.

djezzzl commented 3 years ago

Hi @jduff,

Thank you for pointing this out! I'll do my best to have a look this week.

chamnap commented 3 years ago

I have a similar situation where it works fine on 0.9.1. However, as I tested it's no longer working after 0.9.4. Here's my gist, https://gist.github.com/chamnap/8b827d323148c7ccb6c5612a342a1510. It failed the first example.

djezzzl commented 2 years ago

Hi @chamnap, thank you for sharing this! Version 0.9.4 has fixed the behavior to respect validate: false option which is the default for Rails.

The gem was designed to improve the experience and performance of Rails without breaking its behavior. We can, however, add an opportunity to override that default Rails behavior to improve nested associations validation.

Could you please try to add validate: true option to your has_many :children association in the snippet and share if that helps?

chamnap commented 2 years ago

@djezzzl, I tried adding validate: true to has_many :children, and I still get ActiveRecord::RecordNotUnique exception.

FYI: I've updated my gist.

djezzzl commented 2 years ago

Hello 👋

I was thinking about the issue and the conclusion was: initially, the only idea of the gem was to improve behavior of ActiveRecord in non-breaking way. If Rails team doesn't expect to validate saving of associations, then, I think, we should follow it here.

Of course, we can extend the gem and add an option to change that behavior (and feel free to provide the PR or even create a small gem for that).

If I got the issue right and it persists for ActivieRecord too (without database_validations), then I don't think we should change this behavior and make it to be default.

I'll close the topic but please feel free to open it again if you disagree or I understood the problem wrongly.

jduff commented 2 years ago

That's fair, my main use case was for handling the RecordNotUnique exception that gets thrown regardless of validate option, which might not be the normal use case for this gem. I found another gem that does this for me though so have switched to using that.

You have to draw a line somewhere though and I think your explanation and reasoning makes sense @djezzzl, thanks for writing that up!

djezzzl commented 2 years ago

Thank you too 👍 @jduff

djezzzl commented 2 years ago

Hi @jduff and @chamnap ,

I've just released a new version 1.1.0 with new option available https://github.com/toptal/database_validations#rescue-option that should help with the issue you mentioned. Could you please try it out and let me know if that works for you?

Have a great weekend :+1:

gap777 commented 1 year ago

@jduff What other gem did you choose to use instead?

jduff commented 1 year ago

@gap777 I think I ended up using this one https://github.com/Shopify/activerecord-rescue_from_duplicate

djezzzl commented 1 year ago

Hi @gap777,

Yes, https://github.com/Shopify/activerecord-rescue_from_duplicate is also available.

There is also an option to do a PR that adds support to different modes; one of those could be the desired behavior.

gap777 commented 1 year ago

@jduff I don't think that shopify gem is being maintained... it doesn't seem to work at all with our Rails 7+ app. @djezzzl we had to patch your adapter list to work with our GIS-enabled postgres adapter (postgis), but were hopeful to use this gem... but also found it not to actually produce the validation error (the exception continued to be raised by the adapter), even though we used the rescue option.

jduff commented 1 year ago

Ya, I'm not sure if the state of it anymore either, the project I used it on was from a few years ago. I'm pretty sure I had it working on rails 7 though, but could be wrong.

I don't have any other alternatives, I haven't needed this behavior in my most recent projects.

djezzzl commented 1 year ago

Hey @gap777,

@djezzzl we had to patch your adapter list to work with our GIS-enabled postgres adapter (postgis), but were hopeful to use this gem... but also found it not to actually produce the validation error (the exception continued to be raised by the adapter), even though we used the rescue option.

I would be happy to fix this. Could you please provide more details or, ideally, a minimal set of steps to reproduce the issue?