fix(core): Support WiredTiger engine errmsg format in MongoDB 3.2

[QiyuLi]

The new WiredTiger engine is introduced in MongoDB 3.2.
It changes the output errmsg format for violation of unique index.
This commit adds support for the new format.

Fixes #1245

[trendzetter]

Seems a better solution than previously proposed hacks. Haven't tried it yet.

[trendzetter]

Just tried it. No issues for me. I am running mongod 3.2.3 but the mongodb libraries that npm installs are 2.1.7 (I think that is strange but it works).

[QiyuLi]

@tfernandez The latest driver is 2.1.7
https://github.com/mongodb/node-mongodb-native/releases

[trendzetter]

Possibly my data is not in WiredTiger since I upgraded an existing database.

[QiyuLi]

Yup, existing db file will cause mongodb to use old MMapV1 engine (default engine in mongodb 3.0).

[mleanos]

This change isn't needed. using toLowerCase() and containEql() is better here.

[mleanos]

@QiyuLi This looks pretty good. I left a few comments.

Thanks for the fix :)

[QiyuLi]

@mleanos
The reason why I'm changing it back to userInfoRes.body.message.should.equal('Email already exists'); is:

• to make it consistent with E2E test. See https://github.com/QiyuLi/mean/blob/fix/supoort_mongo_3.2/modules/users/tests/e2e/users.e2e.tests.js#L257-L295
• The expected value is displayed on browser (as warning message to user). So it should be precise.
  For example,
  If we use toLowerCase().should.containEql(...
  Then all test will pass but user will see this:
  

I will change it to !== -1

[mleanos]

@QiyuLi I don't think we need the assertion there to be precise. Also, the assert should have zero impact on what is displayed to the user. IMO, it's just an unnecessary change.

With that said, I'm fine with you leaving it the way you have it, since it was originally that way before the recent changes; unless someone else has objections.

Thanks for explaining your reasoning.

[QiyuLi]

@mleanos I re-submit the commit including the fix !== -1

[mleanos]

@QiyuLi Thank you. LGTM now.

@lirantal @ilanbiala @codydaig LGTY?

Also, are you we ignoring the bitHound builds? They never seem to complete :)

[ilanbiala]

@QiyuLi Why add in more complex support and revert the changes instead of using the looser validation that covers both?

[trendzetter]

@ilanbiala https://cloud.githubusercontent.com/assets/5421502/13476128/5fab7546-e094-11e5-9e37-e4394f7653f8.png I think it is solving a usability issue in the ui too. The test is giving a valid alert and should not be loosened.

[QiyuLi]

@ilanbiala trendzetter did a good summary.
Loosened assert will cause improper alert display on user screen.

[trendzetter]

I have come to understand that this issue and pr should account as a serious warning about code quality and applying best practices. I didn't know at the time I loosened the tests what I was actually doing because I wasn't very familiar with the tests and the issue didn't show up with me. Someone forwarded me this "fix" to include. Good tests should not be fixed rather the code should be fixed to behave according to the use cases. I should not have been allowed to modify the tests. If someone suggests to fix the tests you should always question if there is really something wrong with the tests or is it mangling the tests to suit the issue.

[mleanos]

We definitely need to reconcile the differences in the validation errors that newer versions of MongoDB are generating. That's the main point of the fix here.

As for whether or not the tests need to change, it's up for debate. The decision was made previously to use containEql & toLowerCase; which I think was a good decision.

To be clear, the asserts (in the tests) have no bearing on what messages are sent to the UI. I think there might be some confusion there with the terminology.

I think the fix here is fine, and don't have a super strong opinion whether or not to change the assertions.

@simison mentioned a possible solution with the mongoose-beautiful-unique-validation package. I looked at it & think it's very promising. I can't tell if there are any MongoDB version compatibility issues. If we want to explore this package, I think it could be helpful to have others test this. Preferably using versions of MongoDB > 3.0.7 (as that's what I have tested it with). I pushed up a branch to my fork for anyone that want's to test.

mleanos@84d8c02

[trendzetter]

@mleanos I disagree that it is a good decision to add containEql and tolowercase.
Sending "Email already exists" or """t"'y!ç( b§"ç!"bàv ç!,ute eMAIL aLREaDy eXists" would both satisfy the test but only one is acceptable to send to the UI. These test were testing an exact string with a good reason.

To be clear, the asserts (in the tests) have no bearing on what messages are sent to the UI.

I am not sure if I understand you correctly here. The tests do test what is send to the UI, right? Isn't testing what is in the UI exactly the point of e2e tests?

[mleanos]

What i meant was that we're merely assertng against what is sent to the Ui. So if we use containEql or eql doesn't affect the message sent to the Ui.

[trendzetter]

The e2E test should give an error if what is send is not acceptable output. The exact check does that but the loosened check doesn't.

[mleanos]

@trendzetter You're definitely making a good point. However, I'm not sure we have to be so strict in this case. With that said, I'm fine with the change in this PR.

[trendzetter]

@mleanos If you are not strict you allow for all kinds of weirdness to get through, exactly the opposite of why you added tests. The goal of the e2e or system tests is to get an early warning during development that you have broken end user functionality. This test did in fact just that. I (and others) didn't respond correctly by changing the test instead of looking for the issue. One should only change the old test to the new if the test is checking things that aren't required. I would say that in the case of our software, clear readable errors with normal casing and punctuation is a requirement (although I haven't seen any formal requirements or use cases for this software).
It took me a while to wake up to this but now I remember clearly what we learned during the lessons TDD , computer science (yes, I am a qualified software developer, just new to advanced javascript)

Am I making sense or do I give the impression of a weird fanatic? I can't stress enough how important it is to have the correct understanding about this if you want to benefit from testing.

[mleanos]

@trendzetter I totally understand, and you're just trying to get your point across. I thank you for that.

Let's wait to see if others have an opinion, and if @ilanbiala has more insight. As I said before, I'm indifferent about the use of containEql vs eql at this point.

[ilanbiala]

@trendzetter might be right, but honestly I question our use of the error message directly in the UI. I think that's also an issue. Let's just stick with eql since it is actually what we want and not just some part of the string being that.

[mleanos]

@QiyuLi I think this is ready. I'll merge over the weekend, if no other concerns are brought up.

[trendzetter]

Sorry for the negative tone. I will be glad if those line are restored. It was really a shameful mistake I managed to sneak in. its encouraging and refreshing seeing others catching such mistakes.

[simison]

@trendzetter remember that contributing to open source is all about team work, being polite and compromising sometimes. And thanks for your and anyone else's efforts on meanjs! Have a fun and good day. :-)
</ offtopic >

[trendzetter]

👍

[mleanos]

Thank you @QiyuLi & @trendzetter for helping out here.