Invalid `fields` declaration silently ignored

[dmitriz]

Is this really intended?

db > db.merchants.find({}, {fields: ['id']})
[ anonymous { id: 1 },
  anonymous { id: 2 },
  anonymous { id: 3 },
  anonymous { id: 4 } ]

db > db.merchants.find({}, {fields: []})
[ anonymous { id: 1, name: 'River North' },
  anonymous { id: 2, name: 'Crow and Nail' },
  anonymous { id: 3, name: 'Spotted Sparrow' },
  anonymous { id: 4, name: 'Silver Buckle' } ]

Should the invalid fields declaration not result in error rather than being silently ignored?

[dmfay]

It’s not intentional, but since there’s a clear default behavior when the option isn’t specified at all I’m not especially put out by it falling back to that. If you feel strongly enough about it to write up a test and submit a pull request, go right ahead :)

[dmitriz]

Thank you for your answer. It is good to hear it is not intentional ;)

I have been trying to understand the best way to deal with it, and perhaps this option represents a special case of the map transformer? Then passing the empty array with fields:[] would correspond to mapping the result array over x=>{}, so perhaps the correct result should be the array of empty objects?

There seem to be very few tests for this option, e.g. here only testing for the length of the result array, where the whole table seems to have only one row, and the testing is needed actually for the fields returned inside the rows: https://github.com/dmfay/massive-js/blob/master/test/queryable/find.js#L403

Thus I'm not sure what could be a good PR in that direction that will also be accepted ;)

[dmfay]

There’s another test in the statement suite but that’s still pretty light coverage. The thing is you can’t select nothing so the options are either error (either before or at the SQL syntax error of ‘SELECT FROM tbl’) or select everything. The original design went with the latter, but since passing an empty fields list is likely unintended I’m good with the error.

Please make your PR against the v5 branch rather than master since it’s technically a compatibility break.

[dmitriz]

I see, thank you, that other test looks more like a test for how things work internally. Not knowing about these, I would feel better to only look for the final effects acting directly on the database and bypassing any SQL.

I am also a bit uncertain from where the User table gets initialized inside the test (it is not visible from the test file). I would be interested to maybe have a new table populated with more data for more thorough tests in that place, and ideally explicitly require the file initializing the table to make it instantly visible.

It would make things easy but I wonder whether such changes would be acceptable.

[dmfay]

The test ‘before’ methods call a function named resetDb in test/helpers/index.js which loads a schema by the passed name from test/helpers/scripts/$name/schema.sql. This cuts down on fixture boilerplate since it’s always doing the same thing with the schema script no matter what’s actually in it. If you need to modify that script to add more inserts or a new table just make sure it doesn’t break any other tests which use it :)

[dmitriz]

I've made this test PR (to master by mistake for now), just to see if you would accept something like that in principle: https://github.com/dmfay/massive-js/pull/584

Here are the file changes: https://github.com/dmfay/massive-js/pull/584/commits/7b1e6a1e02245fa751be00d56bc44bd79660fca7

[dmitriz]

There are no tests for errors here: https://github.com/dmfay/massive-js/blob/master/test/queryable/find.js

Are they located somewhere else?

What style is used for testing errors inside promises?

[dmfay]

All the SQL methods revolve around Database.query... which doesn't have tests for the error case either, so that's a coverage gap. But find, count, etc don't need it nearly as urgently since any rejected promises would be coming from query.

I've more or less settled on return promiseyFunction(...).then(() => { assert.fail(); }).catch(/* some other assertions */); as in test/table/deep-insert.js but there are a handful of tests in other styles. Massive is compatible back to Node 6 so try/catch on await isn't happening yet.

[dmitriz]

Couldn't get it working, still throwing errors: https://github.com/dmitriz/massive-js/tree/v5-test-polish1

  1) find
       casing issues
         throws for empty subset of fields:

      AssertionError: expected 'assert.fail()' to equal 'The "fields" array cannot be empty'
      + expected - actual

      -assert.fail()
      +The "fields" array cannot be empty

      at db.Users.find.then.catch.err (test/queryable/find.js:426:30)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)

[dmfay]

Your lib/statement/select.js still sets fields to ['*'] if it's empty. You need to throw in the constructor if fields is empty and catch it in the Queryable methods so you can safely return a rejected promise.

There is a reject test for find but it's catching a "natural" error (trying to use a primary key criterion with a view) rather than testing a specially generated rejection.

[dmitriz]

I've tried to throw there instead but that lead to hundreds of failed tests, essentially due to this line: https://github.com/dmitriz/massive-js/blob/v5-test-polish1/lib/statement/select.js#L51

So I have tried to put it here instead: https://github.com/dmitriz/massive-js/commit/6d3ce25a5cd037b8fcb72005c4443771cc0f0804#diff-1b81dc4c5fe2d223e0790ef2ccc192eeR75

I don't know why the test is failing.

[dmfay]

There are three cases:

• options.fields is undefined, in which case the Select's fields needs to be ['*'] for interpolation into the emitted SQL
• options.fields is defined as an empty array, which is where you're trying to raise an error
• options.fields is defined as anything else, in which case the existing logic takes over

The error check needs to happen in the Select, since otherwise find starts behaving differently from findOne, findDoc, etc. I think what you ran into was that you were testing this.fields to see if it was an empty array, instead of testing the options.fields passed into the constructor. Try that instead.

[dmitriz]

Please see here: https://github.com/dmfay/massive-js/compare/v5...dmitriz:v5

The error is added but it does not throw in the test, I don't know why.

[dmfay]

It's a common mistake with equivalence in JavaScript :slightly_smiling_face: Arrays are objects, and object equality tests by reference. So this works:

const a1 = [1, 2, 3];
const a2 = a1;

a1 === a2; // true

But this doesn't:

const a1 = [1, 2, 3];
const a2 = [1, 2, 3];

a1 === a2; // false

Testing if (options.fields && options.fields.length === 0) throws as expected.

[dmitriz]

You are right of course, too much category theory can make one blind ;)

So the test is failing now as expected, which is good ;) What is not so good, the error doesn't get caught with this:

    it('throws error when field array is empty', function () {
      return db.Users.find({}, {fields: []}).then(res => assert.fail("Should not show up"))
        .catch(err => assert.ok(err));
    });

I don't have much experience with Mocha used that way, so not sure what is the right way of testing the failure here.

Here is the branch with this test: https://github.com/dmitriz/massive-js/tree/v5-fields

[dmfay]

My turn to be out of practice -- of course if you throw an exception that isn't caught then it won't reject the Promise :woman_facepalming: Best place to catch is in Database.query() so errors have to be thrown from format() instead of in the constructor:

From 9a832f0bb612e6e5f47a7206a60f7761319907e1 Mon Sep 17 00:00:00 2001
From: Dian Fay <dian.m.fay@gmail.com>
Date: Tue, 24 Apr 2018 22:22:13 -0400
Subject: [PATCH] catch errors from statement format() in db.query()

---
 lib/database.js         |  7 ++++++-
 lib/statement/select.js | 12 ++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/lib/database.js b/lib/database.js
index b95353f..b1d516b 100644
--- a/lib/database.js
+++ b/lib/database.js
@@ -333,7 +333,12 @@ Database.prototype.query = function (query, params = [], options = {}) {
   if (query instanceof this.pgp.QueryFile || _.isString(query)) {
     sql = query;
   } else {
-    sql = query.format();
+    try {
+      sql = query.format();
+    } catch (e) {
+      return Promise.reject(e);
+    }
+
     params = query.params;
     options = query;
   }
diff --git a/lib/statement/select.js b/lib/statement/select.js
index 8052db7..1807063 100644
--- a/lib/statement/select.js
+++ b/lib/statement/select.js
@@ -48,10 +48,6 @@ const Select = function (source, criteria = {}, options = {}) {
       break;
   }

-  if (options.fields && options.fields.length === 0) {
-    throw new Error('The fields array cannot be empty');
-  }
-
   this.fields = [];

   _.castArray(options.fields || []).forEach((f) => {
@@ -66,6 +62,10 @@ const Select = function (source, criteria = {}, options = {}) {
     this.fields.push(`${expr} AS ${name}`);
   });

+  if (options.fields && options.fields.length === 0) {
+    this.error = 'The fields array cannot be empty';
+  }
+
   if (this.fields.length === 0) {
     this.fields = ['*'];
   }
@@ -96,6 +96,10 @@ const Select = function (source, criteria = {}, options = {}) {
  * @return {String} A SQL SELECT statement.
  */
 Select.prototype.format = function () {
+  if (this.error) {
+    throw new Error(this.error);
+  }
+
   let sql = `SELECT ${this.fields.join(',')} FROM `;

   if (this.only) { sql += 'ONLY '; }
-- 
2.17.0

[dmitriz]

I have managed to get the failure tested on this branch: https://github.com/dmitriz/massive-js/commits/failure-test-pass

Here is the commit:

https://github.com/dmitriz/massive-js/commit/b58cde170198b34f50dab4edfc9c3e09fb48b3cc

The test message is made intentionally different for now to see that it is actually caught. At the end, we probably just should test for the failure, not the specific message.

However, when I move the throw statement to Database.query, the same failure test passes:

https://github.com/dmitriz/massive-js/commits/failure-test-in-database

So does it mean, that might not be the right place?

[dmfay]

There's a difference between throwing an exception and returning a rejected promise. If you look at the patch above I modified Database.query to catch exceptions coming from the statement's format method, and threw from in there instead of the statement constructor.

The test is passing in your second commit because fields is never empty by the time it makes it to Database.query -- the constructor adds * to build the SQL statement. That's why my patch has to cache the error instead of just testing it in format. Try applying it and see how that works for you! There's probably some room for improvement but it should at least get you going down the right path.