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.

Sign up for GitHub

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

Jump to bottom

Add case insensitive indexes support #417

Merged
merged 1 commit into from
Oct 23, 2018
Merged

Add case insensitive indexes support #417

merged 1 commit into from
Oct 23, 2018

Conversation

angfal
Copy link
Contributor

@angfal angfal commented Feb 18, 2018

Description

MongoDB allows to create and search by case insensitive indexes. This fixes add an additional collation option for find requests to support this feature.

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@slnode
Copy link

slnode commented Feb 18, 2018

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@dhmlau
Copy link
Member

dhmlau commented Apr 4, 2018

@slnode test please

@angfal angfal requested a review from shimks as a code owner April 4, 2018 09:06
b-admike
b-admike reviewed Apr 4, 2018
View reviewed changes
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@angfal Thank you for your contribution! Could you also update the README to let users know how to leverage this feature? You can use code snippets from the test(s) you wrote as well.

test/mongodb.test.js Outdated
@@ -2016,6 +2041,22 @@ describe('mongodb connector', function() {
});
});

it('should allow to find using case insensitive index', function(done) {
Category.create({ title: 'My Category' }, function(err, category1) {
should.not.exists(err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo should.not.exists(err) -> should.not.exist(err)

test/mongodb.test.js Outdated
Category.create({ title: 'My Category' }, function(err, category1) {
should.not.exists(err);
Category.create({ title: 'MY CATEGORY' }, function(err, category2) {
should.not.exists(err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@angfal
Copy link
Contributor Author

angfal commented Apr 26, 2018

@b-admike Sorry, missed the email about the code review results. Fixed. Any additional suggestions according to README?

@dhmlau
Copy link
Member

dhmlau commented Apr 27, 2018

@slnode test please

@angfal
Copy link
Contributor Author

angfal commented Apr 29, 2018

Looks like someone downgrade MongoDB version which used to run tests. This fixes are required MongoDB 3.4+

@dhmlau
Copy link
Member

dhmlau commented Jun 8, 2018

@slnode test please
The logs in the build no longer exist. kicking off again

b-admike
b-admike approved these changes Jun 12, 2018
View reviewed changes
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@angfal Thank you for your contribution; LGTM

@dhmlau
Copy link
Member

dhmlau commented Jun 13, 2018

@slnode test please

@angfal
Copy link
Contributor Author

angfal commented Jun 13, 2018

Guys, the version of mongodb is too low that's why my tests fail. How are we going to resolve it? I can check the available mongodb version and skip my tests it if the version is less than 3.4 What do you think about this approach?

@dhmlau
Copy link
Member

dhmlau commented Sep 22, 2018

@angfal , could you please rebase your PR? Tests should be running fine now.

@angfal
Copy link
Contributor Author

angfal commented Sep 22, 2018

@dhmlau done!

@dhmlau
Copy link
Member

dhmlau commented Sep 24, 2018

@slnode test please

@angfal
Copy link
Contributor Author

angfal commented Sep 24, 2018

@dhmlau The same error. MongoDB version is less than 3.4
{ name: 'MongoError', message: 'server localhost:27017 does not support collation' }

b-admike
b-admike reviewed Sep 24, 2018
View reviewed changes
test/mongodb.test.js
it('should create case insensitive indexes', function(done) {
db.automigrate('Category', function() {
db.connector.db.collection('Category').indexes(function(err, result) {
var indexes = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's check for err object here first; if (err) done(err);

test/mongodb.test.js Outdated
@@ -2652,6 +2677,22 @@ describe('mongodb connector', function() {
});
});

it('should allow to find using case insensitive index', function(done) {
Category.create({title: 'My Category'}, function(err, category1) {
should.not.exist(err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, let's change this assertion to if (err) done(err); (see @bajtos' comment here for more context).

test/mongodb.test.js Outdated

Category.find({where: {title: 'my cATEGory'}}, {collation: {locale: 'en', strength: 1}},
function(err, categories) {
should.not.exist(err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (err) done(err);

test/mongodb.test.js Outdated
Category.create({title: 'My Category'}, function(err, category1) {
should.not.exist(err);
Category.create({title: 'MY CATEGORY'}, function(err, category2) {
should.not.exist(err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (err) done(err);

@angfal
Copy link
Contributor Author

angfal commented Sep 24, 2018

@b-admike Fixed.

Any ideas what to do withthe mongodb version error (my comment above)?

@dhmlau
Copy link
Member

dhmlau commented Sep 27, 2018

@angfal , our CI system is using mongodb version 3.2. Since it will be going to EOL this month, we're planning to update to a newer version of mongodb, as well as making "v3.4 or later" as the requirement. In this case, it will be a new semver-major version for this connector.

@angfal
Copy link
Contributor Author

angfal commented Sep 27, 2018

@dhmlau Got it. Let's wait the end of this month then and we can merge it. Thank you

@virkt25 virkt25 mentioned this pull request Sep 29, 2018
Merged
2 tasks
@dhmlau dhmlau mentioned this pull request Oct 22, 2018
Closed
6 tasks
@b-admike
Copy link
Contributor

@angfal Thank you for being patient. I've rebased your PR to kick off a new build in CI which now uses mongodb 3.4 thanks to @jannyHou. Once the tests are good (except Windows), we should be good to merge 👍

@b-admike
Copy link
Contributor

@slnode test please

@b-admike b-admike merged commit a93d107 into loopbackio:master Oct 23, 2018
@b-admike
Copy link
Contributor

Thank you @angfal! Your patch has landed 🎉

@angfal angfal deleted the #414 branch October 23, 2018 19:52
@dhmlau
Copy link
Member

dhmlau commented Nov 6, 2018

@angfal , your changes in this PR is in the newly released [email protected]. Thanks for your contribution.

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

@b-admike b-admike b-admike approved these changes

@dhmlau dhmlau Awaiting requested review from dhmlau dhmlau is a code owner

@jannyHou jannyHou Awaiting requested review from jannyHou

@virkt25 virkt25 Awaiting requested review from virkt25

Assignees
No one assigned
Labels
breaking-change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@angfal @slnode @dhmlau @b-admike