jgaskins / perpetuity

Persistence gem for Ruby objects using the Data Mapper pattern
http://jgaskins.org/blog/2012/04/20/data-mapper-vs-active-record/
MIT License
250 stars 14 forks source link

Associations #1

Closed jgaskins closed 12 years ago

jgaskins commented 12 years ago

I'm trying to decide how to deal with relationships between objects. Let's say we have two classes, Article and Comment. An Article should have a collection of Comments associated with it.

There are a couple ways we can do this. One way would be to explicitly call the CommentMapper, like so:

article = ArticleMapper.retrieve article_id
article.comments = CommentMapper.comments_for article

Or we could go with something like

article = ArticleMapper.retrieve article_id
comments = article.comments

The first example seems to be the most true to the Data Mapper pattern, but the second one is ridiculously convenient and can be done with a little metaprogramming. If we are storing the article_id field inside each Comment record, we could define a method on the article object when it's retrieved that retrieves its comments if they haven't been retrieved, something like:

article.define_singleton_method :comments do
  @comments ||= CommentMapper.retrieve(article_id: self.id)
end

This method wouldn't exist before retrieving the article, which means it would fall back to its attr_reader or whatever method it was using as its comments attribute. I'm pretty sure this violates the Data Mapper pattern and just moves back toward the ActiveRecord pattern, unfortunately. I just like the convenience of it.

Another way of handling this that I've been thinking of is to get the ArticleMapper class to stuff the comments into the article:

article = ArticleMapper.first
ArticleMapper.load_comments_for article

There are advantages and disadvantages to each. If you have to load each association explicitly, you see how much you're actually hitting the database. This allows you, for example, to trim up a Rails controller by culling some of the calls if you don't actually need them. However, for pages that are expected to have a lot of data on them, this could become ugly very quickly.

graemenelson commented 12 years ago

Thanks for writing this gem!

I wonder if you couldn't provide the ability for the Mapper to wrap the raw results from the database, then it could be up to the Mapper to handle the association behaviour which could be to load now or defer until later through a Lazy Load pattern.

Also, I am currently using this on a project where I am striving to use the DCI architecture, and it would be nice to have an instance of a Mapper instead of calling class methods. This would allow me to extend the mapper with particular roles based on the current context. I think you could still have class methods, like retrieve, find, etc... it would just create a default mapper under the hood... something like:

class ArticleMapper < Perpetuity::Mapper

  def self.default_mapper
     @@default_mapper ||= self.new()
  end

  def self.retrieve(...)
    self.default_mapper.retrieve(...)
  end

end

But in a DCI type architecture, I could do something like:

  mapper = ArticleMapper.new
  mapper.extend LoadArticlesWithLazyLoadingOfComments
  mapper.retrieve(...)
jgaskins commented 12 years ago

I agree, having a nontrivial class with nothing but class methods is a code smell. However, I'm not sure yet exactly the sort of interface I'd want in instantiated mappers and I'm open to suggestions.

I'm also not quite sure about DCI, as I have only a passing familiarity with it and I've never seriously used it.

You mentioned you're using Perpetuity in a project. I wouldn't really recommend that at the moment (at least not one that someone's paying for) since it's still very, very young and a lot of things aren't implemented. I think I've only done about two weeks of active development on it. It'll save and load objects, definitely, but you'd have to write some of your own methods for loading associated objects.

As far as wrapping the raw results for lazy loading goes, I'd actually quite like to do that. That way, if we end up getting this ported over to anything non-MongoDB, the results will be in a single format. That would make loading associations a lot easier and we'd then be able to implement a difference between associated and embedded objects more easily (since many NoSQL solutions allow embedding, but not joins, and 1 DB query is better than n+1).

graemenelson commented 12 years ago

I haven't given much thought on the design of the interface for instantiating mappers. I was going to take a deeper look at this code over the weekend. Maybe I will have some suggestions then ;)

As for the project I am working on, it's really simple with only three models and I wanted to be able to work with plain old ruby objects instead of ActiveRecord or DataMapper. I probably could find a way to use ActiveRecord or DataMapper but it seems overkill for my project.

jgaskins commented 12 years ago

Been thinking some more about the associations this weekend and so far, the best idea I could come up with was to delegate all of the association behavior to the DB layer within Perpetuity (such as Perpetuity::MongoDB).

Right now, the Mapper class micromanages too much and it depends entirely on MongoDB's implementation —specifically, the mongo gem. For example, the query criteria to the retrieve method is passed completely untouched to the DB layer. This was due to laziness on my part (but I'll use the "minimum viable product" excuse) and I'll fix it as soon as I get some free time and motivation.

Anyway, the idea is that the DB layer would know best how to persist associated objects and interpret these fields in a record/document. We have a second argument in the Mapper#attribute call that, for some reason or another, never gets used — I'm reasonably sure it's because I implemented it and then changed my mind. What we could do is tell the DB layer to save the object and if the attribute isn't serializable to the DB, it'll save the attribute's ID field as whatever type the DB uses for attributes (integers for SQL DBs, BSON::ObjectId for Mongo, etc).

Along with separating DB-specific logic, which is always an epic win and paves the way for adding other database types, this will make loading associated objects far easier. When you make whatever call we decide to use for loading them, it'll do something like this:

# Turns out I did implement this already, but it's an ugly call and the current implementation's bad
ArticleMapper.load_association! article, :author
# => article.author = UserMapper.find(article.author)

Since ArticleMapper tells us that the :author attribute is an instance of the User class (using our imagination), the value of article.author fresh out of the DB will be the ID of the User object, it makes it dead simple.

Additionally, this may be another good reason to instantiate mappers. This:

article_mapper = ArticleMapper.new
article = article_mapper.find(pretend_i_am_an_id)
article_mapper.load_associations! :author, :comments

seems better than this:

article = ArticleMapper.find(another_imaginary_id)
ArticleMapper.load_associations! article, :author, :comments
graemenelson commented 12 years ago

I started to sketch out some ideas, then realized that I started this project 2 weeks ago and it should probably already have a MVP. I found I was spending too much time mucking around with persistence code. I decided to move along and use mongo mapper for now, even though it's not a true Data Mapper pattern and it has more functionality than I need.

That being said, I think load_associations! on an instance looks cleaner to me than having the load_associations! as a class call. I was also going to look into ways to re-use an instance if possible, so as to limit the number of instances being generated.

I also notice that the attribute wasn't taking type when I was diving deeper into the code.

Some ideas I did have was to allow more settings on the attribute, like so:

attribute :articles, Array, {:mapper => ArticleMapper, :lazy => true}

Seemed like supplying the mapper would work, since the mapper knows what class it works with, such as Article in this case. The lazy attribute would allow you specify if the associate was loaded now or through a Lazy Load pattern.

Good luck on this project. I will revisit it after I finish my project ;)

jgaskins commented 12 years ago

@graemenelson Yeah, I didn't expect it to be production-ready yet. :-) I'm glad you gave it a shot, though. I've somehow gotten a little attention on this, so I might continue developing it if it's something people want to see.

graemenelson commented 12 years ago

I can see a real benefit to having a simple persistence framework that uses the Data Mapper pattern. I know that the data mapper framework is moving towards implementing the data mapper pattern, but I think it still tries to do too much. I also was unable to figure out when version 2.0 would be coming out.

By the way, this project already had an impact on cleaning up my current project. I really like the way you implemented the configure bits, so I borrowed the idea ;)