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

Add Connection Specific Errors #2576

Merged
merged 16 commits into from Dec 8, 2014

Conversation

2 participants
@DavidTPate
Contributor

DavidTPate commented Nov 14, 2014

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.

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 14, 2014

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

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

This comment has been minimized.

@janmeier

janmeier Nov 14, 2014

Member

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

@janmeier janmeier self-assigned this Nov 14, 2014

DavidTPate added some commits Nov 14, 2014

Updated checks of error messages to use the new values which are what…
… 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

This comment has been minimized.

Contributor

DavidTPate commented Nov 14, 2014

@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.

@@ -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()

This comment has been minimized.

@janmeier

janmeier Nov 19, 2014

Member

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');

This comment has been minimized.

@DavidTPate

DavidTPate Nov 19, 2014

Contributor

Thanks, I'll get those updated.

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 19, 2014

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?

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

This comment has been minimized.

@janmeier

janmeier Nov 19, 2014

Member

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

This comment has been minimized.

Member

janmeier commented Nov 27, 2014

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

@DavidTPate

This comment has been minimized.

Contributor

DavidTPate commented Nov 30, 2014

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

DavidTPate added some commits Dec 4, 2014

Updated tests so that they are relying a little bit more on expect fo…
…r checking and also checking the actual type of error thrown.
Removed filters only allowing the tests to occur on Postgres and Post…
…gres native to see if they are passing now.
Wrap errors from SQLLite in the new connection error objects. Update …
…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

This comment has been minimized.

Contributor

DavidTPate commented Dec 4, 2014

@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.

@@ -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) {

This comment has been minimized.

@janmeier

janmeier Dec 4, 2014

Member

Woops :)

@janmeier

This comment has been minimized.

Member

janmeier commented Dec 4, 2014

@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?

DavidTPate added some commits Dec 4, 2014

@DavidTPate

This comment has been minimized.

Contributor

DavidTPate commented Dec 4, 2014

@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

This comment has been minimized.

Member

janmeier commented Dec 4, 2014

@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

This comment has been minimized.

Member

janmeier commented Dec 4, 2014

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

Filtering out mariadb for the configuration tests that weren't previo…
…usly running for MariaDB to try and narrow down the issue.
@DavidTPate

This comment has been minimized.

Contributor

DavidTPate commented Dec 4, 2014

@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?

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

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

Add Connection Specific Errors

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

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

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

@janmeier

This comment has been minimized.

Member

janmeier commented Dec 8, 2014

Solid work @DavidTPate !

@janmeier

This comment has been minimized.

Member

janmeier commented Dec 8, 2014

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

@DavidTPate DavidTPate deleted the DavidTPate:feature/mysql-connection-failure-errors branch Dec 9, 2014

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