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 (one-to-many, many-to-one) #244

Closed
wants to merge 19 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@hlegius
Contributor

hlegius commented Sep 30, 2015

/cc @heruku

I've copied #56 from heruku and started to grow the association .preload idea (started by him) to get it working properly.
First, I tried to fork heruku's PR and rebase from there, but I've got no success on it due tons of new features which has been added since them. So, I decided to fork Lotus' repo, get diff from #56 and reapply file by file, paying attention to each new detail.

Todo:

  • Associations within queries, i.e:
class ArticlesRepository
  def self.by_category(category)
    query.where(category_id: category.id).preload(:category)
  end

  def with_category
    query.preload(:category)
  end

  def published
    query.where(published: true)
  end
end

flower = CategoryRepository.find_by_name('Flowers') # <= Category Object
article = ArticlesRepository.by_category(flower).first # or all, or last, etc.
article # <= Article Object
article.category_id # <= Integer (category_id field)
article.category # <= Category Entity (same as flower var)
  • Associations with queries, i.e. ArticlesRepository.with_category.published.all
article = ArticlesRepository.with_category.published.first # or all, or last, etc.
article # <= Article Object
article.category_id # <= Integer
article.category # <= Category Entity
  • Associations with preload at the end of sentence, i.e. ArticlesRepository.published.with_category.all
article = ArticlesRepository.published.with_category.first # or all, or last, etc.
article.category # <= Category Entity

Known Limitations (so far)

# any Adapter
flower = CategoryRepository.find_by_name('Flowers')
article = ArticlesRepository.with_category.by_category(flower).first
article.category.articles # nil
# I have some ideas here, but need to chat with you guys before :]
# Memory Adapter

article = ArticlesRepository.published.with_category.first # or all, or last, etc.
# Error: Method `.with_category` not found

# but, single query wrapped inside Repository will work fine

article = ArticlesRepository.by_category(category).first # or all, or last, etc.
article.category # <= Category object

Also, refers to #35

@AlfonsoUceda

This comment has been minimized.

Member

AlfonsoUceda commented Oct 3, 2015

@hlegius nice job, and thanks to @heruku too, would you like to create a test for one_to_many file? because there is one for test/model/associations/many_to_one_test.rb

# entity Article
# attribute :id, Integer
# attribute :user_id, Integer
# association :user, [User], foreign_key: :id, collection: :articles

This comment has been minimized.

@AlfonsoUceda

AlfonsoUceda Oct 3, 2015

Member

I think this example should be User instead [User]because we have a singular association

# end
# end
#
# @return Lotus::Model::Adapters::Memory::Query

This comment has been minimized.

@AlfonsoUceda

AlfonsoUceda Oct 3, 2015

Member

please put this above @example, add also @since x.x.x

# end
# end
#
# @return Lotus::Model::Adapters::Sql::Collection

This comment has been minimized.

# end
# end
#
# @return Lotus::Model::Adapters::Sql::Query

This comment has been minimized.

# ManyToOne association:
# association :articles, Article, foreign_key: :user_id, collection: :articles
#
# @param association_name, relation_type(array, const), *options

This comment has been minimized.

@AlfonsoUceda

AlfonsoUceda Oct 3, 2015

Member

fix params doc

instance_eval(&blk) if block_given?
end
# Define a new association between Entities
#
# @example

This comment has been minimized.

@AlfonsoUceda

AlfonsoUceda Oct 3, 2015

Member

examples at the bottom please

@@ -74,10 +82,31 @@ class Collection
def initialize(name, coercer_class, &blk)
@name = name
@coercer_class = coercer_class
@attributes = {}
@associations, @attributes = {}, {}

This comment has been minimized.

@AlfonsoUceda

AlfonsoUceda Oct 3, 2015

Member

could you break it in two lines?

@@ -400,14 +429,20 @@ def serialize(entity)
# Deserialize a set of records fetched from the database.
#
# @param records [Array] a set of raw records
# @param records [Array] a set of raw records, associations' array

This comment has been minimized.

@AlfonsoUceda

AlfonsoUceda Oct 3, 2015

Member

break it in two lines

@@ -400,14 +429,20 @@ def serialize(entity)
# Deserialize a set of records fetched from the database.

This comment has been minimized.

@AlfonsoUceda

AlfonsoUceda Oct 3, 2015

Member

you could explain here, associations are loaded

@hlegius

This comment has been minimized.

Contributor

hlegius commented Oct 3, 2015

@AlfonsoUceda no problems. Thanks for review'ng. I'm gonna take a look at it and create the missing test as well.

# query.preload(:articles)
# end
# end
def preload(association)

This comment has been minimized.

@AlfonsoUceda

AlfonsoUceda Oct 4, 2015

Member

what do you think to add splat operator? I think is better this api
preload(:users, :articles) than preload(:users).preload(:articles)

def preload(*associations)
  associations.each do |association|
    @associations << association
  end
end

cc @lotus/core-team

This comment has been minimized.

@hlegius

hlegius Oct 4, 2015

Contributor

Personally I prefer your version, but I'd rather don't break the Query's where api contract i.e query.where(foo: :baz).where(baz: :qux). If it isn't a "problem", then I can change, no problems!

let(:article1) { Article.new(user_id: user1.id, title: 'Introducing Lotus::Model', comments_count: '23') }
let(:article1) { Article.new(category_id: category.id, user_id: user1.id, title: 'Introducing Lotus::Model', comments_count: '23') }

This comment has been minimized.

@AlfonsoUceda

AlfonsoUceda Oct 4, 2015

Member

be careful with this because ID is nil, maybe you could pass an id to Category.new

@@ -4,8 +4,9 @@
let(:user1) { User.new(name: 'L') }

This comment has been minimized.

@AlfonsoUceda

AlfonsoUceda Oct 4, 2015

Member

please you should add a test testing repository layer, testing preload

This comment has been minimized.

@hlegius

hlegius Oct 4, 2015

Contributor

Done!

hlegius added some commits Oct 4, 2015

Added Association Preload to Repository's Query Object
In commit, will allow us to build internal queries aligned
and preload their associations as well. i.e.
def self.by_category(category)
  query.where(category_id: category.id).preload(:category)
end
@andresilveirah

This comment has been minimized.

andresilveirah commented Oct 12, 2015

Hey @hlegius nice work! @AlfonsoUceda is there anything impeding this PR to be merged? Anything I can do? I'm currently implementing an example of Association with Lotus and it would be awesome to have this feature.

@jodosha

This comment has been minimized.

Member

jodosha commented Oct 16, 2015

@hlegius This is a great work! 👏

What prevents us to merge this is the lack of implementation of the other half of the job: writing associated entities. Please have a look at this gist: https://gist.github.com/jodosha/431091eebb41606b358a

/cc @lotus/core-team @andresilveira

@jodosha jodosha added the feature label Oct 16, 2015

@jodosha jodosha added this to the v0.5.1 milestone Oct 16, 2015

@jodosha jodosha self-assigned this Oct 16, 2015

@jodosha jodosha added the waiting label Oct 16, 2015

@jodosha jodosha referenced this pull request Oct 16, 2015

Closed

Associations #56

@hlegius

This comment has been minimized.

Contributor

hlegius commented Oct 30, 2015

@jodosha is there anything that I can do to help this feature get merged? I mean, what's your plans to do before merge it? maybe I could help o/

/cc @AlfonsoUceda

@qcam

This comment has been minimized.

qcam commented Oct 30, 2015

Looking forward to this!!! 👍

@jodosha

This comment has been minimized.

Member

jodosha commented Nov 3, 2015

@hlegius Mr @joneslee85 (PING) will help you to finalize this. 😸

@joneslee85

This comment has been minimized.

Member

joneslee85 commented Dec 4, 2015

@hlegius polite ping from me, grab me and we work this out together

@jodosha jodosha modified the milestone: v0.6.0 Dec 18, 2015

@hlegius

This comment has been minimized.

Contributor

hlegius commented Dec 19, 2015

@jodosha do you think it still will be useful somehow? If yes, @joneslee85 can I ping you on Gitter to talk about schedule/tasks to do?

@jodosha

This comment has been minimized.

Member

jodosha commented Dec 19, 2015

@hlegius @joneslee85 Please hold on until we'll release 0.6

@collection.associations[:user].must_be_instance_of Lotus::Model::Associations::ManyToOne
end
it 'defines a many to one association' do

This comment has been minimized.

@regishideki

regishideki Dec 22, 2015

I think description should be 'defines a one to many association'

@hlegius hlegius closed this Mar 16, 2016

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