New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set juggler configs in options by default #4065
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LTGM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👎
I am not a fan of leaking remoting context to juggler. Ideally, loopback-datasource-juggler should not deal with anything that's specific to REST APIs and stay focused on ORM & data access.
In particular case of loopbackio/loopback-datasource-juggler#1662, I would like us to investigate a different design that I consider as more clean and robust:
- In juggler, we allow security related flags to be controlled for each method invocation via
options
object - In LoopBack, we modify
createOptionsViaModelMethod
to set these security-related flags to reasonable defaults.
One of the big benefits I see in this approach: because security-related flags are configured via options
, LB app developers can easily tweak them e.g. via custom options factory (see Customize the value provided to options in our docs).
lib/model.js
Outdated
@@ -492,14 +492,17 @@ module.exports = function(registry) { | |||
} | |||
|
|||
function createOptionsViaModelMethod(ctx) { | |||
var EMPTY_OPTIONS = {}; | |||
var REMOTE_OPTIONS = { | |||
remotingContext: ctx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, my suggestion boils down to the following change in your patch:
remotingContext: ctx, | |
maxDepthOfQuery: 16, | |
maxDepthOfData: 32, |
lib/model.js
Outdated
@@ -492,14 +492,17 @@ module.exports = function(registry) { | |||
} | |||
|
|||
function createOptionsViaModelMethod(ctx) { | |||
var EMPTY_OPTIONS = {}; | |||
var REMOTE_OPTIONS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var REMOTE_OPTIONS = { | |
var DEFAULT_OPTIONS = { |
I thought about setting default values for such flags in Method level
With the PR, developers can still reset such flags by overriding createOptionsFromRemotingContext(ctx) {
return {prohibitHiddenProperties: true};
} I'm open to a slightly different form, for example: const REMOTE_OPTIONS = {
defaultJugglerSettings: {
prohibitHiddenProperties: true
}
} |
bc62718
to
a568638
Compare
@bajtos I reworked the patch to honor datasource/model settings and configure reasonable defaults in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
// Default to `12` for the max depth of a query object | ||
maxDepthOfQuery: ModelCtor._getMaxDepthOfQuery({}, 12), | ||
// Default to `32` for the max depth of a data object | ||
maxDepthOfData: ModelCtor._getMaxDepthOfData({}, 32), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely 👍
@@ -1028,7 +1062,6 @@ describe.onServer('Remote Methods', function() { | |||
return User.login(CREDENTIALS); | |||
}).then(function(token) { | |||
accessToken = token; | |||
userId = token.userId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems rather unrelated, could you please remove it (revert this line)?
a568638
to
f1c613a
Compare
Description
This PR allows set
prohibitHiddenPropertiesInQuery
,maxDepthOfQuery
, andmaxDepthOfData
inoptions
for CRUD operations.Related issues
loopbackio/loopback-datasource-juggler#1662
Checklist
guide