sequel support #281

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants

mrbrdo commented Sep 15, 2013

Sequel support (without migrations, but I can provide them). I guess it's at 95%. I'm using it in a project and seems to work fine.
Be aware of jeremyevans/sequel#705 i.e. in Rails apps, plugins must be loaded into Sequel::Model before doorkeeper is loaded.

mrbrdo commented Sep 15, 2013

if someone could help me out with this migration (how to make the generator), then I can just update the readme and this should probably be good to merge

mrbrdo commented Sep 17, 2013

About the Sequel plugins thing I mentioned, it has been largely mitigated by my PR to sequel-rails (https://github.com/TalentBox/sequel-rails#enabling-plugins).
So, anyone care to comment on this PR?

Owner

tute commented Sep 17, 2013

Hi @mrbrdo, thank you very much for your work! We are actually planning on removing ORM specific code out of doorkeeper (see 7826bf8#commitcomment-3800578). Next steps is to define the way of gems can enhance doorkeeper at runtime, and to remove mongo mapper and mongoid support.

Will leave this issue open until we define those, thank you again.

mrbrdo commented Sep 17, 2013

Okay that sounds really smart, because it was not possible to avoid code duplication while working on this (without serious refactoring). Until then this branch should be good to use if anyone needs it now. Everything is implemented, only the migration has to be copied manually from the source.
I hope this will motivate you to support sequel after you are done with refactor. It really is a very good ORM :)

@stakach stakach referenced this pull request Oct 30, 2013

Closed

Add Couchbase ORM #314

Owner

tute commented Feb 3, 2014

Topic regarding extracting ORM specific code off of doorkeeper: applicake#355

Owner

tute commented May 23, 2014

@mrbrdo I checked your code and found that tests are not running with Sequel gem. Did you do browser testing, or how did you test?
Thank you!

mrbrdo commented May 23, 2014

I'm using it on several projects in production. I don't remember how I tested, it was a long time ago, sorry.

Owner

tute commented Jul 11, 2014

@mrbrdo thank you for sharing your work. I'll close this issue though in favor of #355, also because it's not mergeable and it doesn't have tests. Thank you again!

@tute tute closed this Jul 11, 2014

tmaier commented Aug 21, 2014

#355 got closed but not merged.

@mrbrdo Are you planning to work on this again? This would be great!

Owner

tute commented Aug 27, 2014

@tmaier: @jasl made some refactors needed for this kind of features. It's on an open PR, I can't make myself time to truly review yet though, but he is the man currently more related with this.

Contributor

jasl commented Aug 27, 2014

I have some busy work to do this week, I could merge this PR to my branch in next week

Owner

tute commented Aug 27, 2014

I actually meant the refactor PR so we can treat this as a separate repository that interacts with doorkeeper. Planning to move ORM specifics out not in! :) Thanks for your quick response.

mrbrdo commented Dec 3, 2014

so what is the status on this @tute ? we are still without sequel support?

Owner

tute commented Dec 3, 2014

@jasl, do we already have any ORM-specific repository for doorkeeper 2, as a model to create new ones?

Owner

tute commented Dec 4, 2014

Thank you @jasl. @mrbrdo doorkeeper currently has no sequel support AFAIK, and anyone is very welcome to create an extension along the lines of https://github.com/jasl/doorkeeper-orms or https://github.com/advancedcontrol/doorkeeper-couchbase.

mrbrdo commented Dec 4, 2014

https://github.com/doorkeeper-gem/doorkeeper/blob/master/lib/doorkeeper/config.rb#L39
https://github.com/doorkeeper-gem/doorkeeper/blob/master/lib/doorkeeper/models/concerns/ownership.rb#L7

Nope. Too bad you did not look over my commits before doing that refactor. It's pretty obvious:
mrbrdo/doorkeeper@e912e4d#diff-2dac7b5ba275c2921a0ad8385afea15bL28
And that's not the only issue (e.g. Revocable etc). I'm not sure what was actually changed except moving files around.

Owner

tute commented Dec 4, 2014

The refactor facilitates the extension of doorkeeper without modifying it. My goal is that a PR like this becomes it's own gem/repository, as I won't be able to successfully maintain many different ORMs, something the community will do better.

Version 2 with these refactors wasn't released yet, if you can explain further what could be improved before we release it will help. Thanks for your work and input!

mrbrdo commented Dec 4, 2014

Well my suggestion is that you look at the commits I made in this PR, that will give you a better idea of what parts are currently not designed well for extension with different frameworks.
I would avoid making any assumptions about the query methods (where, find_by...) and define some kind of adapter interface for that. Also not call anything directly on the Model class, but instead always go through adapter (including find). So for example even if the ORM doesn't have save, update_attribute methods, the adapter can return a wrapper object with those methods, but user can still access the models via his ORM. Don't make assumptions about relationship methods (e.g. application.tokens.build, application.tokens.find). All validations and relationships (belongs_to, has_many) should be defined by adapter and not by core library. You could abstract it and implement in core but I don't see any point in it.

Contributor

jasl commented Dec 4, 2014

@mrbrdo
https://github.com/doorkeeper-gem/doorkeeper/blob/master/lib/doorkeeper/config.rb#L39 seems is my missing, check it out later(busy playing WoD in these days 😂 ).

I agree you partly that defining a set of interfaces and let ORM-specific implement them is better. But I finally not to do that.

  • Doorkeeper have not defined interfaces yet, defining them also needs to refactor controllers, that would take a great of time and too many work.
  • I think my work is giving a chance to extend Doorkeeper support more ORMs, and keeping it simple. so I decide to not do any abstraction on models, and let them include Mixins manually. The official implementation of these Mixins are design for ActiveRecord-like, if not compatible, not use it, just implements these methods.

It seems has reached the goal, doorkeeper-couchbase is a good sample to prove this.

I have plan to discover Doorkeeper's controllers in future, and back to working on refactor models.

mrbrdo commented Dec 5, 2014

Yes I guess that setup_application_owner is the only hard blocker. I understand what you mean, if there is not time to do a proper refactor then I agree with you.

Owner

tute commented Dec 8, 2014

@mrbrdo your ideas would let people use doorkeeper in other frameworks, and I'd love to see that in the future. I certainly want to move in that direction, thank you for your more detailed explanation.

For now, people do plug in the gem in frameworks with no rails gems, but they have to indeed monkey patch some sore points.

I am happy to start merging refactors toward this goal, one at a time, backwards compatible whenever possible.

Thank you again!

mrbrdo commented Dec 9, 2014

No worries. @tute @jasl don't forget about that setup_application_owner method

@jasl jasl referenced this pull request Dec 16, 2014

Merged

small improve #522

Contributor

jasl commented Dec 16, 2014

@mrbrdo In PR #522 I moved loading Ownership on initialize Doorkeeper.

mrbrdo commented Dec 16, 2014

I don't understand how that is better?
If you force include Ownership into Application I cannot add support for Sequel without monkey-patching. Why don't you just include Ownership in the Application model itself, so I can provide my own Application model without you including stuff into it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment