Fix #161 - Wrong queries causes unhandled exceptions #162

regevbr · 2019-11-20T21:40:19Z

Fixes #161 and consolidates #132 and #125

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Commit messages are following our guidelines

slnode · 2019-11-20T21:40:22Z

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

regevbr · 2019-11-21T18:39:35Z

@jannyHou @dhmlau can you please review the PR?

dhmlau · 2019-11-26T17:15:52Z

@slnode test please

lib/sql.js

regevbr · 2019-11-26T20:59:27Z

@bajtos @dhmlau I have revied the changes proposed in #125 and #132 and add unit tests.
This should fix cover to cover hanging queries and faulty fields usage. Please let me know what you think

bajtos

You are on the right track, keep going 👏

lib/sql.js

test/sql.test.js

regevbr · 2019-11-29T19:12:23Z

@bajtos I have fixed all according to your comments, can you please review again?

bajtos

Almost there! I have few minor/stylistic comments, the actual implementation & tests look good.

I'll am taking a longer break now, won't be able to follow up on this discussion until January. Can somebody from @strongloop/loopback-maintainers take over the review process of this pull request please?

bajtos · 2019-12-13T16:07:31Z

lib/sql.js

+  } catch (err) {
+    if (cb) {
+      cb(err);
+    }


Nitpick: let's shorten this block as follows:

if (cb) cb(err); return;

Same comment applies to other similar places too.

bajtos · 2019-12-13T16:07:53Z

lib/sql.js

+  const msg = g.f('Unknown property %s is used for model %s', key, model);
+  const error = new Error(msg);
+  error.statusCode = error.status = 400;
+  error.code = 'UNKNWON_PROPERTY';


Typo.

Suggested change

error.code = 'UNKNWON_PROPERTY';

error.code = 'UNKNOWN_PROPERTY';

bajtos · 2019-12-13T16:11:25Z

test/sql.test.js

+    });
+  });
+
+  it('should ignore if invalid order by statement is used by all', function(done) {


If find it a bit difficult to spot why some tests are ignoring invalid statements while others are throwing an error. Can you please group the tests for "should ignore" using describe block?

describe('with ignoreUnknownProperties: true', () => { it('should ignore if invalid order by statement is used by all', function(done) { // ... }); // ... });

bajtos · 2019-12-13T16:13:35Z

test/sql.test.js

@@ -270,6 +293,57 @@ describe('sql connector', function() {
    expect(orderBy).to.eql('ORDER BY `NAME` ASC,`VIP` DESC');
  });

+  it('builds order by with non existent field throws', function() {
+    expect(() => {
+      connector.buildOrderBy('customer', ['nam?e', 'name']);


I have mixed feeling about the value nam?e, I believe it's not a valid property name in LoopBack. Can you please use a different property name, for example unknown?

Suggested change

connector.buildOrderBy('customer', ['nam?e', 'name']);

connector.buildOrderBy('customer', ['unknown', 'name']);

bajtos · 2019-12-13T16:14:48Z

@slnode ok to test

regevbr · 2019-12-23T11:57:58Z

@bajtos I see that there are failing tests in downstream but I can't open the results to check why - the website is unreachable...

bajtos · 2020-01-07T14:37:56Z

@regevbr

I see that there are failing tests in downstream but I can't open the results to check why - the website is unreachable...

Unfortunately, cis-jenkins is accessible only from inside IBM network :( Let me check the errors for you.

Cassandra

Click to expand the log

  1) cassandra custom tests find and order by str:
     AssertionError: expected Error {
  status: 400,
  statusCode: 400,
  code: 'UNKNWON_PROPERTY',
  message: 'Unknown property id is used for model CASS_SORTABLE'
} to not exist
      at test/cass.custom.test.js:277:20
      at allCb (node_modules/loopback-datasource-juggler/lib/dao.js:1740:7)
      at Cassandra.find [as all] (node_modules/loopback-connector/lib/sql.js:1521:12)
      at invokeConnectorMethod (node_modules/loopback-datasource-juggler/lib/dao.js:172:21)
      at node_modules/loopback-datasource-juggler/lib/dao.js:1755:7
      at doNotify (node_modules/loopback-datasource-juggler/lib/observer.js:155:49)
      at doNotify (node_modules/loopback-datasource-juggler/lib/observer.js:155:49)
      at Function.ObserverMixin._notifyBaseObservers (node_modules/loopback-datasource-juggler/lib/observer.js:178:5)
      at Function.ObserverMixin.notifyObserversOf (node_modules/loopback-datasource-juggler/lib/observer.js:153:8)
      at Function.ObserverMixin._notifyBaseObservers (node_modules/loopback-datasource-juggler/lib/observer.js:176:15)
      at Function.ObserverMixin.notifyObserversOf (node_modules/loopback-datasource-juggler/lib/observer.js:153:8)
      at Function.find (node_modules/loopback-datasource-juggler/lib/dao.js:1753:10)
      at Context.<anonymous> (test/cass.custom.test.js:274:19)

  2) cassandra custom tests find and order by num:
     AssertionError: expected Error {
  status: 400,
  statusCode: 400,
  code: 'UNKNWON_PROPERTY',
  message: 'Unknown property id is used for model CASS_SORTABLE'
} to not exist
      at test/cass.custom.test.js:290:20
      at allCb (node_modules/loopback-datasource-juggler/lib/dao.js:1740:7)
      at Cassandra.find [as all] (node_modules/loopback-connector/lib/sql.js:1521:12)
      at invokeConnectorMethod (node_modules/loopback-datasource-juggler/lib/dao.js:172:21)
      at node_modules/loopback-datasource-juggler/lib/dao.js:1755:7
      at doNotify (node_modules/loopback-datasource-juggler/lib/observer.js:155:49)
      at doNotify (node_modules/loopback-datasource-juggler/lib/observer.js:155:49)
      at Function.ObserverMixin._notifyBaseObservers (node_modules/loopback-datasource-juggler/lib/observer.js:178:5)
      at Function.ObserverMixin.notifyObserversOf (node_modules/loopback-datasource-juggler/lib/observer.js:153:8)
      at Function.ObserverMixin._notifyBaseObservers (node_modules/loopback-datasource-juggler/lib/observer.js:176:15)
      at Function.ObserverMixin.notifyObserversOf (node_modules/loopback-datasource-juggler/lib/observer.js:153:8)
      at Function.find (node_modules/loopback-datasource-juggler/lib/dao.js:1753:10)
      at Context.<anonymous> (test/cass.custom.test.js:287:19)

The previous CI build passed and the error message is mentioning your newly introduced error code, therefore I am inclined to think that this is a legitimate problem. Maybe the Cassandra test suite is making an invalid call and we need to fix those two tests first?

DashDB

Click to expand the log

> Cleaning up existing schemas ...
/home/jenkins/workspace/ds/loopback-connector-dashdb~master/pretest.js:58

if (err) throw err;

^

Error: [IBM][CLI Driver] SQL30081N  A communication error has been detected. Communication protocol being used: "TCP/IP".  Communication API being used: "SOCKETS".  Location where the error was detected: "9.30.198.63".  Communication function detecting the error: "selectForConnectTimeout".  Protocol specific error code(s): "115", "", "".  SQLSTATE=08001

This is not related to your changes. Let's see if the problem goes away in the next re-run.

PostgreSQL

Click to expand the log

  1) postgresql connector
       json data type
         allows ordering by nested json properties:
     Error: Unknown property address.city is used for model Customer
      at errorPropertyNotFound (node_modules/loopback-connector/lib/sql.js:1004:17)
      at PostgreSQL.SQLConnector.buildOrderBy (node_modules/loopback-connector/lib/sql.js:1269:13)
      at PostgreSQL.SQLConnector.buildSelect (node_modules/loopback-connector/lib/sql.js:1464:29)
      at PostgreSQL.find [as all] (node_modules/loopback-connector/lib/sql.js:1519:17)
      at invokeConnectorMethod (node_modules/loopback-datasource-juggler/lib/dao.js:172:21)
      at /home/jenkins/workspace/ds/loopback-connector-postgresql~master/node_modules/loopback-datasource-juggler/lib/dao.js:1730:7
      at doNotify (node_modules/loopback-datasource-juggler/lib/observer.js:157:49)
      at doNotify (node_modules/loopback-datasource-juggler/lib/observer.js:157:49)
      at Function.ObserverMixin._notifyBaseObservers (node_modules/loopback-datasource-juggler/lib/observer.js:180:5)
      at Function.ObserverMixin.notifyObserversOf (node_modules/loopback-datasource-juggler/lib/observer.js:155:8)
      at Function.ObserverMixin._notifyBaseObservers (node_modules/loopback-datasource-juggler/lib/observer.js:178:15)
      at Function.ObserverMixin.notifyObserversOf (node_modules/loopback-datasource-juggler/lib/observer.js:155:8)
      at Function.find (node_modules/loopback-datasource-juggler/lib/dao.js:1728:10)
      at Context.<anonymous> (test/postgresql.test.js:745:16)

This problem looks legitimate to me too. I think your code is not taking into account queries for nested properties?

bajtos · 2020-01-07T14:38:54Z

Merge branch 'master' into #161

Please don't use git merge master and use git rebase master instead. We are keeping our git history linear.

regevbr · 2020-01-07T14:40:51Z

@bajtos thanks will do, I will try to fix everything when I can

stale · 2020-03-07T14:46:15Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2020-03-21T15:42:23Z

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

lcsdms · 2021-05-05T18:30:32Z

Hey @bajtos and @regevbr , I've came from #125 with the same problem.

As I understand there's a need to fix those problems on the last tests right ? Is this the place to start if I'm contributing ?

regevbr requested review from dhmlau, emonddr and jannyHou as code owners November 20, 2019 21:40

Fix Bug loopbackio#161 - hanging queries

2295415

regevbr force-pushed the #161 branch from 4189787 to 2295415 Compare November 20, 2019 21:44

Fix Bug loopbackio#161 - hanging queries

cf28b7a

dhmlau added the community-contribution Patches contributed by community label Nov 25, 2019

bajtos reviewed Nov 26, 2019

View reviewed changes

lib/sql.js Outdated Show resolved Hide resolved

Fix Bug loopbackio#161 - hanging queries

eac608a

Fix Bug loopbackio#161 - hanging queries

4dfe851

bajtos suggested changes Nov 28, 2019

View reviewed changes

lib/sql.js Outdated Show resolved Hide resolved

lib/sql.js Outdated Show resolved Hide resolved

lib/sql.js Outdated Show resolved Hide resolved

test/sql.test.js Outdated Show resolved Hide resolved

regevbr added 3 commits November 29, 2019 20:45

Fix Bug loopbackio#161 - hanging queries

35f96fe

Fix Bug loopbackio#161 - hanging queries

1eacf47

Fix Bug loopbackio#161 - hanging queries

572716c

bajtos approved these changes Dec 13, 2019

View reviewed changes

bajtos requested a review from a team December 13, 2019 16:15

Merge branch 'master' into loopbackio#161

0c5e808

bajtos self-assigned this Jan 7, 2020

stale bot added the stale label Mar 7, 2020

stale bot closed this Mar 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #161 - Wrong queries causes unhandled exceptions #162

Fix #161 - Wrong queries causes unhandled exceptions #162

regevbr commented Nov 20, 2019 •

edited

Loading

slnode commented Nov 20, 2019

regevbr commented Nov 21, 2019

dhmlau commented Nov 26, 2019

regevbr commented Nov 26, 2019

bajtos left a comment

regevbr commented Nov 29, 2019

bajtos left a comment

bajtos Dec 13, 2019

bajtos Dec 13, 2019

bajtos Dec 13, 2019

bajtos Dec 13, 2019

bajtos commented Dec 13, 2019

regevbr commented Dec 23, 2019

bajtos commented Jan 7, 2020

bajtos commented Jan 7, 2020

regevbr commented Jan 7, 2020

stale bot commented Mar 7, 2020

stale bot commented Mar 21, 2020

lcsdms commented May 5, 2021

	error.code = 'UNKNWON_PROPERTY';
	error.code = 'UNKNOWN_PROPERTY';

	connector.buildOrderBy('customer', ['nam?e', 'name']);
	connector.buildOrderBy('customer', ['unknown', 'name']);

Fix #161 - Wrong queries causes unhandled exceptions #162

Fix #161 - Wrong queries causes unhandled exceptions #162

Conversation

regevbr commented Nov 20, 2019 • edited Loading

Checklist

slnode commented Nov 20, 2019

regevbr commented Nov 21, 2019

dhmlau commented Nov 26, 2019

regevbr commented Nov 26, 2019

bajtos left a comment

Choose a reason for hiding this comment

regevbr commented Nov 29, 2019

bajtos left a comment

Choose a reason for hiding this comment

bajtos Dec 13, 2019

Choose a reason for hiding this comment

bajtos Dec 13, 2019

Choose a reason for hiding this comment

bajtos Dec 13, 2019

Choose a reason for hiding this comment

bajtos Dec 13, 2019

Choose a reason for hiding this comment

bajtos commented Dec 13, 2019

regevbr commented Dec 23, 2019

bajtos commented Jan 7, 2020

bajtos commented Jan 7, 2020

regevbr commented Jan 7, 2020

stale bot commented Mar 7, 2020

stale bot commented Mar 21, 2020

lcsdms commented May 5, 2021

regevbr commented Nov 20, 2019 •

edited

Loading