fix(users): User email to accept null regardless of unique

[farajfarook]

Provider Twitter (refer twitter.js file) is not producing an email address on callback. When two different users signs up using twitter the second user will not be able to sign up if the first user didn't set the email address manually.

This is due to the error of email already exist, in actual terms this is the null value which is conflicting the unique constraint.

[farajfarook]

@codydaig @ilanbiala @lirantal any updates or comments?

[ilanbiala]

@farajfarook can you give an example of what the data look like when you have the error and what the data look like with this PR?

[mleanos]

@farajfarook Does this http://stackoverflow.com/questions/7955040/mongodb-mongoose-unique-if-not-null describe what you're attempting to fix with this change?

For a change like this, since it's model related, I think we should have tests accompany this PR.

Side Note:
Are you able to properly authenticate with Twitter, using the master branch?

[farajfarook]

@ilanbiala it doesn't change any way in which data is stored in the DB. It simply accepts the creation of the second user who is having the email as null. Forget providers and twitter, just refer the code

 email: {
    type: String,
    unique: true,
    lowercase: true,
    trim: true,
    default: '',
    validate: [validateLocalStrategyEmail, 'Please fill a valid email address']
 },

It accepts blanks and also it's unique. - In the sense, it's not accepting two documents with the blank values to be stored in email field.

@mleanos Yes you are right. And yes I can properly authenticate with twitter. But the problem comes when the second user tries to authenticate with twitter.

[mleanos]

@farajfarook I think what @ilanbiala was asking, and I'm curious too, is what the User data looks like when this error occurs.

Is the email field present in the User's document, with a null value, or an empty string? Or does the email field get omitted? I probably would have guessed the latter to be true, without having looked at the email field and seeing the default value set.

I'm wondering if we even want to add the sparse setting to the User schema. My worry is that it could interfere with the index on the User collection, that may have adverse effects for certain users of this project since certain implementations could utilize indexing in many different ways; our forcing this option might not work for them. Also, sparse is a setting intended for use with indexes; the issue you're fixing here isn't necessarily related to indexing.

Would it be better to just remove the default: '' setting? Since it's not required, any User documents that aren't saved with the email field will just omit it from the document. Then we won't have this unused field, nor will we have the conflict.

[mleanos]

@farajfarook I just tested my suggested fix, and it didn't work. I just removed the default: '' setting. I verified that the email field wasn't present in the User document. However, I got the "Email already exists" error. It appears that even though the field doesn't exist in the document, mongodb still counts it as a null value; I expected it to not even recognize the field & to not create an index.

I then implemented your fix, and was able to resolve the issue. One potential hiccup that I read about, and experience with my testing, is that if the sparse setting is added to an existing field that has been indexed on the database, then the sparse setting won't be applied.

I think this is probably fine for a framework, but may cause some confusion or problems for users that upgrade an existing application with this fix.

@ilanbiala @codydaig @rhutchison Any thoughts?

After testing this, and doing some more research I think this is a good use for the sparse setting. Unless there are some other points made, this LGTM.

[ilanbiala]

@farajfarook @mleanos if that is the case, will reindexing fix the issue?

[mleanos]

@farajfarook Did you decide against this fix? Have you resolved the issue in a different way?

[mleanos]

@ilanbiala To get the sparse setting to work on a previously indexed field:

1. Stop the application
2. Add the sparse option to the field in the mongoose schema definition.
3. Drop the existing index for the field
4. Restart the application; mongoose will perform the ensureIndex method when the schema is registered.

This seems like a rather painless process for someone to perform. I would like it for this option to be implemented.

I came across this issue again as I was working on a bug I found in our GitHub passport strategy implementation. It's the exact scenario that @farajfarook is describing with Twitter. In our GitHub strategy, we're not handling a case where the email field could empty/null/missing from the API response. There's a simple fix for it, but I think it should be coupled with adding the sparse option to our Email field.

I'm going to submit a PR for the fix since it doesn't look like this PR will be re-submitted.

@farajfarook I apologize that I didn't recognize this to be the proper fix when you originally submitted this PR. I just didn't have enough information to know if it was the wise choice for us. I really appreciate you bringing this to our attention and introducing the sparse option.