Add Connection Specific Errors #2576

DavidTPate · 2014-11-14T03:53:04Z

This PR was created to address #2567, it adds new error constructors related specifically to connections. Each constructor inherits from the newly created ConnectionError constructor which inherits from the BaseError constructor. It is created in the same vein as the errors emitted from MySQL Queries. Only the errors related to connections were wrapped, there are some unrelated ones within the Connection Managers (such as checking that the required dialect package is installed) which I did not modify.

…anager for MySQL to utilize these errors.

…n manager to use the new errors.

…rs so they aren't breaking up the grouping.

…Added tests for the errors.

janmeier · 2014-11-14T07:17:46Z

Code looks good, however, some of the tests fail

This test https://github.com/sequelize/sequelize/blob/master/test/sequelize.test.js#L121 should probably just be changed to something like expect(this.sequelizeWithInvalidConnection.authenticate()).to.eventually.throw... instead of asserting on the error message

janmeier · 2014-11-14T07:19:13Z

lib/errors.js

+ * @constructor
+ */
+error.ConnectionTimedOutError = function (parent) {
+  error.ConnectionTimedOutError.call(this, parent);


The error is calling it self, which results in a stack overflow (also applies to some of the other errors

… is propagated from the connection failure. As pointed out in the [PR](#2576 (comment)) for this functionality we would ideally not be checking error messages and instead be checking the constructors of the objects to make sure that they are an instance of the correct error type. There's some issues right now with two different constructors being used and not being able to use `instanceof`. We could fall back to the constructor name or some other check, but at the end of the day that isn't really any better than just checking the message.

DavidTPate · 2014-11-14T18:25:40Z

@janmeier I went through and toyed with it some to change the tests to instead check the types that would be returned but I ran into a few issues. It looks like the constructor being used to create the Errors is a different instance from that which is available within the Tests which is causing instanceof to fail.

I'm unfamiliar with the composition of Sequelize and I don't know if you've already ran into this problem (or if I'm doing something wrong based on your current setup). So instead for the time being I just updated the error message checks to match what is returned from the connection failure. If you have some suggestions or a concrete example of where this is used with Sequelize I'd be more than glad to revisit it and update the tests.

janmeier · 2014-11-19T16:08:25Z

test/configuration.test.js

@@ -19,7 +19,7 @@ describe(Support.getTestDialectTeaser("Configuration"), function() {

      var seq = new Sequelize(config[dialect].database, config[dialect].username, config[dialect].password, {storage: '/path/to/no/where/land', logging: false, host: '0.0.0.1', port: config[dialect].port, dialect: dialect})
      seq.query('select 1 as hello').error(function(err) {
-        expect(err.message).to.match(/Failed to find (.*?) Please double check your settings\./)
+        expect(err.message).to.match(/connect EINVAL/)
        done()


Remove the done callback from the function and do

return expect(seq.query('select 1 as hello')).to.eventually.be.rejectedWith(seq.InvalidConnectionError, 'connect EINVAL');

Thanks, I'll get those updated.

janmeier · 2014-11-19T16:09:14Z

For reference

https://github.com/domenic/chai-as-promised/blob/master/test/should-promise-specific.coffee

Also, I noticed that some of the tests in configuration.test are PG only - do they pass for mysql also now?

janmeier · 2014-11-19T16:10:00Z

lib/dialects/mysql/connection-manager.js

+          case 'ECONNREFUSED':
+            reject(new sequelizeErrors.ConnectionRefusedError(err));
+            break;
+          case 'ER_ACCESS_D2ENIED_ERROR':


This will never trigger because of the 2 - it was because we couldn't reliably catch the error at the time the codewas written as far as I rememeber - perhaps we can now?

janmeier · 2014-11-27T09:51:44Z

Ping @DavidTPate Just need those tests updated, and maybe you can also address my other comment about access denied :)?

DavidTPate · 2014-11-30T15:02:29Z

@janmeier Will do, just been a busy time around the office so I haven't had a chance.

…r checking and also checking the actual type of error thrown.

…y match if able.

…gres native to see if they are passing now.

…the ConnectionError constructor so that it can handle a falsey error object being sent as a parameter. Update the configuration tests to support SQLite and it's custom messages as well as the lack of authentication.

DavidTPate · 2014-12-04T20:46:09Z

@janmeier All the tests are updated and look to be good. I removed the PG only restrictions and they are all working as expected as well. To the expect calls I just had to add a notify call so that it would call done once completed. I also removed the 2 in ER_ACCESS_D2ENIED_ERROR so that those are actually captured when using the mysql dialect.

janmeier · 2014-12-04T20:47:23Z

test/sequelize.test.js

@@ -104,7 +104,7 @@ describe(Support.getTestDialectTeaser("Sequelize"), function () {
            })
        })

-        it('triggers the actual adapter error', function(done) {
+        it.only('triggers the actual adapter error', function(done) {


janmeier · 2014-12-04T20:48:36Z

@DavidTPate Looks good - However you accidentally added an it.only ;)

Weird that you had to call notify to get it working. Did you remove the done callback completely, so the function takes no args?

…o be removed.

DavidTPate · 2014-12-04T21:24:41Z

@janmeier I didn't remove the done callback completely because I wasn't aware that chai handles eventually.be.rejectedWith synchronously. Based on your comment I went ahead and removed the notify call and done from the arguments and it was working as expected. I made sure to make the test fail as well to ensure that it would still catch failures even though it is not operating asynchronously and it worked as expected.

Looks like an issue with mariadb tests has surfaced now since I've removed the .only. I'll look into it.

janmeier · 2014-12-04T21:27:43Z

@DavidTPate Everything that comes from a chain of eventually assertions returns a promise (courtesy of chai-as-promised). When you return that promise chain, mocha waits for it to complete, before completing the test (unless your function takes an argument :) )

janmeier · 2014-12-04T21:28:47Z

An issue to say the least - a segfault :O ...

…usly running for MariaDB to try and narrow down the issue.

…t to do flow control on it...

DavidTPate · 2014-12-04T21:45:40Z

@janmeier I added the restrictions for those 2 methods that used to be PG only to not run for MariaDB (still runs for everything else) and everything went off without a hitch. I'm still not sure what the root cause of it is, but this brings it to the same state of testing as it was before my updates, should that be addressed in a separate issue?

…lure-errors Add Connection Specific Errors

janmeier · 2014-12-08T20:29:15Z

Solid work @DavidTPate !

janmeier · 2014-12-08T20:30:27Z

You're very welcome to continue to debug the two mariadb errors in a separate issue :)

DavidTPate added 6 commits November 13, 2014 21:34

Create new errors related to connections and updated the Connection M…

9ef2cf5

…anager for MySQL to utilize these errors.

Added logic for when an error doesn't have an code property.

ffc35bb

Added an error for connections timing out. Updated Postgres connectio…

6e0cf9d

…n manager to use the new errors.

Wrapped the errors for Maria DB in the new ConnectionError object.

043502b

Moved the connection errors to after the Database and Validation erro…

1a76270

…rs so they aren't breaking up the grouping.

Added the connection errors to Sequelize just like the other errors. …

6a05659

…Added tests for the errors.

janmeier reviewed Nov 14, 2014
View reviewed changes

janmeier self-assigned this Nov 14, 2014

DavidTPate added 2 commits November 14, 2014 09:25

Fixed errors calling themselves 🤦

2d844be

janmeier reviewed Nov 19, 2014
View reviewed changes

DavidTPate added 4 commits December 4, 2014 14:50

Updated tests so that they are relying a little bit more on expect fo…

9465875

…r checking and also checking the actual type of error thrown.

Removed the 2 from ER_ACCESS_DENIED_ERROR so that it will actuall…

2ed9eaf

…y match if able.

Removed filters only allowing the tests to occur on Postgres and Post…

28ebbb7

…gres native to see if they are passing now.

janmeier reviewed Dec 4, 2014
View reviewed changes

DavidTPate added 2 commits December 4, 2014 16:19

Remove .only 🤦

cabd7ef

Remove done and make the tests synchronous so that notify can als…

6df0b93

…o be removed.

Filtering out mariadb for the configuration tests that weren't previo…

6e72f2c

…usly running for MariaDB to try and narrow down the issue.

Added in a return call. Was adding this from the old code and forgo…

0de0fa2

…t to do flow control on it...

janmeier added a commit that referenced this pull request Dec 8, 2014

Merge pull request #2576 from DavidTPate/feature/mysql-connection-fai…

5b1dee6

…lure-errors Add Connection Specific Errors

janmeier merged commit 5b1dee6 into sequelize:master Dec 8, 2014

janmeier added a commit to janmeier/sequelize that referenced this pull request Dec 8, 2014

Add changelog for sequelize#2576, and remove console.log

9e9cfb0

DavidTPate deleted the feature/mysql-connection-failure-errors branch December 9, 2014 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Connection Specific Errors #2576

Add Connection Specific Errors #2576

DavidTPate commented Nov 14, 2014

janmeier commented Nov 14, 2014

janmeier Nov 14, 2014

DavidTPate commented Nov 14, 2014

janmeier Nov 19, 2014

DavidTPate Nov 19, 2014

janmeier commented Nov 19, 2014

janmeier Nov 19, 2014

janmeier commented Nov 27, 2014

DavidTPate commented Nov 30, 2014

DavidTPate commented Dec 4, 2014

janmeier Dec 4, 2014

janmeier commented Dec 4, 2014

DavidTPate commented Dec 4, 2014

janmeier commented Dec 4, 2014

janmeier commented Dec 4, 2014

DavidTPate commented Dec 4, 2014

janmeier commented Dec 8, 2014

janmeier commented Dec 8, 2014

Add Connection Specific Errors #2576

Add Connection Specific Errors #2576

Conversation

DavidTPate commented Nov 14, 2014

janmeier commented Nov 14, 2014

janmeier Nov 14, 2014

Choose a reason for hiding this comment

DavidTPate commented Nov 14, 2014

janmeier Nov 19, 2014

Choose a reason for hiding this comment

DavidTPate Nov 19, 2014

Choose a reason for hiding this comment

janmeier commented Nov 19, 2014

janmeier Nov 19, 2014

Choose a reason for hiding this comment

janmeier commented Nov 27, 2014

DavidTPate commented Nov 30, 2014

DavidTPate commented Dec 4, 2014

janmeier Dec 4, 2014

Choose a reason for hiding this comment

janmeier commented Dec 4, 2014

DavidTPate commented Dec 4, 2014

janmeier commented Dec 4, 2014

janmeier commented Dec 4, 2014

DavidTPate commented Dec 4, 2014

janmeier commented Dec 8, 2014

janmeier commented Dec 8, 2014