Skip to content

Default scope #296

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

Merged
merged 10 commits into from
Oct 9, 2014
Merged

Default scope #296

merged 10 commits into from
Oct 9, 2014

Conversation

fabien
Copy link
Contributor

@fabien fabien commented Sep 7, 2014

See #276 - this loosely follows http://guides.rubyonrails.org/active_record_querying.html#applying-a-default-scope

It consists of two parts, both static methods:

applyProperties - which, as its relation's namesake, will apply property values from the default scope.

The values will be gathered from either a properties object (or function) on the model definition's settings, or straight from the default scope's where clause.

Unlike default values, these values are always enforced, and are regarded as immutable. None of the manipulation methods can override these values (as this could pose security holes and other inconsistencies). Set properties: false to disable this enforcing of properties.

In order to be compatible with LDL notation (model.json), properties has been aliased to attributes.

applyScope - this will apply the base conditions (where, order, skip, limit, fields, ...) for any operation on the model, and cannot be circumvented in any way, except for the order which is to be considered harmless.

They both feed off the defaultScope static method by default, for the scope to apply. In turn it uses the scope specified on the model definition's settings (as available to LDL as well), which can be a function as well, to dynamically generate the scope at runtime (think of: conditions involving a timestamp, eg. 'recent items').

Overriding any of these methods is also considered a viable and very powerful technique. Especially whenever strongloop/loopback#337 lands.

Note that the properties will be applied even on a new instance, and enforced whenever persisting data. This could be the basis for true immutable properties (the current instance will be provided as a context to a function, where appropriate).


In order to test this in a more real-world scenario, I adapted the Memory connector to support the concept of collections, that can be set by the user. The MongoDB connector supports this as well, whereas SQL connectors can use a table setting for this purpose (both mongo as well as postgresql have been tested with default-scope.test.js). In essence, the test case mimics a Single Table Inheritance setup (see http://www.martinfowler.com/eaaCatalog/singleTableInheritance.html).

Each model is persisted to the same table, and the model type is stored in a kind property using default scope/enforced properties.

To take this further, and return instances of the correct Model class (Thing, Tool, Widget), the newly introduced 'lookupModel' method was introduced, which will accept an object (either an instance or raw data) to determine the right type at runtime.


Finally, by introducing this feature, some connector methods are becoming more or less obsolete - lookups just by their id are no longer sufficient to cover the additional scope conditions that might be set. It has to be seen if this needs further optimization, but for most connectors there's little to no difference in the way the final query will be performed. And thus, performance should not suffer at all, except for the default scope incurred filtering, if any (make sure you use indexes).

@altsang altsang added the #review label Sep 7, 2014
@fabien fabien changed the title Feature/default scope Default scope Sep 7, 2014
@raymondfeng
Copy link
Contributor

@fabien Nice work! Would you please resolve the conflicts?

@raymondfeng
Copy link
Contributor

BTW, please sign the CLA at https://cla.strongloop.com/agreements/strongloop/loopback-datasource-juggler. We now have centralized CLA management that integrates with github PRs. The signing will be one-time thing per repo.

@fabien
Copy link
Contributor Author

fabien commented Sep 13, 2014

@raymondfeng done - we can probably add require('loopback-datasource-juggler/test/default-scope.test.js'); to the imported tests in MongoDB and PostgreSQL (I tested them both).

Would you mind adding those and re-test to verify?

@raymondfeng
Copy link
Contributor

Sure, I'll run the tests before merging the PR.

}
cb(err, obj);
}.bind(this));
this.findOne(byIdQuery(this, id), cb);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabien This change is questionable. I understand you want to unify the variants of find, but we should try to honor the implementation from the underlying connector. For example, mongodb and other connectors might have more efficient/specific way of findById than findOne.

As a side note, we plan to make the signature of findById closer to find or findOne. For example, we should be able to provide filter options, such as include and fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raymondfeng I'd agree, if it weren't for the fact that you'd want to apply the scope on findById and findOne as well.

How else can you ensure that the object with the id your fetching is actually passes any further scoping conditions?

You might be able to re-use the applyFilter function of the Memory connector to test this, after the item has been fetched, but this has a lot of drawbacks too (you can't use MongoDB native query conditions for example).

Regarding the signature, that would be a good thing - I might be able to work on that.

@kwiky
Copy link

kwiky commented Sep 24, 2014

Wow, that a feature i need now ;)

I am searching for a default scope {"where": {"published": "true"}}
and a "unpublished" scope {"where": {"published": "false"}} only readable/writable by an admin role

@fabien fabien force-pushed the feature/default-scope branch from a005740 to 0e49dc9 Compare October 9, 2014 16:14
@fabien
Copy link
Contributor Author

fabien commented Oct 9, 2014

Let's not forget to add require('loopback-datasource-juggler/test/default-scope.test.js'); to the other connector tests - MongoDB and PostgreSQL should work already.

raymondfeng pushed a commit to loopbackio/loopback-connector-mongodb that referenced this pull request Oct 9, 2014
@raymondfeng
Copy link
Contributor

@fabien
Copy link
Contributor Author

fabien commented Oct 9, 2014

@raymondfeng already done then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants