[Dashboard] Fix missing state on short URL alias match redirect by ThomThomson · Pull Request #163658 · elastic/kibana

ThomThomson · 2023-08-10T19:37:28Z

Summary

Fixes an issue where URL state from short URLs could be lost on an alias match redirect.

Before

After

Note
In the after gif - and in any testing, you'll notice that the fixed version shows an error in the Lens Embeddable. This is because the short URL contains a panels key with an un-migrated version of the by value Lens panel on the Dashboard. This would not have happened if I had used a by reference panel, and newer versions of Kibana have warnings in place to prevent the saving of un-migrated state to a short URL in this way.

To test:

Download and extract this ES data which has two Dashboards in two spaces (with a URL conflict) and has a short URL pointing to the conflicting Dashboard URL with a filter attached.
esData_short_url_alias_redirect.zip
Start ES pointing to where you extraced that data yarn es snapshot -E path.data=path/to/data
Try navigating to the short URL {Your_local_url}/s/new-space/goto/efbe7e20-37ae-11ee-8876-5107f2fdfac9
The Dashboard should load properly, with the filter set.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

ThomThomson · 2023-08-10T21:36:45Z

@elasticmachine merge upstream

nreese · 2023-08-11T13:37:31Z

src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts

  expect(dashboard).toBeUndefined();
 });

+test('does not get initial input when provided validation function returns redireted', async () => {


nit - typo redireted => redirected

How did loading input cause the filters to disappear?

Basically when state is loaded into the Dashboard via the _a param, the _a param gets deleted so it doesn't keep overriding the Dashboard's current state.

This process was happening before the redirect instead of after.

so getInitialInput modifies the URL, then redirect was getting called. Is that correct?

Yes exactly.

ok, thanks for the explaination.

There is a bit of a problem with encapsulation, where a create_dashboard test is protecting against a side effect from a callbacks.

I think a clarity in callback name, or test naming, or comments should be added to explain this side effect

How about one or some of the following

rename getInitialInput callback to getInitialInputAndRemoveStateFromUrl to clarify what callback does and highlight side effect of callback.

add comment where getInitialInput callback is called that method modifies URL

renaming the test case to include the "why". Something like "does not modify URL when validation function returns redirected"

The encapsulation concern is completely valid. The reason I left it generic in this way is because the Dashboard container code is usable in many places - most of which have no special URL treatment at all. The method getInitialInput can theoretically have any number of side effects.

IMO, in all of these cases it is actually more correct for getInitialInput to be called only once we're certain that the loaded saved object (if there is one) passes validation. This is why I named the test this way and didn't rename anything to mention the URLs.

Thanks for explaining, I had not considered portable dashboards where there is no URL.

How about just a comment in the testing stating that for dashboard application, side effect of getInitialInput removes _a from URL?

Great idea. Added a comment in aee1b09

nreese

changes LGTM. Thanks for adding the comment for the test case

kibana-ci · 2023-08-11T16:13:39Z

💚 Build Succeeded

Buildkite Build
Commit: aee1b09

Metrics [docs]

✅ unchanged

History

💚 Build #149419 succeeded 4318716
💔 Build #149400 failed bf0ecc5

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

elasticmachine · 2023-08-11T16:14:09Z

Pinging @elastic/kibana-presentation (Team:Presentation)

…tic#163658) Fixes an issue where URL state from short URLs could be lost on an alias match redirect. (cherry picked from commit 4de6111)

…#163658) (#163746) # Backport This will backport the following commits from `main` to `8.9`: - [[Dashboard] Fix missing state on short URL alias match redirect (#163658)](#163658)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

mistic · 2023-08-17T18:45:41Z

This pr didn't make it into the build candidate for v8.9.1. Updating the labels.

## Summary This PR fixes the alias redirect problem that was noted in the attached SDH. For context, as part of the [Links panel work](#166896), I added [dashboard caching](#162285) to make navigation more efficient - however, when implementing this, I did not consider the redirect behaviour. Specifically, we were **always** adding dashboards to the cache, even if the load outcome meant a redirect was necessary - so, because the meta info for the dashboard fetch result was cached, this meant that the `'aliasMatch'` outcome was **also** cached. Because of this, after a redirect happened, the second attempt to load the dashboard would return the result from the **cache** rather than fetching the dashboard from the CM client - but, as described previously, the cached dashboard would still appear as though it required a redirect because the cached outcome was `'aliasMatch'`. Therefore, because of hitting this early return on **both** fetch attempts (before the redirect, after the redirect)... https://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171 ... the dashboard information would not get updated. _(Oof!)_ Despite the lengthy description of the problem, the solution is quite simple - just don't add dashboards to the cache if they require a redirect. And then everything works! 🎉 ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f **After** https://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1 ### Testing To test, I followed the directions from [this PR description](#163658) to get a dashboard that requires a redirect - then, I created a markdown panel with the **old** (from the Default space) dashboard ID in the new space to see the same redirect behaviour that the customer in the SDH was seeing. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

## Summary This PR fixes the alias redirect problem that was noted in the attached SDH. For context, as part of the [Links panel work](elastic#166896), I added [dashboard caching](elastic#162285) to make navigation more efficient - however, when implementing this, I did not consider the redirect behaviour. Specifically, we were **always** adding dashboards to the cache, even if the load outcome meant a redirect was necessary - so, because the meta info for the dashboard fetch result was cached, this meant that the `'aliasMatch'` outcome was **also** cached. Because of this, after a redirect happened, the second attempt to load the dashboard would return the result from the **cache** rather than fetching the dashboard from the CM client - but, as described previously, the cached dashboard would still appear as though it required a redirect because the cached outcome was `'aliasMatch'`. Therefore, because of hitting this early return on **both** fetch attempts (before the redirect, after the redirect)... https://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171 ... the dashboard information would not get updated. _(Oof!)_ Despite the lengthy description of the problem, the solution is quite simple - just don't add dashboards to the cache if they require a redirect. And then everything works! 🎉 ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f **After** https://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1 ### Testing To test, I followed the directions from [this PR description](elastic#163658) to get a dashboard that requires a redirect - then, I created a markdown panel with the **old** (from the Default space) dashboard ID in the new space to see the same redirect behaviour that the customer in the SDH was seeing. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 27b34d5)

# Backport This will backport the following commits from `main` to `8.12`: - [[Dashboard] Only cache non-alias dashboards (#175635)](#175635)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>

## Summary This PR fixes the alias redirect problem that was noted in the attached SDH. For context, as part of the [Links panel work](elastic#166896), I added [dashboard caching](elastic#162285) to make navigation more efficient - however, when implementing this, I did not consider the redirect behaviour. Specifically, we were **always** adding dashboards to the cache, even if the load outcome meant a redirect was necessary - so, because the meta info for the dashboard fetch result was cached, this meant that the `'aliasMatch'` outcome was **also** cached. Because of this, after a redirect happened, the second attempt to load the dashboard would return the result from the **cache** rather than fetching the dashboard from the CM client - but, as described previously, the cached dashboard would still appear as though it required a redirect because the cached outcome was `'aliasMatch'`. Therefore, because of hitting this early return on **both** fetch attempts (before the redirect, after the redirect)... https://github.com/elastic/kibana/blob/4565b1bfba91cd24f73f8ce37fa7be73283a4453/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts#L165-L171 ... the dashboard information would not get updated. _(Oof!)_ Despite the lengthy description of the problem, the solution is quite simple - just don't add dashboards to the cache if they require a redirect. And then everything works! 🎉 ### Videos **Before** https://github.com/elastic/kibana/assets/8698078/983b81a6-acf3-4e71-804a-6ed75c36896f **After** https://github.com/elastic/kibana/assets/8698078/028369ff-f73f-43e6-9abb-b35f0d2f33e1 ### Testing To test, I followed the directions from [this PR description](elastic#163658) to get a dashboard that requires a redirect - then, I created a markdown panel with the **old** (from the Default space) dashboard ID in the new space to see the same redirect behaviour that the customer in the SDH was seeing. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

load state from URL after dashboard SO load and validation

bf0ecc5

Merge branch 'main' into aliasRedirect/fixMissingFilters

4318716

nreese reviewed Aug 11, 2023

View reviewed changes

ThomThomson added 2 commits August 11, 2023 11:04

Fix spelling of redirected

ef1271b

Add comment explaining bug to test

aee1b09

nreese approved these changes Aug 11, 2023

View reviewed changes

ThomThomson marked this pull request as ready for review August 11, 2023 16:14

ThomThomson requested a review from a team as a code owner August 11, 2023 16:14

ThomThomson added the release_note:fix label Aug 11, 2023

ThomThomson merged commit 4de6111 into elastic:main Aug 11, 2023

kibanamachine added v8.10.0 backport:skip This PR does not require backporting labels Aug 11, 2023

ThomThomson added backport:prev-minor v8.9.2 and removed backport:skip This PR does not require backporting labels Aug 11, 2023

ThomThomson mentioned this pull request Aug 11, 2023

[8.9] [Dashboard] Fix missing state on short URL alias match redirect (#163658) #163746

Merged

kibanamachine added the v8.9.1 label Aug 11, 2023

sorenlouv added auto-backport Deprecated - use backport:version if exact versions are needed v8.8.0 and removed auto-backport Deprecated - use backport:version if exact versions are needed v8.8.0 labels Aug 11, 2023

mistic removed the v8.9.1 label Aug 17, 2023

Heenawter mentioned this pull request Jan 25, 2024

[Dashboard] Only cache non-alias dashboards #175635

Merged

3 tasks

ThomThomson mentioned this pull request Jan 30, 2024

[Tech Debt] Alias match functional tests #175926

Closed

Conversation

ThomThomson commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

ThomThomson commented Aug 10, 2023

Uh oh!

nreese Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

ThomThomson Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nreese Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

ThomThomson Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

nreese Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

ThomThomson Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

nreese Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

ThomThomson Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Aug 11, 2023

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

elasticmachine commented Aug 11, 2023

Uh oh!

mistic commented Aug 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ThomThomson commented Aug 10, 2023 •

edited

Loading

ThomThomson Aug 11, 2023 •

edited

Loading