Objection.js: Improved (database) errors

[koskimas]

I would love to add a cross-db error API but as I have mentioned before it would be best implemented as a separate library. Then it could possibly be moved to knex at some point.

Currently errors thrown from db for example in case of foreign key constraint violation differ from db to db and are usually difficult to reason with. I would love to have something like:

class DbError extends Error { ... }

class ConstraintViolationError extends DbError {
  get column() { ... }
  get table() { ... }
  get constraintName() { ... }
}

class ForeignKeyConstraintViolationError extends ConstraintViolationError {
  ...
}

// etc.

If someone is looking for an open source project, here's one that I think would benefit the whole node.js DB scene.

EDIT Check out db-errors library I'm working on.
EDIT 2 There's now a db-errors plugin for objection

[koskimas]

@fl0w suggested an approach where we would only add an unique key to the existing errors thus making this a non-breaking change.

The only problem I can think of is that the errors thrown by databases pretty much already contain all meaningful property names like code, message, type etc. At least postgres errors do, and that's enough. Our property would need to be something that doesn't conflict with any of the existing properties. Maybe we could use errorSymbol = Symbol() and something like

const { errorSymbol, errorCodes } = require('db-error');

function onError(error) {
  if (error[errorSymbol].code === errorCodes.ConstraintViolation) {
    console.log(error[errorSymbol].column)
    console.log(error[errorSymbol].constraintName)
  }
}

[koskimas]

Actually the above approach has at least one severe downside: The custom information would not appear in the stack trace or error message. So uncaught errors would be as confusing as always.

[capaj]

IMHO this issue should be placed in knex rather than here. This is a feature lacking on their side. They already normalise API for all the supported DBs, it should be doable to normalize errors as well.

BTW I'd definitely prefer the former approach of extending errors. Breaking changes are always a pain, but for our app we have a central place where we handle common SQL errors so it would be quite easy to refactor it.

[elhigu]

Here is some discussion from knex about this error normalization:

https://github.com/tgriesser/knex/issues/2191
https://github.com/tgriesser/knex/issues/522

I have also vague feeling that there was more discussion about it, but I couldn't find any other issues.

My feeling is that it would be better also for knex to be in completely separate package, because keeping those error parsers uptodate sounds like a quite huge work (there are not many maintainers there).

Also currently one can use for example postgresql/mysql dialect implementations also with other compatible databases, even if their error format may be different.

Some work was started here, but I have not checked the code https://www.npmjs.com/package/knex-generic-errors

[koskimas]

The library @elhigu linked has seen no progress after it was created. Also it seems to be knex specific.

I started doodling something together here. I've only implemented a UniqueViolationError and I already ran into trouble. The information content of mysql and sqlite errors is very minimal.

For a UniqueViolationError you would like to get the table, columns and the constraint that failed. Well... Here's what we can actually scavenge from the errors:

• postgres: table, columns, constraint
• mysql: constraint
• sqlite: table, columns

😞

The only "unified" part is the error class. Other errors will probably have similar limitations.

[devinivy]

Do we know if that's a limitation of the DBs versus the drivers?

[koskimas]

For mysql it seems like it's a DB limitation:

https://dev.mysql.com/doc/refman/5.7/en/error-messages-server.html
https://dev.mysql.com/doc/refman/5.5/en/error-types.html

AFAIK Mysql only provides

• error code: mysql_errno()
• SQLSTATE value: mysql_sqlstate()
• message: mysql_error()

Even the constraint name needs to be parsed from the error message, but that's not too bad since the error message format is documented in mysql docs and shouldn't change.

[fl0w]

I mean, even if it was "only" unified, well documented error class, error handling via objection would get easier for the consumer and it would also be less scary to push a objection v1 with room for future improvements, no?

[koskimas]

@fl0w Yeah, probably. I mean, that's the best we can do. The other option would be to leave out all information that cannot be gathered for all clients, but that would leave us with no data 😄

[fl0w]

Would it be sane to abstract the problem even further, i.e. allow user space to hook on transforms when a driver throws?

So, objection doesn't really care about what the errors was - and out of the box objection will (re-)throw the driver/knex error. However, if a user registers some error parser/transform (which may be cross db compatible and 3rd party, or not) objection then passes the error to user defined fn? Given enough time, maybe the community will naturally land in a practice?

[koskimas]

Adding an error handling hook is a good idea, but doesn't solve this issue. I believe that the reason there are no solutions to this is that parsing the DB errors is really hacky and no one wants to publish those hacks as a library 😄

Example: postgresql UniqueViolationError parser

'use strict';

const UniqueViolationError = require('../../../../errors/UniqueViolationError');

const CODE = '23505';
const UNIQUE_COLUMNS_REGEX = /Key \((.+)\)=\(.+\) already exists/;

module.exports = {
  error: UniqueViolationError,

  parse: (err) => {
    if (err.code !== CODE) {
      return null;
    }

    const colsMatch = UNIQUE_COLUMNS_REGEX.exec(err.detail);

    if (!colsMatch) {
      return null;
    }

    const cols = colsMatch[1]
      .split(',')
      .map(it => it.trim())
      .map(it => stripParens(it));

    return {
      table: err.table,
      columns: cols,
      constraint: err.constraint
    };
  },

  subclassParsers: []
};

function stripParens(it) {
  if (it[0] === '"' && it[it.length - 1] === '"') {
    return it.substr(1, it.length - 2);
  } else {
    return it;
  }
}

[koskimas]

I'll continue writing db-errors to see how big of a mess it becomes. I'll start with the most useful errors. What errors would you like to catch? I'm quessing unique constraint violation is one of them, but what else?

[capaj]

@koskimas for example the error I get from knex is when I mistype a column name in a where condition in a select.

Error: ER_BAD_FIELD_ERROR: Unknown column 'a' in 'where clause'
    at Query.Sequence._packetToError (/home/capaj/git_projects/looop/project-alpha/back-end/node_modules/mysql/lib/protocol/sequences/Sequence.js:52:14)
    at Query.ErrorPacket (/home/capaj/git_projects/looop/project-alpha/back-end/node_modules/mysql/lib/protocol/sequences/Query.js:77:18)
    at Protocol._parsePacket (/home/capaj/git_projects/looop/project-alpha/back-end/node_modules/mysql/lib/protocol/Protocol.js:279:23)
    at Parser.write (/home/capaj/git_projects/looop/project-alpha/back-end/node_modules/mysql/lib/protocol/Parser.js:76:12)
    at Protocol.write (/home/capaj/git_projects/looop/project-alpha/back-end/node_modules/mysql/lib/protocol/Protocol.js:39:16)
    at Socket.<anonymous> (/home/capaj/git_projects/looop/project-alpha/back-end/node_modules/mysql/lib/Connection.js:103:28)
    at emitOne (events.js:115:13)
    at Socket.emit (events.js:210:7)
    at addChunk (_stream_readable.js:266:12)
    at readableAddChunk (_stream_readable.js:253:11)
    at Socket.Readable.push (_stream_readable.js:211:10)
    at TCP.onread (net.js:585:20)

[koskimas]

@capaj How could that be improved? Also why would you want to handle that case in your API? How could the server or the client react to that error other than to respond with a 500 and a "whoops, a programmer **ed up"?

Currently that error would become an instance of the base class DBError, so you could add if (err instanceof DBError) as a last branch where you simply respond with a 500 (and possibl the error message).

[kblomster]

Foreign key constraint violation would be nice, I think. Maybe data format violations as well (that is, string too long, integer too big, etc), but that one is somewhat less useful since you probably want to handle that with validators in js anyway.

[koskimas]

@kblomster fk contraint violation has already been implemented. Data format violation is an excellent idea! I'll add that next. I already implemented check constraint violation.

[koskimas]

The most important thing I need to do before I can consider releasing the db-errors library is a test matrix that runs the test suite with a bunch of driver and db versions. Some of the stuff is parsed from error messages and even though they are documented, I don't trust that they will stay the same.

[kblomster]

@koskimas Once it's more mature, would you accept a PR implementing support for MSSQL?

[koskimas]

@kblomster absolutely!

[capaj]

@koskimas I got that error when working on a dynamic query engine which allowed client to make query into any table. You're right it's not a common usecase.

[koskimas]

I was able to setup travis to run the tests on multiple versions of mysql and postgres. I'll add multiple versions of the node db drivers also just to make sure.

@kblomster I also added DataError but I'm not able to parse any useful data from those errors (like table or column that failed). The only way to parse that info would be to parse SQL, and we would need a full-blown SQL parser for that. I don't know how useful that error is. The error message contains the info, but as SQL.

[koskimas]

There's now a db-errors plugin for objection. I thought we could first try out the library as a plugin and if it turns out useful, we can add it to objection or even try to get it merged into knex.

[jtlapp]

Validation errors might need a message. When I JSON.stringify(err), I find that err.message is not set. When I retrieve err.message, I get a JSON string rendering of err.data. A getter probably does that, but that output is just weird.

The stack trace is convenient but non-standard, showing JSON in place of a string:

  Error: {
    "firstName": [
      {
        "message": "should have required property 'firstName'",
        "keyword": "required",
        "params": {
          "missingProperty": "firstName"
        }
      }
    ]
  }
      at Function.createValidationError (/Users/joe/repos/objection/examples/koa-inversify/node_modules/objection/lib/model/Model.js:345:12)
      at parseValidationError (/Users/joe/repos/objection/examples/koa-inversify/node_modules/objection/lib/model/AjvValidator.js:190:21)
      ...

My expectation is that err.message return a single human-readable line; any client app that generically dumps errors likely has the same expectation. This may force some apps to special-handle ValidationError merely to print the message consistently with other errors.

Consider that the error message is often what an end user needs to report to the dev to start a conversation about assistance. JSON messages are more intimidating for end users and may not even register as something they need to report, perhaps thinking it's just code.

One possibility is to concat the validation error messages together, yielding something like: "should have required property 'firstName'; should have required property 'lastName'". Another is to show only the first and signal when there are more: "should have required property 'firstName' and other errors". And of course the message could be uninformative, as in "Invalid model property."

I personally can live with JSON messages by remembering to special-process ValidationError, but we may want to revisit whether non-standard error reporting should make it into 1.0.0.

[koskimas]

@jtlapp I'm currently refactoring ValidationErrors for the 1.0 release. I've changed the error messages to a more human readable format. You probably shouldn't show the ValidationError messages to the users though.

ValidationErrors will also have a type field for distinquishing between model validation errors and other kinds of validation errors.

[jtlapp]

Sounds good. Thank you!

[koskimas]

I think this can be closed for now. I won't add db-errors to objection yet. Let's wait until it matures a bit and even then we should try to get it merged into knex instead. For now, the objection-db-errors plugin can be used.

Validation errors have now been refactored and can be tested by installing the most recent 1.0 RC using npm install objection@next.