Fix/geo null #1334

paulussup · 2017-04-19T03:25:58Z

Description

Add check for null and undefined.

Related issues

#1332
connect to #1332

None

Checklist

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

slnode · 2017-04-19T03:25:59Z

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

slnode · 2017-04-19T03:26:04Z

Can one of the admins verify this patch?

slnode · 2017-04-19T03:26:04Z

Can one of the admins verify this patch?

slnode · 2017-04-19T03:26:04Z

Can one of the admins verify this patch?

ebarault

thanks for contributing this fix, please review the requested changes

ebarault · 2017-04-19T08:22:51Z

lib/geo.js

@@ -24,7 +24,7 @@ exports.nearFilter = function nearFilter(where) {
          var ret = nearSearch(el, parentKeys.concat(clauseKey).concat(index));
          if (ret) return ret;
        });
-      } else {
+      } else if (clause[clauseKey] !== null && clause[clauseKey] !== undefined) {


we usually prefer early return to keep the code simpler to read, i suggest:

Object.keys(clause).forEach(function(clauseKey) { if (typeof clause[clauseKey] !== 'object' || !clause[clauseKey]) return; if (Array.isArray(clause[clauseKey])) { // array // object different than null, undefined, 0 } else { // object other than null, undefined, 0 } }

ebarault · 2017-04-19T08:28:14Z

test/basic-querying.test.js

@@ -153,6 +153,27 @@ describe('basic-querying', function() {
          done();
        });
    });
+    it('should query by ids and null condition', function(done) {


is it required for this test to search among these 4 users? unless i'm missing something could you simplify with only 2 different users?

ebarault

LGTM: @ssh24 i let you check

ssh24 · 2017-04-19T10:55:58Z

test/basic-querying.test.js

+        createdUsers[1].id],
+        {where: {vip: null}}, function(err, users) {
+          should.exist(users);
+          should.not.exist(err);


@paulussup Could you please swap L161 with L162? It is always a good idea to check err first.

fixed @ssh24

ssh24 · 2017-04-19T10:56:32Z

@slnode test please

ssh24 · 2017-04-19T11:09:53Z

@slnode test please

ssh24 · 2017-04-19T12:46:23Z

@slnode test please

ssh24 · 2017-04-19T13:10:13Z

test/basic-querying.test.js

+      User.findByIds([
+        createdUsers[0].id,
+        createdUsers[1].id],
+        {where: {vip: null}}, function(err, users) {


@paulussup @ebarault I am not sure if this test case checks for the fix in the right away. The fix is however correct.

Shouldn't the filter do near query with a null value and see if the null value it is taken care of? Something like this test case but with a null value filter: 64f64ca#diff-8fae24fa21c82e23f7f43ccdae4fcf90R599

paulussup · 2017-04-19T13:18:11Z

To be honest I copy pasted the test above written by @raymondfeng and made the required edits. The reason that I chose to just test the null value is that the documentation uses querying a field as null as an example. Because this was not tested the error was able to make its way in to master.

…

On 19 Apr 2017, at 10:40 pm, Sakib Hasan ***@***.***> wrote: @ssh24 commented on this pull request. In test/basic-querying.test.js <#1334 (comment)>: > @@ -153,6 +153,18 @@ describe('basic-querying', function() { done(); }); }); + it('should query by ids and null condition', function(done) { + User.findByIds([ + createdUsers[0].id, + createdUsers[1].id], + {where: {vip: null}}, function(err, users) { @paulussup <https://github.com/paulussup> @ebarault <https://github.com/ebarault> I am not sure if this test case checks for the fix in the right away. The fix is however correct. Shouldn't the filter do near query with a null value and see if the null value it is taken care of? Something like this test case but with a null value filter: 64f64ca#diff-8fae24fa21c82e23f7f43ccdae4fcf90R599 <64f64ca#diff-8fae24fa21c82e23f7f43ccdae4fcf90R599> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1334 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJCRmHAIRoYImdENLnkPD_ZQzsYZrDyhks5rxge2gaJpZM4NBJ0R>.

ssh24 · 2017-04-19T14:09:03Z

@paulussup That is fine. We can still keep this test case and just add that extra test case.

The test case is also failing on mongodb and cassandra connector so need to look into that.

paulussup · 2017-04-19T14:29:18Z

I guess that is because those datasources don’t default the value to null when persisting like the sql datasources.

…

On 19 Apr 2017, at 11:39 pm, Sakib Hasan ***@***.***> wrote: @paulussup <https://github.com/paulussup> That is fine. We can still keep this test case and just add that extra test case. The test case is also failing on mongodb and cassandra connector so need to look into that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1334 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJCRmL9ipkuDFmBoWVoHdJboBVribxfFks5rxhWBgaJpZM4NBJ0R>.

ssh24 · 2017-04-19T16:14:44Z

@slnode test please

ssh24 · 2017-04-19T17:01:55Z

@ebarault This fix looks good to me. Could you do me a favour and review the changes I committed, i.e fixing and adding new test cases?

@slnode test please

ssh24

Fix LGTM. Waiting on CI and @ebarault to review my test fixes.

ssh24 · 2017-04-19T18:23:25Z

@slnode test please

ebarault

@ssh24: i see your point.
My main concern is about test naming, please see my inlined comments

ebarault · 2017-04-19T18:34:24Z

test/basic-querying.test.js

@@ -153,6 +153,26 @@ describe('basic-querying', function() {
          done();
        });
    });
+    it('should query by ids and null condition', function(done) {
+      // due to some connectors not being assigning null value by default
+      // the query is filtered by the fields which may consist null value


typo: "which may consist in null value ..."

ebarault · 2017-04-19T18:39:09Z

test/basic-querying.test.js

+      User.findByIds([
+        createdUsers[0].id,
+        createdUsers[1].id],
+        {fields: {name: true, vip: true}}, function(err, users) {


i'm afraid i don't understand, the test is named: 'should query by ids and null condition' and we are not querying by null value here. I suspect it should be vip: null here?

or i'm missing this: one of the user has an undefined property "vip" and the test should be named something like: 'should query by ids and condition, allowing results where conditions match null properties'

I have to say I don't see how this test is even testing the behaviour that caused this bug. The problem arose when traversing the where object for the near key and the value is {where: {field: null}}. Would it not make more sense to set vip explicitly to null in this test case or create new test seed data with explicit null values.

@paulussup Good question. Your test case was not wrong however it had a loophole where it would not check for connectors which does not put null values for fields which are not set (it would instead have it as undefined, for example mongodb).

Improved the test case to take care of that situation by filtering by fields (name and vip) and then get those instances back and then perform a check if they are either null or undefined. After fixing this it works with the downstream connectors such as mongodb and cassandra.

I accept that it fixes the downstream issue. If there was a test which actually set the value explicitly to null in the test I think this too would allow the downstream connectors to query the null value and return records. Mongo and Cassandra can both store null values.

@paulussup Yes I agree with you. Would you like to fix the test cases to go with the changes you proposed? We can just add a property vip with value null on the second user of createdUsers and just query that with your previous test case.

Please be sure to pull latest from your branch if you haven't already.

ebarault · 2017-04-19T18:40:06Z

test/basic-querying.test.js

@@ -593,6 +613,19 @@ describe('basic-querying', function() {

    describe('geo queries', function() {
      describe('near filter', function() {
+        it('supports a near query with null condition', function(done) {


could you move this test right after the 'supports a basic "near" query' test please?

if i'm right in my previous comment the test name should also be revised to reflect that we are not querying on a null condition, but that results on the conditions could come with empty properties

something like:
'supports a compound near query with conditions, allowing results where conditions match null properties'

ssh24 · 2017-04-19T19:21:00Z

@ebarault Updated the test messages.

@slnode test please

paulussup · 2017-04-20T14:40:22Z

Sure no problems.

…

On 20 Apr 2017, at 9:00 pm, Sakib Hasan ***@***.***> wrote: @ssh24 commented on this pull request. In test/basic-querying.test.js <#1334 (comment)>: > @@ -153,6 +153,26 @@ describe('basic-querying', function() { done(); }); }); + it('should query by ids and null condition', function(done) { + // due to some connectors not being assigning null value by default + // the query is filtered by the fields which may consist null value + // or not exist at all + User.findByIds([ + createdUsers[0].id, + createdUsers[1].id], + {fields: {name: true, vip: true}}, function(err, users) { @paulussup <https://github.com/paulussup> Yes I agree with you. Would you like to fix the test cases to go with the changes you proposed? We can just add a property vip with value null on the second user of createdUsers and just query that with your previous test case. Please be sure to pull latest from your branch if you haven't already. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1334 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJCRmBT3oH42krpyL0lEoYnNslZA8joHks5rx0HlgaJpZM4NBJ0R>.

ssh24 · 2017-04-21T18:46:35Z

@paulussup Any updates on fixing the test cases?

paulussup · 2017-04-21T19:27:51Z

Haven’t had a chance yet.

…

On 22 Apr 2017, at 4:16 am, Sakib Hasan ***@***.***> wrote: @paulussup <https://github.com/paulussup> Any updates on fixing the test cases? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1334 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJCRmPbFORPiy_rdnhQWsxCVI0b1_VxRks5ryPmMgaJpZM4NBJ0R>.

Some connectors uses a non existent prop instead of allowing null Modified test case to look if null exists or the prop is non existent

ssh24 · 2017-04-22T23:19:25Z

@paulussup I have fixed the test cases myself.

ssh24 · 2017-04-22T23:40:38Z

@slnode test please

ssh24 · 2017-04-24T19:05:04Z

@slnode test please

ssh24 · 2017-04-24T21:21:17Z

@slnode test please

ssh24 · 2017-04-24T22:35:43Z

@slnode test please

ssh24 · 2017-04-24T23:28:05Z

PR ready to be landed. @paulussup Thank you again for the patch!

…ype.

* create sequence for nosql id (#1354) (Janny) * Fix order of query results (Loay) * Add DateString type (Kevin Delisle) * datatype.test: use predefined date (Kevin Delisle) * Update api documents (Loay) * Datasource documentation tune-up (Kevin Delisle) * Added unit tests specific to DateType where null (#1349) (Andrew McDonnell) * Fix/geo null (#1334) (paulussup) * replace exception thrown for invalid dates (Diana Lau) * Revert PR #1326 (#1336) (Sakib Hasan) * Make lib peerDepend on loopback-connector (#1326) (Russ Tyndall) * Add test case using updateAttributes (Loay) * Fix forceId bug for updateOrCreate (Loay) * Fix typo in description (jannyHou) * Fix relations test case (loay)

ebarault requested a review from ssh24 April 19, 2017 08:09

ebarault suggested changes Apr 19, 2017

View reviewed changes

ebarault approved these changes Apr 19, 2017

View reviewed changes

ssh24 reviewed Apr 19, 2017

View reviewed changes

ssh24 mentioned this pull request Apr 19, 2017

Fix for #1328 Attempting to where filter on type Date = null fails with backtrace #1329

Closed

2 tasks

ssh24 self-assigned this Apr 19, 2017

ssh24 force-pushed the fix/geo_null branch 2 times, most recently from 64494dd to fa680d1 Compare April 19, 2017 15:50

ssh24 force-pushed the fix/geo_null branch from 3714bdc to f32455f Compare April 19, 2017 16:55

ssh24 approved these changes Apr 19, 2017

View reviewed changes

ssh24 force-pushed the fix/geo_null branch from f32455f to 86e3f7c Compare April 19, 2017 18:18

ebarault suggested changes Apr 19, 2017

View reviewed changes

ssh24 mentioned this pull request Apr 21, 2017

Fix to avoid beaking filters for null fields #1338

Closed

2 tasks

probinson and others added 8 commits April 22, 2017 19:17

fix check for null

e98a1d4

add tests

459d5f7

fix for early return

c5be960

change test to two users and simplify

7892228

check error first

7bc6201

Allow check for null and non-existent value

38a5ed0

Some connectors uses a non existent prop instead of allowing null Modified test case to look if null exists or the prop is non existent

Check for null value with geo near query

146c77e

Apply requested changes

8bc313e

ssh24 force-pushed the fix/geo_null branch from a13c4a8 to 19bff07 Compare April 22, 2017 23:17

Fix simple query test case with null value

ecfc6d7

ssh24 force-pushed the fix/geo_null branch from 19bff07 to ecfc6d7 Compare April 22, 2017 23:28

BDD for connectors w//o null support

5af3d59

ssh24 force-pushed the fix/geo_null branch from 7858253 to 5af3d59 Compare April 24, 2017 18:36

ssh24 merged commit e9ff88f into loopbackio:master Apr 24, 2017

ssh24 added the apex label Apr 24, 2017

ssh24 added this to the Sprint 34 - Apex milestone Apr 24, 2017

pastcompute added a commit to pastcompute/loopback-datasource-juggler that referenced this pull request Apr 25, 2017

Added unit test for loopbackio#1328 loopbackio#1334 specific to DateT…

a1079df

…ype.

pastcompute mentioned this pull request Apr 25, 2017

Added unit tests specific to DateType where null #1349

Merged

2 tasks

Fix/geo null #1334

Fix/geo null #1334

Conversation

paulussup commented Apr 19, 2017 • edited by ssh24

Description

Related issues

Checklist

slnode commented Apr 19, 2017

slnode commented Apr 19, 2017

slnode commented Apr 19, 2017

slnode commented Apr 19, 2017

ebarault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebarault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssh24 commented Apr 19, 2017

ssh24 commented Apr 19, 2017

ssh24 commented Apr 19, 2017

Choose a reason for hiding this comment

paulussup commented Apr 19, 2017 via email

ssh24 commented Apr 19, 2017

paulussup commented Apr 19, 2017 via email

ssh24 commented Apr 19, 2017

ssh24 commented Apr 19, 2017

ssh24 left a comment

Choose a reason for hiding this comment

ssh24 commented Apr 19, 2017

ebarault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssh24 commented Apr 19, 2017

paulussup commented Apr 20, 2017 via email

ssh24 commented Apr 21, 2017

paulussup commented Apr 21, 2017 via email

ssh24 commented Apr 22, 2017

ssh24 commented Apr 22, 2017

ssh24 commented Apr 24, 2017

ssh24 commented Apr 24, 2017

ssh24 commented Apr 24, 2017

ssh24 commented Apr 24, 2017

paulussup commented Apr 19, 2017 •

edited by ssh24