handle invalid inputs for destroyAll #1597

b-admike · 2018-06-21T13:36:13Z

Description

Fixes strongloop/loopback#3094. At the moment, there is just a test to reproduce the issue (which I wasn't able to locally, so will gather more feedback).

Checklist

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

nitro404 · 2018-09-02T18:17:00Z

lib/dao.js

  options = options || {};

-  assert(typeof where === 'object', 'The where argument must be an object');
+  // assert(typeof where === 'object', 'The where argument must be an object');


I believe you will still need checks for the type of the value of where, since you can now pass through null values and such.

nitro404 · 2018-09-02T18:17:54Z

lib/dao.js

+  if (typeof where === 'object' && Object.keys(where).includes('where')) {
+  /** Option 1
+   *  throw new Error('This method does not accept filter objects. Please pass
+  in a where object');


I like this option best, personally.

virkt25 · 2018-09-06T18:06:28Z

test/manipulation.test.js

+    });
+
+    it('should delete all for empty object', function(done) {
+      Person.destroyAll({}, function(err, info) {


I think this is the dangerous behaviour we want to eliminate. If a user wants to destroyAll, then it should be called as such (with undefined or nothin) but if an object is present then it should be a filter and not an empty object IMO.

Wouldn't such change be backwards incompatible and require a semver-major release?

Yes. This is a good discussion point. I agree with changing the usage to use undefined value for the where object (which is also the case when it's omitted in API Explorer since it's optional) to delete all records. That means changing our default assignment to undefined since in dao.destroyAll we default to an empty object. Then, we need to make sure that the connector specific methods also adhere to this behaviour. I also want to implement rejection of empty objects and to advise users to use undefined instead. Yes that would mean a semver-major release, but perhaps we can mitigate it by introducing a flag for this new behaviour as well? Please keep in mind that this is also what the users complained about in the original issue.

Please note that detecting {} can be tricky.

Should we treat {userId: undefined} the same way as {}? Or does the first example mean "match all records that have no userId set"? With SQL connectors, this can be probably implemented as a NULL check, but I don't know what's the level of support in noSQL connectors for such query.

perhaps we can mitigate it by introducing a flag for this new behaviour as well?

Yeah, we will have to.

Please keep in mind that this is also what the users complained about in the original issue.

As I understand the discussion in the original issue, the biggest pain point is the situation when people accidentally wrap the condition inside where, which is an extension of the situation when they make a typo in a property name.

The case of where={} is an edge case that was brought up later, as we started to discuss possible fixes.

I would personally focus on the first part (i.e. fix connectors to stop ignoring unknown properties) and leave changes in handling of the empty where object for later.

Baby steps FTW!

Should we treat {userId: undefined} the same way as {}?

I don't think that userId: undefined is the same as an empty object. I think it makes more sense for that to be treated as match all records that have no userId set while an empty object should not be allowed. If the intent is to delete all object then the user should call destroyAll() with no arguments.

Any reason we can't have a breaking change with this module? I wouldn't consider this a breaking change considering I think this was a design oversight that we are fixing but to adhere to semver we can do a major release (at least that's my opinion).

I agree with making fixes in small increments. I'll remove this test case for now (thoughts?). We could definitely follow up with issues regarding how we would like to handle empty objects. I agree that {userId: undefined} shouldn't be the same as {} but I think we should either reject it or yes treat it as match all records that have no userId set.

bajtos

Let's make the new test code more clean please. (I do understand you took inspiration from existing tests, I don't blame you for the poor style we have in this codebase.)

Besides the comments below, please add a test showing what happens when a user makes a typo and uses a property name names instead of name in the query (see loopbackio/loopback-connector#132 (comment)):

Person.destroyAll({names: 'John'});

bajtos · 2018-09-07T07:19:44Z

test/manipulation.test.js

+    });
+
+    it('should delete all for empty object', function(done) {
+      Person.destroyAll({}, function(err, info) {


Wouldn't such change be backwards incompatible and require a semver-major release?

bajtos · 2018-09-07T07:32:35Z

test/manipulation.test.js

+    });
+
+    after(function deleteFixtures(done) {
+      Person.destroyAll(done);


Cleaning the database after the tests is an antipattern, see https://loopback.io/doc/en/lb4/Testing-your-application.html#clean-the-database-before-each-test

Start with a clean database before each test. This may seem counter-intuitive: why not reset the database after the test has finished? When a test fails and the database is cleaned after the test has finished, then it’s difficult to observe what was stored in the database and why the test failed. When the database is cleaned in the beginning, then any failing test will leave the database in the state that caused the test to fail.

bajtos · 2018-09-07T07:38:30Z

test/manipulation.test.js

+          {name: 'John'},
+          {name: 'Jane'},
+          {name: 'Mary'},
+        ], done);


Sharing test data with so many tests is not ideal. When reading the test code and the assertions, it's difficult to understand where the "magic string" names are coming from.

A better solution is to create a helper function for creating an arbitrary list of person instances, and then call that helper from each test.

Also note that new tests can use Promise-based flow control to keep the code simpler.

function givenNamedPeople(namesList) { return Promise.all( namesList.map(name => Person.create({name}) ); } it('should not delete using condition from filter', function() { return givenNamedPeople(['John', 'Jane']) .then(() => Person.destroyAll({where: {name: 'John'}}).should.be.rejectedWith(/Filter object detected/)) .then(() => Person.find({fields: 'name'}) .then(users => { users.should.containDeep([ // ... ]); ); });

Feel free to keep using callbacks if you prefer, in which case you should use something like async.map instead of Promise.all.

bajtos · 2018-09-07T07:40:48Z

test/manipulation.test.js

+        should.exist(err);
+        err.should.match(/Filter object detected/);
+        Person.find({fields: 'name'}, function(err, users) {
+          should.not.exist(err);


This is an anti-pattern I have been fighting for years.

When Person.find fails, this assertions says something along the lines "Expected {Error} to not exist" and shows a stack trace pointing to this very line, throwing the original stack trace away.

A better solution is to pass err to test callback. That way the test fails with the original error triggered by Person.find and provides all information from the original error (message, stack trace, error code, etc.)

if (err) return done(err);

bajtos · 2018-09-07T07:40:57Z

test/manipulation.test.js

+
+    it('should not delete all for bad and condition', function(done) {
+      Person.destroyAll({and: [{name: 'Jane'}, {where: {name: 'John'}}]}, function(err, info) {
+        should.not.exist(err);


if (err) return done(err);

bajtos · 2018-09-07T07:41:24Z

test/manipulation.test.js

+
+    it('should normalize filter with undefined property', function(done) {
+      Person.destroyAll({name: undefined}, function(err, info) {
+        should.not.exist(err);


if (err) return done(err);

bajtos · 2018-09-07T07:41:29Z

test/manipulation.test.js

+      Person.destroyAll({name: undefined}, function(err, info) {
+        should.not.exist(err);
+        Person.find({fields: 'name'}, function(err, users) {
+          should.not.exist(err);


if (err) return done(err);

bajtos · 2018-09-07T07:42:35Z

test/manipulation.test.js

+        should.not.exist(err);
+        Person.find({fields: 'name'}, function(err, users) {
+          should.not.exist(err);
+          users.should.containDeep([]);


Isn't this assertion always true as long as users is any array? I think you need to call users.should.deepEqual instead.

Yes! I found that out after debugging and was looking for an alternative. Thank you :-)

bajtos · 2018-09-07T07:42:40Z

test/manipulation.test.js

+        should.not.exist(err);
+        Person.find({fields: 'name'}, function(err, users) {
+          should.not.exist(err);
+          users.should.containDeep([]);


Isn't this assertion always true as long as users is any array? I think you need to call users.should.deepEqual instead.

bajtos · 2018-09-07T07:42:47Z

test/manipulation.test.js

+        should.not.exist(err);
+        Person.find({fields: 'name'}, function(err, users) {
+          should.not.exist(err);
+          users.should.containDeep([]);


Isn't this assertion always true as long as users is any array? I think you need to call users.should.deepEqual instead.

virkt25 · 2018-09-07T15:10:37Z

test/manipulation.test.js

@@ -1686,7 +1700,73 @@ describe('manipulation', function() {
    });

    // TODO: implement destroy with filtered set
-    it('should destroy filtered set of records');
+    it.skip('should destroy filtered set of records');


Reminder: This should be implemented / removed to cleanup the code.

virkt25 · 2018-09-07T15:19:30Z

test/manipulation.test.js

+    });
+
+    it('should delete all for empty object', function(done) {
+      Person.destroyAll({}, function(err, info) {


Should we treat {userId: undefined} the same way as {}?

I don't think that userId: undefined is the same as an empty object. I think it makes more sense for that to be treated as match all records that have no userId set while an empty object should not be allowed. If the intent is to delete all object then the user should call destroyAll() with no arguments.

Any reason we can't have a breaking change with this module? I wouldn't consider this a breaking change considering I think this was a design oversight that we are fixing but to adhere to semver we can do a major release (at least that's my opinion).

bajtos · 2018-09-11T14:50:00Z

Any reason we can't have a breaking change with this module? I wouldn't consider this a breaking change considering I think this was a design oversight that we are fixing but to adhere to semver we can do a major release (at least that's my opinion).

LoopBack users cannot choose the juggler version they are using, juggler version is controlled by loopback's dependencies specified in its package.json.

When we release a new semver-major version of juggler (4.0), we need to decide what to do with [email protected].

Is it going to keep using juggler 3.x and not receiving this fix? To me, that sounds as not very useful to LB 3.x users.

If we upgrade loopback dependencies to use [email protected], then we need to publish such change as [email protected]. Can we release [email protected] when our intention is to release loopback-next as the 4.0 version of LoopBack?

virkt25 · 2018-09-12T01:14:39Z

LoopBack users cannot choose the juggler version they are using, juggler version is controlled by loopback's dependencies specified in its package.json.

That makes sense. I was thinking of juggler as an individual module. I think @b-admike has a fix which won't be breaking.

stale · 2019-07-11T00:59:32Z

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.

dhmlau · 2019-07-15T22:59:44Z

@b-admike, are you still interested in completing this PR? Thanks.

stale · 2019-09-13T23:47:01Z

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 · 2019-09-28T00:30:38Z

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.

b-admike self-assigned this Jun 21, 2018

b-admike requested review from dhmlau, jannyHou, shimks, virkt25 and zbarbuto as code owners June 21, 2018 13:36

b-admike mentioned this pull request Jun 21, 2018

PersistedModel.destroyAll has very dangerous behaviour if the filter is not passed in correctly. strongloop/loopback#3094

Closed

2 tasks

b-admike force-pushed the fix/destroyall branch from dc21355 to 7fb58ea Compare July 23, 2018 18:18

b-admike force-pushed the fix/destroyall branch from 7fb58ea to ba16c48 Compare August 1, 2018 13:23

nitro404 reviewed Sep 2, 2018

View reviewed changes

b-admike changed the title ~~[WIP] handle bad filter(s) for destroyAll~~ handle invalid inputs for destroyAll Sep 6, 2018

b-admike mentioned this pull request Sep 6, 2018

fix: reject filter objects in destroyAll method loopbackio/loopback-connector#132

Closed

2 tasks

fixup: move tests to manipulation.test.js

42e2f1c

b-admike force-pushed the fix/destroyall branch from ba16c48 to 42e2f1c Compare September 6, 2018 14:45

virkt25 reviewed Sep 6, 2018

View reviewed changes

bajtos suggested changes Sep 7, 2018

View reviewed changes

virkt25 reviewed Sep 7, 2018

View reviewed changes

fixup! apply feedback and add logic for memory connector

23a54d8

b-admike mentioned this pull request Sep 24, 2018

Add case insensitive indexes support loopbackio/loopback-connector-mongodb#417

Merged

2 tasks

stale bot added the stale label Jul 11, 2019

stale bot removed the stale label Jul 15, 2019

stale bot added the stale label Sep 13, 2019

stale bot closed this Sep 28, 2019

rmg deleted the fix/destroyall branch March 18, 2021 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle invalid inputs for destroyAll #1597

handle invalid inputs for destroyAll #1597

b-admike commented Jun 21, 2018 •

edited

Loading

nitro404 Sep 2, 2018

nitro404 Sep 2, 2018

virkt25 Sep 6, 2018

bajtos Sep 7, 2018

b-admike Sep 7, 2018 •

edited

Loading

bajtos Sep 7, 2018

virkt25 Sep 7, 2018

b-admike Sep 10, 2018

bajtos left a comment

bajtos Sep 7, 2018

bajtos Sep 7, 2018

bajtos Sep 7, 2018

bajtos Sep 7, 2018

bajtos Sep 7, 2018

bajtos Sep 7, 2018

bajtos Sep 7, 2018

bajtos Sep 7, 2018

b-admike Sep 7, 2018

bajtos Sep 7, 2018

bajtos Sep 7, 2018

virkt25 Sep 7, 2018

virkt25 Sep 7, 2018

bajtos commented Sep 11, 2018

virkt25 commented Sep 12, 2018

stale bot commented Jul 11, 2019

dhmlau commented Jul 15, 2019

stale bot commented Sep 13, 2019

stale bot commented Sep 28, 2019

handle invalid inputs for destroyAll #1597

handle invalid inputs for destroyAll #1597

Conversation

b-admike commented Jun 21, 2018 • edited Loading

Description

Checklist

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

b-admike Sep 7, 2018 • edited Loading

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

bajtos 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

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

bajtos commented Sep 11, 2018

virkt25 commented Sep 12, 2018

stale bot commented Jul 11, 2019

dhmlau commented Jul 15, 2019

stale bot commented Sep 13, 2019

stale bot commented Sep 28, 2019

b-admike commented Jun 21, 2018 •

edited

Loading

b-admike Sep 7, 2018 •

edited

Loading