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

[ananzh]

Description

Allow migrate global state. I also thought about capture all other states besides _g, _a, and _q` like customized states, but our current helper func requiring us to know the key in advance. Given customized keys might have various formats, it is easy to make new bugs.

export function getStatesFromOsdUrl(
  url: string = window.location.href,
  keys?: string[]
): Record<string, unknown> {
  const query = parseUrlHash(url)?.query;

  if (!query) return {};
  const decoded: Record<string, unknown> = {};
  Object.entries(query)
    .filter(([key]) => (keys ? keys.includes(key) : true))
    .forEach(([q, value]) => {
      decoded[q] = decodeState(value as string);
    });

  return decoded;
}

Issues Resolved

#6766

Screenshot

bug-fix.mov

Changelog

• fix: [Discover][Bug] Migrate global state from legacy URL

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
• New functionality includes testing.
• New functionality has been documented.
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[ananzh]

Will add a ftr test in the ftr repo.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.98%. Comparing base (8c99ce4) to head (bfceca2).
Report is 31 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6780      +/-   ##
==========================================
- Coverage   67.56%   66.98%   -0.59%     
==========================================
  Files        3428     2687     -741     
  Lines       67337    51653   -15684     
  Branches    10994     9324    -1670     
==========================================
- Hits        45499    34601   -10898     
+ Misses      19167    14778    -4389     
+ Partials     2671     2274     -397     

Flag	Coverage Δ
Linux_1	?
Linux_2	55.12% <ø> (-0.01%)	⬇️
Linux_3	45.29% <100.00%> (+0.14%)	⬆️
Linux_4	34.82% <ø> (+0.10%)	⬆️
Windows_1	?
Windows_2	?
Windows_3	?
Windows_4	?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[virajsanghvi]

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?

[ananzh]

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.

[ananzh]

Will add the unit test.

[abbyhu2000]

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;
...

[ananzh]

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.

[abbyhu2000]

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
}

[ananzh]

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.

[abbyhu2000]

Will add a ftr test in the ftr repo.

I think unit test is more helpful and needed for function migrateUrlState in this case https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6780/files#diff-1738b52af72f333e3c7154730455af2dc46debf51bd704ebc5324db3030b1c1aR59