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

fix(navigation): prevent updating ngrx state when no components received from backend #17673

Merged
merged 5 commits into from
Jul 26, 2023

Conversation

Platonn
Copy link
Contributor

@Platonn Platonn commented Jul 24, 2023

When the bug happens

The edge-case bug happens when OCC returns inconsistent responses among /pages API vs /components API. In particular, when OCC /pages API returns at least one CmsLink component (in the cms navigation) that the OCC /components API has no idea about (so /components API returns empty response when asked for details of this CmsLink component id), then Spartacus NavigationService goes into infinite loop trying to load details of this CmsLink component over and over again.

When this edge-case could happen in reality: when the cached APIs /pages and /components had cache different invalidation times. The following is a detailed example:

  • Customer is caching all OCC APIs behind a proxy cache
  • their CMS business user removed some CmsLink from the CmsNavigation
  • a little later a race condition happened: the cache of /pages API was NOT YET invalidated, but the cache of /components API was ALREADY invalidated. This means, that the stale /pages API was still returning ID of the removed CmsLink component as part of CMS page structure. But the up-to-date /components API asked in a followup call for details of this component, didn't return any data, because this component no longer existed (was removed).
  • Then the SSR was rendering some page and it called the OCC /pages API, and received in (stale cached) response the ID of this removed CmsLink (among also other navigation component IDs as usual)
    Then during the same render SSR called the endpoint /components asking for the details of this CmsLink, but the (up-to-date) API didn't return any details of this component.
  • Then Spartacus went into an infinite loop, trying to load the details of this missing CmsLink component via the /components API over and over again.

Note: such an infinite loop could happen regardless whether Spartacus runs in the browser (CSR) or in NodeJS (SSR).

Why Spartacus needs to load missing components?

One could ask: Why Spartacus bothers to load missing components from the /components API? Answer: there is a valid business usecase for it - components that are hidden for anonymous users (due to a CMS restriction mechanism), but visible for logged in users are considered "missing" when the user logs in, and their details need to be loaded after the user logs in. For more, see the old Spartacus github bug ticket:  NavigationNodes(cx-category-navigation) with CMS restrictions(e.g. loggedInUser) not shown until a Browser refresh #5969., which introduced the logic of loading missing cms components' details.

Why this PR prevents infinite loop?

Short answer: now we don't update ngrx state, when receiving empty response from /components OCC API. This breaks the circuit: load missing cms components -> received empty response -> update ngrx state -> load missing cms components -> ...

Explanation: From Spartacus point of view, when /pages API returns IDs of components that are in the cms structure, then Spartacus tries to load details of all those components via /components API. Currently, when /components API returns an empty response for the given ID, the ngrx state object reference is updated despite that this empty response didn't actually changed the ngrx state - it's only the state's object reference that changed for no good reason. Such a change of object's reference triggers an emission of RxJs observable with this state. This leads to emiting a new value from the $data observable in the stream returned by NavigationService.getNavigationNode(). Inside the logic of this stream, Spartacus recognizes that some components's details are still missing, and therefore tries to load them by calling CmsService.loadNavigationItems(navigation.id, missingItems) - see the source code of the branches release/4.3.1 and develop-6.3.x.

In order to break the infinite loop, it suffices that we fix the Spartacus ngrx navigation-item-entry.reducer.ts (see its source code, which is the same in both branches: release/4.3.x and develop-6.3.x). The fixed version should check for the length of the components payload - if it's empty, then no state change should happen.

fixes https://jira.tools.sap/browse/CXSPA-4157

@Platonn Platonn requested a review from a team as a code owner July 24, 2023 15:56
@github-actions github-actions bot marked this pull request as draft July 24, 2023 15:56
@Platonn Platonn marked this pull request as ready for review July 24, 2023 16:09
@Platonn
Copy link
Contributor Author

Platonn commented Jul 24, 2023

QA steps

We need to simulate that OCC /pages API is returning ID of some CMS component, but the /components API should return empty response when being asked for this component ID. In the case of customer it can happen due to different timings of invalidation of the OCC cache in between the endpoints /components (earlier cache invalidation) vs. /pages (late cache invalidation).
In our case for local development it suffices to implementing a custom Angular HTTP_INTERCEPTOR which never returns an information about some specific CMS component. The following is an example code of such interceptor implemented e.g. in the file app.module.ts: https://gist.github.com/Platonn/e70928786c8267dd40173b0d65608e08
Then open the storefront and observe in the ChromeDevTools Network tab the infinite loop of the http calls to /components endpoint.

Comment on lines -27 to -29
{
...{},
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code cleanup is done by the way. it has nothing to do with the main purpose (bugfix) of this PR

@cypress
Copy link

cypress bot commented Jul 24, 2023

4 flaky tests on run #41094 ↗︎

0 119 2 0 Flakiness 4

Details:

Merge 4d912cd into 0750b89...
Project: spartacus Commit: 6eb7b69103 ℹ️
Status: Passed Duration: 09:44 💡
Started: Jul 26, 2023 8:31 AM Ended: Jul 26, 2023 8:41 AM
Flakiness  b2b/regression/checkout/b2b-credit-card-checkout-flow.core-e2e.cy.ts • 1 flaky test • B2B

View Output Video

Test Artifacts
B2B - Credit Card Checkout flow > should checkout using a credit card Output Screenshots
Flakiness  ssr/pages.core-e2e.cy.ts • 3 flaky tests • SSR

View Output Video

Test Artifacts
SSR > should render homepage Output Screenshots
SSR > should render PLP Output Screenshots
SSR > should render PDP Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@github-actions github-actions bot marked this pull request as draft July 24, 2023 17:46
@Platonn Platonn marked this pull request as ready for review July 25, 2023 10:14
@github-actions github-actions bot marked this pull request as draft July 25, 2023 12:22
@Platonn Platonn marked this pull request as ready for review July 25, 2023 13:03
@github-actions github-actions bot marked this pull request as draft July 25, 2023 13:28
@Platonn Platonn marked this pull request as ready for review July 25, 2023 13:40
@github-actions github-actions bot marked this pull request as draft July 25, 2023 14:02
@Platonn Platonn marked this pull request as ready for review July 25, 2023 14:15
@github-actions github-actions bot marked this pull request as draft July 25, 2023 14:34
@Zeyber Zeyber marked this pull request as ready for review July 26, 2023 08:16
@Zeyber Zeyber merged commit bb274cd into develop-6.3.x Jul 26, 2023
22 checks passed
@Zeyber Zeyber deleted the feature/CXSPA-4157 branch July 26, 2023 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants