Define API for classes utilizing OmniAuth::Identity::Model

[pboling]

@btalbot - Working on a fix.

Fixes

• Fix breaking changes introduced by #86's introduction of :on_validation

Added

• Define #save, #persisted? and ::create on Omniauth::Identity::Model
• Add @since YARD tags to interface methods
• Refactor Omniauth::Strategies::Identity.registration_phase to support Omniauth::Identity::Model-inheriting classes that do not define #save.
  • This support will be dropped in v4.0.

[pboling]

@btalbot release 3.0.5 is out with the fix. Please let me know if there are any issues.

[btalbot]

Hi @pboling

Thank you for the updates, they are much better than what existing previously. However, there are two issues I've run into while testing code with your changes.

1. The previous model's initializer was never called by the strategy so the creator was free to use whatever arguments to the new method they wished. The updated strategy always attempts to call the model's new method with the attributes hash even if the strategy might later attempt to call ::create instead. This breaks old model implementations that do not expect their initializer to be called with a hash. I think that to remain backwards compatible the strategy must only use ::create and never expect to call the model initializer directly.
2. The method #saving_instead_of_creating? will always return true for any model that extends the OmniAuth::Identity::Model since that model does define #save and #persisted? methods even if they do raise an NotImplementedError. Maybe just attempting to call the new #save method and then handling the NotImplementedError with the fallback method would work?
   

   omniauth-identity/lib/omniauth/strategies/identity.rb

   Lines 144 to 146 in 9f55885

   	def saving_instead_of_creating?
   	@identity.respond_to?(:save) && @identity.respond_to?(:persisted?)
   	end

[pboling]

Oops, I didn't see this before I released more fixes in 3.0.7. I will continue working on this @btalbot. Both of the points you make are valid, and haven't been fully resolved by the most recent release.

[pboling]

@btalbot The fix for both issues was fairly simple. It is impossible to use the options[:on_validation] unless also using new/save, so I just made the presence of that option the decider. If the option isn't set, then create works fine. I think that resolves the issues you found.

It just means that some implementations can't use options[:on_validation], which should surprise no-one.

[btalbot]

That is simple and does seem to allow the ::create method to be used when #validating? is false. How should validation be configured to use the #save but without actually using the validation feature? Seems like we will need to create a valid on_validation block which just always returns true?

[pboling]

@btalbot No, the default for options[:on_validation] is nil. If it is provided by an implementation that implementation has always, since the introduction of the feature a few weeks ago, had to support new and save, so there is no change for those users.

[btalbot]

What is expected config for an app to use new and save but with no (or default) on_validation?

[pboling]

They will not use new and save. Wasn't that the point of your initial issue? We can't introduce new and save in a breaking way for existing implementations. Introducing them as part of an entirely new feature, which on_valdiation is, is not breaking.

In this context, the only reason to use new and save instead of create is to validate before persisting. I also undeprecated create. We'll just keep this pattern for now.

[btalbot]

Yes, the 3.0.8 release does resolve the backward compatibility issue and tests using existing implementations now pass. Thank you.

A future step is to remove the use of the deprecated create method and use new and save instead. When doing that, I'll just also add a lamba for on_validation = lamda { |_| true } to allow them to be used.

[btalbot]

Also, future implementers will need to know that the on_validation call() provides a hash parameter and not env or req as most of the other blocks do.

For example

on_login = lambda { |env| puts "env is #{env}" }
on_validation = lambda { |args| puts "env is #{args[:env]}" }