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

Remove Adapters #498

Closed
moonglum opened this issue Jan 4, 2014 · 12 comments
Closed

Remove Adapters #498

moonglum opened this issue Jan 4, 2014 · 12 comments
Milestone

Comments

@moonglum
Copy link

moonglum commented Jan 4, 2014

I have the idea to remove the adapters and support for specific ORMs/ODMs and instead just assume that the user model is an ActiveModel with the possibility to define before_save and after_save handlers. This would require the developers to implement a number of methods by themselves (see Tradeoffs).

Tradeoffs

For the user of the library that has the following consequences as far as I can see:

  1. He or she has to add the fields by hand (for example the field for the email, the hashed password, the salt and additional attributes depending on used submodules). So for a developer of ActiveRecord this means adding a migration, for a developer of MongoID adding a few field statements to the model.
  2. Add a handful of methods to the model: find_by_credentials and find_by_id for the simple case. If the developer is using token features two additional methods have to be added.

Does anyone see any additional tradeoffs?

Why?

  1. This would solve the problem of addressing each of the ORMs/ODMs by hand. There are currently three adapters and pull request or issues asking for support for more adapters including Datamapper support Datamapper support #472, Couchbase support Couchbase support #454, Sequel support Sequel Support #414 and CouchDB support CouchDB support #291.
  2. This would remove the current complexity of running the tests. Instead of tests for each of the adapters and the requirement of running each of the supported databases, only one test suite is needed running against ActiveModel without any external dependencies on databases.
  3. No adapter specific bugs like Username and email not persisting using Mongoid #343, mongomapper adapter credential_regex slow #352, Tagged emails don't work as username with mongoid #340, In spec/rails3_mongoid, errors raise spec/controller_activity_logging_spec.rb #238, Reset password + Mongoid = "ArgumentError (wrong number of arguments (1 for 0)):" #187, MongoDB: authentications as embedded document #163...
  4. Give the developer more freedom of modeling how to save credentials, tokens etc.

Work

I'm more than happy to work on this and send a pull request if you're interested in this change. If you want further investigation, I'm also happy to do that.

@mje113
Copy link

mje113 commented Jan 6, 2014

+1

I was having the same thought when writing a couchbase adapter... I opted for doing it as a separate gem instead of a PR: https://github.com/mje113/sorcery-couchbase

The first pain I ran into when working on a PR for CB support was the fact that there are development gem dependencies that aren't jruby friendly, and since I'm working in jruby, it was a non-starter.

It would be much more developer-friendly if instead of simply removing the ORMs, there were removed to their own gems. Then you'd have the same functionality you get today, with only the inclusion of an additional gem 'sorcery-activerecord' or whatever.

@moonglum
Copy link
Author

moonglum commented Jan 6, 2014

I think if you really remove the concept of adapters from sorcery, it is still possible to write gems like couchbase-sorcery that do the things you need to do by hand with the concept described above automatically. But users who don't want to use them can wire things up by hand.

What do you think?

@mje113
Copy link

mje113 commented Jan 6, 2014

Oh sure, wiring things up manually as an option makes sense. Just saying that instead of removing the ORM adapters altogether, they could be moved into separate gems. So we're in agreement I think.

Also, ideally you would also want a set of lint tests that would make it easy to ensure your custom ORM library (or in-app code) complies with what sorcery expects. Something like: https://github.com/rails/rails/blob/master/activemodel/lib/active_model/lint.rb

@kirs
Copy link
Collaborator

kirs commented Jan 8, 2014

I totally like this idea 👍
The mail issue I see here is that developers will have to copy paste more code while the general purpose of gems is to reduce amount of copy pasted code and put them into shared libraries.

I'd try this as an experiment for 1.x version of Sorcery.
Your contributions will be appreciated 😏

@moonglum
Copy link
Author

moonglum commented Jan 8, 2014

@mje113 I think the best option would be to have a gem like activerecord-sorcery which requires sorcery and does all the wiring that is needed. If the programmer doesn't want to wire everything by him- or herself than the gem will do all of that. Similar gems can be provided for couchbase, mongoid etc.

@kirs Glad you like it 😄 I would be really happy to see this in sorcery. If you need any help, just tell me. I can also do a spike for this functionality so you can see if you like it 😸

@mje113
Copy link

mje113 commented Jan 8, 2014

@moonglum Right, that's exactly the approach I took with sorcery-couchbase. The main pain-point is knowing that I had everything implemented correctly--which is why I think that a good set of Lint tests provided by sorcery would help orm gem authors to fulfill their side of the "contract".

@kirs Happy to lend a hand as well.

@arnvald
Copy link
Collaborator

arnvald commented Feb 8, 2014

I like the idea and will be happy to work on it. Let me know if you need any help.

@kirs
Copy link
Collaborator

kirs commented Feb 9, 2014

I've created the sorcery-rails organization to store external adapters.

@arnvald
Copy link
Collaborator

arnvald commented Mar 23, 2014

Hi,

we've started discussing the roadmap for 1.0 release. If anyone's interested in the discussion, join us here: sorcery-rails/sorcery-issues#1

Removing adapters is one of our goal for 1.0 release.

@arnvald
Copy link
Collaborator

arnvald commented Apr 18, 2014

Here's the first step to remove adapters: arnvald@1c1d96a

There's still some work to be done there, especially on the interface of adapters, but I hope we'll have some working version of Sorcery and adapters gems in following weeks

@arnvald arnvald added this to the v1.0.0 milestone May 10, 2014
@arnvald
Copy link
Collaborator

arnvald commented Jan 4, 2015

Adapters were removed in f5115cd

Before 1.0 release I'll publish them as external gems (sorcery_mongoid is already published on github). Right now I'm closing the issue.

@arnvald arnvald closed this as completed Jan 4, 2015
@moonglum
Copy link
Author

moonglum commented Jan 4, 2015

Yay 👍

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

4 participants