buuhuu commented 2 years ago

Description

The change removes model caching and replaces it with a request scoped cache maintained by the MagentoGraphqlClient model. It allows components of the same type to be used more than once on a page (in particular product list) while preserving the optimisation done to not query the commerce backend multiple times for the same component.

Existing tests cover this behaviour already.

Related Issue

CIF-2761

Motivation and Context

Support multiple Product List components on a single page.

How Has This Been Tested?

Unit tests, locally manually

Screenshots (if appropriate):

Types of changes

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[X] I have signed the Adobe Open Source CLA.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[X] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes and the overall coverage did not decrease.
[X] All unit tests pass on CircleCi.
[X] I ran all tests locally and they pass.

codecov[bot] commented 2 years ago

Codecov Report

Merging #862 (4510f93) into master (8e4c33e) will decrease coverage by 0.09%. The diff coverage is 58.06%.

@@             Coverage Diff              @@
##             master     #862      +/-   ##
============================================
- Coverage     69.53%   69.43%   -0.10%     
- Complexity     1160     1164       +4     
============================================
  Files           320      320              
  Lines          8475     8497      +22     
  Branches       1269     1273       +4     
============================================
+ Hits           5893     5900       +7     
- Misses         2168     2183      +15     
  Partials        414      414

Flag	Coverage Δ
integration	`54.98% <58.06%> (-0.11%)`	:arrow_down:
jest	`86.40% <ø> (ø)`
karma	`95.44% <ø> (ø)`
unittests	`90.86% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...onents/internal/models/v1/product/ProductImpl.java	`73.14% <ø> (ø)`
...nternal/models/v1/productlist/ProductListImpl.java	`82.02% <ø> (+4.49%)`	:arrow_up:
...onents/internal/models/v2/product/ProductImpl.java	`75.00% <ø> (ø)`
...nternal/models/v2/productlist/ProductListImpl.java	`100.00% <ø> (+14.28%)`	:arrow_up:
...ents/internal/client/MagentoGraphqlClientImpl.java	`39.61% <47.82%> (-4.28%)`	:arrow_down:
...rch/internal/services/SearchFilterServiceImpl.java	`61.40% <85.71%> (-6.60%)`	:arrow_down:
...ch/internal/services/SearchResultsServiceImpl.java	`70.64% <100.00%> (ø)`
...omponents/internal/models/v1/common/PriceImpl.java	`65.38% <0.00%> (-1.93%)`	:arrow_down:
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8e4c33e...4510f93. Read the comment docs.

LSantha commented 2 years ago

I'm not sure if we need this feature for every possible call to the MagentoGraphqlClient. It would be cleaner to expose executeCached() and use it only in the models where caching was enabled so far. For other models in the majority of cases the data is just kept in request scope for no other reason than to stress the garbage collector.

adobe / aem-core-cif-components

CIF-2761: replace model caching #862