Fixing a datamapper bug

DataMapper, a ruby ORM, has become fairly popular of late. I ran across an extremely annoying bug yesterday. This explains the bug and a fix.

DM allows you to make updates to the database in a manner that includes a where clause. This could mean efficiency wins or concurrency wins. So for example if I have a payment model and a payment moves from pending to say success or failure, I might have code that looks like this.

The problem with this is if I have multiple threads, processes w/e and I want to make sure the same payment is never updated twice. I shouldn't have one thread/process mark it a success and a different thread/process mark it a failure subsequently. The previous code is susceptible to race conditions. Ideally I'd like to write one line of SQL that says update the payment where status is pending. Then I'd write this.

This actually does only fire a single query which is an update clause with an inner where clause. I've appended to the gist the relevant line from the log file. This is because I've used update! instead of update.

The bug occurs when the status field is one of the more complex datamapper types like say enum. I've created an example which reproduces the error.

The first part of the test using update succeeds, but the second one with update! fails. It turns out superman is still in limbo! The reason for this is the Collection class in the DM module has a bug. The update! method has the following piece of code. Below that compare with similar code from the private _update method of the Resource class

The difference is that in one case the code says property.valid?(value) and in the other case, it (correctly) says property.valid?(property.get!(model_instance)) . This difference is important for special types like Enums. When I say update!(:status => :alive), a new model instance is created and the modified properties are asked to validate the values being inserted to verify that they are sane.

So a property of type Enum[:dead, :alive, :limbo]  is asked to validate the value being inserted. The value being inserted is :alive, but the underlying primitive value stored is 1 (since :alive is the 2nd field in a 0 based array). So property.valid?(1) is being checked, which is obviously false. 

In the second case, property.get!(model_instance) would actually return :alive. So property.valid?(:alive) is checked, which is true.

Now obviously there's a fair amount of indirection going on here for whatever reason. Anyway this is the quickfix I used to monkey patch my Collection class and move on.

Anyway it would be nice if the behavior of _update and update! could be moved to a single place so here is a proposed change.