Rails 2.3.5 Bug With accepts_nested_attributes_for

The Setup

At work, our application has a series of questions it asks users, and an administrator can set a target of what he thinks (or wants) the answer to be.

class Question < ActiveRecord::Base
  has_many :targets
  accepts_nested_attributes_for :targets, :allow_destroy => true
end

class Target < ActiveRecord::Base
  belongs_to :question
end

The administrator fills the form out, choosing a target or not, and Rails will save, update, or destroy it as normal. Well, almost. Apparently there is a bug in Rails 2.3.5 that will save the child model twice, and we're experiencing that now.

What I Want To Do

Upgrade to Rails 3.1, get all the new hotness and bug fixes, and not worry about this problem.

What I'm Going To Do

Since we don't have the resources to upgrade Rails (yet...), we need a workaround. Here's what we came up with.

after_save :ensure_one_target_while_in_draft

def ensure_one_target_while_in_draft
  if draft? && targets.count > 1
    targets[1..-1].each { |t| t.destroy }
  end
end

This problem only comes up when a question has not been activated and is still in draft mode. Once it's activated, the administrator cannot change the target but only add additional ones. So while in that state, and if there are multiple targets, destroy all but the first. This needs to be in an after_save callback because the double-save does not happen until after the question is saved. It's not elegant, but it works. I don't want to try to patch Rails and remember to deal with that later when we finally do upgrade, and the situation is isolated enough that there isn't a performance hit and it's easy to remove once it's no longer necessary. Elegant? No. But it's a bug fix for the framework, it's well documented in the code, and it works.

Rails Security Refactor: Protect Those Attributes!

Rails in the enterprise is still a fairly new concept, but the same web development principles we have also exist in this new realm. One no-brainer is security, and one no-brainer part of security is protecting data from malicious user input.

The basic Rails ways to protect attributes on a model are attr_protected and attr_accessible. If neither are set, it's easy to imagine a situation where a user updates his attributes and the corresponding model has a boolean admin field on it. The user can trivially submit post data that looks like this:

params: { "user" => { :email => "foo@bar.com", :first_name => "Joe", :last_name => "Hacker", :admin => "true" } }

Oops! Now Mr. Hacker is Mr. Admin!

So how do we get from here to (more) secure? Throwing attr_accessible on the model to whitelist is the safest, but it can cause a lot of unknown breakage if there aren't tests around the fields, which there probably aren't because why would someone test the fields for accessibility if they are automatically accessible? An interim step is to create a blacklist using attr_protected to only protect specified fields, get tests around these protected fields, and then upgrade to attr_accessible.

For our user model, let's protect that admin field:

Class User < ActiveRecord::Base
  attr_protected :admin
end

And the tests:

describe User do
  describe "with protected fields" do
    context "including admin" do
      let(:user) { Factory.build(:user, :admin => false) }

      it "cannot mass-update" do
        user.update_attributes({ :admin => true })
        user.admin.should be_false
      end
    end
  end  
end

We instantiate a user object with admin set to false, try to update that field, and ensure that it did not get updated. If we use user.update_attribute(:admin, true), the test would fail because that skips all the ActiveRecord protection, so we use user.update_attributes(). Doing this for all the fields we want to protect will eventually get us to the point where we can swap out attr_protected with the easier-to-deal-with attr_accessible. Since we need to be explicit with attr_protected, it can get to be difficult to maintain quickly since we need to remember to add each new field we want to protect to the list and test it.

Class User < ActiveRecord::Base
  # attr_protected :id, :admin, :awesomeness_rating, :money
  attr_accessible :email, :first_name, :last_name
end

That's better. Tests and iterative development to the rescue!

Our Interview Process

Our Hiring Process

At Crowdcast, we're currently hiring developers, and through much practice we have come up with an interview process that maximizes our chances of finding a candidate who will be a good fit.

There are a few steps that aren't always in the same order, and we may even skip some depending on the candidate.

The Phone Screen

I'm not involved in this first step. The engineering manager will talk to a candidate initially to find out if there are any immediate red flags and to clarify his experience. If nothing strange happens (believe me, there have been strange happenings), we'll schedule the candidate to come in or give him a preliminary coding test.

The Rails Test

We send the client a small Rails coding project that should only take a few hours. We'd like him to create a Rails application with some basic functionality and minimal markup and styling (design is a bonus but not a requirement), put the project on his Github account, and send us a url to the code. This is a Fizzbuzz-style question for the Rails framework, and it also implies that he has a Github account. You do have one, right? If the candidate can already show us Rails code, we may skip this step and just bring him in to the office.

The Onsite Interview

We then bring the candidate in for a few interview rounds. I'll ask a candidate about previous experience and what he enjoys working on to establish a little history and familiarity and to decode what's written on his resume. I'll next go through a technical screening about Ruby, some basic questions and some slightly-less basic questions, and I'll let the candidate explain as much or little as he'd like. A good one will definitely have a lot of say for some of the answers, and a short reply is usually an indication of minimal experience with the subject matter.

Next, we'll get into some Rails-specific questions, both methods and techniques. It's certainly not necessary to know everything about the framework, especially given its development speed, but there are some core ideas that get used often that he should be familiar with. If the candidate can teach me something, that makes me very happy.

The Pair Programming Exercise Part I

A new wrinkle we've introduced. Since I'll be pair programming with someone, I'd like to know how he thinks and how we'll work together. We'll sit at one computer and, with the candidate driving, work through a small Ruby problem. We'll do TDD (we use Rspec but that's not a requirement). The problem should take 20 - 40 minutes and leaves a lot of room to delve into interesting design issues and refactorings. This is the fun part.

The Pair Programming Exercise Part II

The last step is to invite the candidate back to pair program on real code for a few hours. We need to make sure we're a fit for each other, and this gives him an opportunity to engage our code base and be part of the team. This is not just for me but for everyone in the office, and of course the candidate, to assess the interactions.

Post Mortem

Does any interview process find the perfect candidate? Not that I've discovered. We can't go as in-depth as Hashrocket, but we can get partially there with the multiple pairing exercises and full team interaction. It's so incredibly important to get along with coworkers since we spend as much time at work as sleeping (actually, it's probably more time at work), so just having the technical chops is not enough. We hope that our process gives us the knowledge to make the right decisions for everyone involved.

Testing controller JSON responses in Rspec

I was recently rewriting some controller specs because they were way too heavy: all the models were saved to the database and there was no mocking. While trying to test the JSON response of an action, I got the following exception:

ActiveSupport::JSON::Encoding::CircularReferenceError

Here is the relevant controller code:

format.json do
  render :json => as_json(@questions)
end

Here is the test's mock:

question = mock_model(Question)

A little cryptic, right? After a little digging, I changed the mock to this:

question = mock_model(Question, :as_json => {'foo' => 'bar'})

Ah, there we go!

The spec ended up looking like this:

context "as json" do
  it "lists the questions" do
    question = mock_model(Question, :as_json => {'foo' => 'bar'})
    Question.should_receive(:find_ordered_subjects).and_return([question])
    get :index, :format => 'json'
    response.body.should == "[{\"foo\":\"bar\"}]"
  end
end

In our application_controller.rb, an as_json() method called to_json, which would call as_json in the test, resulting in a circular reference. Oops.

Don't forget to stub as_json()!

Emailing with Delayed Job

We used Delayed Job to queue emails sent out to users, both to offload that blocking process and for scheduling. It has worked well so far, but recently there were some strange bugs popping up. Some emails were stuck in the queue, and the error message was about bad YAML syntax.

Delayed Job serialized objects in its handler field, and, with some user input that's not encoded properly, created incorrect YAML. For example, this could happen:

id: 1
  foo: 'here is some 'text'
  bar: 'something else'

Notice the odd number of single quotes in foo? Yeah, that's bad. Since we already had that kind of data saved, we needed another way to fix this.

Instead of having methods in the notifier.rb file like so:

def forgot_password(user)
  ...
end

We did it like this:

def forgot_password(user_id)
  user = User.find(user_id)
  ...
end

Delayed Job serialized just the user id and not the entire user object, so any potentially harmful data wasn't saved. This was more expensive since the objects had to get instantiated again, but sending out email wasn't expensive for our app, so this solution worked well.

If you ever get strange YAML syntax errors from delayed job, perhaps this method will work for you.

Attaching Events to a Disabled Submit Button

There was a form that had a few required fields, and I wanted to show a message when the user hovered over the submit button when not every field was completed. The problem was that the submit button was disabled until the fields are filled in, and I couldn't attach an event to a disabled form element.

One solution is to add an invisible element over the button.

var $disabledSubmit = $('#submit_wrapper input:disabled');
var $disabledSubmitParent = $('#submit_wrapper');
var $overlay = $('<div />');
$overlay.css({
  position: 'absolute',
  top: $disabledSubmit.position().top,
  left: $disabledSubmit.position().left,
  width: $disabledSubmit.outerWidth(),
  height: $disabledSubmit.outerHeight(),
  zIndex: 10,
  opacity: 0
});
$overlay.mouseover(this.submitHoverOver);
$overlay.mouseout(this.submitHoverOut);
$disabledSubmitParent.append($overlay);

This created an overlay over the button that handled the hiding and showing (submitHoverOver()/submitHoverOut()) of the message.

When the form was ready to submit and the button was enabled, we needed to do two things. The first was to lower the z-index of the overlay so the user can access the button.

$overlay.css('z-index', -1);

The second was to unbind the events on the overlay so we didn't continue to show the message.

$overlay.unbind();

If the user changed the data to be in a bad state, we disabled the submit button. We also needed to reattach the events and crank up the z-index of the overlay.

$overlay.css('z-index', 10);
$overlay.mouseover(this.submitHoverOver);
$overlay.mouseover(this.submitHoverOut);

Now, instead of putting up an error message or more text about required fields, the user would be directed to finish the form if he hadn't done so when he tried to submit.

My name is Danny...and I make mistakes.

Yes, it's true. Here's what I did, and here's the reaction.

At work a few weeks ago, I was going through some callback code that sets some meta data on a model's associations. I noticed that a related flag wasn't getting set as well, that it was only set in one other specific instance with the meta data. Hmm...let's fix that, shall we? Flag added, moving on.

Star wipe to this week.

We found a bug when displaying historical data, and I quickly realized that the display was wrong because it was skipping over objects it shouldn't, objects that were flagged when they shouldn't be. Cue pants pooping.

There was a fix, and I would just need to run a script that would update the flag for all the associated models affected after the callback happens and ignore the other ones because those were the ones that explicitly have the flag set at the other, correct, time. But should I tell anyone or do I just run the script and say that I fixed the display bug? Well, WWJD (what would Jack Nicholson do)? He'd tell everyone, damn it, because he's like that. Keeping it real. Not like Chuck Norris.

Anyway, I sent out an email admitting what happened, and I included a high-level explanation of what happened along with a technical one. I explained that there is a fix and we won't lose any data, and that it would fix the current display bug but that we need to do some tests to make sure it didn't affect any other parts of the application.

My manager's response? "Hey, man, shit happens. Glad you fixed it." That's why I like working here.