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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(repository): enable inclusion with custom scope #4263

Merged
merged 1 commit into from Dec 9, 2019
Merged

Conversation

@agnes512
Copy link
Contributor

agnes512 commented Dec 5, 2019

Implements #3453

This will allow using custom scope in inclusion. Example:

Order: [{id: 1, customerId: 1, name: "to be excluded"}, {id: 2, customerId: 1, name: "to be included"}]
result = await customerRepo.find({
     include: [
       {
         relation: 'orders',
         scope: {where: {id: 2}, include: [{relation: 'customer'}]},
       },
     ],
   });

the result should be:

[{ id: 1, 
   name: 'IBM', 
   orders:[
      {id: 2, 
       customerId: 1, 
       name: "to be included",
       customer: {id: 1, name: 'IBM'}
      }
   ]
 }]

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@agnes512 agnes512 force-pushed the custom-scope branch 3 times, most recently from 8dcf736 to a7e4828 Dec 5, 2019
whereClause: Where<Target>,
scope: Filter<Target>,
): Promise<Filter<Target>> {
scope.where = Object.assign(whereClause, scope.where);

This comment has been minimized.

Copy link
@bajtos

bajtos Dec 5, 2019

Member

I believe we have utility functions to combine multiple where clauses safely, to ensure operators like and and or are handled correctly. @raymondfeng do you remember where to find these helpers? I think it's something like FilterBuilder and WhereBuilder.

This comment has been minimized.

Copy link
@agnes512

agnes512 Dec 6, 2019

Author Contributor

Applied

@agnes512 agnes512 force-pushed the custom-scope branch from a7e4828 to fe5a70e Dec 5, 2019

```
// need to fix this
GET http://localhost:3000/customers?filter[include][0][relation]=orders&filter[include][0][scope][0]filter[where][name]=ToysRUs

This comment has been minimized.

Copy link
@agnes512

agnes512 Dec 6, 2019

Author Contributor

This url filters[name]=ToysRUs on orders. But I don't know what the syntax to query nested inclusion is.
I tried

http://localhost:3000/customers?filter[include][0][relation]=orders&filter[include][0][scope]filter[include][0][relation]= manufacturers

but it doesn't work. I need some help here, thanks!

This comment has been minimized.

Copy link
@bajtos

bajtos Dec 6, 2019

Member

My recommendation is to use qs.stringify to convert the filter object to a query string.

Inside packages/rest:

$ node
> require('qs').stringify({filter: {include:[{relation: 'orders', scope: {include:[{relation: 'manufacturers'}]}}]}})
'filter%5Binclude%5D%5B0%5D%5Brelation%5D=orders&filter%5Binclude%5D%5B0%5D%5Bscope%5D%5Binclude%5D%5B0%5D%5Brelation%5D=manufacturers'

url-decoded:

$ node
> decodeURI(require('qs').stringify({filter: {include:[{relation: 'orders', scope: {include:[{relation: 'manufacturers'}]}}]}}))
'filter[include][0][relation]=orders&filter[include][0][scope][include][0][relation]=manufacturers'

This comment has been minimized.

Copy link
@bajtos

bajtos Dec 6, 2019

Member

Personally, I would show users how to encode the filter via JSON:

encodeURIComponent(JSON.stringify(filter))

There is a subtle difference in the output of encodeURI and encodeURIComponent, one of them is not correctly encoding special characters that must be encoded in query string values. I hope I remember it correctly that encodeURIComponent is the function to use.

This comment has been minimized.

Copy link
@agnes512

agnes512 Dec 6, 2019

Author Contributor

I still couldn't find a good way to encode it.

  • qs.stringify and encodeURIComponent give different results.

  • I tried to print out the encoded filter and found something weird, for filter

{filter: {include:[{relation: 'orders', scope: {include:[{relation: 'manufacturers'}]}}]}}

With qs.stringify, the outer relation is encoded to {relation:"outer"} as expected. But the inner one is not correct: {[relation: "inner"]}. Idk if the extra [] is causing invalid filter problems.

Even I would like to leave the encode part to users, I still think it's better to have at least one simple and working nested inclusion syntax as an example in docs.

@bajtos
bajtos approved these changes Dec 6, 2019
Copy link
Member

bajtos left a comment

I quickly skimmed through the changes and don't see any obvious problems. Please get at least one more approval before landing.

docs/site/HasMany-relation.md Outdated Show resolved Hide resolved
docs/site/HasMany-relation.md Outdated Show resolved Hide resolved

```
// need to fix this
GET http://localhost:3000/customers?filter[include][0][relation]=orders&filter[include][0][scope][0]filter[where][name]=ToysRUs

This comment has been minimized.

Copy link
@bajtos

bajtos Dec 6, 2019

Member

Personally, I would show users how to encode the filter via JSON:

encodeURIComponent(JSON.stringify(filter))

There is a subtle difference in the output of encodeURI and encodeURIComponent, one of them is not correctly encoding special characters that must be encoded in query string values. I hope I remember it correctly that encodeURIComponent is the function to use.

@agnes512 agnes512 force-pushed the custom-scope branch from de042b7 to 20499e9 Dec 6, 2019
@agnes512 agnes512 marked this pull request as ready for review Dec 6, 2019
@agnes512 agnes512 requested a review from raymondfeng as a code owner Dec 6, 2019
@agnes512 agnes512 force-pushed the custom-scope branch from 20499e9 to b919794 Dec 6, 2019
Besides specifying the relation name to include, it's also possible to specify additional scope constraints.
However, this feature is not supported yet. Check our GitHub issue for more information:
[Include related models with a custom scope](https://github.com/strongloop/loopback-next/issues/3453).
### More use cases - query multiple relations:

This comment has been minimized.

Copy link
@dhmlau

dhmlau Dec 6, 2019

Member

Since we're duplicating the contents for the 3 relations pages, should we put it at the top level to avoid duplication?

This comment has been minimized.

Copy link
@agnes512

agnes512 Dec 6, 2019

Author Contributor

It doesn't make sense to me if we put it at Relation.md. How about I link this part from belongsTo and hasOne to hasMany?

@agnes512 agnes512 force-pushed the custom-scope branch 2 times, most recently from b387803 to b0041d7 Dec 6, 2019
Copy link
Contributor

jannyHou left a comment

馃憤 LGTM, commented for the url.

});
```

<!-- FIXME: the url isn't being converted to JSON correctly. Add an example url once it's fixed

This comment has been minimized.

Copy link
@jannyHou

jannyHou Dec 9, 2019

Contributor

Maybe we can wait until #2208 finished and see what's the generated url that explorer gives us, then update the example here.

This comment has been minimized.

Copy link
@agnes512

agnes512 Dec 9, 2019

Author Contributor

Sounds good

@agnes512 agnes512 force-pushed the custom-scope branch from b0041d7 to 46596fa Dec 9, 2019
@agnes512 agnes512 merged commit 4a0d595 into master Dec 9, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.01%) to 92.247%
Details
@agnes512 agnes512 deleted the custom-scope branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.