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.

Sign up for GitHub

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

[Discover][Bug] Migrate global state from legacy URL #6780

Merged
merged 3 commits into from
Jun 4, 2024
Merged

[Discover][Bug] Migrate global state from legacy URL #6780

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
60944e1
[Discover][Bug] Migrate global state
ananzh May 13, 2024
f4207da
Changeset file for PR #6780 created/updated
opensearch-changeset-bot[bot] May 13, 2024
bfceca2
add unit test
ananzh Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/6780.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- [Discover][Bug] Migrate global state from legacy URL ([#6780](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6780))
2 changes: 2 additions & 0 deletions src/plugins/discover/public/migrate_state.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

line #136 should move before line #116 and then line #116 should also include testing if global state is null or not. We only return path if both app state and global state are null. So we do not return path if app state is null and we miss the states from global state.

const appState = getStateFromOsdUrl<LegacyDiscoverState>('_a', oldPath);
const globalState = getStateFromOsdUrl<any>('_g', oldPath);

if (!appState && !globalState) return path;
...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we would like to proceed with the migration regardless of the presence of the global state (_g).

First of all, checking for both appState and globalState together and returning the path if either is null could miss valid migrations.

Here are some 2.9 urls with/withoug _g:

  • with _g, index pattern has a timestamp

If we remove _g from the url

http://localhost:5601/app/discover#/view/2e72c2f0-21a8-11ef-b7a3-09997417d2e2?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15w,to:now))&_a=(columns:!(_source),filters:!(('$state':(store:appState),meta:(alias:!n,disabled:!f,index:ff959d40-b880-11e8-a6d9-e546fe2bba5f,key:category.keyword,negate:!f,params:(query:'Men!'s%20Clothing'),type:phrase),query:(match_phrase:(category.keyword:'Men!'s%20Clothing')))),index:ff959d40-b880-11e8-a6d9-e546fe2bba5f,interval:auto,query:(language:kuery,query:Thursday),sort:!())

then refresh the url, it will turn to

http://localhost:5601/app/discover#/view/2e72c2f0-21a8-11ef-b7a3-09997417d2e2?_a=(columns:!(_source),filters:!(('$state':(store:appState),meta:(alias:!n,disabled:!f,index:ff959d40-b880-11e8-a6d9-e546fe2bba5f,key:category.keyword,negate:!f,params:(query:'Men!'s%20Clothing'),type:phrase),query:(match_phrase:(category.keyword:'Men!'s%20Clothing')))),index:ff959d40-b880-11e8-a6d9-e546fe2bba5f,interval:auto,query:(language:kuery,query:Thursday),sort:!())&_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))

, where a default _g is added

  • without _g: index pattern without time series data

By default, for idx without time series, there is a default _g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now)) but if remove _g from the url

http://localhost:5601/app/discover#/view/2e72c2f0-21a8-11ef-b7a3-09997417d2e2?_a=(columns:!(_source),filters:!(),index:c915dbd0-21a8-11ef-b7a3-09997417d2e2,interval:auto,query:(language:kuery,query:''),sort:!())&_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))

it will turn to

http://localhost:5601/app/discover#/view/2e72c2f0-21a8-11ef-b7a3-09997417d2e2?_a=(columns:!(_source),filters:!(),index:c915dbd0-21a8-11ef-b7a3-09997417d2e2,interval:auto,query:(language:kuery,query:''),sort:!())

without adding back _g with default state.

Therefore, it is possible for a URL to be without global state, either by mistake or non-time-series idx and still require proper migration.

Second, if the globalState is null (_g=!n), migration can still proceed. Just like the previous example, it will either add a default one, which is the same as 2.9 experience.

Therefore, it is more robust if we don't check global state in the migration function and just let opensearch_dashboards_utils handles the logic.

Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,9 @@
indexPattern: index,
},
};
const _g = getStateFromOsdUrl<any>('_g', oldPath);

Check warning on line 136 in src/plugins/discover/public/migrate_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/discover/public/migrate_state.ts#L136 

Added line #L136 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this path only entered if the url uses url fragments? I ask because it looks like parseUrlHash would just parse the url hash, so if it wasn't using fragments, you'd potentially override a validly set _g query parameter.

Also, does your comment about ftr tests address the lack of test coverage here?

Copy link
Member Author

Choose a reason for hiding this comment

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

My current understanding is that the legacy urls are all like http://localhost:5601/app/discover#/, there should be no issue with parsing and handling _g.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add the unit test.

Copy link
Member

Choose a reason for hiding this comment

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

we should also define the type here like the previous LegacyDiscoverState from line #114. From my memory, i think global state should include time filter and global filter, and then we can do something like the previous code:

const _g = {
    timefilter,
    filters
}

Copy link
Member Author

Choose a reason for hiding this comment

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

It is beneficial to define the type for globalState to ensure type safety and clarity. However, since the globalState can include various parameters (time, filters, freshInterval and etc), we should ensure that the type definition encompasses all possible states. So I put any for now to make sure any states can be migrated.


path = setStateToOsdUrl('_g', _g, { useHash: false }, path);

Check warning on line 138 in src/plugins/discover/public/migrate_state.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/discover/public/migrate_state.ts#L138 

Added line #L138 was not covered by tests
path = setStateToOsdUrl('_a', _a, { useHash: false }, path);
path = setStateToOsdUrl('_q', _q, { useHash: false }, path);

Expand Down
Loading