New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom Errors #39

Closed
tgriesser opened this Issue Aug 13, 2013 · 5 comments

Comments

Projects
None yet
2 participants
@tgriesser
Owner

tgriesser commented Aug 13, 2013

Implement custom Error that can be used for easier debugging, maintaining the query, bindings, and other information.

@koskimas

This comment has been minimized.

Collaborator

koskimas commented Sep 25, 2013

This would be great! (If this means what I think it means)

My problem is this: In the 0.44 version an error like this:

{ [error: duplicate key value violates unique constraint "user_username_unique"]
name: 'error',
length: 166,
severity: 'ERROR',
code: '23505',
detail: 'Key (username)=(user1) already exists.',
hint: undefined,
position: undefined,
internalPosition: undefined,
internalQuery: undefined,
where: undefined,
file: 'nbtinsert.c',
line: '396',
routine: '_bt_check_unique'
}

was thrown for unique constraint violation. From this I was able to hack out the column that failed and I was able to use the error code. Now in the 0.45 version the error contains just a message string.

It would be great if the errors thrown by the database driver could be passed with the error somehow.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Sep 25, 2013

Ah, thanks for pointing this out @koskimas!... So the change I made in 0.4.5 was to do this:

return chain.then(builder.handleResponse).otherwise(function(e) {
  var err = new Error(e.toString() + ' - ' + '{sql: ' + sql + ', bindings: ' + bindings + '}');
      err.originalStack = e.stack;
      throw err;
});

which is sort of a mess... first, because it's using object notation for the sql / bindings, but it's impossible to parse because it is after the e.toString() + ' - ' +... and then the original stack isn't really useful either, is it?

What do you think about something like this

return chain.then(builder.handleResponse).otherwise(function(error) {
  var newError = new Error('{sql: ' + sql + ', bindings: ' + bindings + ', message: ' + error.toString() + '}');
      newError.clientError = error;
      throw newError;
});

That way, you'd have the new error which contains the sql and bindings sent to the builder, which I find is often what I need to debug, as well as the toString of the original error... then if you want it you can access clientError on the newError object to get the original error and all of the associated info.

Does that sound like a good compromise, or do you have any suggestions?

tgriesser added a commit that referenced this issue Sep 25, 2013

@koskimas

This comment has been minimized.

Collaborator

koskimas commented Sep 25, 2013

The clientError property would work perfectly in my case. I'm not sure how wise it is for me to count on the pg error to remain the same in the future since it says "need to document how to handle errors..." in the documentation of node-postgres :D

By the way, I just recently discovered knex and I think it's awesome! Keep up the great work.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Sep 25, 2013

Thanks, will do! Also, for reference, here's the new implementation of the error in 0.4.6:

// Since we usually only need the `sql` and `bindings` to help us debug the query, output them
// into a new error... this way, it `console.log`'s nicely for debugging, but you can also
// parse them out with a `JSON.parse(error.message)`. Also, use the original `clientError` from the
// database client is retained as a property on the `newError`, for any additional info.
return chain.then(builder.handleResponse).otherwise(function(error) {
  var newError = new Error(JSON.stringify({sql: sql, bindings: bindings}));
      newError.clientError = error;
  throw newError;
});

@tgriesser tgriesser closed this Sep 25, 2013

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Sep 25, 2013

@koskimas - also, if you like Knex, you might be interested in checking out Bookshelf if you haven't already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment