thinky icon indicating copy to clipboard operation
thinky copied to clipboard

JSON format the "duplicate key" error?

Open techguydave opened this issue 11 years ago • 11 comments

A duplicate primary key message currently looks like this:

[Error: Duplicate primary key `username`:
{
        "firstName":    "Hank",
        "lastName":     "Hill",
        "username":     "propanerocks"
}
{
        "firstName":    "Hank",
        "lastName":     "Hill",
        "username":     "propanerocks"
}]

Informative, but not something I'd like to spit out to an API output, especially with sensitive fields like passwords and whatnot. I want to say "Username exists" or something, but the problem is that the error might not necessarily be a dupe key.

So with a catch-all error handler, it'd be nice if the error looked more like this:

{
   "message": "Error: Duplicate primary key `username`",
   "items": {
      "input/submitted/etc": {
         "firstName": "Hank",
         "lastName": "Hill",
         "username": "propanerocks"
      },
      "original": {
         "firstName": "Hank",
         "lastName": "Hill",
         "username": "propanerocks"
      }
   }
}

That way, at the very least I can spit out the error message by itself.

techguydave avatar Nov 21 '14 18:11 techguydave

Just to be sure, you basically would like to retrieve all errors instead of the first one that happens?

neumino avatar Nov 21 '14 19:11 neumino

Not exactly. Let me try to be more specific. Let's say I have code that looks like this:

user.save(function(err, user){
    if(err){ // This is what I have an issue with
        res.send(409, 'Duplicate user'); // The error may not always be 'duplicate', though
        return next();
    } else {
        res.send(200, user);
    }
});

The problem I have is that err gets called regardless of whether the issue is with a duplicate key or a field that doesn't conform to a strict type, or whatever other errors there are. So when I have to deal with it, I want to spit out a specific HTTP status code, as well as tell the API consumer what just went wrong.

The way it currently works, is that the messages are a string or some weird form of an object. On top of that, the current error message displays everything. If my user items include a field in the database that stores the hashed password, that is being output along with the error message. And since it's not an array or JSON object, it's much more difficult to remove that part or isolate just the "Duplicate primary key" message.

I'm basically wondering if the error messages can be formatted as a legal JSON object for easier consumption.

techguydave avatar Nov 21 '14 20:11 techguydave

+1 – a consistent error object with codes or some such would be amazing. Should this actually be a feature of RethinkDB though?

mstade avatar Nov 26 '14 18:11 mstade

So I was going to implement that a few months ago, but then code errors got on the table at RethinkDB. Looking at the tracker, it was pushed back, so maybe thinky shouldn't wait.

For the time being, I plan to extend a bit the errors returned. I'll add a field short_message that does not contain the old/new document, only the text message.

Then there's still a need for a regex somewhere since the call to save could fail for multiple reasons:

  • master non available
  • duplicate primary key
  • network issues
  • etc.

neumino avatar Nov 26 '14 18:11 neumino

FYI: RethinkDB 2.1 is coming with a better error hierarchy but still no codes as far as I know. So there's still some work to do here.

neumino avatar Aug 09 '15 02:08 neumino

What's the current practice for parsing errors? There doesn't seem to be any easy way to match "Duplicate primary key"...

avimar avatar Sep 18 '15 02:09 avimar

The best practice is sending a whitelist of error. If you want to return something better than just a 500 error, parse the error and send back your own message (or the same if you want). Don't send back thinky's error back to the client.

This is true for thinky but also for any other software.

neumino avatar Sep 18 '15 04:09 neumino

That's true, but it's not an error, duplicate primary key is just a bunch of text. Woops, cross-posted to #328.

avimar avatar Sep 18 '15 11:09 avimar

From @avimar

This seems quite messy, but is the code I currently seemingly need:

    if(err.name==="Document failed validation") message = "Number is invalid";
    else if(err.message.indexOf("Duplicate primary key")!==-1) message = "Number already exists";

Can we get duplicate primary key as an actual error thrown by thinky, instead of being passed directly from the DB?

neumino avatar Sep 18 '15 14:09 neumino

You are welcome to submit a pull request.

neumino avatar Sep 18 '15 14:09 neumino

Latest entry in the "brittlest code in the universe"-contest:

try
{
    yield model.save( data );
}
catch( err )
{
    const errObj = JSON.parse( 
        err.message.substr( 
            err.message.indexOf( '{' ) 
        ,   err.message.lastIndexOf( '}' ) 
        ) 
    );
    const errStr = errObj.first_error.substr( 
        0 
    ,   errObj.first_error.indexOf( '\n') - 1 
    );
    // use errStr
}

kevinkleine avatar Jun 01 '16 12:06 kevinkleine