Use options.remotingContext for conditional settings #1662
Conversation
raymondfeng
requested review from
b-admike,
dhmlau,
jannyHou,
virkt25 and
zbarbuto
as code owners
raymondfeng
force-pushed
the
set-default-remote-options
branch
from
47c28bf
to
786a901
Compare
| @@ -345,7 +345,7 @@ Memory.prototype.findOrCreate = function(model, filter, data, callback) { | |||
| }); | |||
| } | |||
|
|
|||
| self._models[model].model.include(nodes[0], filter.include, {}, function(err, nodes) { | |||
| self._models[model].model.include(nodes[0], filter.include, options, function(err, nodes) { | |||
There was a problem hiding this comment.
options || {} in case the option is undefined.
lib/dao.js
Outdated
| // See https://github.com/strongloop/loopback-datasource-juggler/issues/1651 | ||
| var maxDepth = this._getSetting('maxDepthOfQuery', options); | ||
| if (maxDepth == null) { | ||
| if (isRemote) { |
There was a problem hiding this comment.
As I commented in strongloop/loopback#4065 (review), juggler should not be dealing specifics of remoting. I prefer us to allow juggler API callers to provide custom maxDepthOfQuery via options, which we already have for free thanks to this._getSetting.
lib/dao.js
Outdated
| // See https://github.com/strongloop/loopback-datasource-juggler/issues/1651 | ||
| var maxDepth = this._getSetting('maxDepthOfData', options); | ||
| if (maxDepth == null) { | ||
| if (isRemote) { |
There was a problem hiding this comment.
Ditto: As I commented in strongloop/loopback#4065 (review), juggler should not be dealing specifics of remoting.
raymondfeng
force-pushed
the
set-default-remote-options
branch
from
786a901
to
f9131aa
Compare
|
@bajtos PTAL |
There was a problem hiding this comment.
I don't think it's a good idea to move DataAccessObject._normalize into a different file:
- The format of Filter object is to specific to DAO only, KeyValue APIs use different filter properties.
- It is pretty much impossible to review changes made in that method.
- Such huge change will be difficult to back-port to older versions, because there is no automated way how to ensure the backport is not accidentally back-porting changes in
_normalizeimplementation that are not supposed to be back-ported.
The rest of the patch LGTM.
| @@ -336,7 +336,7 @@ function sanitizeQuery(query, options) { | |||
| const prohibitedKeys = options.prohibitedKeys; | |||
| const offendingKeys = []; | |||
| const normalizeUndefinedInQuery = options.normalizeUndefinedInQuery; | |||
| const maxDepth = options.maxDepth || 12; | |||
| const maxDepth = options.maxDepth || Number.MAX_SAFE_INTEGER; | |||
There was a problem hiding this comment.
Is it a good idea to allow arbitrary deep queries? Or is this reverting back to the old behavior we used to have in juggler before recent security-related fixes?
There was a problem hiding this comment.
This is a default value for the utility method itself. Juggler already sets 32 as the default value.
There was a problem hiding this comment.
So can we use 32 as the default value in this place too? I'd rather avoid all possibilities of implicitly infinite depth.
Please check the coverage data, there may be an important use case (test scenario) that you forgot to cover. |
The refactoring is to fix #1663. |
Description
This PR uses
optionsfrom remote requests to decide default values for certain security-related settingsRelated issues
See #1638 (comment)
#1663
Checklist
guide