Sinatra Error Blues
When working on someone else’s code it is often hard to tell if something you don’t like is bad code, or if it is just a style of thinking you need to get used too. For instance, when updating a model with a restricted parameter (such as id), should it raise an error, or just ignore the parameter? On one hand if you have a new developer he may be confused as hell when he has unexpected bugs because the restricted columns are silently ignoring their parameters. I have even been tripped up a few times myself! An error would be much clearer and give you a path to instantly fix it. On the other hand, if you truly know your tools, then this is expected behavior. It allows you to cut down on error handling and makes the intent of the code much clearer.
This conflict relates to my first thought: is it bad code because it might have pitfalls that other programmers will hit? The pragmatic programmer in me screams “Crash Early.” However, there are paradigms that you have to be responsible for, and the other part of me screams “Keep it Clean!”
Sequel and its paradigm
By default Sequel raises an error for EVERYTHING. Anything wrong it throws its hands up and takes you out. It’s annoying when you are trying to quickly prototype something. Sequel has the ability to switch the errors off, but it seemed like a strange default behavior to me.
As I was working on mail a few things fell into place that led me to write a plugin to Sequel, proc_error_handling. Soon I’ll be talking about the plugin specifically, however Sinatra has some pretty cool error handling which I should talk about first.
Sinatra has this nifty error block now. You specify any object, and if that object is thrown the block will catch it and return the result of the block. Normally you may have something like this:
...
# response is only json
if @model.save
status(201)
content_type('application/json')
{:status => 201, :resource => @model.to_hash}.to_json
else
status(400)
content_type('application/json')
{:status => 400, :errors => @model.errors}.to_json
end
If you have a bunch of different models you end up seeing much of this control structure repeated. Yes it shows you handling expected cases, but it is the same process for each controller. Anyone using this service to create a resource is assuming it’s going to succeed and only enters the failure case when their code fails! Keeping that in mind Sinatra lets us clean up the code quite a bit.
# We are providing JSON only
after do
content_type('application/json')
end
helpers do
def resource_created(resource)
status(201)
{:status => 201, :resource => resource.to_hash}.to_json
end
end
# Creation of a new message. We split up the creation into 3 steps.
# Instead of using the create method, we make sure $resource is populated
# so that if something happens we have a handler to the object
post '/message' do
# Need to make sure $resource has an object for the error
$resource = Message.new
$resource.set(params[:message] || {})
$resource.save
resource_created($resource)
end
# Called for any errors that happen during resource creation
error Sequel::Error do
status(400)
content_type('application/json')
{:status => 400, :errors => $resource.errors}.to_json
end
Unfortunately the down side with this is now you have to use begin and rescue blocks for errors you don’t want to propagate down to Sequel::Error, for instance if someone supplied an account_id for the message, and previously you had declared it as restricted, the code turns much uglier.
post '/message' do
begin
# Need to make sure $resource has an object for the error block
$resource = Message.create
$resource.set(params[:message] || {})
$resource.save
rescue Sequel::Error
raise $! unless $!.message =~ /restricted column/
params[:message].delete_if { |p| Message.restricted_columns.include? p.to_sym}
retry
end
resource_created($resource)
end
...
This can be cleaned up if you filter the params first:
helpers do
def filter_restricted(klass,values)
values.delete_if { |p| Message.restricted_columns.include? p.to_sym}
end
end
post '/message' do
# Need to make sure $resource has an object for the error
$resource = Message.create
$resource.set(filter_restricted(params[:message] || {}))
$resource.save
resource_created($resource)
end
...
Still not a bad solution, but I found myself writing many of these pre-filtering methods to avoid errors I didn’t want to handle. I was annoyed because the code was being executed even when it wasn’t needed. It just moved the ifs and elses to a different place. I also didn’t like that I had to use $resource (or @resource) for every resource so the error handling could work. To fix these problems I began work on proc_error_handling to get rid of any naming schema and global dependancies. I will talk about the plugin in detail soon, but I think the code is enough to get excited about.
after do
content_type('application/json')
end
helpers do
def resource_created(resource)
status(201)
{:status => 201, :resource => resource.to_hash}.to_json
end
def filter_restricted
proc do |klass, vals|
# vals contains the hash used to try to update the model
# delete_if returns the modified hash with removed columns
# We want to retry updating the model if we managed to delete restricted columns
# This only runs if there was a problem so no pre-error checking!
:retry unless vals == vals.delete_if {|k,v| klass.restricted_columns.inlude? k }
end
end
# Instruct the proc error handling what to do when an unhanded error occurs
Sequel::ProcErrorHandling.on_error do |bad_resource|
ENV['resource_error'] = bad_resource
end
end
# Create a message
post '/message' do
account = get_account(params)
raise SecurityError unless account
# Create a message, but silently ignore errors if restricted_columns passed
# In the future when using Message it will still error on restricted columns!!
@message = Message.create(params[:message],filter_restricted)
resource_created(@message)
end
error Sequel::Error do
error = $!
resource = ENV['resource_error']
status(400)
{:status => 400, :errors => resource.errors}.to_json
end
error SecurityError do
status(401)
{:status => 401, :status_message => 'Unauthorized'}.to_json
end
Pretty sexy yeah?