-
Notifications
You must be signed in to change notification settings - Fork 0
feature / added-get-by-ids-and-transofrmed-to-static-fn-call #33
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
feature / added-get-by-ids-and-transofrmed-to-static-fn-call #33
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 significantly enhances 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant improvement by refactoring the GraphQL query generation in MagentoProductQuery from string constants to static builder methods. This change enhances maintainability, reduces code duplication, and makes the query generation more robust. The addition of the getByIdsQuery functionality is also a valuable new feature.
While the refactoring is well-executed, I've identified a high-severity issue in the new getByIdsQuery implementation where missing pagination parameters could lead to incomplete data being returned. I've included suggestions to fix this.
Additionally, it appears this pull request may be incomplete. Several files in the repository (MagentoQueryBuilder, MagentoQueryBuilderTest, and magento-sync.php) still reference the old DEFAULT_QUERY constant that has been removed. This will cause fatal errors. Please ensure all references to the removed constants are updated to use the new static methods.
| query GetProductsByIds(\$ids: [String!]) { | ||
| products(filter: { entity_id: { in: \$ids } }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The products query in Magento is paginated by default, typically returning 20 items per page. When fetching by a list of IDs, if more than 20 IDs are provided, only the first 20 products will be returned, leading to silent data loss. To ensure all requested products are fetched, you should add a $pageSize variable to the query. The caller will then be responsible for providing this value, which should ideally be the count of the IDs being requested.
query GetProductsByIds(\$ids: [String!], \$pageSize: Int) {
products(filter: { entity_id: { in: \$ids } }, pageSize: \$pageSize) {| * Variables: | ||
| * - $ids: [String!] (e.g., ["325465", "1924192", "1924190"]) | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for variables should be updated to include $pageSize. The caller needs to be aware that they must provide this parameter to prevent unexpected pagination from Magento, which could lead to incomplete data being returned when fetching a large number of products by ID.
* Variables:
* - $ids: [String!] (e.g., ["325465", "1924192", "1924190"])
* - $pageSize: Int (e.g., the number of IDs, to fetch all products at once)
*/
get by ids functionality added