Add ilike and nilike operators #1091

duffn · 2016-09-16T12:57:33Z

Closes #633

slnode · 2016-09-16T12:57:34Z

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

slnode · 2016-09-16T12:57:35Z

Can one of the admins verify this patch?

slnode · 2016-09-16T12:57:35Z

Can one of the admins verify this patch?

slnode · 2016-09-16T12:57:35Z

Can one of the admins verify this patch?

duffn · 2016-09-16T13:02:23Z

Originally talked about here: #633 (comment)

The original issue was closed because regexp was added and it was thought that we didn't need ilike and nilike because regexp could handle all situations. However, we should have ilike and nilike operators as some databases may not support case-insensitive regexp.

superkhau · 2016-09-17T01:36:40Z

Can you add some tests to verify your changes and prevent regressions in the future?

duffn · 2016-09-17T14:38:46Z

I added tests for ilike and nilike which also required that I added those two operators to the memory connector.

I also added like and nlike tests that were missing.

superkhau

Please run the linter after you make your changes -- npm run lint or npm test (which runs the linter after your tests pass).

superkhau · 2016-09-19T02:03:06Z

lib/dao.js

@@ -1661,7 +1665,8 @@ DataAccessObject._coerce = function(where) {
          operator = 'regexp';
        } else if (operator === 'regexp' && val instanceof RegExp) {
          // Do not coerce regex literals/objects
-        } else if (!((operator === 'like' || operator === 'nlike') && val instanceof RegExp)) {
+        } else if (!((operator === 'like' || operator === 'nlike' ||
+          operator === 'ilike' || operator === 'nilike') && val instanceof RegExp)) {


Please fix indentation. See our style guide at https://github.com/strongloop/loopback-contributor-docs/blob/master/style-guide.md#indentation-of-multi-line-expressions-in-if

I've corrected per the style guide, but the linter does not give any errors or warnings on any of the files.

The linter seems to fail to recognize indentation sometimes. See these lines:

When not feasible, then indent the second and next lines by two levels.
One level of indentation makes it difficult to tell the difference between the condition and the branch body.

Read the good/bad examples: https://github.com/strongloop/loopback-contributor-docs/blob/master/style-guide.md

superkhau · 2016-09-19T02:03:53Z

test/basic-querying.test.js

@@ -469,6 +469,58 @@ describe('basic-querying', function() {
      });
    });

+    it('should support "like" that is satisfied', function(done) {
+      User.find({where: {name: {'like': 'John'}}}, function(err, users) {


remove single quotes around like

I've removed all single quotes.

superkhau · 2016-09-19T02:04:28Z

test/basic-querying.test.js

@@ -469,6 +469,58 @@ describe('basic-querying', function() {
      });
    });

+    it('should support "like" that is satisfied', function(done) {
+      User.find({where: {name: {'like': 'John'}}}, function(err, users) {
+        should.not.exist(err);


if (err) return done(err) for better stack traces.

I've updated all per the suggestion. This does not match the rest of the tests in this file - I can update those in another PR.

Thanks, this is fine. All those other parts of the file need to change to this style anyways (if you would like to submit a PR for that, I can help approve that one too ;)).

If you decide to go and refactor that file, you can rename the set of tests in this PR too -- the wording is pretty bad -- it('supports the "like" operator', function(...) or something along those lines would be better.

superkhau · 2016-09-19T02:04:55Z

test/basic-querying.test.js

+    it('should support "like" that is satisfied', function(done) {
+      User.find({where: {name: {'like': 'John'}}}, function(err, users) {
+        should.not.exist(err);
+        users.should.have.property('length', 1);


users.length.should.equal(1);

I've updated all per the suggestion. This does not match the rest of the tests in this file - I can update those in another PR.

superkhau · 2016-09-19T02:05:21Z

test/basic-querying.test.js

+    });
+
+    it('should support "like" that is not satisfied', function(done) {
+      User.find({where: {name: {'like': 'Bob'}}}, function(err, users) {


Remove single quotes around like

superkhau · 2016-09-19T02:06:36Z

test/basic-querying.test.js

+
+    it('should support "ilike" that is not satisfied', function(done) {
+      User.find({where: {name: {'ilike': 'bob'}}}, function(err, users) {
+        should.not.exist(err);


superkhau · 2016-09-19T02:06:40Z

test/basic-querying.test.js

+    it('should support "ilike" that is not satisfied', function(done) {
+      User.find({where: {name: {'ilike': 'bob'}}}, function(err, users) {
+        should.not.exist(err);
+        users.should.have.property('length', 0);


superkhau · 2016-09-19T02:06:45Z

test/basic-querying.test.js

+    });
+
+    it('should support "nilike" that is satisfied', function(done) {
+      User.find({where: {name: {'nilike': 'john'}}}, function(err, users) {


superkhau · 2016-09-19T02:06:47Z

test/basic-querying.test.js

+
+    it('should support "nilike" that is satisfied', function(done) {
+      User.find({where: {name: {'nilike': 'john'}}}, function(err, users) {
+        should.not.exist(err);


superkhau · 2016-09-19T02:06:51Z

test/basic-querying.test.js

+    it('should support "nilike" that is satisfied', function(done) {
+      User.find({where: {name: {'nilike': 'john'}}}, function(err, users) {
+        should.not.exist(err);
+        users.should.have.property('length', 5);


superkhau · 2016-09-19T19:38:32Z

test please

superkhau · 2016-09-21T02:15:23Z

@duffn The unfortunate part about this PR is that adding in those operations causes all the downstream connectors to fail (because they have no implementation). Do you have time to submit PRs for those connectors too? This feature should really go in together with the connector updates.

duffn · 2016-09-21T02:57:44Z

Oh my. That's a lot, but I'll consider it. If I do this, is there anything special I should do to coordinate?

superkhau · 2016-09-21T20:49:52Z

Oh my. That's a lot, but I'll consider it. If I do this, is there anything special I should do to coordinate?

The ugly thing is that once you create those PRs, we'll need to merge in this PR first before the other PRs pass. I'm reluctant to merge this in until we have PRs for those other connectors too. @bajtos @raymondfeng We really need to do something about this, what should we do in this case? This PR is LGTM, but downstream connectors will fail if I merge.

superkhau · 2016-09-22T00:11:49Z

Related issue for postgres connector: loopbackio/loopback-connector-postgresql#93 (comment)

bajtos · 2016-09-22T08:07:40Z

The ugly thing is that once you create those PRs, we'll need to merge in this PR first before the other PRs pass. I'm reluctant to merge this in until we have PRs for those other connectors too. @bajtos @raymondfeng We really need to do something about this, what should we do in this case? This PR is LGTM, but downstream connectors will fail if I merge.

IMO, we need a way for connectors to signal what features they support. The shared test suite in juggler should skip tests for features that are not supported.

cc @raymondfeng @Amir-61

duffn · 2016-09-22T15:11:18Z

So regarding the regex discussion here loopbackio/loopback-connector-postgresql#93 (comment), that's fine for PostgreSQL and likely many databases, but I'm writing a connector for Redshift and it does not support case-insensitive regex, however, it does support ilike, hence this PR.

Amir-61 · 2016-09-22T18:31:02Z

@duffn @superkhau

IMO, we need a way for connectors to signal what features they support. The shared test suite in juggler should skip tests for features that are not supported.

I believe you can do similar approach to what @bajtos and I did in PR#866.

CC: @bajtos and @raymondfeng

superkhau · 2016-09-22T18:57:31Z

@Amir-61 I agree with the land and add unsupported test case approach for now. We'll need to revisit how connectors broadcast what they support as stated by @bajtos at #1091 (comment).

superkhau · 2016-09-22T19:58:37Z

IMO, we need a way for connectors to signal what features they support. The shared test suite in juggler should skip tests for features that are not supported.

This is one key point for the DAO architecture discussions that @Amir-61 is working on.

superkhau · 2016-09-22T21:54:26Z

@bajtos @raymondfeng Any final comments before landing?

superkhau · 2016-09-22T21:55:31Z

@duffn Can you help with submitting PRs for the unsupported comment in the downstream connectors (before landing this so at least we have warning messages to cover those areas).

bajtos · 2016-09-23T13:54:04Z

@superkhau I am opposed to landing this patch until we figure out how to keep downstream CI builds of our connectors green. We have invested a lot of time in the recent weeks to get our CI builds green, landing this patch will undo most of that effort.

I believe you can do similar approach to what @bajtos and I did in PR#866.

I am afraid this approach cannot be used easily, because test/basic-querying.test.js does not provide any hook for injecting configuration :(

I see the following options how to move forward:

Quick hack: attach the information about connector capabilities to the global getSchema method. This will make it easy to use it or it.skip in the tests:

    var itWhenIlikeSupported = getSchema.connectorCapabilities.ilike ? it : it.skip.bind(it);

    itWhenIlikeSupported('should support "ilike" that is satisfied', function(done) {
      User.find({where: {name: {ilike: 'john'}}}, function(err, users) {
        if (err) return done(err);
        users.length.should.equal(1);
        users[0].name.should.equal('John Lennon');
        done();
      });
    });

Alternatively, require the connectors to define a global object connectorCapabilities, similarly how a global getSchema is expected.

Refactor the shared test suite to export a function that can accept connectorCapabilities and getSchema as arguments, to avoid global variables. This can be a follow-up story for 1)
Alternatively, store the metadata in the connector/datasource instance. Define a new property dataSource.capabilities or dataSource.feature that can hold information about additional capabilities supported by the connector. We already have dataSource.getType() that's backed by connector's prototype.getType(). The downside is that we won't be able to write it.skip tests, because the datasource is created only after all tests have been already declared.

I am personally voting for 1).

@superkhau @raymondfeng @Amir-61 thoughts?

bajtos

IMO we need to figure out how to keep downstream connectors green on CI before landing this patch.

superkhau · 2016-09-23T22:03:17Z

I am opposed to landing this patch until we figure out how to keep downstream CI builds of our connectors green. We have invested a lot of time in the recent weeks to get our CI builds green, landing this patch will undo most of that effort.

I agree we do not land this unless CI stays green. I'm also gonna vote for solution 1 as it looks like the smoothest path to landing.

@duffn Thoughts? Would you like to help submit the other PRs based on this approach?

superkhau

Please make the changes based on approach 1 suggested by @bajtos before we land this

duffn · 2016-09-24T08:59:04Z

Yes, I will try to find the time to submit the additional PRs.

bajtos · 2016-10-05T12:54:31Z

@slnode ok to test

bajtos · 2016-10-05T12:57:20Z

As far as I am concerned, the code change in this pull request looks good enough to me. @superkhau could you please work with @duffn to tie up any remaining loose ends? Should we mention the two new test flags somewhere in the documentation for building connectors?

superkhau · 2016-10-06T06:52:30Z

Should we mention the two new test flags somewhere in the documentation for building connectors?

Yes we should. 👍

cc @crandmck

superkhau · 2016-10-06T06:56:03Z

@duffn Can you rebase and squash before we land this? Let's try to land those other related PRs first though.

duffn · 2016-10-09T14:03:13Z

Alright, I think I got all of the PRs submitted.

superkhau · 2016-10-12T06:17:16Z

@duffn I managed to land and backport all the connector capability PRs. Can you rebase this PR and lets see how CI fares.

Closes #633

duffn · 2016-10-13T00:37:25Z

Fingers crossed.

superkhau · 2016-10-13T01:08:15Z

@duffn LGTM, gonna land this as the failures are known failures for those downstream connectors. Thank you for the huge contribution. ;)

superkhau · 2016-10-13T01:09:08Z

@duffn Can you submit another PR to loopback to document the two new flags? See https://github.com/strongloop/loopback.io/

@crandmck Can you guide @duffn to where he can add those notes?

Closes #633 Backport of #1091

crandmck · 2016-10-14T23:54:17Z

@duffn

Please do update the docs; you can open a PR for docs just as for the code.

Docs for ilike and nilike operators: http://loopback.io/doc/en/lb2/Where-filter.html#operators - source in https://github.com/strongloop/loopback.io/blob/gh-pages/pages/en/lb2/Where-filter.md

Please at least add it to the table in that section. If there's more to say about usage, you can also add a subsection as there is for AND/OR and regexp.

Ideally, you would also add an example in http://loopback.io/doc/en/lb2/Where-filter.html#examples

Should we mention the two new test flags somewhere in the documentation for building connectors?

If you want to address this as well, we'd probably add a new section to http://loopback.io/doc/en/lb2/Building-a-connector.html - source in https://github.com/strongloop/loopback.io/blob/gh-pages/pages/en/lb2/Building-a-connector.md.

Don't worry to much about wording and formatting. I can always clean it up afterward.

Thanks!!

crandmck added the #community contribution label Sep 16, 2016

superkhau self-assigned this Sep 17, 2016

superkhau suggested changes Sep 19, 2016

View reviewed changes

superkhau approved these changes Sep 19, 2016

View reviewed changes

duffn mentioned this pull request Sep 21, 2016

Fixes #92 - Allow case insensitive like and not like statements. loopbackio/loopback-connector-postgresql#93

Closed

superkhau mentioned this pull request Sep 22, 2016

Fixes #633 - Pass through case insenstive search params. #634

Closed

bajtos suggested changes Sep 23, 2016

View reviewed changes

superkhau suggested changes Sep 23, 2016

View reviewed changes

bajtos approved these changes Oct 5, 2016

View reviewed changes

superkhau approved these changes Oct 6, 2016

View reviewed changes

Add ilike and nilike operators

a23076d

Closes #633

superkhau merged commit 1ee0442 into loopbackio:master Oct 13, 2016

superkhau removed the #community contribution label Oct 13, 2016

superkhau pushed a commit that referenced this pull request Oct 13, 2016

Add ilike and nilike operators

c8d89ee

Closes #633 Backport of #1091

superkhau mentioned this pull request Oct 13, 2016

Backport: Add ilike and nilike operators #1136

Merged

superkhau pushed a commit that referenced this pull request Oct 13, 2016

Add ilike and nilike operators

87be17e

Closes #633 Backport of #1091

superkhau added a commit that referenced this pull request Oct 13, 2016

Add ilike and nilike operators (#1136)

a9d3816

Closes #633 Backport of #1091

duffn mentioned this pull request Oct 15, 2016

Add documentation for ilike and nilike operators loopbackio/loopback.io#116

Merged

alireza-ahmadi mentioned this pull request Dec 15, 2016

Add ILIKE functionality loopbackio/loopback-connector-postgresql#194

Merged

Add ilike and nilike operators #1091

Add ilike and nilike operators #1091

Uh oh!

Conversation

duffn commented Sep 16, 2016

Uh oh!

slnode commented Sep 16, 2016

Uh oh!

slnode commented Sep 16, 2016

Uh oh!

slnode commented Sep 16, 2016

Uh oh!

slnode commented Sep 16, 2016

Uh oh!

duffn commented Sep 16, 2016

Uh oh!

superkhau commented Sep 17, 2016

Uh oh!

duffn commented Sep 17, 2016

Uh oh!

superkhau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duffn Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duffn Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superkhau commented Sep 19, 2016

Uh oh!

superkhau commented Sep 21, 2016

Uh oh!

duffn commented Sep 21, 2016

Uh oh!

superkhau commented Sep 21, 2016

Uh oh!

superkhau commented Sep 22, 2016

Uh oh!

bajtos commented Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duffn commented Sep 22, 2016

Uh oh!

Amir-61 commented Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superkhau commented Sep 22, 2016

duffn Sep 19, 2016 •

edited

Loading

duffn Sep 19, 2016 •

edited

Loading

bajtos commented Sep 22, 2016 •

edited

Loading

Amir-61 commented Sep 22, 2016 •

edited

Loading

superkhau commented Sep 23, 2016 •

edited

Loading