Doesn't handle failed validations when saving a new instance

[contentfree]

It appears that last week changes were made to handle validation failures when updating an object. But no such changes were added for handling Sequel::ValidationFailed when saving a new user (during sign up, for example).

[contentfree]

It's possible that this is something to be configured on Sequel's side (with the raise_on_save_failure flag). Though maybe raising (in update_attributes as well) and using rescue_with would be more appropriate.

[rosenfeld]

Actually I only merged the patch assuming it fixed some real problem but I didn't have time to ask why that was needed. Could you please explain what scenarios are affected by this validation issues with regards to Devise? I've just returned to Brazil from my vacations in NY and I have a lot to do before I'm back to work on Monday since I lost my wallet in the plane in which I got to JFK as soon as my vacations started... It would be easier for me to find a proper fix once I understand what are the problematic use cases...

[rosenfeld]

Sorry, but if I don't get any feedback from you so that I can understand what is the use case in the next week or so, I'll have to close this ticket because I have no idea of what is the real problem here.

As you can see, AR's implementation for update_attributes doesn't raise:

http://api.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-update_attribute

So, if you think Devise is not handling sign-up correctly, then most probably you should open a ticket on the Devise gem, not here, unless it works with AR but not with Sequel. If this is the case I need to understand how to reproduce the problem or I won't be able to fix it.

[rosenfeld]

With regards to your other comment, as you can see in the link below, Model#update doesn't support any options hash so that we could specify the raise_on_save_failure flag:

http://sequel.jeremyevans.net/rdoc/classes/Sequel/Model/InstanceMethods.html#method-i-update

[contentfree]

You're right that update_attributes doesn't raise, which is the problem that "last week['s] changes"(b375c39) handled because Sequel does raise a ValidationFailed exception when saving, by default.

Either A) the same thing should happen when persisting a new instance – again, because Sequel will raise a ValidationFailed exception when saving which is not how AR's save() method works (save()! will raise though) – or B) last week's change should be reverted and documentation added to tell people to configure Sequel with its raise_on_save_failure flag.

B is probably preferable though configuring all of Sequel to return false on a save failure (vs raising an error as it does by default) might be too drastic a change.

Which is better in your opinion? (Either way, it should be consistent between persisting new users and updating existing users which is not the case after b375c39).

[rosenfeld]

I'm against B approach because I don't think this plugin should impose any restrictions on the main application and should work with Devise regardless of raise_on_save_failure default behavior.

I think it's ok to explain this issue in the README and let the user decide what to do and maybe provide some helper method to aid them but I don't know how to fix this in a transparent way.

Maybe the user is okay with changing the save behavior for the User's class only, maybe not. Maybe they're okay with changing the default behavior for raise_on_save_failure, maybe not.

The main problem to me is that Devise is supposed to use only the methods of orm-adapter, so ideally this gem wouldn't even exist if they did so. From the orm-adapter gem:

https://github.com/ianwhite/orm_adapter

ORM adapter provides a consistent API for these basic class or 'factory' methods. It does not attempt to unify the behaviour of model instances returned by these methods. This means that unifying the behaviour of methods such as model.save, and model.valid? is beyond the scope of orm_adapter.

So Devise shouldn't rely on save if you ask me and it would be probably a good idea to start some discussion in the Devise project with regards to ORM support and ask them to work with orm-adapter to add more methods if needed.

If you have other ideas I'd like to hear them.

[contentfree]

A "hackish" method would be to add a rescue ValidationFailed for the initial save like b375c39 did with updating but adding, to both, a check for Sequel's current setting for raise_on_save_failure and either reraise the exception if raise_on_save_failure is true or return false if raise_on_save_failure is false.

(And then bring it up to the Devise folks, but this would fix this issue while also respecting however the user has configured Sequel to behave)

Edit: What I said above is nonsense… if raise_on_save_failure is configured then Sequel already follows it. What b375c39 is doing should be reverted, IMO, and documentation added explaining that "If Sequel is configured to raise on save failure then Devise may work unexpectedly". With b375c39 in place, it has already broken expected behavior (for those coming from Sequel into Devise rather than from ActiveRecord+Devise into Sequel) because it now doesn't raise on save failure even when configured to do so.

Or…?

[rosenfeld]

You mean this commit? 1e56e5f

[rosenfeld]

If so, this commit doesn't break anything because Sequel doesn't define update_attributes. We simply implement this new method like AR so that Devise is happy when calling it. Unfortunately Devise also calls save, which Sequel also defines and may behave differently. That's why it's a harder decision for this particular case...

[rosenfeld]

By the way, I'd be happy to merge in any changes to the README describing the problem of incompatibilities between Sequel and Devise when raise_on_save_failure is true (the default). I'd like to state what exact known features are known to be broken in such cases so that the user can decide whether it's a relevant feature or not for their application and aid them on deciding upon changing that global param.

[rosenfeld]

Maybe an hackish solution that I might consider is something like this:

# this would be some module to include to User if raise_on_save_failure is true
def save(*args)
  return super(raise_on_failure: false) if caller.first =~ /devise/
  super
end

What do you think?

[contentfree]

Yes, that commit. Sorry, I don't know how I ended up with that other one.

The "hack" looks good to me. Do you need to merge that option with the args?

[rosenfeld]

It doesn't make sense to merge any options if it was Devise who called save because Devise doesn't support Sequel directly unfortunately, so most likely it would pass any AR option if any was passed...

[rosenfeld]

It seems it passes the validate option sometimes and we may have to understand what it does and translate it to Sequel:

https://github.com/plataformatec/devise/search?utf8=%E2%9C%93&q=save

How I'd wish for Devise to be more friendly to other ORM solutions rather then relying too much on AR or Mongo API... :(

[contentfree]

According to http://sequel.jeremyevans.net/rdoc/files/doc/validations_rdoc.html#label-Skipping+Validations Sequel will handle validate: false as expected.