Pivotal Labs

Main menu

Skip to primary content
Skip to secondary content
  • About
  • Case Studies
  • Team
    • Executives
    • Locations
      • San Francisco (HQ)
      • Boston
      • Boulder
      • Denver
      • London
      • Los Angeles
      • New York
  • Community
    • Blogs
    • Tech Talks
    • Events
  • Careers
    • Lifestyle
    • Principles & Practices
    • Benefits
    • FAQ
    • Apply
  • Contact
    • Press Room
    • Press Releases
    • In The News
    • Press Kit
  • All
  • Labs
  • Standup
  • Tracker
Adam Milligan

Beware the frumious nested attribute

Adam Milligan
Sunday, September 20, 2009

Nested attribute assignment is one of the recent additions to Rails that made a great deal of sense, and made a lot of people happy. Chances are you’ve either used nested attribute assignment by now, or you worked on an older project that really could have used it. If you haven’t yet, check it out and see what you think.

Unfortunately, not all is well in Railstown. Nested attribute assignment is slick, and the related implementation of #fields_for makes it even slicker, but #fields_for can cause you some headaches if you’re not careful. Possibly if you are careful as well.

Consider a standard example of where you might want nested attribute assignment:

class CatLady < ActiveRecord::Base
  has_many :cats
  accepts_nested_attributes_for :cats

  def crazy?
    true
  end
end

And in your edit view:

<% form_for @cat_lady do |cat_lady_form| %>
  <table>
    <tbody>
      <% cat_lady_form.fields_for :cats do |cat_fields| %>
        <tr>
          <td><%= cat_fields.text_field :name %></td>
          <td><%= cat_fields.text_field :nickname %></td>
          <td><%= cat_fields.text_field :burial_preferences %></td>
        </tr>
      <% end %>
    </tbody>
  </table>
<% end %>

This seems fine, but when you look in more detail you discover that #fields_for will emit a hidden <input /> element for each cat associated to our cat lady. It does this at the point where you make the #fields_for call, much like the way #form_for emits the <form /> element. Unfortunately, that means that #fields_for emits the <input /> element as a sibling of the <tr /> element for the related cat; and thus, as a direct child of the <tbody /> element. Oops. The HTML standard doesn’t allow <tbody /> elements to have <input /> elements as children.

Most browsers won’t complain about this, but Safari 4 will (and so, I’d guess, will any other WebKit-based browser, like Chrome). Safari not only complains, it helpfully moves the <input /> element to a valid position. So, instead of this (you’ll have to imagine a bit; the markdown renderer for this blog is actually modifying my invalid HTML example to try to make it valid):

<table>
  <tbody>
    </tbody></table><input name="cat_lady[cats_attributes][0][id]" type="hidden" value="423" />
    <tr>
      <td><input name="cat_lady[cats_attributes][0][name]" type="text" /></td>
      ...
    </tr>
  </tbody>
</table>

you end up with this:

<input name="cat_lady[cats_attributes][0][id]" type="hidden" value="423" />
<table>
  <tbody>
    <tr>
      <td><input name="cat_lady[cats_attributes][0][name]" type="text" /></td>
      ...
    </tr>
  </tbody>
</table>

Seems innocuous enough, doesn’t it? After all, it’s still inside the form, so the browser will still submit the value along with everything else. However, the HTML that you sent to the browser is still invalid, and Safari still spits out the errors, which is probably not the best way to gain your users’ confidence. Also, any JavaScript you’ve written that depends on the DOM structure you lay out might fail, but only in some browsers (and not, for a change, only in IE!).

Now, someone will point out that you can solve this problem by not using tables. True, but that solution has two drawbacks: first, it’s entirely reasonable, even potentially very desirable, to use a table for this type of data; second, the hidden ID input will end up outside whatever container element you create for your nested model. This may not generate invalid HTML, but it may generate conceptually improper HTML. For instance, what if we change the above HTML to look like this:

<div class="menagerie">
  <input name="cat_lady[cats_attributes][0][id]" type="hidden" value="423" />
  <div class="cat">
    <input name="cat_lady[cats_attributes][0][name]" type="text" /></td>
    ...
  </div>
</div>

It doesn’t take too much imagination in the drag-and-drop Web 2.1 world to come up with some form of DOM manipulation that will dissociate the cat div from its associated ID element. And, of course, if the server receives the nested cat attributes without an ID it will helpfully make a new cat model. We don’t want this; crazy cat lady has enough cats already.

So, what to do?

We knocked around some ideas, and the most reasonable seems to be to add the capability to manually insert the hidden ID field (and, potentially, the hidden _destroy field) to the form builder object created by #fields_for. So, the #fields_for block from the edit form above would look something like this:

<% cat_lady_form.fields_for :cats, :omit_hidden_fields => true do |cat_fields| %>
  <tr>
    <%= cat_fields.hidden_fields %>
    <td><%= cat_fields.text_field :name %></td>
    <td><%= cat_fields.text_field :nickname %></td>
    <td><%= cat_fields.text_field :burial_preferences %></td>
  </tr>
<% end %>

It’s also possible to automatically determine if the block for each nested model called the #hidden_fields method, which would obviate the need for the explicit option; I haven’t decided if I like that approach.

I’m open to suggestions for better fixes, or tweaks to this one. In any case, look for a Rails patch for this some time in the coming week.

  • 0 Shares
  • Share on Facebook
  • Share on Twitter
Adam Milligan

Little, shiny robots

Adam Milligan
Saturday, August 29, 2009

One of my favorite computer games when I was growing up was Robot Odyssey; I imagine it will come as a surprise to no one that I was a nerdy kid. This article is a little bit of a tribute to that game, and the coolness of solving complex problems with a handful of simple concepts combined in clever ways.

Imagine you want to write a web-based game that involves robots. The robots in your game are a bit like the robots in Robot Odyssey: you program them with a list of simple instructions and when you turn them on they follow those instructions faithfully. Let’s say, for the sake of argument, that your robots can Walk Forward, Turn Left, Turn Right, Jump, and Beep.

You model each of the moves your Robot can make as an Action, and a Program as a list of Actions. Since each Robot can execute any number of Actions as part of its Program, and any number of Robots can execute Actions, you join Programs and Actions through the Instructions table. Like so:

class Robot < ActiveRecord::Base
  has_one :program
end

class Program < ActiveRecord::Base
  belongs_to :robot
  has_many :instructions
  has_many :actions, :through => :instructions
end

You build a whiz-bang interface for programming each Robot using JavaScript, which will PUT a collection of Action IDs to programs#update (each Robot creates a blank program on initialization, so no need for programs#create).

This seems pretty straightforward, doesn’t it? Unfortunately, it won’t work; the update will fail to reliably record the uploaded Actions. Here are some examples:

@program.action_ids  # []
@program.action_ids = [1, 4] # Forward, Jump

@program.action_ids # [1, 4]
@program.action_ids = [3, 1, 2, 5] # Right, Forward, Left, Beep

@program.action_ids # [3, 1, 2, 5]
@program.action_ids = [5, 5, 5] # Beep, Beep, Beep

@program.action_ids # [5]   ?????

What happened here? The problem lies with the way ActiveRecord collection associations update themselves. Here is the interesting code (slightly paraphrased) from association_collection.rb:

# Replace this collection with +other_array+
# This will perform a diff and delete/add only records that have changed.
def replace(other_array)
  ...
  delete(@target.select { |v| !other_array.include?(v) })
  concat(other_array.select { |v| !@target.include?(v) })
end

As you can see (or you could just read the comment), this only adds an element to a collection if that element isn’t already in the collection (and it acts similarly for deletion). Sadly, it doesn’t take into account how many times an element appears. So, above, when I tried set the action_ids to [5, 5, 5] it saw that the action_ids collection already contained that ID and moved on. If I had set the action_ids collection to [5, 5, 5] when it already contained [1, 4], the result would have been the expected [5, 5, 5].

Now, I’m on the fence with regard to whether I consider this a bug or just a somewhat inconsistent, but expected, behavior. To begin with, the fix is annoyingly nontrivial, and would potentially have a noticeable performance impact. Far more importantly, I’m not sure how often this might reasonably cause a real problem. In the case of your Robots game, you’d probably care quite a lot about what order your Robot executes its Actions in, so you’d likely have a position column, or something similar, on the instructions table. Given that, you’d probably update the Program by sending in a list of nested attributes which would create Instructions, each with the correct position and associated to the correct Action.

Even so, this is behavior worth knowing about. In the infinitude of possible update scenarios someone will want to update their HMT associations this way. I know I initially wrote code to do this as a temporary experiment, and ended up spending the rest of the day trying to figure out why my updates did the wrong thing a small percentage of the time.

  • 0 Shares
  • Share on Facebook
  • Share on Twitter
Joe Moore

Standup 06/23/2009: Multi-table Inheritance?

Joe Moore
Tuesday, June 23, 2009

Ask for Help

“Has anyone implemented mutli-table (class table) inheritance in Rails, as apposed to single table inheritance (STI)?”

  • There are some plugins that nobody has tried, such as inherits_from
  • What about a view to represent the super set of tables and rows?

“We’re looking for a dead-simple, drop-in JS rating or ‘starting’ plugin.”

Check out the start-rating jQuery plugin. Any other suggestions?

  • 0 Shares
  • Share on Facebook
  • Share on Twitter

Monkey Patch Du Jour

Alex Chaffee
Sunday, May 24, 2009

The following monkey patch gives a bit more information in the ActiveRecord SQL logs. Instead of just saying “User Load” it also says the file and line number in your code that asked AR to perform the operation. That way you can have a hope of tracking it down and optimizing it away if at all possible.

  User Load (0.2ms) views/main_page.rb:107:in `filters_box'   SELECT * FROM `users`

Code after the jump. I guess you put it in environment.rb with all the other monkeys.

    module ActiveRecord
      module ConnectionAdapters
        class AbstractAdapter
          def log_info(sql, name, ms)
            if @logger && @logger.debug?
              c = caller.detect{|line| line !~ /(activerecord|active_support|__DELEGATION__)/i}
              c.gsub!("#{File.expand_path(File.dirname(RAILS_ROOT))}/", '') if defined?(RAILS_ROOT)
              name = '%s (%.1fms) %s' % [name || 'SQL', ms, c]
              @logger.debug(format_log_entry(name, sql.squeeze(' ')))
            end
          end
        end
      end
    end
  • 0 Shares
  • Share on Facebook
  • Share on Twitter
Adam Milligan

Refactoring a dead horse

Adam Milligan
Sunday, May 10, 2009

A while back I made the point that the HasOneThroughAssociation class in Rails shouldn’t be a subclass of HasManyThroughAssociation. I also submitted a Rails patch in which I changed the superclass of HasOneThroughAssociation from HasManyThroughAssociation to HasOneAssociation and moved the shared Through functionality into a module. Despite support from the teeming millions, Rails core team member Pratik rejected the patch for being “just a refactoring.”

Despondent, I brought the issue up here at Pivotal a few months later, after the Release of Rails 2.3. Nate added a callout for support for the ticket to the daily Pivotal standup blog. The response was heartwarming (thanks to all who added a +1), but resulted only in Pratik removing himself from the ticket (I assume so he would stop getting comment notifications).

All is not lost, however. At RailsConf last week, Jeff Dean — Pivot, raconteur, international playboy, and refactoring paladin — called out the Rails core team regarding their stance on refactorings. The response: bullish (sort of).

Taking into account the responses from the Rails core team, I’ve done the following:

  • Recreated the patch for current Rails edge. Fortunately, I had to make only one small change to the patch from six months ago.
  • Ran a search on GitHub for any code using the HasOneThroughAssociation class (thanks to Jeff for the idea). As far as I can tell no code outside of Rails depends on the implementation of that class.
  • Added Jeremy Kemper as the owner of the Rails ticket. He was the most immediately supportive of refactoring patches, so hopefully he’ll advocate for this one.

We’ll see what happens.

  • 0 Shares
  • Share on Facebook
  • Share on Twitter
Joe Moore

DRY, Targeted, and Reusable Testing of ActiveRecord Extensions

Joe Moore
Wednesday, March 18, 2009

At Pivotal, we are passionate about test driven development, keeping things DRY, and writing readable and understandable code. Satisfying all of these desires can be challenging, especially when writing test code. In particular, ActiveRecord extensions present several challenges: which models using an extension should we test? How do we both test our extension in isolation while also testing all model’s usage of that extension? Is it even worth it?

The answer is yes, it is worth it, and it’s also fairly easy, readable, understandable, and DRY. I will present both a common problem and a solution, using a cumulation of technologies and techniques from multiple Pivotal projects, in particular using acts_as_fu to create laser-targeted, isolated, and disposable ActiveRecord models for testing extensions and RSpec shared behaviors to minimize the amount of duplicated test code.

Often we find common patterns in ActiveRecord models and we wish to share that functionality by mixing in a module of shared code, or even mixing that module in to ActiveRecord::Base itself. How should we go about testing these ActiveRecord extensions? Not only do we want to test the extension, but also test that models using that extension are doing so properly. We’ve blogged about dynamically creating ActiveRecords to test extensions in the past, but Pivot Pat Nakajima’s acts_as_fu plugin is a far better tool for this.

The Setup

Let’s say we have three models: Pivotal, Lab, and Pivot. Both a Pivot and a Lab belongs_to Pivotal, have a name, nickname, some common validation, etc:

# app/models/pivotal.rb
class Pivotal < ActiveRecord::Base
end

# app/models/lab.rb
class Lab < ActiveRecord::Base
  RANDOM_NICKNAMES = ["New Hotness", "LOL-Cat Factory", "Tweet Machine"]
  belongs_to :pivotal
  validates_presence_of :name

  def nickname
    "#{self.name}, 'The #{RANDOM_NICKNAMES[rand(RANDOM_NICKNAMES.length)]}'"
  end
end

# app/models/pivot.rb
class Pivot < ActiveRecord::Base
  RANDOM_NICKNAMES = ["New Hotness", "LOL-Cat Factory", "Tweet Machine"]
  belongs_to :pivotal
  validates_presence_of :name

  def nickname
    "#{self.name}, 'The #{RANDOM_NICKNAMES[rand(RANDOM_NICKNAMES.length)]}'"
  end
end

The specs for Lab and Pivot might look like the following:

# spec/models/lab_spec.rb
describe Lab do
  before(:each) do
    @lab = Lab.new
  end

  it "should require name" do
    @lab.should have(1).errors_on(:name)
    @lab.name = 'Stealth Startups'
    @lab.should have(0).errors_on(:name)
  end

  it "should generate a random nickname" do
    @lab.name = 'Stealth Starup'
    @lab.nickname.should_not be_blank
    @lab.nickname.should include(@lab.name + ", 'The ")
  end

  it "should belong to Pivotal" do
    @lab.should respond_to(:pivotal)
    @lab.should respond_to(:pivotal=)
    @lab.should respond_to(:pivotal_id)
    @lab.should respond_to(:pivotal_id=)
  end
end

# spec/models/pivot_spec.rb
describe Pivot do
  before do
    @pivot = Pivot.new
  end

  it "should require name" do
    @pivot.should have(1).errors_on(:name)
    @pivot.name = 'Joe Moore'
    @pivot.should have(0).errors_on(:name)
  end

  it "should generate a random nickname" do
    @pivot.name = 'Joe Moore'
    @pivot.nickname.should_not be_blank
    @pivot.nickname.should include(@pivot.name + ", 'The ")
  end

  it "should belong to Pivotal" do
    @pivot.should respond_to(:pivotal)
    @pivot.should respond_to(:pivotal=)
    @pivot.should respond_to(:pivotal_id)
    @lab.should respond_to(:pivotal_id)
  end
end

DRYing Up the Models

Yuck, look at all that duplication! Let’s start eliminating it by pulling the common model code into an ActiveRecord extension named belongs_to_pivotal:

# lib/belongs_to_pivotal.rb
module BelongsToPivotal
  RANDOM_NICKNAMES = ["New Hotness", "LOL-Cat Factory", "Tweet Machine"]
  module ClassMethods
    def belongs_to_pivotal
      belongs_to :pivotal
      validates_presence_of :name

      instance_eval do
        include BelongsToPivotalInstanceMethods
      end
    end
  end

  module BelongsToPivotalInstanceMethods
    def nickname
      "#{self.name}, 'The #{BelongsToPivotal::RANDOM_NICKNAMES[rand(BelongsToPivotal::RANDOM_NICKNAMES.length)]}'"
    end
  end

  def self.included(base)
    base.extend(ClassMethods)
  end
end

Now our Models look like this:

# app/models/lab.rb
class Lab < ActiveRecord::Base
  belongs_to_pivotal
end

# app/models/pivot.rb
class Pivot < ActiveRecord::Base
  belongs_to_pivotal
end

You’ll need to add “ActiveRecord::Base.send :include, BelongsToPivotal” to config/initializers/new_rails_defaults.rb or some other initializer.

Testing the Extension with acts_as_fu

The models are looking better, but what about the specs? In the “old days” I would create a spec named belongs_to_pivotal_spec.rb and use one of the two Models in that spec. But, when you do that, you get all the the “baggage” from that Model, such as any other methods, associations, inherited methods and properties, etc. Let’s use acts_as_fu to write a spec that tests BelongsToPivotal in isolation.

# spec/lib/belongs_to_pivotal_spec.rb
describe BelongsToPivotal do
  before(:all) do
    # Using acts_as_fu to create a model specifically for our extension
    build_model :belongs_to_pivotal_models do
      # we will need these columns in the database
      string :name
      integer :pivotal_id

      # Call our extension here
      belongs_to_pivotal
    end
  end

  before(:each) do
    @pivotal_model = BelongsToPivotalModel.new
  end

  # Look, it's all of the model specs!
  it "should require name" do
    @pivotal_model.should have(1).errors_on(:name)
    @pivotal_model.name = 'Pivotal Model'
    @pivotal_model.should have(0).errors_on(:name)
  end

  it "should generate a random nickname" do
    @pivotal_model.name = 'Pivotal Model'
    @pivotal_model.nickname.should_not be_blank
    @pivotal_model.nickname.should include(@pivotal_model.name + ", 'The ")
  end

  it "should belong to Pivotal" do
    @pivotal_model.should respond_to(:pivotal)
    @pivotal_model.should respond_to(:pivotal=)
    @pivotal_model.should respond_to(:pivotal_id)
    @pivotal_model.should respond_to(:pivotal_id=)
  end
end

Now that our ActiveRecord extension is well tested, how do we make sure that our two models are actually using it? One technique is to check that each model responds to the specific methods added by our extension:

#spec/models/lab_spec.rb
describe Lab do
  ...
  it "should belong_to_pivotal" do
    @lab.should respond_to(:pivotal)
    @lab.should respond_to(:pivotal=)
    @lab.should respond_to(:pivotal_id)
    @lab.should respond_to(:pivotal_id=)
    @lab.should respond_to(:name)
    @lab.should respond_to(:name=)
    @lab.should respond_to(:nickname)
  end
  ...

This does not feel very satisfying. We are duplicating some of the tests from belongs_to_pivotal_spec.rb and not verifying that we are getting the validations. A crazy coincidence could result in these methods all being defined without actually using our extension.

Another technique, though some would call it a hack, is to provide a hook within the extension itself so we can check for it later:

# lib/belongs_to_pivotal.rb
module BelongsToPivotal
  ...
  module ClassMethods
    ...
    # We can check this to see if a model uses this extension
    def belongs_to_pivotal?
      self.included_modules.include?(BelongsToPivotalInstanceMethods)
    end
  end
  ...
end

Let’s update belongs_to_pivotal_spec.rb to test this method:

# spec/lib/belongs_to_pivotal_spec.rb
describe BelongsToPivotal do
  before(:all) do
    # Using acts_as_fu to create a model specifically for our extension
    build_model :belongs_to_pivotal_models do
      ...
    end

    # Create a model that does not use our extension
    build_model :never_belongs_to_pivotal_models do
      # do nothing
    end
  end
  ...
  it "should know if it belongs_to_pivotal" do
    BelongsToPivotalModel.belongs_to_pivotal?.should be_true
    NeverBelongsToPivotalModel.belongs_to_pivotal?.should be_false
  end
end

#spec/models/lab_spec.rb
describe Lab do
  it "should belong_to_pivotal" do
    Lab.belongs_to_pivotal?.should be_true
  end
end

#spec/models/pivot_spec.rb
describe Pivot do
  it "should belong to Pivotal" do
    Pivot.belongs_to_pivotal?.should be_true
  end
end

Using RSpec Shared Behaviors

How much further can we go? Notice that our two Model specs are 5 whole lines long! Unacceptable! All kidding aside, we can DRY this up just a bit more by using RSpec’s shared behaviors.

In spec/spec_helper.rb

# spec/spec_helper.rb
describe 'it belongs to pivotal', :shared => true do
  it "should belongs_to_pivotal" do
    described_class.belongs_to_pivotal?.should be_true
  end
end

Now we can use this shared behavior in our specs:

#spec/models/lab_spec.rb
describe Lab do
  it_should_behave_like "it belongs to pivotal"
end

#spec/models/pivot_spec.rb
describe Pivot do
  it_should_behave_like "it belongs to pivotal"
end

I hope that these techniques are helpful. Feel free to post your own!

  • 0 Shares
  • Share on Facebook
  • Share on Twitter
Joe Moore

Standup 1/27/2009: Nested Model Forms Soon

Joe Moore
Tuesday, January 27, 2009

Interesting Things

  • application.rb vs. application_controller.rb: As we all know, ApplicationController breaks with Rails convention and lives in the application.rb file, not application_controller.rb. Be careful if you create an application_controller.rb file of your own, as this can confuse Rails class loading and might result in Rails deciding not to load application.rb.

  • Google Webmaster Tools: Note that if you are using Google Webmaster Tools that statistics are different for www.example.com and example.com (sans www).

  • Nested Model Forms are coming in Rails 2.3! There is even a patch in progress.

  • 0 Shares
  • Share on Facebook
  • Share on Twitter
Adam Milligan

Keeping your errors in line

Adam Milligan
Saturday, December 27, 2008

How many times has this happened to you? You get a cool design for your website, and you spend a bunch of time lining up all of your images and roundy-corner widgets and input boxes just… so…, and everything looks great. But, then you submit a form without typing in your favorite ice cream (a required field, of course), and suddenly your layout is splattered about like an extra large scoop of rocky road in the hands of a two year old. It’s enough to make you want to stab your eyes out with a hedge trimmer.

The reason is, of course, that ActionView automagically wraps a div around any input element for an ActiveRecord attribute with validation errors. This div has the class ‘fieldWithErrors’, which makes it all red and nasty looking to tell your wayward user that they need to cough up some more information.

Now, input elements are inline elements, so they don’t break page flow. At the same time, div elements are block elements, so they break page flow and use up as much width as is available to them. When ActionView renders a form for your ActiveRecord object with errors, it alters the HTML structure that you’ve so painstakingly laid out, inserting these disruptive divs where previously there were only gentle, friendly input elements. Not to mention that it might be inserting these div elements inside other HTML elements that, according to the standard, mustn’t contain div elements. BOOM. Browser-specific nastiness all over the place.

So, a fellow by the name of Alex MacCaw opened a Rails ticket with a patch to change this fieldWithErrors div element to a span. Span elements are, of course, the inline version of div elements.

Based on some of the feedback, and the fact that ActionView modifying the HTML structure of my views annoys me, I created a somewhat more invasive patch for this ticket.

I changed ActionView to not add an any elements to the HTML structure, but to add the ‘fieldWithErrors’ class directly onto the input element for the attribute with errors.

I also added a #field_error_options class accessor on ActionView::Base, with which you can customize the HTML attributes that ActionView adds to these inputs. The value of #field_error_options defaults to

{ :class => 'fieldWithErrors' }

This will combine with any class option you’ve specified on your tag builder, so

<%= text_field @foo, :bar, :class => 'wibble' %>

will generate

<input type='text' name='foo[bar]' class='fieldWithErrors wibble' />

when the bar field fails validation on @foo.

ActionView::Base still has the #field_error_proc class accessor, so you can still add HTML structure to input elements for invalid attributes if you so choose. However, I changed this to default to nil.

Check it out, see what you think. And keep away from the hedge trimmer.

  • 0 Shares
  • Share on Facebook
  • Share on Twitter
Adam Milligan

has_one-ish :through

Adam Milligan
Saturday, December 27, 2008

Rails has had the has_one :through association for a while now, and you probably use it on occasion. But, has it ever given you the heebie-jeebies a little bit? Maybe something happens now and then that doesn’t seem quite right? Well, the reason for this is that HasOneThroughAssociation, the class that ActiveRecord uses (shockingly) to implement has_one :through associations, is a subclass of HasManyThroughAssociation.

Whhaaaaaaaaaat?

Yes, oddly enough, a has_one :through association is a collection that is special-cased to always return just one element. Seems kind of dirty, doesn’t it?

I have two, related, problems with this inheritance relationship. First, it violates Liskov substitutability. One cannot in any way argue that a has_one :through association IS A has_many :through association; substituting one for the other would not be likely to maintain a program’s correctness. Second, the inheritance relationship exists to share implementation, not interface. Among others, the Gang of Four have described, persuasively, why this is a terrible idea.

More practically, I’ve now run into two serious bugs caused by this relationship. The first one caused newly created has_one :through associations to return an empty array rather than nil. I fixed that here, with the help of wunderkind David Stevenson. The second I ran into in the last few days while working on this patch, which I’ve written about here. It turns out that trying to get #method_missing to behave sensibly for all non-collection associations is difficult when one of those associations inherits a pile of collection-specific functionality.

So, in order to make my #method_missing patch work I had to rewrite HasOneThroughAssociation with a more appropriate superclass (I used HasOneAssociation). You can find that patch here. Hopefully this change will make its way into the Rails codebase and will make lives easier for generations to come.

  • 0 Shares
  • Share on Facebook
  • Share on Twitter
Adam Milligan

#method_missing makes me eat my words

Adam Milligan
Saturday, December 27, 2008

A while back I wrote about private methods in ActiveRecord objects, and how Rails 2.2 makes them behave as they should. ActiveRecord associations will no longer respond to private methods defined on their targets; however, my colleague Joseph pointed out that they also no longer respond to methods defined via #method_missing on their targets. Which sucks horse poop through a straw, to some extent.

In my original post I wrote this, fully aware that I was writing lies, but mostly in a rush to get on to my point at the time:

Sometimes I’m forced to use #send:

method_name = extract_method_name_from_the_aether
some_object.send(method_name)

In order to make this code correct with regard to access control I have to add cruft:

method_name = extract_method_name_from_the_aether
some_object.send(method_name) if some_object.respond_to?(method_name)

The astute reader will note that the second code block doesn’t actually approximate the behavior of calling a private method via function call syntax. It should look more like this:

method_name = extract_method_name_from_the_aether
raise NoMethodError if some_object.private_methods.include?(method_name)
some_object.send(method_name)

Two things to notice here: first, the original code didn’t throw an exception if it failed to call the method; second, and much more importantly, the set of private methods is not the complement of the set of methods an object will respond to. So, using !#respond_to? as the condition for preventing the method call is too restrictive. The fact that this code will prevent calling a non-private method defined via #method_missing clearly shows this.

Now, I know the Rails core team chose to use #respond_to? in this particular case for a good reason (which my original patch did not take into consideration): performance. Some simple investigation quickly shows that #include? is an order of magnitude slower than #respond_to? Method calls happen quite a lot, so slow is bad.

I’ve submitted another patch that should correct the #method_missing behavior without dragging everything to a halt. Building on my previous examples, the code looks something like this:

method_name = extract_method_name_from_the_aether
raise NoMethodError if !some_object.respond_to?(method_name) && some_object.private_methods.include?(method_name)
some_object.send(method_name)

Note that the conditional expression is now redundant; the first condition cannot be false if the second condition is true. However, an object will likely respond to the majority of method calls sent to it. The first conditions will (quickly) evaluate to false in these cases, short-circuiting the conditional. In the minority of cases where the first condition evaluates to true, the second condition will (slowly, but correctly) determine if the method call violates access control.

Unfortunately, sensible as it may seem, this change actually breaks has_one :through associations, because of collection-specific functionality they inherit from has_many :through associations. (Oh yes, has_one :through associations are collections, based on their place in the ActiveRecord inheritance hierarchy. Or, they were; read more about that here).

And, finally, I think it’s important to note that none of these machinations in Rails would be necessary if #send (and #send!) actually worked as it should. Heads up, Matz.

  • 0 Shares
  • Share on Facebook
  • Share on Twitter

Topics

  • agile (778)
  • rails (113)
  • testing (87)
  • ruby (83)
  • ruby on rails (70)
  • jobs (62)
  • javascript (54)
  • techtalk (44)
  • rspec (38)
  • activerecord (29)
  • productivity (29)
  • gogaruco (29)
  • ironblogger (29)
  • git (28)
  • nyc (27)
  • rubymine (25)
  • bloggerdome (22)
  • mobile (22)
  • cucumber (20)
  • process (19)
  • pivotal tracker (19)
  • jasmine (19)
  • design (18)
  • ios (18)
  • webos (17)
  • objective-c (17)
  • android (16)
  • palm (16)
  • "soft" ware (16)
  • fun (15)
  • tracker ecosystem (15)
  • ci (15)
  • cedar (15)
  • rails3 (14)
  • performance (14)
  • bdd (14)
  • gem (13)
  • tdd (13)
  • selenium (12)
  • css (12)
  • goruco (12)
  • bundler (12)
  • meetup (11)
  • railsconf (11)
  • nyc-standup (11)
  • capybara (10)
  • mac (10)
  • mojo (10)
  • chef (10)
  • api (10)
Subscribe to activerecord Feed
  1. ←
  2. 1
  3. 2
  4. 3
  5. →
  • About
  • Case Studies
  • Team
  • Community
  • Careers
  • Contact
  • Labs
  • Events

Contact Us

contact@pivotallabs.com
+1 415-77-PIVOT
TwitterLinkedInFacebook

Pivotal Tracker

Tracker is the award-winning agile project management tool that enables real-time collaboration around a shared, prioritized backlog.
Visit pivotaltracker.com >