Limit in nested scope for hasMany relations

[fabripeco]

Hi,

I'm wondering if the 'limit' param works if added to the scope of nested relations, to limit the number of items of a hasMany relation. I have a strange behaviour in the response.
I have a BusinessPartner model with hasMany relation to Addresses

@hasMany(() => BuspartnerAddress, {keyTo: 'clientId', name: 'addresses'})
addresses: BuspartnerAddress[];

I want to get the businessPartners with only the first address for each one. My filter param looks like

{
  "order": ["name ASC"],
  "where": {
    "bustype": "CUSTOMER"
  },
  "include": [
    {
      "relation": "addresses",
      "scope": {
        "skip": 0,
        "limit": 1,
        "where": {
          "deleted": false
        }
      }
    }
  ]
}

The where clause in the scope of the include works well, but the limit: 1 param returns the address for only one businessPartner, not one for each businessPartner. If I increase the value of the limit, e.g. 10, the response returns max 10 address, differently distribuited on the businessPartners.
It looks like the query on the related models was performed only once and not for each 'parent' instance (businessPartner).

Am I wrong in something? Is this the intended behaviour?

I'm using a postgresql connector and this is my ecosystem (Nodejs v12.16.2 npm v6.14.4 - Postresql 12)

├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── @loopback/[email protected]
├── [email protected]
├── [email protected]

Thank you very much

PS: maybe related to #3453

[agnes512]

@fabripeco thank you reporting this. I can reproduce it on my end. And you're right, it is because of the way we query related data. In here, we query all related models with all target foreign keys. i.e when it fetches related models, it has filter:

{
  limit:10
  where:{
    userId:{inq: [1, 2, ...],
    deleted: false
}

instead of two separate queries.

The workaround I can think about atm is to manipulate the results D:
To fix the issue, we might need to do something similar to #5592, or change the way we query related models.

[fabripeco]

Ok, thanks for your reply.
Maybe another option for you could be to query the related models with only the where param (and sort and fields params) on the target repository and afterwards to accomplish the pagination params in memory (skip and limit). In this way you can keep a single query on the target repository. Just an idea.
Thank you!

[kpayson]

Maintainers's note: A separate issue, #6924, has been created.

I am having the same issue I think. In my schema Tenants have multiple Clients and Clients have multiple Providers. When I write a query for tenants that includes Clients and includes the Providers inside the Clients I get a max depth exceeded error.

  public async exportTenantData(
    @param.path.number('tenantId') tenantId: number
  ): Promise<any> {

    const inclusions = [
      {
        relation: 'clients',
        scope: {
          include: [
            {
              relation: 'providers',
              scope: {
                fields: { id: true }
              }
            },
          ]
        }
      },
    ];

    const filter = { include: inclusions };
    return this.tenantRepository.findById(tenantId, filter);
  }

[stale]

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

[stale]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

[collaorodrigo7]

This should not have been closed w/o a fix. Also, This filter worked fine on LB3.
I am happy to take a look and try to help to solve it but I will need some guidance.
As a workaround, I first query the source model and then run multiple queries to get the related model includes for each id of the source model.
Is this how lb3 used to do it?
@agnes512 do you have any ideas on the best approach for this?

[collaorodrigo7]

Thanks, @achrinza
Do you have any suggestions on how we could implement it, and if what I mentioned is how LB3 used to do it and if the goal will be to make it like that?

[achrinza]

Apologies for letting it go stale; I'm not familiar with the LB3 codebase so unfortunately I can't provide info on how it's been done historically, but I think your idea makes sense given the current architecture of the @loopback/repository package.

If I'm not mistaken. this is the part of the code that needs changing:

loopback-next/packages/repository/src/relations/has-many/has-many.inclusion-resolver.ts

Lines 69 to 75 in cb29cbe

	const targetsFound = await findByForeignKeys(
	targetRepo,
	targetKey,
	sourceIds,
	scope,
	options,
	);

We can see it passes all foreign key instances into findByForeignKeys. As you've suggested, we could change this to instead run a new query for each foreign key and compare the result tally after each query with the user-specified limit.

Though I think we should introduce a new filter type (i.e. parentLimit? subLimit?, though I'm open to suggestions) so as to not break backwards compatibility; Both limit and the new parentLimt should be able to work together and independently.

[collaorodrigo7]

Thanks @achrinza! Sorry I was away this weekend.
I think I am not getting why we need a different limit property. When the limit is passed on the scope of the inclusion filter, I think, it makes sense to assume it is talking about the related model include limits. Contrary, if the limit is passed on the parent level, then it's obvious it is referring to the parent level limit?. Here is a simplified version of the original example in this issue:

{
  "limit": 10,
  "include": [
    {
      "relation": "addresses",
      "scope": {
        "limit": 5
      }
    }
  ]
}

Here the first limit 10 should be applied to the source model, and the limit 5 should be applied to the related model addresses.
So the resulting object should have up to 10 instances of the source/parent model and each source model instance UP TO 5 instances of the related model address.
This also allows having multiple includes, each one with its own limits.
While I am not familiar with how lb3 did this neither, I think this is how that query will behave in lb3.

If you still think we should introduce the parentLimit/subLimit, could you please provide an example of how will it be used?

[mrbatista]

@collaorodrigo7 @achrinza
I need to execute query for each source model found like mongodb connector
This is code reference to include functionality.

[achrinza]

Sorry for missing your message @collaorodrigo7

I think I am not getting why we need a different limit property.

Apologies if it was a bit confusing; The suggestion was more in the spirit of keeping backwards compatibility, but I think it should be fine to replace the original limit behaviour to a more correct implementation.

Just that IMO we should continue to provide the original behaviour somehow or provide a way for people to emulate the current behaviour, as the bug may have become an expected "feature" for some users.

I admit my original proposal was confusing and unintuitive; Maybe using relation.scope.totalLimit for the old behaviour would better;

The current behaviour is acting like a "global limit" for the relations, so that means relation.scope.limit: 5 would only return 5 "sub-models" regardless of how many parents were returned.

In contrast, the expected behaviour should mean that relation.scope.limit: 5 should be on a per-parent basis; That is, if 5 parents are returned, a max of 5x5=25 children can be returned, akin to an SQL LEFT JOIN.

[fabripeco]

Maybe using relation.scope.totalLimit for the old behaviour would better;

I agree because the limit param is inside the scope of the relation. If someone really need the total number of related items differently distributed among the parent items (a bit strange, but possible) it's better to use a different param.
Thanks!

[collaorodrigo7]

Thank you all. I am working on other things at the moment, but I am gonna try to look at this soon and submit a PR. Hopefully this weekend

[collaorodrigo7]

Sorry for the delay, the PR is ready. At the moment it is behaving the same as LB3 used to.
About the backward compatibility, I am not sure what we will do with things other than limit.
While this issue is specific about limit, this PR solves for any filter in the scope, such as order, where, so to keep the backward compatability we will need a totalLimit, totalOrder, totalWhere ?. I don't know what do you guys think @mrbatista @achrinza @fabripeco
Let me know and I can make changes accordingly.
Thank you all.

Example of what the PR does:

Say we have:

folders=[{name: 'f1', id: 1},
          {name: 'f2', id: 2},
          {name: 'f3', id: 3},
          {name: 'f4', id: 4}]

files=[
          {id: 1, title: 'file1', folderId: 1},
          {id: 2, title: 'file3.A', folderId: 3},
          {id: 3, title: 'file3.D', folderId: 3},
          {id: 4, title: 'file3.C', folderId: 3},
          {id: 5, title: 'file3.B', folderId: 3},
          {id: 6, title: 'file2.D', folderId: 2},
          {id: 7, title: 'file2.A', folderId: 2},
          {id: 8, title: 'file2.C', folderId: 2},
          {id: 9, title: 'file2.B', folderId: 2},
        ]

Where folders have many files through folderId. If we query:

 folderRepo.find({
          include: [
            {relation: 'files', scope: {limit: 3, order: ['title ASC']}},
          ],
        });

Will return

[
  {
    "id": 1,
    "name": "f1",
    "files": [{ "id": 1, "title": "file1", "folderId": 1 }]
  },
  {
    "id": 2,
    "name": "f2",
    "files": [
      { "id": 7, "title": "file2.A", "folderId": 2 },
      { "id": 9, "title": "file2.B", "folderId": 2 },
      { "id": 8, "title": "file2.C", "folderId": 2 }
    ]
  },
  {
    "id": 3,
    "name": "f3",
    "files": [
      { "id": 2, "title": "file3.A", "folderId": 3 },
      { "id": 5, "title": "file3.B", "folderId": 3 },
      { "id": 4, "title": "file3.C", "folderId": 3 }
    ]
  },
  { "id": 4, "name": "f4" }

[achrinza]

so to keep the backward compatability we will need a totalLimit, totalOrder, totalWhere ?

While the code for order and where has changed, I don't think the actual behaviour has changed:

where is always scoped on a "per-record" basis; So the new code shouldn't affect this behaviour.

order is different because its scoped across multiple records; This is because with can order by multiple columns. However, the position of each child record in a parent is still effectively the same.

Hence, limit should be the only filter affected as its scoped on a "per-query" basis.

[collaorodrigo7]

Oh, I see what you mean!. Thanks.
So how should I make this change, should I change the Filter object and add this extra property? Should I just inline extend the Filter object for the Scope section only?.
This will also mean we have to add a note to the filters documentation about this change.

[Uptade]
I updated the Inclusion filter adding the totalLimit optional parameter in the scope property, as well as in the findByForeignKeys params

[allanpaiste]

This alternative inclusion lookup implementation is currently chosen to be used in overly aggressive manner.

The following scope configuration will also trigger the use of it:

{
  relation: 'someRelation',
  scope: {
    fields: ['a', 'b', 'c']
  }
}

As such, I would strongly recommend this ...

    // since there is a scope, there could be a where filter, a limit, an order
    // and we should run the scope in multiple queries so we can respect the
    // scope filter params

... to be implemented in code.

Although it eludes me why order and filter are included in the explanation as those can remain global. That execution path should really be chosen only when limit is specified (and even then I'd suggest there to be a warning logged in development env about the degraded performance).