DataMapper is inherently broken

This contains overtones of a post I’ve been resisting writing for months in the knowledge that the people who already agree with it will find it obvious while the people who don’t will bitch and moan and tell me that I’m stupid and perpetuate the same tired contentless arguments they always use.

So, having explained why this post is useless, I now add the caveat that I am writing it anyway, for the very simple reason that I am so angry at being proven right on the subject time and time again, and in particular at having had an entire morning wasted by this fuckwittery, that I am overriding my internal censor.

DataMapper is a ruby ORM library. It is inherently broken. Why? Because it considers booleans to be a superior solution to exceptions. This means that there is essentially no way to save data without falling into subtle and pernicious traps.

In DataMapper when you save an object, it returns false if it fails to save. You then need to ask it why it failed to save if, for some bizarre reason, you feel the need to care about little things like your data not making it into the database.

So, the code you end up writing all over the place looks like the following:

   my_account = Account.new(:name => "Jose")
   if my_account.save
     # my_account is valid and has been saved
   else
     my_account.errors.each do |e|
       puts e
     end
   end

Before anyone claims this is a contrived and silly example, here’s the source of it.

What’s the problem here?

Well… earlier I was called to help some colleagues. They were seeing really weird behaviour where save was failing but errors was empty. I was sure this must be a bug somewhere – it shouldn’t be possible. We even checked before the safe and valid? was returning true.

Which reminds me, another and sometimes recommended way of writing the above is to instead do…

   my_account = Account.new(:name => "Jose")
   if my_account.valid?
     my_account.save
     # my_account is valid and has been saved
   else
     my_account.errors.each do |e|
       puts e
     end
   end

Warning: If you do this, you are screwed. Your code is broken, even if it happens to work right now, and you will be bitten by it. Change it.

So, why is this code wrong?

Because the errors do not necessarily live on the object you are trying to save.

Suppose I instead had:

my_account = Account.new(:customer => Customer.new(:name => "jose"))

and tried to save this.

Well, it might fail to save, but return no errors.

Why? Because, it first has to save the customer. And that might fail to save. And if it does you won’t see any errors on the object you’re saving and have to hunt around to figure out what’s going wrong.

So, as well as having to explicitly check for errors on everything you attempt to save in order for your code to be correct, you must also explicitly check for errors on anything IT might helpfully decide to save for you.

This is untenable.

Code which fails should fail noisily. I shouldn’t have to work to find out why it went wrong, it should TELL me. If DataMapper did the obvious thing and threw an exception when something failed to validate then it would cut out a great deal of work and debugging effort, it would mean that I could simply handle the creation of a bunch of data and deal with the errors in one place instead of having to check everything for failures. It would mean that one could write code without living in a constant state of paranoia that you might make a mistake and forget to check a return code somewhere and then waste hours debugging when it finally bites you. And it would mean that not all the examples in their documentation would be wrong.

This entry was posted in Uncategorized on by .

24 thoughts on “DataMapper is inherently broken

  1. Colin Howe

    I think this is one of those great examples of exception hate caused by Java.

    Java’s use of checked exceptions throughout the JDK has caused people to resent exceptions. Primarily because the JDK has a lot of checked exceptions for things that you often don’t care about, e.g. close on BufferedReader requires you to catch an exception. Seems overkill to me.

    As a result of exception hate we’re seeing more languages without checked exceptions and more people avoiding using exceptions where they’re needed (i.e. here).

    Pity :(

    Reply
  2. david Post author

    Strongly agreed. I think between them, Java and C++ have created so much irrational exception fear and hatred that APIs across the world and in many different languages have suffered as collateral damage.

    Reply
  3. James Iry

    > you must also explicitly check for errors on anything IT might helpfully decide to save for you

    I never thought I’d see the day when somebody invented an error reporting mechanism worse than a global errno. Mr. MacIver you have renewed my faith in humanity’s spirit of continuous innovation. Bless you, sir, bless you.

    Reply
  4. knowtheory

    Perhaps to be clearer, i should say you will be able to say:

    User.raise_on_save_failure = true # all user instances will now blow up.
    DataMapper.raise_on_save_failure = true # EVERYTHING will blow up if it doesn’t save successfully. Hope you’ve got a white knight fetish.

    And if you’re a super micromanaging sort you can even tell particular instances that they need to blow up when not successfully saved:

    @user.raise_on_save_failure = true # will blow up if @user.save fails.

    Hope that helps. DataMapper 1.0 should be released ’round about RailsConf.

    Reply
  5. Dan Kubb

    Actually @knowtheory’s reply contains a small typo which is my fault. Edge DM now allows a way to specify exceptions globally, per-model and per-instance basis. If save would normally return false, it can be made to raise an exception instead.

    DataMapper::Model.raise_on_save_failure = true # globally
    User.raise_on_save_failure = true # per-class
    user.raise_on_save_failure = true # per-instance

    Please give this behaviour in edge DM a try and give us some feedback on whether or not it meets your needs. This will be in DM 1.0, but there will likely be a gem release in a few weeks and it would be nice to get some feedback beforehand.

    Reply
  6. david Post author

    Huh. Thank you. You’ve actually partially restored my faith in humanity (at least the subset who write code). I’ll check it out. :-)

    Reply
  7. Pingback: Links « Otaku, Cedric's weblog

  8. noah

    I don’t really see how an exception is better. It tells you the thing failed to save, but so what? It will still be the case that it was some tangentially related object and you’ll have to have a global error handling routine to deal with it because it could be almost anything, which is pretty much the same situation as before.

    The only difference is that you know which object caused the failure. But you still have to handle a customer save failure in your account save code. That’s ugly. That’s bad code. We might do better to ask WHY an account update is causing a customer update as well. That points to a problem with your code. Did you not know that you needed to update the customer first? Or are you altering the customer without knowing it?

    You’ve focused on a micro problem with the API you’re using, but you may have a bigger macro problem in how your application is structured.

    Reply
  9. david Post author

    Allow me to present two possible scenarios to you:

    Scenario 1: My code is wrong. And an unexpected thing fails to save. Something returns false. If I’ve done everything right and checked in all the right places (remember that bit where we entered this scenario assuming my code was wrong? So how likely do you think this is?), I can see this rather than having a silent bug. I spend an hour plus tracking down what exactly went wrong where.

    Scenario 2: My code is wrong. An unexpected thing fails to save. An exception is thrown, it’s obvious where the problem is because I’ve got an exact stack trace as to where it failed and an error message saying which object it was. It takes me 5 minutes to fix.

    Which sounds better to you?

    So, yes, the code responsible for me running into this was wrong. That’s the point. The issue is not that this behaves poorly when you get everything right. An API which makes your life hell when you do something wrong is an inherently broken API, because making mistakes is a fact of life.

    Reply
  10. Pingback: Links « Beautiful Discovery

  11. Kevin Bombino

    It seems like what I’d really like is something where #save returns false if that object fails to save as a result of a validation on it, but throws an exception if there’s an error bubbling up from a child object somewhere. Because that’s what really makes debugging a pain in the ass.

    Reply
  12. Chris Cummer

    raise_on_save_failure is a nice stop-gap, but it doesn’t seem to address the real crux of the issue, which is: _which_ item failed to save? For example, my User model fails to save. I get this exception raised:

    User#save returned false, User was not saved

    That’s great, but I know that because it’s not in the database and I’m still looking at the create form in my app. What I’d really like to know is _why_ it didn’t save. Unfortunately since the User instance is in fact correct, calling errors() on it reveals no info.

    Reply
  13. Nathan Long

    Still having this problem with DM 1.2. What I can’t understand is this: how is it possible that `@instance.valid?` returns true and `@instance.save` returns false? This makes no sense.

    When I say “@instance.valid?”, I’m asking “are you OK to save?” If it response `true`, it should save. Period.

    If it won’t save, it should respond `false` to `valid?`, and it should have some `errors` that I can inspect.

    Right now, I’m having this conversation with my models: “Are you valid? (Yep.) Ok, save then. (Nope.) Why not? (I’m not telling.)”

    Reply
    1. david Post author

      That’s usually the result of some dependent model failing to save.

      Except when it’s not. Sometimes it’s instead the result of a bug. And it’s really hard to distinguish these two cases.

      Reply
  14. Jason

    I think there is a “valid” use for both valid and save – the former checks validations, and the latter saves an object to the database.

    There are, however, definitely some difficulties working with the error handling of running transactions. In place of exceptions or a simple boolean, I think a separate “TransactionResult” should be returned. Something like…

    transactionResult = Foobar.new({:a => “b”}).save
    #

    There could be nested results also to help track down what objects are failing.

    Exceptions should be “exceptional” – saved for things like a bad database connection or other serious failure to save.

    Reply
    1. Jason

      Looks like some of that got stripped out in comment form, should be at hash:

      # TransactionResult @sucess=false @failure_type=:validation @errors=[...]>

      Reply
  15. opensourceame

    I’m finding DataMapper horrible having come from Doctrine on PHP. This just one of many annoying issues I’m having with it. Another is that if I try to update a parent and child object from a hash I get errors when the child object already exists. DM should be smart enough to detect which child I’m updating and fetch from, then update the db accordingly.

    The most annoying thing of all is that checking methods (valid?, etc) give different results to actually saving / updating. Sooo wrong!

    Reply
  16. Dan Tao

    Pretty late to the party here. I read this a while back and decided to write a gem to “correct” this behavior: dm-noisy-failures (https://github.com/dtao/dm-noisy-failures). It effectively solves the problem you describe in this post. I just blogged about it (http://philosopherdeveloper.com/posts/unbreaking-datamapper.html) and would be curious to hear your thoughts, though I suspect you’ve moved on from DataMapper since writing this a good 3 years ago (!).

    Reply
  17. trololol

    oh man, I’m late here but almost got burned by this. “Where did my data go”? The whole NoSQL skiddie movement full of crap like ‘bro exceptions on errors aren’t webscale’ and ‘so what if the default behavior is to save to /dev/null FOR SPEED” needs to go die in a toxic waste fire.

    The shame is i ‘fixed’ the problem in my code pretty quickly. It’s such a small issue it’s hard to realize how deeply pernicious it is. So much for ‘halt and catch fire’ being a sane default… this is what happens when you have an entire generation of “computer ‘scientists’” who have never read the words “Segmentation Fault” printed across their terminal (do they even have terminals?) screen?

    Reply

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>