Skip to content
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

Throw the original error from authenticate #4209

Merged
merged 2 commits into from
Aug 5, 2015

Conversation

seanjh
Copy link
Contributor

@seanjh seanjh commented Jul 29, 2015

When the authenticate method catches errors, it wraps those errors in a new Error() call, rather than just throwing the original error. This gobbles up the original name and message into the new error's message parameter, making it more difficult to make use of the error result than necessary.

What's currently happening:

var seqError = new sequelize.Error('YOLO');
try {
  throw new Error(seqError);
} catch (e) {
  if (e instanceof sequelize.Error) {
    console.log('Hooray, I can tell this is a Sequelize error!');
  } else {
    console.log('Bummer. I cannot easily distinguish this from a generic error.');
  }
}
Bummer. I cannot easily distinguish this from a generic error.

What would be preferred:

var seqError = new sequelize.Error('YOLO');
try {
  throw seqError;
} catch (e) {
  console.log(util.inspect(e));
  if (e instanceof sequelize.Error) {
    console.log('Hooray, I can tell this is a Sequelize error!');
  } else {
    console.log('Bummer. I cannot easily distinguish this from a generic error.');
  }
}
Hooray, I can tell this is a Sequelize error!

@@ -932,7 +932,7 @@ Sequelize.prototype.drop = function(options) {
*/
Sequelize.prototype.authenticate = function(options) {
return this.query('SELECT 1+1 AS result', Utils._.assign({ raw: true, plain: true }, options)).return().catch(function(err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not even sure we need a catch clause here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catching to wrap an error in an error, the original quote was definitely wonky.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @seanjh Can you remove this cath completely, then I'll merge :)

@janmeier
Copy link
Member

Yeah, makes sense :)

janmeier added a commit that referenced this pull request Aug 5, 2015
Throw the original error from authenticate
@janmeier janmeier merged commit 55946ad into sequelize:master Aug 5, 2015
@janmeier
Copy link
Member

janmeier commented Aug 5, 2015

Thanks! :)

janmeier added a commit that referenced this pull request Aug 5, 2015
@seanjh seanjh deleted the throw-original-auth-error branch August 5, 2015 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants