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

Fix issues with string search #235

Closed
wants to merge 7 commits into from
Closed

Fix issues with string search #235

wants to merge 7 commits into from

Conversation

dzcpy
Copy link
Contributor

@dzcpy dzcpy commented Jan 21, 2015

We shouldn't change all string searches into regexp ones. It will invalid all indexes and greatly hurt performance.
Tasks for converting wildcard characters like % to regexp are already done previously and we should only do it when it's with certain matched modifier like like. So in parseValue method we should not do it again.
Let's say in some edge cases, when a user wants to search string with null or true or anything contains '%' as a string value, he / she cannot do it since it's been converted to other types and will return wrong result.
Also if the schema type is date or datetime, it's better to support string type date query, they might from a blueprint query so it can't be searched if we don't convert it to Date object.

dzcpy added 4 commits January 21, 2015 15:05
We shouldn't change all string searches into regexp search. It will invalid all indexes and greatly hurts performance. Tasks for converting wildcard characters like `%` to regexp are already done previously with matched modifier so in `parseValue` method we no need to do it again. Let's say in some edge cases, when a user wants to search string with `null` or `true` or something contains '%' as a string value, he/she cannot do it since it's been converted to regexp. Also if the schema type is `date` or `datetime`, it's better to support string type date query, they might from a blueprint query so it can't be searched if we don't convert it to `Date` object.
Why should we convert the string to regexp? Since when doing mongodb search, there's no way to do a case sensitive search then
@particlebanana
Copy link
Contributor

It undocumented but I just added support for what I'm calling wl-next features into Waterline sequel. The first one added is case sensitive search. The reason we have case insensitve search in the first place is from the days when waterline only worked on mysql.

The way wl-next is implemented is that you add a flag on the connection config like so:

mysql: {
    adapter : 'sails-mysql',
    host    : 'localhost',
    user    : 'root',
    database: 'sails_mysql',
    wlNext: {
        caseSensitive: true
    }
},

Which then gets passed down to the adapter config and in the sql case passed to waterline-sequel to build a sql query. balderdashy/waterline-sequel@62ec93e

If we add that to the mongo adapter it would be great! I can't merge something that breaks the backwards functionality and makes searches case sensitive but I CAN merge something that uses that flag to turn the feature on.

@dzcpy
Copy link
Contributor Author

dzcpy commented Jan 22, 2015

@particlebanana I understand your concern. So if I modify it a bit according to your suggestion, will it be merged?
The case insensitive stuff isn't the only thing in this PR. There are also some functional errors need to be fixed. Also, currently a user can't query a date typed field via blueprint in any way. These fixes are all included in this PR.

@dzcpy
Copy link
Contributor Author

dzcpy commented Jan 22, 2015

By the way, wlNext option looks a bit weird in the configuration file. Can we simply use this?

mysql: {
    adapter : 'sails-mysql',
    host    : 'localhost',
    user    : 'root',
    database: 'sails_mysql',

   // wlNext options:
    caseSensitive: true
},

@dmarcelino
Copy link
Member

👍 for:

   // wlNext options:
    caseSensitive: true

@dmarcelino
Copy link
Member

@andyhu, I've added case insensitivity tests for not and greaterThan in balderdashy/waterline-adapter-tests#53. Can you pull those changes to your machine and check if your changes make sails-mongo pass those tests? Thanks!

cc: @andypham

@dmarcelino
Copy link
Member

I've pulled theses changes and ran the case insensitivity tests for not and greaterThan and they still fail.

@dmarcelino
Copy link
Member

Raised #265 to address the case insensitive issue.

@particlebanana
Copy link
Contributor

Going with #238 via #380

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants