added collection adapter for active record

[HoneyryderChuck]

Referring to #27

This is just a discussion starting point. As you may have seen, I add the collection adapter only to ActiveRecord. This is because I wanted to discuss with you the gem extension, the validity of the additional API, and if we agree on it, some corrections to the specs/internal implementation I added, and other possible extensions to the general purpose API, before writing adapters to other ORMs.

So, this is my point of view: I think that the collections not only represent an abstraction present in all ORM implementations, they also offer some general CRUD scoped methods that can be useful to the further extension of certain gems. From that perspective it would be interesting to bring them all together.

So, please don't close the request before we discuss this further. I think it would be more interesting to add this to the existing gem instead of adding a new "collection_orm_adapter" gem.

[josevalim]

@TiagoCardoso1983 Thanks!

The problem with this approach is that we are assuming most ORMs will allow you to navigate collections using associations proxies. As soon as you find an ORM that does not support user.notes as a proxy, orm_adapter suddenly becomes incompatible.

[HoneyryderChuck]

You're right, but I think we can always work around it. The focus of the task is to extract an adapter decorator from the collection. How we access the associated collection is not a concern of the gem, I'd say, maybe the spec would need to be rewritten then.

Nevertheless, the gem is only compatible with the 4 named ORM's, and I think there's no such hinderance for them, right?

[josevalim]

@TiagoCardoso1983 the goal of orm_adapter is to write code that is going to work with different ORMs, this seems to lead users to the opposite direction. I know it is not as convenient, but the most adequate way today to work with associations in orm_adapter is exactly by doing:

Note.to_adapter.find(user_id: user.id)

Because we don't to assert anything about how associations behave.

Nevertheless, the gem is only compatible with the 4 named ORM's

We ship support for 4 ORMs by default, but other ORMs have their own implementation too (as we don't intend to support all in the gem).

[HoneyryderChuck]

Hum... I see your point. Couldn't one just find a way to "decorate" the access to a collection?

One possible way of doing this could be:

User.to_adapter.get_association(primary_keys, :collection_name)

One could come up with a better method signature, though.

[josevalim]

If we do that, we are then pushing associations handling to orm adapter...

[ianwhite]

@TiagoCardoso1983 thanks for the contribution and the discussion starting point!

It may be right that popular ORMs currently have collection functionality, and that an abstraction is very doable, but orm_adapter's purpose is to support a minimal set of functionality. (pretty much the functionality needed to make devise work :). Just enough functionality to do CRUD, so that gem authors can easily work with the basic features of popular ORMs.

So I think that orm_adapter-collections would be a good candidate for a separate gem.

[HoneyryderChuck]

@josevalim true, so let's change the wording. Maybe the notion of "collection" is not exactly what I'm trying to get at, but more the notion of "scoped" adapter. The way I see it, the current orm adapter implementation has a basic API for finding, creating and destroying ORM objects. What if I said to this adapter "hey, my scope is this, user_id must be 2", and then all these operations would take this into account?

@ianwhite I know the main purpose of this gem is to provide an ORM thin wrapper for devise, but it does tackle a key point regarding ORM basic API support and writing ORM-agnostic gems for other cases. I'm not fond of the idea of a separate gem anymore after taking José's concerns into account, I'd prefer to convince you of the "usefulness" of the idea of a scoping to the adapter. Maybe it could be something devise could take advantage of :)

[josevalim]

What if I said to this adapter "hey, my scope is this, user_id must be 2", and then all these operations would take this into account?

What is "my scope" here?

[HoneyryderChuck]

Taking some active record examples:

User.find(1).notes # notes belonging to user 1
Note.where(:user_id => 1) # same thing

The scope would be a set of conditions passed to the adapter. These conditions would be therefore always used in its operations:

Note.to_adapter(:user_id => 1).find_all # only notes belonging to user 1
Note.to_adapter(:user_id => 1).build # returns instance of Note with user_id = 1

we could then move away from the "collection" discussion and take it for what they are: a subset of persisted objects.

[josevalim]

At first, I don't see any issue for the proposal above... it is basically a scope of options that is passed for underlying adapter when called. How would an implementation for that look like?

[HoneyryderChuck]

The default scope would be passed in the adapter initializer in the form of an hash. These would be valid conditions (limit, offset and the like wouldn't make sense here). All underlying methods would then use this hash.

One could then use these scoped adapters to provide "convenience" adapters for collections in ORMs which support them.

[josevalim]

It just seems to be about passing default options around. It seems fine for me as long as it is not doing any assumption about the underlying ORM.

[HoneyryderChuck]

The collection wrappers would then be ORM-specific. I see may I confused you by inserting the spec in the shared example.

[josevalim]

So I don't see how we could reach an agreement here if each collection wrapper would be ORM specific. It would be, once again, mixing the orm semantics with orm adapter, instead of isolating them.

[ianwhite]

I'm a bit confused, so here's a summary of what I think is going on, there are now two features under discussion.

scoped adapters. This means keeping a hash of attrs per adapter object, which is merged with all attr methods (e.g. User.to_adapter(group_id: 3).find(name: 'fred') would find using conditions {group_id: 3, name: 'fred'}. The implementation would have to account for the simple memoization strategy we currently employ. This implementation should be able to be implemented in base only. @josevalim is this what you think is ok?

collection wrappers, A new adapter class, for collections, with an api similar to base (but with #build added), together with orm specific code to decorate collections as adapters with code like user.notes.to_adapter. @TiagoCardoso1983 is this what you want, or are you now proposing just the above option, or something else?

[josevalim]

Thanks @ianwhite. scoped adapters is exactly what i think that would be ok. :)

[ianwhite]

Ok, so @TiagoCardoso1983, if you want to have a crack at option 1 scoped adapters, I think that the api should be something like this:

User.to_adapter(scope: {group_id: 1})

to allow for extension of adapter creation in the future, or by other gem authors. @josevalim wdyt?

If you want something like option 2 collection wrappers, I think the best option is that you make a new gem that depends on orm_adapter, and injects the new functionality you want. It may be that in the future we'll change our minds, and your gem will be a great proof of concept.

[HoneyryderChuck]

Sounds good. I'll keep you posted about the sub-gem then. I'll openly "campaign" for its insertion in your gem, though :) . But lets talk about it again as soon as there is a definite prototype.

Should we close this issue and develop it in a new one, or should I rewrite and push the scoped adapters to here?

[HoneyryderChuck]

By the way, I see the tests support a very old version of mongoid (at least). Should I update this?

[HoneyryderChuck]

Hi, so here's a first draft.

Main points of discussion:

• I removed the memoization of the adapter on a class variable, since now it can be called with other arguments, and that can have some implications concerning cached classes and race conditions.
• The datamapper implementation of the get method looks a bit cluttered. I'm open to suggestions.
• I had to update the mongoid spec more than I thought I had to. It seems it has been supporting an older version of it, is it (has_many_related, belongs_to_related...). I updated it to be compatible with the latest version. I hope you're ok with it. What concerns me is why travis can't get this.
• I get a few errors running "rake spec" task locally on the datamapper spec, because the origin gem (mongoid dependency) "refines" the symbol class in such a way that interferes with the DM query conditions. Would be cool to discuss a way to better isolate the specs.