Worst case scenario

Monday, August 3, 2009

Years ago, after I finished college but before I started working professionally with software, I spent a couple years working as a paramedic. I learned a lot from that job, not least about interacting with people who really, really don’t want you in their lives.

One of the calls I remember most vividly happened around three in the morning, not long after schools had let out for the summer. A group of recently graduated high school girls had rolled their Ford Bronco on the highway. When we arrived an engine company was on scene, busily cutting the remains of the car into fun size Bronco strips. I followed the trajectory implied by the hole in the windshield and found my patient, the driver, on the pavement some distance from the car.

While I set about preparing to package her up for a quick trip to the hospital another engine company arrived. As I started my cursory physical exam the lieutenant rushed over and demanded I stop. To understand his reasoning you have to realize that the process of emergency medicine affords little or no dignity to trauma patients: life comes before limb; modesty comes well down the list.

So, what the fire lieutenant objected to was that I was cutting the clothes off a sixteen year old girl in the middle of the highway, directly illuminated by the halogen scene lights from our bus. He demanded that we package and transport the patient fully clothed. If you haven’t worked with firefighters, know this: they travel in packs. My partner was hanging IV bags and working the radio for another bus, so it was me, a supine patient, and five firefighters. They got what they wanted.

Of course, to protect trauma patients’ spines you have to package them fairly thoroughly. You basically strap them down to a six foot board so they can’t move, and once you finish it’s pretty much impossible to get their clothes off or do a half decent physical exam without jeopardizing their spinal cords. Which means, as the attending medic, when we got to the ED I was the one who had to explain to the trauma surgeon and ED physician why I was handing over a patient who could have had a piece of windshield glass the size of a grapefruit sticking into her kidney for all I knew. Not a shining career moment.

I remember that call not because the patient was badly hurt (just some broken bones; she was lucky), or because I made a huge difference in someone’s life, but because of the lieu’s argument for not doing the full, expected, physical exam. As his minions packaged my patient like gift-wrappers at Macy’s, and my partner made a break for the driver’s seat, he told me these exact words:

“She’s suffered enough trauma. She’ll be okay.”

Now, I knew at the time he was quite likely right (her chances having escaped major injuries were actually better than you might imagine), and it’s actually quite difficult to explain to patients why you have to cut their clothes off (try it some time), poke them, shock them, or tie them down. It’s very, very tempting to do the easy thing in these cases, and 99 out of 100 times if you avoid making the patient, or yourself, uncomfortable they do fine. But, one out of 100 times the patient dies from a ruptured spleen or flash pulmonary edema. That’s the worst case scenario; and that’s the only scenario medics really care about.

Now (finally on to my point, dear reader), I recently submitted a patch to Rails that I, and many of my colleagues, believe will help prevent invalid functional tests, and therefore prevent bugs. The response from the Rails core team:

The vast bulk of tests just pass the id in those places and they’ll work fine. Users overloading to_param are in the minority and we shouldn’t spam everyone else just to satisfy them.

Ignoring for a moment the somewhat unscientific characterization of the prevalence of #to_param overriding, the message is that the patch is potentially annoying for people with improperly written tests and most of those improperly written tests probably won’t be a problem. It’s more comfortable for the Rails team to let broken tests slide and hope all will be well, than to bother developers over the cases in which broken tests lead to broken production.

I realize that comparing HTTP 500 responses to ruptured spleens might seem overly draconian. But, consider this: while the consequences of doing the wrong thing in software are much less dire compared to medicine, it’s so much easier to do the right thing. If you doubt this, drop me a line; I’ll be happy to nasally intubate you. I guarantee after that experience spending half an hour fixing your controller tests won’t seem so bad.

"Missing host to link to!" Rails 2.3 Upgrade Issue

Pivotal Labs

Monday, August 3, 2009

During the process of upgrading a project from Rails 2.2.2 to Rails 2.3.2 several of our tests were breaking with the error:

Missing host to link to! Please provide :host parameter or set default_url_options[:host]

This error was most commonly occurring in model specs where we had mixed in ActionController::UrlWriter in order to get access the named routes (e.g. invitation_path) inside of the model class. I believe this change in a behavior is the result of this patch to Rails but I am not certain. Interestingly the code falls apart in the tests but it still works fine within the browser.

With the assistance of Adam Milligan we were able to find an acceptable way to handle setting the default_url_options in the test environment.

# app/models/invitation.rb
class Invitation < ActiveRecord::Base
  include ActionController::UrlWriter

  ...
end

# spec/models/invitation_spec.rb
describe "Invitation" do
  before(:all) do
    Invitation.default_url_options[:host] = 'localhost'
  end
  after(:all) do
    Invitation.default_url_options[:host] = nil
  end
  ...
end

As I wrap up I want take a moment a properly shame myself for generating urls in the model. There is definitely a good argument that you should not be using named_routes in your models and I am eager to agree. Rails makes it hard to do for a reason and if you find yourself ever explicitly including UrlWriter take a step back and think the problem over. You may find yourself needlessly going down the wrong path and a different approach is in order.

Standup 08/03/2009: Pre Dev Camp is looking for a home in SF

Pivotal Labs

Monday, August 3, 2009

Interesting Things

If you’re on Rails 2.3+ you can use NestedScenarios instead of FixtureScenarios and FixtureScenarioBuilder and your life will be better.

Ask for Help

The Pre Dev Camp in San Francisco is looking for a home this weekend. If you have a location you can donate that can hold about 150 people with laptops, has power and wifi, please contact the organizer, Luke Kilpatri at (650) 745-5302 or luke@lukek.ca
“Does anyone have experience, good or bad, with RJB (the Ruby to Java bridge)?”
General consensus was murmur murmur murmur no.

More on Wapcaplet

Adam Milligan

Sunday, August 2, 2009

Yesterday I wrote about Wapcaplet, which is really little more than a Rails patch that didn’t get accepted, but that some of us think Rails actually quite needs. To that end I submitted a second patch, which does the same thing but, by default, outputs a warning rather than raising an exception. I also included some methods for modifying the behavior on ActionController::TestCase. Specifically, if you want to ensure your tests aren’t broken:

ActionController::TestCase.treat_parameter_type_warnings_as_errors

Or if you, like Pierre, don’t care:

ActionController::TestCase.ignore_parameter_type_warnings

I don’t know if these changes will make the behavior of the patch palatable enough for the core team to commit it. We’ll see. After creating the ticket I considered pulling the new behavior back into Wapcaplet; I’ve decided not to for a few reasons:

First and foremost, no one pays attention to warnings. I can’t count the times I’ve preached myself blue about eliminating compiler/interpreter warnings, to little or no effect. I recently broke the builds for several projects by deleting a method that had been deprecated for a year and half, and which generated a fairly annoying deprecation warning on every build for every project that used it (keep in mind that at Pivotal projects will build many times a day).
Any patch applied to Rails will affect every Rails project that upgrades. I believe people should fix their broken tests, but I accept that this change will break a lot of tests. I can accept warnings as a way to show people what may be broken without bringing the world down on their heads. Wapcaplet, on the other hand, is entirely opt-in; no need to handle users with kid gloves.
I believe that a test failure is the right behavior. We’re talking about broken tests, they should act that way.

Remember, the lion ate Pierre.

Wapcaplet

Adam Milligan

Saturday, August 1, 2009

Imagine for a moment that you run a big, important company. It’s important to you that your big, important company be successful at promoting, manufacturing, and distributing your big, important product, so you have decreed that the company must show a profit each and every quarter. In fact, your internal accounting software enforces this. For example:

describe "QuarterlyReportsController#create" do
  it "should reject quarterly reports that show a net loss" do
    post :create, :quarterly_report => { :net => -100 }
    response.response_code.should == 400
  end
end

Ignoring the somewhat misguided domain requirements, this test is wrong because it probably won’t fail when it should. It’s an example of a problem in Rails controller testing that bites everyone sooner or later.

The problem is that HTTP requests don’t send their parameters as integers, or booleans, or Date objects. No, HTTP request parameters are just big piles of strings. Rails does a good job of hiding the process of converting these strings into the integers and booleans and Date objects that make sense in your domain, but ActiveRecord handles that little bit of sleight of hand (using the column types from your database schema), not ActionController.

So, when you execute that test up above, the value of params[:quarterly_report][:net] will be the integer value -100, which is a value that the controller will never receive from a real HTTP request. This test fails to test a real case.

Now, if you try to use this value as an integer, either in the controller or by overriding #net= in the model, the test will still pass. But, as soon as you send a real request to the controller (hopefully not in production) you’ll find yourself on the business end of a 500 response. The test is broken.

In order to prevent this sort of brokenness I wrote a small patch to Rails (available here), which was summarily, and I will admit not unexpectedly, rejected. After that, I turned the patch into a tiny plugin called Wapcaplet (available here). It simply checks the parameters you pass to functional tests, and throws a friendly exception if you pass something with an inappropriate type. It accepts strings and instances of TestUploadFile in all cases. For parameter that Rails uses for routing it will also accept subclasses of ActiveRecord::Base.

Some examples:

# POST users/
post :create, :user => { :likes_ice_cream => true } # BOOM
post :create, :user => { :likes_ice_cream => "1" } # OK

post :create, :user => { :best_friend => @other_user } #BOOM
post :create, :user => { :best_friend_id => @other_user.id } # BOOM
post :create, :user => { :best_friend_id => @other_user.id.to_s } #OK

# GET users/:id
get :show, :id => @user.id # BOOM
get :show, :id => @user.to_param # OK
get :show, :id => @user # OK

# PUT users/:user_id/profile
put :update,
  :user_id => @user,
  :profile => { :photo => File.open("some/file" } # BOOM
put :update,
  :user_id => @user,
  :profile => { :photo => fixture_file_upload("some/file", "image/jpg" } #OK

Incidentally, notice that this will catch the pathological case, which seems to afflict every Rails developer, in which you pass #id rather than #to_param for a routing parameter.

In anticipation of the misguided comments that I know will come, no, it would not be better to have the plugin (or patch) call #to_s or #to_param on every incoming parameter to functional tests. Consider the example of boolean values:

true.to_s # => "true"
false.to_s # => "false"
true.to_param # => true
false.to_param # => false

Neither #to_s nor #to_param return a value likely to appear in a real HTTP request. It doesn’t take much imagination to come up with other examples of types that would not convert to meaningful request values. Worse, it takes only a little more imagination to come up with a scenario in which the implicit string conversion in the test would create subtlely wrong behavior that would be an enormous pain to track down.

So, take Wapcaplet out for a spin, I hope it saves you some time. Special thanks to Parker for getting bit by this problem enough times to get angry and demand I fix it.