feat: use objection error types in error-handler

[robbyphillips]

This PR changes the errorHandler function to use the error classes created by objection. This removes the need to parse individual error types by db type, since it's already handled by objection.

Inspired by this objection recipe: https://vincit.github.io/objection.js/recipes/error-handling.html#examples

This should close #130.

[dekelev]

Thanks! I'll check it out tomorrow. בתאריך יום ה׳, 19 בנוב׳ 2020, 19:59, מאת Rob Phillips ‏< notifications@github.com>:

…

This PR changes the errorHandler function to use the error classes created by objection. This removes the need to parse individual error types by db type, since it's already handled by objection. Inspired by this objection recipe: https://vincit.github.io/objection.js/recipes/error-handling.html#examples This should close #130 <#130>. ------------------------------ You can view, comment on, or merge this pull request online at: #131 Commit Summary - new error handler - feat: use objection errors in error-handler - cleanup commented out lines File Changes - *M* package-lock.json <https://github.com/feathersjs-ecosystem/feathers-objection/pull/131/files#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519> (10125) - *M* src/error-handler.js <https://github.com/feathersjs-ecosystem/feathers-objection/pull/131/files#diff-0a502163240ffc42be6784b74dc879742fb2273986a469867b79889b64b36ffa> (151) - *M* test/index.test.js <https://github.com/feathersjs-ecosystem/feathers-objection/pull/131/files#diff-af95cb6f5f65fe70216ddce70325e785438d5aaff956a0666ebf46ef6402f526> (240) Patch Links: - https://github.com/feathersjs-ecosystem/feathers-objection/pull/131.patch - https://github.com/feathersjs-ecosystem/feathers-objection/pull/131.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#131>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB5E3LAFYSFHJW6BWFJVODSQVMIRANCNFSM4T3XLPQQ> .

[GVanderLugt]

This worked well and solved my problem. I am getting an appropriate error now and am no longer getting a 500 for my unique violation error that I described in my issue (#130). However, the error message still isn't great. It's exposing the actual DB query. I suppose this should be handled by an app level error hook. Unless we feel like writing a parser to give good human readable error messages for all of these different errors 😆

insert into "users" ("created_on", "email", "isVerified", "name", "password", "updated_on", "verifyChanges", "verifyExpires", "verifyShortToken", "verifyToken") values ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) returning "id" - duplicate key value violates unique constraint "users_email_unique"

[dekelev]

You can use the FeathersJS error hook to handle server errors before they sent out the client and after you logged them in your system. https://docs.feathersjs.com/api/errors.html#error-handling בתאריך יום ו׳, 20 בנוב׳ 2020, 4:01, מאת Gerrit VanderLugt ‏< notifications@github.com>:

…

This worked well and solved my problem. I am getting an appropriate error now and am no longer getting a 500 for my unique violation error that I described in my issue (#130 <#130>). However, the error message still isn't great. It's exposing the actual DB query. I suppose this should be handled by an app level error hook. Unless we feel like writing a parser to give good human readable error messages for all of these different errors 😆 insert into "users" ("created_on", "email", "isVerified", "name", "password", "updated_on", "verifyChanges", "verifyExpires", "verifyShortToken", "verifyToken") values ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) returning "id" - duplicate key value violates unique constraint "users_email_unique" — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#131 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB5E3MFEXLFKBD7FIZILQ3SQXEXNANCNFSM4T3XLPQQ> .

[dekelev]

@robbyphillips It looks great! It seems like a breaking-change version. can you please add a migration section in the bottom of the README for migrating to v7.x? Also, the Error handling section might need an update.

[dekelev]

Perfect! I'll release it tomorrow. Thank you for your contribution.

[dekelev]

v7.0.0 is released

[robbyphillips]

Thanks!

[GVanderLugt]

Thanks @robbyphillips!

[OMFG5716]

To add above the UniqueViolation error has the columns key undefined:

Due to columns key being undefined it's only throwing this:

Trying to work on this and create a separate pull request. How would you proceed? Anybody affected with similar issue? This is going to be my first PR if I go ahead with this. The naïve solution for now is not include the columns affected just show the underlying 'sqlMessage' or better yet show the 'constraint' key in the message as a column.

[robbyphillips]

@OMFG5716 --

Objection gets the errors from the underlying sql drivers with this lib: https://github.com/Vincit/db-errors, and in the tests I just mocked the object we should be getting from that lib.

So first step would be to see if we can figure out why we're not getting what's expected, possibly because that db-errors lib returns a different shape for mysql or whatever. If that's true, then the solution is to add a different conditional to check for error.columns before making the error message.

[robbyphillips]

The error parser for mysql definitely returns a different shape: https://github.com/Vincit/db-errors/blob/6ed1975c135484e127643250e1e64b2bd8fc35a8/lib/parsers/mysql/DBError/ConstraintViolationError/UniqueViolationError/parser.js#L20

My test was based on the error constructor defined here: https://github.com/Vincit/db-errors/blob/master/lib/errors/UniqueViolationError.js, but it was a bad assumption on my part that all those keys would actually exist for every parser.

[dekelev]

@OMFG5716 Issue is fixed in v7.1.2