Add SQL message to error object

[AdamVig]

This provides the message associated with a SQL error (can be customized when throwing a user error in MySQL) in the error object. This is useful for passing messages through directly from the database to the end user.

The motivation for this change is my usage of Knex.js on an API I am building. I am using MySQL triggers for data validation and I want to pass the error messages straight from the triggers to the end user via the API.

See this issue comment for an example of the error object format I see in Knex.js. This pull request will add a property to the error object called sqlMessage that has just the message from the thrown MySQL error.

Thanks for maintaining this project!

[dougwilson]

Thanks for the pull request! Can you update the PR to follow the contributing guide (https://github.com/mysqljs/mysql#contributing)? Notably the following seems to be missing: (3) include tests

Let me know if you need help updating it and I can :)

[AdamVig]

Will do! Sorry about that, wasn't sure if a change this small qualified for a new test or not.

Thanks for the quick review.

[AdamVig]

@dougwilson I added tests for the function that I changed. I couldn't find any tests for the Sequence file, so I added a new test file.

I tried to match the existing code and testing styles, but please don't hesitate to tell me if I made any mistakes.

[dougwilson]

Hi @AdamVig it's no problem at all! So we don't test the Sequence class directly on purpose, that's why it wasn't in there. I would expect the test for this to be exactly what you're trying to achieve: it should test "passing messages through directly from the database to the end user" so a test should be interacting with the actual MySQL server and then asserting that the resulting Error object's sqlMessage property is the exact string from the server.

[AdamVig]

Thanks for the pointers! I added an integration test for this feature. I used test/integration/connection/test-bulk-insert.js as a starting point.

[dougwilson]

Nice! Only one last nit: I cannot re-run this test without manually going into the db and dropping the table and trigger between the tests. Can you make the tests re-runnable? Typically the tests use temporary tables, for example. If you want to make a table and drop it manually, then make sure you perform the hook within a handler that will run even when the test fails.

[AdamVig]

Good catch! (And thank you for all the feedback.)

I added a query to drop the table after the test runs. I'm not sure it will run when the test fails, though. Can you give me an example of how to do that?

[dougwilson]

Hi @AdamVig I'll take a shot and creating an example when I get to a computer, but just wanted to respond now. Generally I would say that all code paths just need to lead to dropping the table in your code. For example, I would change it such that when CREATE TRIGGER fails, instead of just throwing an error and ending the test, I would make the callback drop the table and then throw the error. Then in your INSERT callback, I would just drop the table before even validating the expected response, and then delay the response validation until after the table has been dropped.

[AdamVig]

That explanation was helpful - I may not need an example after all. I am used to test frameworks with hooks like test.always(), so this approach is new to me.

My latest commit follows your advice, and I think it drops the table in all situations in which the test could fail.

[dougwilson]

Great! I think I'll just go ahead and wrap up this PR so it doesn't sit forever. The error handling doesn't work still, unfortunately, so I'll just tweak it to work. You can test this by making a typo in the CREATE TRIGGER and see it errors but the table is never dropped. This is because the DROP is queued to send over the network, but then the test is immediately crashed with the assert.ifError right below that line.

Changes made.

[AdamVig]

Thank you @dougwilson!