fix: reject filter objects in destroyAll method

[b-admike]

Description

Connect to loopbackio/loopback-datasource-juggler#1597 and strongloop/loopback#3094. The first commit ensures that we reject filter objects passed on to the destroyAll method. This should be applicable to all the SQL connectors.

EDIT: The second commit ensures that we reject objects with keys that are not part of the model properties in destroyAll method. This should be applicable to all the SQL connectors.

Related issues

• connect to <link_to_referenced_issue>

Checklist

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

[virkt25]

Shouldn't the fix live in Juggler?

[virkt25]

Are the test failures related to this PR?

[b-admike]

@virkt25 The fix could live in juggler, but based on strongloop/loopback#3094 (comment), I opted to have it here and since juggler's dao is the high level entry point for destroyAll, I thought it made more sense to scope the fix in this repository. Another discussion point is whether or not we want to have a flag for this behaviour.

EDIT: Also, the failures on db2/dashdb are not related to this PR.

[bajtos]

IMO, the change proposed above is not really fixing the problem.

For example, consider a Product model with a name property, the developer making a typo in their query and calling Product.destroyAll({names: 'Pen'}). Right now, the entire SQL table is deleted. IMO, the correct behavior is that no records should have been deleted.

I think the root cause of this whole issue lies here.

    var p = props[key];
     if (p == null) {
       // Unknown property, ignore it
       debug('Unknown property %s is skipped for model %s', key, model);
       continue;
     }

Instead of ignoring unknown properties, we need to construct a WHERE statement that is not going to match any record, or perhaps throw an error as you proposed above (but please use a different error message to better describe the new intent).

[b-admike]

Good catch! Thank you for illustrating what is IMO another big issue from the original ticket (among others). While I'm working on that, I want to keep what I have because it ensures rejection of filter objects. Do you have suggestions on how I can improve that (test case is here)?

[bajtos]

How about the following?

 var p = props[key];
 if (p == null) {
-  // Unknown property, ignore it
-  debug('Unknown property %s is skipped for model %s', key, model);
-   continue;
+  throw new Error(
+    `Unknown property ${key} used in a "where" condition for model ${model}`
+  );
}

[bajtos]

Another discussion point is whether or not we want to have a flag for this behaviour.

This is an important question. I think the new behavior should be enabled by default, because it makes more sense and is safer for LB users.

However, there may be applications out there that rely on the current behavior and will break after update. To mitigate that, I think it makes sense to introduce a feature flag allowing applications to switch to the old behavior.

Since the new feature flag is relevant only for SQL-based connector, I think it makes most sense to define it at datasource/connector-setting level (configured in server/datasources.json in LB 3.x and src/datasources/{name}.datasource.json in LB 4. IIRC, datasource/connector settings are available inside connector methods via this.settings.

[b-admike]

Another discussion point is whether or not we want to have a flag for this behaviour.

This is an important question. I think the new behavior should be enabled by default, because it makes more sense and is safer for LB users.

However, there may be applications out there that rely on the current behavior and will break after update. To mitigate that, I think it makes sense to introduce a feature flag allowing applications to switch to the old behavior.

Since the new feature flag is relevant only for SQL-based connector, I think it makes most sense to define it at datasource/connector-setting level (configured in server/datasources.json in LB 3.x and src/datasources/{name}.datasource.json in LB 4. IIRC, datasource/connector settings are available inside connector methods via this.settings.

That sounds great. Thank you for pointing out where it should be defined too! I'll take a crack at it.

[bajtos]

1. Please review all places that may call _buildWhere and make sure they report catch the new error and report it via the callback. An easy example: save. We need to look for places that are calling other helpers that eventually call _buildWhere, e.g. code calling _constructUpdateQuery.

2. Please add tests to cover these changes.

3. What's your opinion on the feature flag discussed above - should it be implemented as part of this pull request?

[bajtos]

It would be nice if we could list all unknown properties in the error message.

[stale]

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]

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.