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

doesn't return error correctly when changing email to an already existing one. #12

Closed
beeplin opened this Issue Oct 22, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@beeplin
Copy link

beeplin commented Oct 22, 2016

I am using feathers-mongoose to handle mongo database on the server side, and make the users schema like this:

new Schema({
  username: { type: String, required: false, unique: true },
  email: { type: String, required: true, unique: true },
  password: { type: String, required: true },

  isVerified: { type: Boolean, required: false },
  verifyExpires: { type: Date, required: false },
  verifyToken: { type: String, required: false },
  resetExpires: { type: Date, required: false },
  resetToken: { type: String, required: false },

  slugType: {type: String, require: false},

  createdAt: { type: Date, 'default': Date.now },
  updatedAt: { type: Date, 'default': Date.now }
});

Here the username and email are both unique.

So, when I call changeEmail from client and try to change an email to an existing one, it will cause duplicate key error on the server side. But currently this error is not caught by users.update(..., function (err, user12) {...} source code here, and so the error is consumed on the server-side without feeding back to front-end, leading to a time-out error on the front-end.

I don't know why this error is not caught be users.update, might be a bug of feathers. But at least we can wrap a try/catch around the user.update block to avoid this, I guess.

@beeplin

This comment has been minimized.

Copy link
Author

beeplin commented Oct 22, 2016

another possible reason: this code uses call-back syntax of update instead of the recommended promise syntax. changing it to promise might solve the problem. coz I got this warning from the server console:

(node:44788) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 45): GeneralError: E11000 duplicate key error collection: xx.users index: email_1 dup key: { : "xxx" }

Seems promise can catch this reject, I guess.

@eddyystop

This comment has been minimized.

Copy link
Owner

eddyystop commented Oct 24, 2016

The expectation was that checkUniqueness would be used first to prevent any attempts to create duplicate keys. However you are right that changeEmail should handle this better.

Feathers internally uses promises. It calls callbacks from within .then and .catch. This causes issues first because the callback is running as a promise on the microtask queue. Secondly if the callback throws, the .catch is run and the callback may be called again.

These are reasons why Feathers wants to deprecate callbacks.

This repo will be converted to use promises internally. The present client-side callback interface will have to remain, but a promise-based one may be introduced.

First however changeEmail has to be changed. Second, some tests which fire up Feathers instances are needed as the current ones use fakes.

Could you please show me the hooks you use for your users service so the Feathers tests cover them? Thanks.

@beeplin

This comment has been minimized.

Copy link
Author

beeplin commented Oct 24, 2016

thanks for the explanations,. learned a lot.

i m currently travelling and might not be able to print my hooks right now. but i am sure they are just the default settings generated by feathers-cli with authentication and verificatiom enabled.

will print them here later.

@eddyystop

This comment has been minimized.

Copy link
Owner

eddyystop commented Oct 24, 2016

Please wait until this here issue is handled before using 0.6.1, as it'll include a fix to 0.6.1.

Its taken a while to figure out how to convert a promise to a callback so the promise's .catch chain doesn't get control should the callback throw.

eddyystop added a commit that referenced this issue Oct 25, 2016

Invoke Feathers service method calls as Promises not callbacks
- Feathers method calls now return Promises not call callbacks.
- Promises converted to callbacks within repo.
- This should fix issue #12
- We have nothing to do should Feathers deprecate callbacks.
- Positions repo to optionally return Promises.
@eddyystop

This comment has been minimized.

Copy link
Owner

eddyystop commented Oct 25, 2016

0.7.0 invokes Feathers service methods as Promises not as callbacks. The promises are converted to callbacks by the repo itself. The expectation is this new conversion would avoid swallowing the error described here.

This change positions the repo to optionally return Promises.

@beeplin

This comment has been minimized.

Copy link
Author

beeplin commented Oct 26, 2016

my hooks:

exports.before = {
  all: [],
  find: [
    auth.verifyToken(),
    auth.populateUser(),
    auth.restrictToAuthenticated()
  ],
  get: [
    auth.verifyToken(),
    auth.populateUser(),
    auth.restrictToAuthenticated(),
    auth.restrictToOwner({
      ownerField: '_id'
    })
  ],
  create: [
    auth.hashPassword(),
    verifyHooks.addVerification()
  ],
  update: [
    auth.verifyToken(),
    auth.populateUser(),
    //auth.hashPassword() // NOTE: verifyReset forbids hashPassword here
    auth.restrictToAuthenticated(),
    auth.restrictToOwner({
      ownerField: '_id'
    })
  ],
  patch: [
    auth.verifyToken(),
    auth.populateUser(),
    //auth.hashPassword() // NOTE: verifyReset forbids hashPassword here
    auth.restrictToAuthenticated(),
    auth.restrictToOwner({
      ownerField: '_id'
    })
  ],
  remove: [
    auth.verifyToken(),
    auth.populateUser(),
    auth.restrictToAuthenticated(),
    auth.restrictToOwner({
      ownerField: '_id'
    })
  ]
};

exports.after = {
  all: [hooks.remove('password')],
  find: [
    verifyHooks.removeVerification(true)
  ],
  get: [
    verifyHooks.removeVerification(true)
  ],
  create: [
    verifyHooks.removeVerification(true)
  ],
  update: [
    verifyHooks.removeVerification(true)
  ],
  patch: [
    verifyHooks.removeVerification(true)
  ],
  remove: [
    verifyHooks.removeVerification(true)
  ]
};
@eddyystop

This comment has been minimized.

Copy link
Owner

eddyystop commented Oct 26, 2016

Pushed 0.8.0. A rewrite was required to return promises from the API in addition to the current callbacks. 0.8.0 is basically 0.7.1 done right. Please read the CHANGELOG.

0.7.1 should have addressed the issue of the server potentially swallowing errors on duplicate keys. However 0.7.1 was around for only 2 days and you likely didn't test the duplicate key issue. Please let me know what your results are with 0.8.0.

The good news is that this repo will get absorbed into feathers-authentication at some point. The API will likely change but any code based on this repo has a future.

Still to do

  • More tests on Promises being returned.
  • Tests with real Feathers instances using your hooks.
  • Add SMS capabilities.
@beeplin

This comment has been minimized.

Copy link
Author

beeplin commented Oct 27, 2016

cool! I will test it asap.

and for SMS support, I got some better ideas. Will talk about that after done test.

@beeplin

This comment has been minimized.

Copy link
Author

beeplin commented Oct 27, 2016

tests are all OK. I tried to create new accounts with existing email, or to rename an old account to an existing name. Both return proper promise rejections.

Will talk about the SMS thing in the other issue.

@beeplin beeplin closed this Oct 27, 2016

eddyystop added a commit to feathers-plus/feathers-hooks-common that referenced this issue Oct 27, 2016

Added promiseToCallback, perhaps more rugged than Feathers' code
The callback is invoked outside the scope of the Promise.
The Promise's .catch() chain is not invoked if the callback throws.
This code is used within feathers-service-verify-reset to resolve
eddyystop/feathers-service-verify-reset#12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.