views:

328

answers:

4

I'm creating a wiki. Each Article has_many Revisions, and an Article belongs_to a single current_revision. So in the database, Articles have a single reference to a Revision's id, and Revisions each have a single reference to the Article they belong to. Before I continue, does this seem like a sane way to do things? It strikes me as fairly unorthodox, but logical, and I'm not sure how others in similar situations set things up.

The trouble is that this type of mutual belongs_to relationship seems to really throw Rails off when creating the models. When I first create an Article, I'd like to also create an initial Revision to go with it.

I added a before_create method and did something like:

initial_revision = self.revisions.build
self.current_revision = initial_revision

but this would cause a stack overflow on save, as Rails apparently tries in a loop to first save the Article, so it has an article_id to stick in the Revision, and then first save the Revision, so it has a current_revision_id to stick in the Article.

When I break things up, and don't create them simultaneously (but still in a transaction), the first one created doesn't get its reference set. For example:

initial_revision = Revisions.create
self.current_revision = initial_revision
initial_revision.article = self

would leave the revision with a null article_id as it missed the save.

I think I could get around this by calling an after_create method as well, just to initialize the variable with an update and save, but this is turning into a gigantic mess, and I feel like in Rails that usually means I'm doing something wrong-headedly.

Can anyone help, or am I stuck creating a little after_create method that saves in the change?

+2  A: 

I has similar problem recently. You need to declare only one way of association. Can your Article be created without Revision, and then Revision added to existing Article?

Or can you point from Article to Revision which is not pointing back? If that should be not possible, then you need to declare Revision as belongs_to :article, and Article :has_many :revisions and has_one :revision, :conditions => { ... }. And add flag 'main revision' to revision model or get last revision by date.

This way you don't provide cyclic dependencies, so it should be easier.

Edit:
This is how I tested it and make it work:

class Article < ActiveRecord::Base
  has_many :revisions
  has_one :current_revision, :class_name => "Revision", :conditions => { :tag => "current" }

  before_validation do |article|
    # add current revision to list of all revisions, and mark first revision as current unless one is marked as current
    article.current_revision = article.revisions.first unless article.current_revision.present?
    article.revisions << article.current_revision if article.current_revision.present? and not article.revisions.member?(article.current_revision)
  end

  after_save do |article|
    article.current_revision.mark_as_current if article.current_revision.present?
  end
end

class Revision < ActiveRecord::Base
  belongs_to :article

  def mark_as_current
    Revision.update_all("tag = ''", :article_id => self.article_id)
    self.tag = "current"
    save!
  end

end

And this is how it works now (dump from script/console):

$ ./script/console
Loading development environment (Rails 2.3.5)
>> a1 = Article.new :name => "A1"
>> a1.revisions.build :number => 1
>> a1.save
>> a1.reload
>> a1.revisions
+----+------------+--------+---------+-------------------------+-------------------------+
| id | article_id | number | tag     | created_at              | updated_at              |
+----+------------+--------+---------+-------------------------+-------------------------+
| 1  | 1          | 1      | current | 2010-02-03 19:10:37 UTC | 2010-02-03 19:10:37 UTC |
+----+------------+--------+---------+-------------------------+-------------------------+
>> a1.current_revision
+----+------------+--------+---------+-------------------------+-------------------------+
| id | article_id | number | tag     | created_at              | updated_at              |
+----+------------+--------+---------+-------------------------+-------------------------+
| 1  | 1          | 1      | current | 2010-02-03 19:10:37 UTC | 2010-02-03 19:10:37 UTC |
+----+------------+--------+---------+-------------------------+-------------------------+
>> a1r2 = a1.revisions.build :number => 2
+------------+--------+-----+------------+------------+
| article_id | number | tag | created_at | updated_at |
+------------+--------+-----+------------+------------+
| 1          | 2      |     |            |            |
+------------+--------+-----+------------+------------+
>> a1r2.mark_as_current
>> a1.revisions
+----+------------+--------+---------+-------------------------+-------------------------+
| id | article_id | number | tag     | created_at              | updated_at              |
+----+------------+--------+---------+-------------------------+-------------------------+
| 1  | 1          | 1      | current | 2010-02-03 19:10:37 UTC | 2010-02-03 19:10:37 UTC |
| 2  | 1          | 2      | current | 2010-02-03 19:11:44 UTC | 2010-02-03 19:11:44 UTC |
+----+------------+--------+---------+-------------------------+-------------------------+
>> a1.revisions.reload
+----+------------+--------+---------+-------------------------+-------------------------+
| id | article_id | number | tag     | created_at              | updated_at              |
+----+------------+--------+---------+-------------------------+-------------------------+
| 1  | 1          | 1      |         | 2010-02-03 19:10:37 UTC | 2010-02-03 19:10:37 UTC |
| 2  | 1          | 2      | current | 2010-02-03 19:11:44 UTC | 2010-02-03 19:11:44 UTC |
+----+------------+--------+---------+-------------------------+-------------------------+
>> a1.current_revision
+----+------------+--------+---------+-------------------------+-------------------------+
| id | article_id | number | tag     | created_at              | updated_at              |
+----+------------+--------+---------+-------------------------+-------------------------+
| 1  | 1          | 1      | current | 2010-02-03 19:10:37 UTC | 2010-02-03 19:10:37 UTC |
+----+------------+--------+---------+-------------------------+-------------------------+
>> a1.reload
>> a1.current_revision
+----+------------+--------+---------+-------------------------+-------------------------+
| id | article_id | number | tag     | created_at              | updated_at              |
+----+------------+--------+---------+-------------------------+-------------------------+
| 2  | 1          | 2      | current | 2010-02-03 19:11:44 UTC | 2010-02-03 19:11:44 UTC |
+----+------------+--------+---------+-------------------------+-------------------------+

Watch for problem with two revisions marked as current before reload revisions collection on article. When you mark one of revisions as current, then you need to reload you whole article object (if you want to use current_revision field) or only revision collection.

And you should probably treat current_revision only as a read-only pointer. If you try to assign another revision to it, then you'll loose previous revision which was pointed by article as current (Rails will remove old referenced object, because of has_one).

MBO
Creating my Article without Revision, and then using an after_create to add the Revision to the existing Article was another approach I tried, but then unless I called save again on the Article in after_create, there was no way to update my current_revision_id, so it was the same ugly code but mirrored.
WIlliam Jones
Unfortunately, I do need bidirectional pointing. The flag for the main revision in the Revision model is a good idea, though, and would seem to solve my problem. Do you think that's a conceptually better approach than using an after_create method just to set/save the variable? I just tried the after_create method and it does work, it's just ugly.The advantage of the after_create method is that then I don't need to be modifying Revisions again after they are created and then initialized.
WIlliam Jones
@williamjones You still have bidirectional pointing in Ruby, that's why you have `has_one/many` and `belongs_to`. You only have unidirectional pointing in database. I try to answer rest of your comments with update in few minutes
MBO
+2  A: 

A Revision is just a version of an Article, right? There's a great Railscast on Model Versioning using the vestal_versions gem which should solve your problem.

John Topley
Not exactly, the Revision stores all the information about the Article, whereas the Article is just an unchanging central hub by which to find Revisions. Still, thank you for the tip on the gem, I'll study that to see if it's better than my current approach.
WIlliam Jones
+2  A: 

I think the best way to have it is to have each Revision belong to an Article. Instead of the cyclical association of each Article belonging to a Revision (Current). Use a has_one relationship to link an article to the latest revision.

class Revision < ActiveRecord::Base
  belongs_to :article
  ...
end

class Article < ActiveRecord::Base
  has_many :revisions
  has_one :current_revision, :order => "version_number DESC"
  ...
end

However in the event of a rollback, you'll have increase the version number of the revision rolled back to.

Also... you can eliminate the version_number field and just order on id if a.version_number > b.version_number and only if a.id > b.id. Which means that rollbacks will result in cloned records with higher ids than the last version.

EmFi
+1  A: 

I've had the same problem in my own app, and although my structure is slightly different I've finally found a solution.

In my app I have something more like this:

class Author < ActiveRecord::Base
  has_many :articles
  has_many :revisions
end

class Article < ActiveRecord::Base
  has_many :revisions
  belongs_to :author
end

class Revision < ActiveRecord::Base
  belongs_to :article
  belongs_to :author
end

So I have a 3-model loop instead.

In my case, I want to save the whole hierarchy (from new) at once. I've found that I can do this by creating a new author, then adding the articles to the author as normal, but when I want to create the revisions, I do it like this (from within the Author class):

def add_new_revision(@author)
  article.revisions = article.revisions.push(Revision.new(:author => @author))
end

(Note that here @author hasn't been saved yet either)

Somehow this works. I've noticed that in the logs, activerecord is inserting the revision after the author and the article have been saved (just like using the after_create handler). I'm not sure why this is treated differently to doing build, but it seems to work (although I wouldn't be surprised if it didn't work for anyone else!)

Anyway, I hope that helps! (Sorry it's so long after you posted the question!)

tensaiji