Skip to content
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

RelationDefinition applyScope/applyMapping #172

Merged
merged 3 commits into from
Jul 11, 2014

Conversation

fabien
Copy link
Contributor

@fabien fabien commented Jul 11, 2014

Add the ability to set static/dynamic filtering (where, order, limit, skip, fields …) and property mapping/transfer (de-normalization) for hasMany/hasOne. This enables LoopBack beforeRemote handlers to dynamically change the relation scope. See the comment in the tests for the rationale.

Based upon this, it should be easy to create a public method to set this scope on the instance-level (setScope({ ... })).

Add the ability to set static/dynamic filtering (where, order, limit,
skip, fields …) and property mapping/transfer (de-normalization) for
hasMany/hasOne.
@slnode
Copy link

slnode commented Jul 11, 2014

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

@raymondfeng
Copy link
Contributor

The path looks great. I wonder if we can have a better name than mapping. To some extent, these properties in mapping are extended foreign keys. Maybe constraints? What do you think?

Meanwhile, I think we should decouple the constraints from query filter for scope definition too.

@fabien
Copy link
Contributor Author

fabien commented Jul 11, 2014

Thanks @raymondfeng you are right, they can be seen as foreign/composite keys. However, you can also use them to transfer properties to de-normalize for example. Since the mapping will only be applied during creation, you should obviously limit this to more or less immutable properties/data. Because you can handle the mapping dynamically, it can also be used to have related items 'inherit' certain (default) attributes from a parent, unless the item already defines it. In short, it will come into play in all kinds of situations.

So mapping denotes a more broad concept - what about transfer? or simply properties?

My app definitely needs the dynamic scoping, so I hope we can merge in something like this soon.

@raymondfeng
Copy link
Contributor

What about foreignProperties or relatedProperties?

@fabien
Copy link
Contributor Author

fabien commented Jul 11, 2014

I updated my comment. Yeah, I'm fine with that - relatedProperties perhaps? However, I always try to keep option keys short and sweet.

@raymondfeng
Copy link
Contributor

I guess I'm cool with properties too as it's already inside the relation definition. If you agree, let's make the changes and I'll merge it in.

@fabien
Copy link
Contributor Author

fabien commented Jul 11, 2014

Sounds good, thanks!

Any thoughts on a public method to set the scope dynamically? For this to work, it should be picked up by applyScope too. Here's what I'm doing in a beforeRemote handler at the moment:

var clientId = ctx.req.clientId; // for example
Object.defineProperty(ctx.instance, '__dynamicScope', {
    writable: false, enumerable: false,
    value: { where: { clientId: clientId } }
});

And then look for __dynamicScope in my scope function. It would be even better if we could standardize this.

@raymondfeng
Copy link
Contributor

I see what you mean. So you would like to inject values from the context into the query. That's pretty common. Would the instance level dynamic scope only apply to relations? What about other scopes?

@fabien
Copy link
Contributor Author

fabien commented Jul 11, 2014

Yes, that is indeed common. Another issue #168 refers to a more generalized approach for other scopes. On the other hand, handling the exact behavior is hard to really generalize. For example, to which relations would the dynamic scope pertain? Inside the dynamic scope function 'this' refers to the current relation definition, so that makes it easy to inspect and act accordingly.

If we were to go for a more general approach for relations, we'd need something like:

setScope(relationName, scope) - ex: instance.setScope('projects', { clientId: x })

Where scope can be an object or function.

Not sure how to handle #168 but I think it might be similar:

setScope(scope) (just 1 argument, the scope object or function only).

@fabien
Copy link
Contributor Author

fabien commented Jul 11, 2014

@raymondfeng no matter how the public API will turn out, I think it's safe to apply this pull request as a low-level means. What do you think?

@raymondfeng
Copy link
Contributor

Sure

@raymondfeng
Copy link
Contributor

@fabien Did you have chance to rename 'mapping' to 'properties'?

@fabien
Copy link
Contributor Author

fabien commented Jul 11, 2014

@raymondfeng yes, just committed.

@raymondfeng
Copy link
Contributor

@fabien Thank you so much. One last thing, would you please sign off the commits per https://github.com/strongloop/loopback/blob/master/CONTRIBUTING.md#signing-patches?

Signed-off-by: Fabien Franzen <[email protected]>
@fabien
Copy link
Contributor Author

fabien commented Jul 11, 2014

@raymondfeng just signed off - will do it immediately next time!

@raymondfeng raymondfeng merged commit 03617b5 into loopbackio:master Jul 11, 2014
@fabien
Copy link
Contributor Author

fabien commented Jul 22, 2014

@raymondfeng I deliberately didn't implement belongsTo scope/property, however, I'm working on that right now. Unfortunately, I've hit a bug it seems.

Using the MongoDB connector, a hasManyThrough setup implicitly coerces the foreignKeys to type 'string', instead of ObjectID - even though I explicitly set: mongodb: { type: 'objectid' } in my through model's fk property.

Looking into it further, it appears this definition that causes the implicit to-string coercion:

"id": { "type": "string", "id": true, "mongodb": { "type": "objectid" } }

When creating the modelTo-instance at HasManyThrough.prototype.create, it will be a string regardless. Leaving out the type parameter throws an exception, so I'm not sure what else I should set this to. In MongoDB itself, the pk is indeed an ObjectID, so I suspect this coercion to take place in LB.

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.

3 participants