feature / pagination-info-get-by-ids-must-be-respected#35
Conversation
Summary of ChangesHello @tomas862, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces robust pagination capabilities to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds pagination support to the getByIdsQuery for Magento products. The changes correctly update the GraphQL query and the corresponding documentation. However, the current implementation for getByIdsQuery uses a specific $ids variable, which is inconsistent with other queries like getDefaultQuery that use a generic $filter variable. This inconsistency prevents the use of getByIdsQuery with the existing MagentoQueryBuilder and MagentoPaginatedFetcher, which could be a significant limitation. My review includes a suggestion to refactor buildByIdsQuery to use the $filter variable, which would improve consistency and integration with the rest of the SDK. I've also included related suggestions to update the documentation to match this proposed change.
| query GetProductsByIds(\$ids: [String!], \$pageSize: Int, \$currentPage: Int) { | ||
| products(filter: { entity_id: { in: \$ids } }, pageSize: \$pageSize, currentPage: \$currentPage) { |
There was a problem hiding this comment.
For better consistency with other queries like getDefaultQuery and to ensure compatibility with MagentoQueryBuilder and MagentoPaginatedFetcher, I suggest changing this query to accept a generic $filter variable instead of a specific $ids variable. This would make it possible to use the paginated fetcher for fetching products by IDs. The query name GetProductsByIds can be kept to distinguish it from GetProducts.
With this change, the caller would be responsible for building the filter, e.g., ['filter' => ['entity_id' => ['in' => $ids]]]. This aligns with how other filters are used in the SDK.
query GetProductsByIds(\$filter: ProductAttributeFilterInput, \$pageSize: Int, \$currentPage: Int) {
products(filter: \$filter, pageSize: \$pageSize, currentPage: \$currentPage) {| $variables = [ | ||
| 'ids' => ['325465', '1924192', '1924190'], | ||
| 'pageSize' => 100, | ||
| 'currentPage' => 1 | ||
| ]; |
There was a problem hiding this comment.
If you apply the suggestion to use a $filter variable in buildByIdsQuery, this example should be updated to show how to use the $filter variable to specify product IDs.
| $variables = [ | |
| 'ids' => ['325465', '1924192', '1924190'], | |
| 'pageSize' => 100, | |
| 'currentPage' => 1 | |
| ]; | |
| $variables = [ | |
| 'filter' => ['entity_id' => ['in' => ['325465', '1924192', '1924190']]], | |
| 'pageSize' => 100, | |
| 'currentPage' => 1 | |
| ]; |
| |--------|-----------|-------------|----------| | ||
| | `getDefaultQuery()` | `$filter`, `$pageSize`, `$currentPage` | Full (all fields) | Standard product sync | | ||
| | `getByIdsQuery()` | `$ids: [String!]` | Full (all fields) | Fetch specific products by ID | | ||
| | `getByIdsQuery()` | `$ids: [String!]`, `$pageSize`, `$currentPage` | Full (all fields) | Fetch specific products by ID | |
There was a problem hiding this comment.
If you apply the suggestion to use a $filter variable in buildByIdsQuery, this part of the documentation should be updated to reflect that the query uses $filter instead of $ids.
| | `getByIdsQuery()` | `$ids: [String!]`, `$pageSize`, `$currentPage` | Full (all fields) | Fetch specific products by ID | | |
| | `getByIdsQuery()` | `$filter`, `$pageSize`, `$currentPage` | Full (all fields) | Fetch specific products by ID | |
| * $variables = [ | ||
| * 'ids' => ['325465', '1924192', '1924190'], | ||
| * 'pageSize' => 100, | ||
| * 'currentPage' => 1 | ||
| * ]; |
There was a problem hiding this comment.
If you apply the suggestion to use a $filter variable in buildByIdsQuery, this example should be updated to show how to use the $filter variable to specify product IDs.
* $variables = [
* 'filter' => ['entity_id' => ['in' => ['325465', '1924192', '1924190']]],
* 'pageSize' => 100,
* 'currentPage' => 1
* ];| * - $ids: [String!] (e.g., ["325465", "1924192", "1924190"]) | ||
| * - $pageSize: Int (e.g., 100) | ||
| * - $currentPage: Int (e.g., 1) |
There was a problem hiding this comment.
If you apply the suggestion to use a $filter variable in buildByIdsQuery, this documentation should be updated to reflect that. The primary variable for filtering would be $filter.
* - $filter: ProductAttributeFilterInput (e.g., {"entity_id": {"in": ["325465", "1924192"]}})
* - $pageSize: Int (e.g., 100)
* - $currentPage: Int (e.g., 1)
tomas862
deleted the
feature/pagination-info-get-by-ids-must-be-respected
branch
1 participant
we cant trust our chunking by ids logic. Therefore we must implement pagination