Update mongodb native to 2.0. Issue #233.

[andypham]

No description provided.

[tjwebb]

nice! thanks @andypham we'll look this over

[andypham]

Thanks Travis.

On Saturday, April 18, 2015, Travis Webb [email protected] wrote:

nice! thanks @andypham https://github.com/andypham we'll look this over

—
Reply to this email directly or view it on GitHub
#260 (comment)
.

[listepo]

+1

[mgenev]

I have a ticket open for the mongo 3.0 support in travis here... Hopefully it's fixed soon. travis-ci/travis-ci#3694

[andypham]

Good news! After upgrading travis to run mongo 2.6.5 all tests passed for node 0.10.x. Awaiting for node 0.11.x tests.

[mgenev]

Error: Failed to connect to MongoDB. Are you sure your configured Mongo instance is running?
Mongo not booting correctly or configured as expected?

[listepo]

Why nodejs 0.11.x? Must be 0.12.x for production

[andypham]

@mgenev Must be an intermittent issue because I can see mongo 2.6.5 loaded fine for node 0.10.x
@listepo Sure. I will update travis to run 0.12.x

[mgenev]

@andypham true, not sure what to make out of that, let's see if upgrading to 0.12 gets rid of it

[andypham]

Green light a GO! All test passed with node 0.10.x and 0.12.x running on mongo 2.6.5.
@tjwebb Can you approve this pull request? Thanks.

[tjwebb]

@andypham will this change make sails-mongo incompatible with older mongo versions?

[andypham]

@tjwebb This change updated the API breaking changes for mongo native driver 2.0. For example, some modifiers no longer take regex for mongo 2.6.5 and 3.0. Before this update the tests were already failing for these regex modifiers.

[dmarcelino]

This PR will put sails-mongo in much better shape! 👍

@devinivy, any thoughts about this?

For reference, this PR fixes #256 and #233.

[dmarcelino]

waterline-adapter-tests should be on devDependencies.

[devinivy]

Aside from @dmarcelino's comment, this looks really great. Thanks, @andypham !!

[andypham]

@devinivy Thanks for looking at this.

[particlebanana]

Without the regex query for something like $ne is it still case in-sensitve? I don't think we have tests for that anywhere.

[dmarcelino]

@particlebanana, https://github.com/balderdashy/waterline-adapter-tests/blob/master/interfaces/queryable/caseInsensitive.test.js addresses case-insensitive queries.

[andypham]

@particlebanana Using case in-sensitive by default will make our query slower. Here is how make regex case in-sensitive query.
http://docs.mongodb.org/manual/reference/operator/query/regex/

Edit: The regex doc above won't work for comparison operators with case in-sensitive. There a a few possible way of doing this:

1. Let the application manage it.
2. Use aggregation.
3. Use map-reduce.

[dmarcelino]

Should '<', 'lessThan', '<=', 'lessThanOrEqual' also be here?

[andypham]

@dmarcelino I didn't add the lessthan operator because waterline adapter test didn't complain. However, with my curiosity I just ran a few query for the lessthan operator with regex and it barf with error same as the above operators "BadValue Can't have RegEx as arg to predicate over field". I will add the lessthan to the list. Thanks.

[dmarcelino]

Regarding @particlebanana's comment above (#260 (comment)) I've added case insensitivity tests for not and greaterThan in balderdashy/waterline-adapter-tests#53. @andypham, can you pull those changes to your machine and check if this version of sails-mongo passes those tests?

By default the waterline API operates in case insensitivity mode (not saying it's good) so we need to abide by that. In order to get the best of both worlds, we can optionally add a caseSensitive config option that would allow the user to change that behaviour, sails-postgresql has this as an experimental feature.

Last, @andypham can you please remove waterline-adapter-tests from the package.json dependencies? It's already included in the devDependencies.

[particlebanana]

@andypham oh I know it's horrendously slow and doesn't allow you to use indexes but it's how the Waterline api works currently (started with MySql as first adapter). We can absolutely change this in a new major version of WL but we can't push breaking changes like that at the adapter level.

To get around this I added a feature flag on the postgres adapter that you can turn on at the application level to allow these like @dmarcelino mentioned. We could do something similar here.

[andypham]

@dmarcelino Let me pull in your test and try it out. Thanks for adding them.

[dmarcelino]

Good stuff, no worries.

[andypham]

@particlebanana The comparison operators can't take regex value as far as I know for mongo 2.6.5 and 3.0.2. So if we enable case-insensitive search mongo datastore itself would complain. I see that before this change the $ne operator had a similar issue with regex and implemented a similar fix. IMO, we need decide with the release version which mongo we should support. Running travis integration test and maintaining backward compatibility for these comparison operators might be quite a challenge.

[andypham]

Do you guys know which mongo version work for regex comparison operators?

[listepo]

Create beta version?

[dmarcelino]

@particlebanana, are regex comparisons widely supported in waterline or is it a sails-mongo specific feature? And have they been working in a case insensitive manner before?

If it's a sails-mongo specific feature and they haven't worked before in case insensitive manner I think we can safely proceed with this.

[andypham]

@particlebanana How should we move forward with this pull request? It looks like this text search case-insensitive issue has been tackled once before #235?
@andyhu Would appreciate your comment here.

[particlebanana]

@dmarcelino the regex comparisons from the waterline api aren't documented I don't believe, if they still exists. The following query should work the same between mongo and mysql though:

User.find({name: { '!': 'clark' }}).exec(/**/);

That should only return the one record from the following list:

[
  { name: 'clark' },
  { name: 'Clark' },
  { name: 'bob' }
]

That's how it works in other adapters unless I'm way off and it's been broken for awhile. If that's not possible to do any longer then we need to figure out the next steps.

As for wlNext obviously that should remove the case-insensitive search.

[dmarcelino]

@particlebanana, balderdashy/waterline-adapter-tests#53 adds the tests for User.find({name: { '!': 'clark' }}).exec(/**/); and also case insensitive greaterThan, more specifically caseInsensitive.test.js#153. These tests are currently broken in both sails-postgresql (balderdashy/sails-postgresql#156) and sails-mongo so I didn't merge yet.

@andypham, did your changes pass these tests when you pulled my waterline-adapter-tests?

@particlebanana, if it passes the tests can you merge this?

[andypham]

@dmarcelino Tests cases for case-insensitive failed.

[dmarcelino]

I see... honestly the tests are already broken (before your change), so I think we should move forward as your change improves things and doesn't break anything new. After merging your changes we should look into PR #235, as you suggested, to try and improve the case insensitive not/greaterThan cases.

@particlebanana, what do you think?

[andypham]

@dmarcelino Agreed. Lets move forward with this and figure out #235.

[dmarcelino]

Happy days, sails-mongo build is now green:

Thanks @particlebanana! Can you please also merge #255: Add badges and return non-zero if build breaks tests? It will help us detect errors earlier, namely during PRs.

And thanks @andypham 😉

[dmarcelino]

PR #235 looked like a potential fix for the case insensitive issue but after trying it the tests still fail, hence I've raised #265.