[Dashboard] Fix Alias Redirect Race Condition #161043
Conversation
|
@elasticmachine merge upstream |
ThomThomson
added
Feature:Dashboard
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
| spaces.redirectLegacyUrl?.({ path, aliasPurpose }); | ||
| return false; // redirected. Stop loading dashboard. | ||
| } | ||
| // navigate to alias on the next tick to allow Dashboard to finish rendering its error state. |
There was a problem hiding this comment.
Instead of event stack hacking, how about changing the return type of validateOutcome to be a enum instead of a boolean, so you can better define outcomes. For example, really there are three outcomes - found, notFound, and redirect. That way, initializeDashboard can just return early in redirect case instead of going into an error state - which it should not enter, because redirect is not really an error.
There was a problem hiding this comment.
Good idea - let me give that a try.
There was a problem hiding this comment.
Great suggestion @nreese! I was able to get that working well, just had to propagate the fact that the embeddable create could return undefined up the chain. This also let me do at least a jest test verifies that in the event of a redirect result, dashboard create should return undefined.
ThomThomson
changed the title
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
ThomThomson
added
the
backport:prev-minor
Fixes a race condition that could happen in the case of an alias match redirect. (cherry picked from commit 1d78311)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
Fixes a race condition that could happen in the case of an alias match redirect. (cherry picked from commit 1d78311)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
# Backport This will backport the following commits from `main` to `8.9`: - [[Dashboard] Fix Alias Redirect Race Condition (#161043)](#161043) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Devon Thomson","email":"devon.thomson@elastic.co"},"sourceCommit":{"committedDate":"2023-07-06T20:19:54Z","message":"[Dashboard] Fix Alias Redirect Race Condition (#161043)\n\nFixes a race condition that could happen in the case of an alias match redirect.","sha":"1d783110b343050be7f6c42d774fa1d6a2ead303","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Dashboard","release_note:fix","Team:Presentation","loe:days","impact:high","v8.9.0","v8.10.0"],"number":161043,"url":"#161043 Fix Alias Redirect Race Condition (#161043)\n\nFixes a race condition that could happen in the case of an alias match redirect.","sha":"1d783110b343050be7f6c42d774fa1d6a2ead303"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"8.9","label":"v8.9.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"#161043 Fix Alias Redirect Race Condition (#161043)\n\nFixes a race condition that could happen in the case of an alias match redirect.","sha":"1d783110b343050be7f6c42d774fa1d6a2ead303"}}]}] BACKPORT--> Co-authored-by: Devon Thomson <devon.thomson@elastic.co>
Summary
This PR fixes a race condition that could happen in the case of an alias match redirect. Before this PR it was possible for a URL update (redirecting to the alias) to happen in the middle of building the errored state of the Dashboard. This would sometimes result in the error from the last dashboard (the old saved object id) being surfaced instead of the redirect properly firing.
To fix this, this PR allows the Dashboard factory to return undefined in the case of a redirect and updates the renderer to support that.
Before
Before.mp4
After
A note on tests
It seems like this alias match functionality breaks often enough that we should cover it with functional tests. Testing this will be difficult because we'll need to spoof a situation where a redirect would be required - which requires multiple spaces, and saved objects from 7.17. This test coverage should be added as a followup because it is important that this PR merged into 8.9.
How to test manually?
esData backup (7.17 ID conflicts).zip
yarn es snapshot -E path.data="/path/to/data"Super Cool New Space{Your address}/s/super-cool-new-space/app/dashboards#/view/326754f0-0ac2-11ee-9f77-63b7bd4c6a36ce23f0a9-47e2-5cf5-82b2-9446e880a0ae