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
[Performance] ProductRepository cache issue #38572
Comments
Hi @Nuranto. Thank you for your report.
Join Magento Community Engineering Slack and ask your questions in #github channel. |
Hi @engcom-Hotel. Thank you for working on this issue.
|
Hello @Nuranto, Thanks for the report and collaboration! We have tried to reproduce the issue in the latest development branch i.e. 2.4-develop and we are able to reproduce the issue. There are 2 cache keys have been generated for 2 calls below:
Second Call
We have made a custom module to reproduce the issue. Please find attached the same for the reference: Hence confirming the issue. Thanks |
✅ Jira issue https://jira.corp.adobe.com/browse/AC-11730 is successfully created for this GitHub issue. |
✅ Confirmed by @engcom-Hotel. Thank you for verifying the issue. |
@magento I am working on this |
Hi @digitalrisedorset! 👋 |
@magento add to contributors team |
Hi @digitalrisedorset! 👋 |
@magento I am working on this |
@Nuranto thanks for raising this issue. I have been looking into a fix today. I have a way to ensure the cache is not duplicated and that is possibly a way forward. observations made during the test phase with this bug investigation
These 2 methods have a fundamental difference I believe:
I have on my local (without adding any extension attribute) a situation where both methods loads different attributes. The screenshot below shows the tier price data for instance are returned when the product load is called whilst the search does not return this data: conclusion from the observationBecause the product data for both methods getList and getById are different, it seems that we do not want the search and the product load to share the same cache hash as it would hide some data that we might need on the product page particularly I am inclined in the end not to add a fix as we want the cache to be different? I'd agree with you that we have some code that is misleading somewhat at the moment. But the fix we are looking to do would genuinely give side-effects that would be tricky to troubleshoot |
I agree with you with the observation. But in that case, the cache should be totally removed from So the fix should probably be to remove the |
@Nuranto I have made a commit today to remove the cache on the getList method as per your recommendation. With more thoughts on it, I see now that your idea is perfectly relevant to our situation. thanks for your input and apologies for not getting it right away in my head |
I added some integration tests for this PR |
Please also see #38384 |
@MaximGns I have added new changes as well as many more tests. Thank you for consolidating this issue |
@magento run all tests |
Hello @digitalrisedorset, I need to tell you that you need to run the above command to its related PR i.e. #38632
Let me run it for you. Thanks |
Preconditions and environment
Steps to reproduce
$this->productRepository->getById(1);
Expected result
getById
method should use cache generated bygetList
Actual result
Product with ID
1
is loaded twice. This is because the cache key is generated withstoreId = null
forgetById
, but withstoreId = 1
(default store) forgetList
.(Same thing with
get
instead ofgetById
)Additional information
get :
magento2/app/code/Magento/Catalog/Model/ProductRepository.php
Lines 278 to 280 in 488c103
getById :
magento2/app/code/Magento/Catalog/Model/ProductRepository.php
Lines 308 to 310 in 488c103
getList :
magento2/app/code/Magento/Catalog/Model/ProductRepository.php
Lines 717 to 727 in 488c103
Release note
No response
Triage and priority
The text was updated successfully, but these errors were encountered: