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

Support inq splitting in findByForeignKeys #3444

Open
7 tasks
bajtos opened this issue Jul 26, 2019 · 8 comments
Open
7 tasks

Support inq splitting in findByForeignKeys #3444

bajtos opened this issue Jul 26, 2019 · 8 comments
Labels
feature needs discussion Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package

Comments

@bajtos
Copy link
Member

bajtos commented Jul 26, 2019

Under the hood, inclusion resolvers are implemented using inq operator:

  1. Gather PK/FK values from source models.
  2. Query target models using inq and PK/FK values from step 1.
  3. Assign target models to navigational property in source models.

This can be problematic when the number of source instances is large, we don't
know if all databases support inq with arbitrary number of items.

To address this issue, LB3 is implementing "inq splitting", where a single query
with arbitrary-sized inq condition is split into multiple queries where each
query has a reasonably-sized inq condition.

Connectors are allowed to specify the maximum inq size supported by the
database via dataSource.settings.inqLimit option. By default, inqLimit is
set to 256.

In this task, we need to improve findByForeignKeys (see #3443) to handle the maximum size of inq parameter supported by the target database (data-source). When the list of provided FK values is too long, then we should split it into smaller chunks and execute multiple queries.

However, because our Repository interface is generic and does not assume that
a repository has to be backed by a data-source, I am proposing to expose
inqLimit via a new property of the Repository interface instead of accessing
the parameter via DataSource settings.

/**
 * Description of capabilities offered by the connector backing the given
 * datasource.
 */
export interface RepositoryCapabilities {
  /**
   * Maximum number of items allowed for `inq` operators.
   * This value is used to split queries when resolving related models
   * for a large number of source instances.
   */
  inqLimit?: number;
}

To preserve backwards compatibility with existing repository implementation, we
cannot add RepositoryCapabilities directly to the Repository class. We need
to introduce a new interface instead that Repositories can (or may not)
implement.

export interface RepositoryWithCapabilities {
  capabilities: RepositoryCapabilities;
}

See #3387 for more details & a prototype implementation.

Acceptance criteria

To allow the helper to detect inqLimit, we need to extend Repository interfaces.

  • Introduce RepositoryCapabilities interface (called ConnectorCapabilities in the spike), this interface will have a single property inqLimit (for now).
  • Introduce RepositoryWithCapabilities interface (called WithCapabilities in the spike),= this interface should define capabilities property.
  • Implement isRepositoryWithCapabilities type guard
  • Implement getRepositoryCapabilities helper

The rest should be straightforward:

  • Modify findByForeignKeys to obtain inqLimit from repository capabilities and implement query splitting (see the spike implementation).
  • Write unit-level tests where we verify what (and how many) queries are called by findByForeignKeys.
  • Write integration-level tests (in repository-tests) to verify that connectors can handle inqLimit they are advertising. For example, create a test that runs a query returning 1000 records.
@agnes512
Copy link
Contributor

Discussion:
From @raymondfeng:
do we want to split the queries for users, or just have a limit amount of queries(i.e throws err msg if they exceed the limit)?

Any thoughts? @strongloop/loopback-maintainers

@agnes512
Copy link
Contributor

I was considering this use case:
Lots of shopping websites use Load More button(or scroll down) to load more items.
This kind of button/function only sends a certain amount of queries as the more queries it sends, the longer it takes.

From my understanding and my experience, it's developer's responsibility to decide how many queries to send.
So it makes more sense to me that we just set a limit for the amount of inq for findByForeignKeys instead of splitting it for them.

@dhmlau
Copy link
Member

dhmlau commented Aug 9, 2019

@agnes512, are we ready to estimate this task?

@dhmlau dhmlau added 2019Q4 and removed 2019Q3 labels Aug 15, 2019
@bajtos
Copy link
Member Author

bajtos commented Aug 16, 2019

In LoopBack 3, we are splitting the queries for the user automatically - see https://github.com/strongloop/loopback-datasource-juggler/blob/814c55c7cd7ac49f3a52729949a5d0aaeb91853d/lib/include.js#L237-L264.

I am fine to put this feature on hold for now and wait if there are any users asking for it.

If we decide to do so, then I'd like to ensure that the enforced inq limit is large enough to support most users and that a helpful error message is reported when the limit is reached. Since #3443 has been already finished, I am proposing to open a new story where we will investigate what errors are reported now, add acceptance-level tests to trigger the scenario & verify the reported error, and ensure consistent behavior for all supported databases.

@dhmlau dhmlau removed the 2019Q4 label Sep 20, 2019
@dhmlau
Copy link
Member

dhmlau commented Sep 20, 2019

According to #1352, this is out of scope for Q4.

@stale stale bot added the stale label Sep 20, 2020
@achrinza achrinza removed the stale label Sep 20, 2020
@stale stale bot added the stale label Jul 14, 2021
@stale stale bot closed this as completed Aug 13, 2021
@cuttingd
Copy link

In LoopBack 3, we are splitting the queries for the user automatically - see https://github.com/strongloop/loopback-datasource-juggler/blob/814c55c7cd7ac49f3a52729949a5d0aaeb91853d/lib/include.js#L237-L264.

I am fine to put this feature on hold for now and wait if there are any users asking for it.

If we decide to do so, then I'd like to ensure that the enforced inq limit is large enough to support most users and that a helpful error message is reported when the limit is reached. Since #3443 has been already finished, I am proposing to open a new story where we will investigate what errors are reported now, add acceptance-level tests to trigger the scenario & verify the reported error, and ensure consistent behavior for all supported databases.

I'm asking for this, if possible. I have some queries that I am not interested in paging and just want the api to return all of the rows even if it's more than 1000 in oracle.

@achrinza
Copy link
Member

Re-opening the issue for further discussion.

@loopbackio loopbackio deleted a comment from stale bot Jul 28, 2022
@loopbackio loopbackio deleted a comment from stale bot Jul 28, 2022
@loopbackio loopbackio deleted a comment from stale bot Jul 28, 2022
@cuttingd
Copy link

For further info on why we need this, please see the bug report I submitted #8773

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature needs discussion Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package
Projects
Status: Icebox
Development

No branches or pull requests

5 participants