making model a Sorcery model

[arnvald]

I'm adding new ticket here as I want to focus here on this one specific issue.

Currently the code that extends model with Sorcery's methods works following way:

• we extend base ORM class with Sorcery::Model and Sorcery::Model::SpecificAdapter
• this adds authenticates_with_sorcery! method to base class
• we then call this method in model we want to become Sorcery model
• this method extends class with more methods and fields depending on ORM

Although Sorcery::Model itself does not add lots of methods to base class, it still does. So, even though Sorcery can be used with just one class in application, all of models get authenticates_with_sorcery! as well as init_orm_hooks! and a few different methods.

What I suggest to do here: instead of extending base class with Sorcery methods, we can include the Sorcery::Model directly in the user class. In this module we'll add proper methods and we'll either include adapter module, or we'll call some adapter init! method that will extend the class with certain methods.

So here's basic example:

config/initializers/sorcery.rb

Sorcery.configure do |c|
  c.adapter = Sorcery::Adapters::Mongoid
end

app/models/user.rb

class User
  include Sorcery::Model
  ...
end

lib/sorcery/model.rb

module Sorcery::Model
  def self.included(base)
    include sorcery_config.adapter
  end
end

lib/sorcery/adapters/mongoid.rb

module Sorcery::Adapters::Mongoid
  def self.included(base)
    base.class_eval do
      field sorcery_config.email_attribute_name, type: String
    end
  end
end

Any thoughts on that?

[kirs]

Nice!

I see at least one disadvantage: we could make authenticates_with_sorcery receive different options like in Devise, but with include Sorcery::Model won't be possible.

[kirs]

From another point of view, this way doesn't pollute AR::Base class.

[arnvald]

Good point with the options. Although we don't allow them right now, it might make sense to do it in the future.

Then, however, each adapter needs to include Sorcery::Model into its ORM's base class. I think it should be easy for adapter's authors, though, I only need to remember to put it into documentation for implementing adapters.

[kirs]

Then, however, each adapter needs to include Sorcery::Model into its ORM's base class

Why do you think so?

[kirs]

I think that any adapter class should be inherited from Sorcery::BaseAdapter or somehow.

[arnvald]

Then, however, each adapter needs to include Sorcery::Model into its ORM's base class

Why do you think so?

Because in order to be able to call authenticates_with_sorcery! in model, this method needs to be available in the base class. We currently do it here for all adapters: https://github.com/NoamB/sorcery/blob/master/lib/sorcery.rb#L57

I think that any adapter class should be inherited from Sorcery::BaseAdapter or somehow.

So you want adapters to be classes instead of modules? My idea was to keep them as modules included in model.

When I get some more stable connection I'll push the code with changes in core and extracted Mongoid adapter, you'll be able to see how it looks and tell me if you have some better idea to implement it.

[arnvald]

@kirs here's the commit:

https://github.com/arnvald/sorcery/commit/1c1d96a50435ed0cbe05fd577699f596685fb680

Some things I found out while working on it:

• currently we extend all the models with Sorcery methods, not just the User model - so for example we use Authentication.find_by_provider_and_uid and it works, because we include our Model::Adapters::Mongoid module into Mongoid::Document and not just to the User class. I'll think about how to improve that
• the methods I added to adapters are temporary, in the end I'd prefer to have some Initializer class that will take User class as an argument and define fields and hooks on this class, but I'm not sure if that makes sense. What's your opinion?
• I've noticed we hold user class config in controller config. I'm not a big fan of this solution, but so far I haven't found any really good way to improve it
• right now some code will have to be repeated across different adapters, like defining callbacks (there's the same syntax for 3 of them, but different for DataMapper). However, as we're going to move from extending model to introducing service layer, we'll be able to remove at least some of these callbacks (like the ones for user activation)

[arnvald]

Ok, here's another step I took. Take a look here: https://github.com/arnvald/sorcery-mongoid and here https://github.com/arnvald/sorcery/commit/ec3ac963bc05b646460e340b31a20df5548e072c

(here's working app: https://github.com/arnvald/sorcery-example-app/tree/mongoid_rails_3_sorcery_v1)

This is just an experiment, I tried to to remove the methods that will just define fields and callbacks from the adapter itself, so that they do not stay in class, as they're called only once. That's why Initializer class was added - I create initializer and let it define base fields and then fields for submodules. Do you think such approach makes sense, or should we keep these init methods directly in adapter module?

[arnvald]

And then suddenly I realized how extremely stupid is the idea of having init methods for submodules in adapter. Instead of having these methods, adapters should have common interface for defining fields and callbacks. Then each submodule should have 'init' method which will define these fields and callbacks. I'll change it later this week.

[arnvald]

I think that any adapter class should be inherited from Sorcery::BaseAdapter or somehow.

So you want adapters to be classes instead of modules? My idea was to keep them as modules included in model.

I've been working on it for a while and I've changed my mind - you're totally right, adapters should be classes and they should inherit from one common class.

I've got just one problem with these class adapters - in some cases I need to wrap an instance (updating object) and in some I need to wrap a class (finding objects). So probably I need two separate adapters then (likeAdapters::MongoidInstanceAdapter and Adapters::MongoidClassAdapter). Does it make sense or maybe there's some better way?

[NoamB]

Keep in mind modules allow the developer to use any base class for his model, while a class forces our own.

Sent from my iPhone

On 27 באפר 2014, at 09:21, Grzegorz Witek notifications@github.com wrote:

I think that any adapter class should be inherited from Sorcery::BaseAdapter or somehow.

So you want adapters to be classes instead of modules? My idea was to keep them as modules included in model.

I've been working on it for a while and I've changed my mind - you're totally right, adapters should be classes and they should inherit from one common class.

— Reply to this email directly or view it on GitHub.

[arnvald]

Keep in mind modules allow the developer to use any base class for his model, while a class forces our own.

I didn't explain it well - I don't want user's class in application to inherit from Sorcery's adapter class. Adapter will be internal wrapper for Sorcery's use. Example: I need to have a method that will define field on the class (like field in Mongoid) in order to add remember_me_token field. I can either extend Mongoid's class, or I can wrap it in adapter which will contain the method for defining fields. The code will look like this:

class Sorcery::Adapters::MongoidClassAdapter < Sorcery::Adapters::BaseClassAdapter
  def initialize(klass)
    @klass = klass
  end

  def define_field(name, options)
    klass.field name, options
  end
end

user_class = Sorcery::Adapters::MongoidClassAdapter.new(User)
user_class.define_field :remember_me_token, type: String

(of course the direct class names will be replaced by config options) The BaseClassAdapter will just contain the information about how to implement custom adapters.