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

Associations #35

Closed
jodosha opened this Issue Jun 28, 2014 · 40 comments

Comments

Projects
None yet
@jodosha
Member

jodosha commented Jun 28, 2014

Support associations between collections.

Lotus::Model::Mapper.new do
  collection :articles do
    # ...
    association :comments, [Comment]
  end

  collection :comments do
    # ...
    association :article, Article, foreign_key: :article_id 
  end
end

The first association is 1-n, where an article has many comments. The [] syntax specifies this.

The second association is n-1, where a comment belongs to an article (lack of []). The foreign_key should be optional and only specified when the following convention isn't respected: association name + _id. In the example above: article + _id.

Associations MUST NOT be loaded by default, but they require an explicit intervention via the preload method (which should be implemented as part of this feature).

class ArticleRepository
  include Lotus::Repository

  def self.by_author(author)
    query do
      where(author_id: author.id)
    end.preload(:comments)
  end
end

IMPORTANT: Let the preload mechanisms to not work with SQL joins, but with repositories instead. This isn't efficient, but we have a gain in terms of flexibility. Imagine the the scenario where the comments above aren't stored in the local database, but fetched from a remote JSON API. Thanks to the mapper we can easily know where to fetch those comments.

@jodosha jodosha added this to the v0.2.0 milestone Jun 28, 2014

@AlfonsoUceda

This comment has been minimized.

Member

AlfonsoUceda commented Jul 1, 2014

@jodosha Why do you put [] in an 1-n association?

@jdickey

This comment has been minimized.

jdickey commented Jul 1, 2014

@AlfonsoUceda Because the target of the association (the comments in that case; the subject would be the related Article instance) is a collection instead of a single item, hence the array (collection) notation.

At least, that was my reading of it.

@jodosha

This comment has been minimized.

Member

jodosha commented Jul 1, 2014

@AlfonsoUceda yes @jdickey is right. This is a convention that I've seen in other Ruby ORMs, we can stick with it, because it's already known.

@AlfonsoUceda

This comment has been minimized.

Member

AlfonsoUceda commented Jul 1, 2014

Ok thanks @jodosha and @jdickey ;)

@AlfonsoUceda

This comment has been minimized.

Member

AlfonsoUceda commented Jul 1, 2014

What do you think about the method association inject in atrributes set de foreign key?

@pablocrivella

This comment has been minimized.

pablocrivella commented Jul 24, 2014

I'm having a hard time understanding how associations would work with the entity.
If do something like article.comments
is CommentRepository.by_article(article) called behind the scenes?

@jodosha

This comment has been minimized.

Member

jodosha commented Jul 24, 2014

@pablocrivella Sorry for the confusion.

I don't want to give entities this faculty. If you look at the example above, where ArticleRepository preloads the comments, it fetches and assign them to the article object.

Pseudo-code:

article = fetch_article_from_database
comments = fetch_comments_for(article)

article.comments = comments

This hypothetical code shouldn't be visible to you, but happen behind the scenes.

To recap: no real associations in entities, they are just an accessor who is fed by the repository.

@pablocrivella

This comment has been minimized.

pablocrivella commented Jul 25, 2014

Will it work with lazy loading? (sorry for the nagging, I'm just curious. I really like the approach you are following, I have tried the repository pattern before but with .Net and nhibernate Orm)

@jodosha

This comment has been minimized.

Member

jodosha commented Jul 25, 2014

@pablocrivella Not a problem at all, feel free to ask and let me know if it makes sense for you.

It will be lazy by default, if you will omit the .preload(:comments) (see above), article.comments will return nil. This because Article#comments is just a dumb getter, so you have to be explicit and set that value via the repository.

@carloslopes

This comment has been minimized.

carloslopes commented Jul 25, 2014

@jodosha What do you think about a #load method? It will use the same logic that the #preload has but will serve to lazy load the associations.

@jodosha

This comment has been minimized.

Member

jodosha commented Jul 25, 2014

@carloslopes Tell me more, please. Where do you see this #load mechanism? I'm not sure if I got your idea.

@carloslopes

This comment has been minimized.

carloslopes commented Jul 27, 2014

@jodosha sorry, sometimes I still think like AR. The #load method would stay on the model, but this will break that separation of concerns that we keep here (and this way is a lot better).

My bad! 😉

@coenert

This comment has been minimized.

coenert commented Jul 29, 2014

@jodosha So wat you mean is that, after serializing the result of the query to an array, preload loops over the result and fill the association attributes? isn't that super inefficient?

@jodosha

This comment has been minimized.

Member

jodosha commented Jul 29, 2014

@coenert Why is that?

At the low level when we translate query results to domain objects (eg. Hash to Comment), we have those objects that we assign to the article. It's O(n), because we need to deserialize comments one by one, but that's common to all the ORMs. Am I missing something?

@coenert

This comment has been minimized.

coenert commented Jul 29, 2014

@jodosha nevermind, i missed something. I know we have to deserialize all those records, but i thought that every time we deserialize 1 entitty we had to fetch all the associated objects with another query. But we can just make a query like:

where(article_id: [id1, id2, id3, .....])

And then assign the result to each article.

I would like to try this one!

@coenert

This comment has been minimized.

coenert commented Aug 2, 2014

Ok, i faced a problem while making an test implementation, and i think we should discuss this problem.

When you define an association in the mapper: association :comments, [Comment] we find the first problem: because we need the Repository to retrieve the related objects we also need the collection in the same mapper, but we just don't what the collection is.

To get the collection i have 2 possible solutions:

  • collection name == Entity name in plurar form and provide api to give specifiy collection name, the rails way. Don't like this one(person -> people problem)
  • loop over collections(in the same adapter or given adapter) and use the first collection where Entity is given Entity. Seems the only reasonable way for me

(bold text is my opinion)

@jodosha

This comment has been minimized.

Member

jodosha commented Aug 4, 2014

@coenert Suppose we have the following mapping:

collection :comments do
  entity Comment
  entity CommentRepository
  # ...
end

adapter :disqus do
  collection :comments do
    entity Comment
    entity RemoteCommentRepository
    # ...
  end
end

collection :articles do
  # ...
  association :comments, [Comment]
  #           [1]        [2]
end

The proposed solution has two arguments:

  1. The name of the association, we should use this name both to lookup for the homonym collection (the first :comments) and to set the attribute into the entity Article#comments. This should lookup the default adapter.
    1a. We want to use a collection that comes from a different adapter. We should allow to specify it (see example 1).
    1b. We want to have a different accessor to be filled into the entity (eg Article#my_comments). We should introduce another option too (see example 2).
    1c. The serialized objects are stored inside the entity. Think of MongoDB's embedded associations, where we don't have a top level :comments collection, but they are stored inside each single article. We should introduce :attribute (or :embedded), as source of this collection. We can use [Comment] to infer the entity`.
  2. The plural/singular form of the association, to distinguish between has one and has many form. Theoretically this could be replaced with an option like multiple: true. Please don't rely on this to lookup the repository.
    2a. If we replace this as shown at point 2, we should introduce a new API to infer the entity in case of embedded collection. See 1c.

Example 1

association :comments, [Comment], adapter: :disqus

Example 2

association :my_comments, [Comment], collection: :comments

Example 3

association :comments, [Comment], attribute: :comments

Now, suppose that we have the following signature:

def association(name, options = {})
  # the collection name should be computed by looking at the attribute `:collection`, or fallback to `name`.
  collection_name = options.fetch(:collection) { name }
end

We can ask to the mapper to retrieve that collection and, finally ask for the repository: mapper.collection(collection_name).repository.

There are two missing pieces in the just depicted solution:

  1. A Mapping::Collection doesn't have access to the mapper. Evaluate if this is needed by the development if this feature
  2. Mapper#collection should accept an adapter option: mapper.collection(collection_name, adapter: :disqus).

Does this clarify the intents?

@coenert

This comment has been minimized.

coenert commented Aug 4, 2014

@jodosha thanks for your complete answer 👌, most of it is clear for me now.

that means that when we have an has one and we want to name it article instead of articles(name of collection) we have to define the association like this:

association :article, Article, collection: :articles

it is 👍 for me, simple api.

About the missing things:

  1. In my test implementation i added mapper as an constructor argument, access to the mapper is mandatory to receive the collection object.
  2. true, i think i will make an seperate PR for this, is that ok?

@coenert coenert referenced this issue Aug 4, 2014

Merged

DSL to register and load adapter instances #48

2 of 2 tasks complete
@carloslopes

This comment has been minimized.

carloslopes commented Aug 4, 2014

@coenert for has one associations you should use:

association :article, Article

You use the brackets when the association is a collection

@coenert

This comment has been minimized.

coenert commented Aug 4, 2014

@carloslopes youre right, but only need to remove the []. edited that

@coenert

This comment has been minimized.

coenert commented Aug 19, 2014

@jodosha I have some concerns about the api, with the current api there is no difference between belongs_to and has_one, how do we know where the foreign_key is located. i thought about how we can change the api so that you can specify that. I came with the idea that an association also an attribute is, but than lazy loaded. what about this api:

Has Many

attribute :my_articles, has.many_of(Article) {
  collection :articles
  foreign_key :author_id
}

Has One

attribute :details, has.one_of(UserDetails) {
  collection :user_details
}

Belongs To

attribute :author, belongs_to(User) {
  collection :users
  foreign_key :author_id
}

But you can also specifiy the options inline:

attribute :author, belongs_to(User), collection: :users, foreign_key: :author_id

i think it is also simpeler to understand. what do you think about it?

@jodosha jodosha modified the milestone: v0.5.0 Jun 30, 2015

@jodosha jodosha added feature and removed enhancement labels Jun 30, 2015

@pascalbetz

This comment has been minimized.

pascalbetz commented Jul 6, 2015

How would associations work for the insert/remove workflow (adding a comment to/removing a comment from an article)? I guess there'd be no support from lotus/model as it's only a "stupid" ruby object?

About the syntax for declaring assocations:
Defining associations with/without []to indicate has many/ belongs to seems a bit limited (as @coenert already pointed out with the has one case (foreign key on the other end)). I'd prefer something more speaking, like the inline version from @coenert.

Sidenote:
I'm new to lotus and doing a lot of Rails currently

@jodosha

This comment has been minimized.

Member

jodosha commented Jul 7, 2015

@pascalbetz Thank you for your opinion. The workflow for add/remove associated records is something under discussion. We're trying to apply a simple design here. 😄

@pascalbetz

This comment has been minimized.

pascalbetz commented Jul 10, 2015

@jodosha

If i understand correctly:

associations are just "stupid" getters/setters. So there is no syncing between the foreign key attribute and the association?

(pseudocode)

article = Article(id: 1, ....)
p article.user_id => nil

user = User(id: 10, ...)

article.user = user
p article.user_id => 10

I guess you don't want to autogenerate methods like

def user=(user)
  self.user_id= user.try(:id)
  @user = user
end

on the entity?

@pascalbetz

This comment has been minimized.

pascalbetz commented Jul 10, 2015

@coenert

Wouldn't the collection/fk/... options be arguments to belongs_to? (assuming belongs_to is a method which returns a type which represents the association)

attribute :author, belongs_to(User, collection: :users, foreign_key: :author_id)
@coenert

This comment has been minimized.

coenert commented Jul 10, 2015

@pascalbetz Probably, but now after some time i think my suggested api isn't in any way simpeler to understand then the one @jodosha initially suggested. I think i missed the point of creating less magic DSL's to reduce complexity(still learning 😄)

@pascalbetz

This comment has been minimized.

pascalbetz commented Jul 10, 2015

@coenert

The proposed API by @jodosha might hit some limits though (has and belongs to many, has many through, has one).

How about

attribute :user_id, Integer...
belongs_to :user, User, foreign_key: :user_id, ...
has_many :comments, Comment

Pretty much like Rails does it. Just keep the associated class as the second argument.

@jodosha jodosha modified the milestone: v0.5.0 Sep 12, 2015

@hlegius hlegius referenced this issue Oct 1, 2015

Closed

Associations (one-to-many, many-to-one) #244

3 of 3 tasks complete
@kirantpatil

This comment has been minimized.

kirantpatil commented Feb 2, 2016

Sir, Any updates on this issue ?

@joneslee85

This comment has been minimized.

Member

joneslee85 commented Feb 3, 2016

@kirantpatil we are in the cycle of improving model so please stay tuned.

@Defman21

This comment has been minimized.

Defman21 commented Jul 31, 2016

Are you still in the cycle of improving Model? I'm missing any sort of relations between tables.

@cllns

This comment has been minimized.

Member

cllns commented Jul 31, 2016

@Defman21 Yup. @jodosha is working on changing the backend of hanami-model to use Ruby Object Mapper (rom). He's started it on the new-engine branch, so you can check it out there, but it's in progress and the docs don't reflect the changes.

@Defman21

This comment has been minimized.

Defman21 commented Jul 31, 2016

I'm happy to see that there's some work going on to solve this issue :)

@elliottmason

This comment has been minimized.

elliottmason commented Sep 27, 2016

In lieu of hanami-model, what would be the downsides of just straight-up using rom? What will hanami-model provide that rom doesn't right now?

I've been wanting to start new projects in Hanami but the lack of model associations is a pretty big hurdle; pretty much the primary reason I still spin up new apps in Rails.

@solnic

This comment has been minimized.

Member

solnic commented Sep 28, 2016

hanami-model will provide more conventional usage than "plain rom" which comes with various conveniences like model inference, auto-timestamps, auto-field renaming etc. Pretty much what you would expect from a full-stack framework.

@senei

This comment has been minimized.

senei commented Nov 27, 2016

attribute :author, belongs_to(User, collection: :users, foreign_key: :author_id)

returns error uninitialized constant Book::User (NameError) -- why
in db it is working correctly

@beauby

This comment has been minimized.

Contributor

beauby commented Nov 27, 2016

@senei Please open a separate issue.

@jodosha

This comment has been minimized.

Member

jodosha commented Nov 30, 2016

@senei Where did you found that syntax? And where it is used?

@jodosha

This comment has been minimized.

Member

jodosha commented Dec 21, 2016

I'm closing this in favor of future, smaller, focused, and actionable proposals.

@jodosha jodosha closed this Dec 21, 2016

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