Allow the default scope to be overridden

[0ff]

When working with scopes + default scope I realized a behavior that does not seem to be intended: Whenever you define a default scope, every include in this scope will be immutable in every request. It will also shadow any call to the scope-remote-method.

In my opinion, this problem was introduced in #569 . Think of the following configuration:

modelA.json:

{
    ...
    "scope": {
        "include": ["modelB"]
    },
    "scopes": {
        "modelBFiltered": {
            "include": [{
                "relation": "modelB",
                "scope": {
                    "where": {
                        "active": true
                    }
                }
            }]
        }
    }
}

test.js

ModelA.find() 
// -> ModelA-Instances, with *all* modelB-relations (as expected)

ModelA.find({
    include: {
        relation: 'modelB',
        scope: {
            where: {
                active: true
            }
        }
    }
})
// -> ModelA-Instances, with *all* modelB-relations 
// (you'd expect only the active ones, but that doesn't work)

ModelA.modelBFiltered() 
// -> ModelA-Instances, with *all* modelB-relations
// (you'd expect only the active ones, but that doesn't work)

So basically, if you define an include in your default scope, you cannot filter on that relation.

This PR does have security-implications, because with the change one might override an include that was intended to be restricted (think my example, but the other way around). I don't see any solution to this but to be honest, that must not be a task the scope should solve, because any relation not explicitly specified in the default scope can be included either way. Related to https://github.com/strongloop/loopback-datasource-juggler/pull/569

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[bajtos]

@raymondfeng @fabien PTAL

[bajtos]

@slnode ok to test

[ebarault]

I was doing the exact same change on the mergeIncludes call when I realized it is not enough, as it does not allow every possible overriding scenarios.

the way the filters are overwritten in the following of the mergeQuery method makes that fields from the default scope would overwrite the fields from the runtime scope. for example for the fields filter see the following excerpt:

currently:

  // Overwrite fields
  if (spec.fields !== false && update.fields !== undefined) {
    base.fields = update.fields;
  } else if (update.fields !== undefined) {
    base.fields = [].concat(base.fields).concat(update.fields);
  }

which overwrites the runtime fields whenever there are fields in the default scope

in order not to overwrite but merge instead, one could want the following behavior:

  if (spec.fields !== false && base.fields === undefined && update.fields !== undefined) {
    base.fields = update.fields;
  } else if (spec.fields !== false && update.fields !== undefined) {
    base.fields = [].concat(base.fields).concat(update.fields);
  }

which replaces fields only if runtime fields are not defined, or merge if some are defined (not clean merge though are duplicates are not checked against, but not worst than currently)

and yet i'm not fully convinced as we have no mean here to control when we absolutely want to overwrite the default fields instead of merging them

the same goes for the other filters.

I'd happy more than happy to submit a PR provided that some guidance and thoughts are suggested. I referenced a PR which is not complete, just for ease of understanding: https://github.com/ebarault/loopback-datasource-juggler/issues/1

---

one further detail for the current PR from @0ff :

as per the comments on the mergeIncludes method, the patch is more to be applied directly on the method signature than on the method call in mergeQuery as source property is expected to be the runtime value of include option

/**
 * Merge include options of default scope with runtime include option.
 * exhibits the _.extend behaviour. Property value of source overrides
 * property value of destination if property name collision occurs
 * @param {String|Array|Object} destination The default value of `include` option
 * @param {String|Array|Object} source The runtime value of `include` option
 * @returns {Object}
 */
// function mergeIncludes(destination, source) {
function mergeIncludes(source, destination) {

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[superkhau]

@raymondfeng Can you help with this contribution? I'm not sure of the implications or BC issues and you have more expertise here.

[raymondfeng]

Between the default scope and the query criteria, I think query criteria should be able to further constrain the match. We need to evaluate the following properties:

• include (recursively with scope)
  • We need to merge the scopes based on the rules of further constraining.
• limit/offset/skip
  • The pagination window from query criteria should be within the one required by the default scope
• where
  • We use and to do the merge so that both conditions are honored
• fields
  • The allowed fields from query criteria should be the same or a subset of the default scope

Can we follow these rules for the merge?

[mrfelton]

I just created https://github.com/fullcube/loopback-ds-resultset-limit-mixin which enables a default/max limit to be applied the way that I believe a default scope should work. I think this is inline with what @raymondfeng was suggesting in that it acts as an overridable hard maximum constraint rather than a hard set non-overridable value that always applies. This prevents the user selecting more items than the limit, but allows them to select less if they choose to.

It would be great if defaultScope could be updated to work this way.

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[bajtos]

Possibly related (or duplicates):

• https://github.com/strongloop/loopback/issues/3279
• https://github.com/strongloop/loopback/issues/1821
• https://github.com/strongloop/loopback/issues/1675

[loay]

@slnode test please

[acrodrig]

Has this been corrected? I am not sure with the issue still being open and the tests failing?

[jmereaux]

Any news on this? Is there a way to override in the query filter the default scope defined on a model ?