Change .all and has_many to return *Collection object

[Adam-Stomski]

My proposal for lazy loaded collections.

Now using .all method queries database immediately, after this change it would return a Granite::ORM::Collection(Model) which forwards missing methods to the array of records.

# before:
Klass.all # queries database [Klass1, Klass2]

# after:
Klass.all # does not query => Granite::ORM::Collection(Klass)
Klass.all.each do |klass| # queries db
  # (...)
end

Now using has_many association methods, loads all related records from the database. After this change, it would return a Granite::ORM::AssociationCollection(Owner, Target). This allows easy extensions on associations (for example #121 should be easy to implement as association.delete_all). Included find_by and all as an example for this.

# before:
student.klasss # => queries database [Klass1, Klass2]

# after:
student.klasss # does not query => Granite::ORM::AssociationCollection(Student, Klass)
student.klasss.each do |klass| # queries db
  # (...)
end

new methods

klasses = student.klasss.all("AND klasss.type = ? LIMIT ?", ["maths", 10]) # => Granite::ORM::Collection(Klass)
klasses.map(&.name) # queries db

klass = student.klasss.find_by(:name, "Klass A1")
klass = student.klasss.find_by!(:name, "Klass A1")
klass = student.klasss.find(1)
klass = student.klasss.find!(1)

I didn't want any new query syntax, just reuse what Granite already has, making associations less dumb, and prevent unnecessary db queries. Also didn't add any docs for now, would like to hear you opinion on this first :)

[robacarp]

Just looking at the doc you have here in the pull, I think it’s a good idea. Granite is currently pretty chatty and I’d like to see that cleaned up however possible.

I haven’t looked at the code here yet, but I suspect I’ve implemented something vaguely similar in the query builder (so I’m obviously onboard with the idea).

Is the idea here to prevent queries from being run on variables that are never used?

[robacarp]

This syntax is an improvement for sure

[Adam-Stomski]

@robacarp I want to improve way working with relations. Since I needed something in the middle to provide methods like student.teachers.all I thought that it would be cool to have consistent API with lazy load using all method without relation too.

I believe that preventing queries from unused variables is a very good thing. I've seen a lot of code written with passing down collection through few methods to not be used because of one condition at the end.

Since these collections forwards missing to the array the only breaking change there is returned type. I believe this is a small problem compared to possibilities it gives and "free" optimization

[eliasjpr]

@faustinoaq we should label this with a breaking change label.

@Adam-Stomski this is nice addition 👍

[drujensen]

I don't think you need the not_nil! since you are using the find_by!

[drujensen]

same here

[drujensen]

and here

[drujensen]

here

[drujensen]

This is an excellent addition. Thanks! 💯

@robacarp we will need to update the query dsl pr

[drujensen]

@robacarp need another approval when you get a chance

[robacarp]

👍 for removing the redundant .not_nil!