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

Migrate old style queries stored in filters array #38945

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Jun 13, 2019

Summary

Prior to version 6.0.0, advanced lucene query settings were stored on the dashboard object. This could cause you to see certain settings applied that were not actually a part of your current lucene query settings.

This fixes that behavior by migrating all older style queries into the newer format, and removes those advanced settings from the dashboard object. This behavior is considered a bug, but note that if you have old dashboards that have never been edited since 6.0.0, you may start to see different query settings applied. The settings will now match whatever you have set up in your advanced settings configuration and not what previously happened to be stored on the dashboard.

Fixes #38933.

Details

Migrates dashboards that used to have queries stored in the filter array on search source and moves it to the query part of search source.

5.x url test added, though in code migration shouldn't be needed for bwc support of URLs because in the url it's already in a different format.

Also fixes #38933.

More migrations will likely follow to further simplify some of the dashboard code.

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@stacey-gammon stacey-gammon changed the title Migrate old style queries stored in filters array and remove now unne… Migrate old style queries stored in filters array Jun 13, 2019
@stacey-gammon stacey-gammon force-pushed the 2019-06-13-migrate_search_source_filter_queries branch from 0e3063c to 94f9793 Compare June 13, 2019 22:44
@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon stacey-gammon force-pushed the 2019-06-13-migrate_search_source_filter_queries branch 2 times, most recently from 6ef31eb to 9a00282 Compare June 14, 2019 00:20
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon stacey-gammon marked this pull request as ready for review June 14, 2019 12:21
@stacey-gammon stacey-gammon added release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.3.0 v8.0.0 labels Jun 14, 2019
@stacey-gammon stacey-gammon added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Jun 14, 2019
@stacey-gammon stacey-gammon force-pushed the 2019-06-13-migrate_search_source_filter_queries branch from cd349d4 to d873a9c Compare June 14, 2019 13:19
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon stacey-gammon force-pushed the 2019-06-13-migrate_search_source_filter_queries branch from d873a9c to 552b610 Compare June 14, 2019 19:16
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon stacey-gammon force-pushed the 2019-06-13-migrate_search_source_filter_queries branch from d81c811 to 575ffe2 Compare June 17, 2019 12:53
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon stacey-gammon force-pushed the 2019-06-13-migrate_search_source_filter_queries branch 2 times, most recently from 7009f8b to e7d5215 Compare June 18, 2019 12:42
@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon stacey-gammon force-pushed the 2019-06-13-migrate_search_source_filter_queries branch from 0b3a55a to 3b40c2a Compare June 18, 2019 18:38
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

This LGTM, but just wanted to throw out the question, do we want to do the same migrations for visualizations/saved searches? Seems odd to have dashboards migrated but not the others.

@stacey-gammon
Copy link
Contributor Author

do we want to do the same migrations for visualizations/saved searches?

That sounds like a good idea to me. @timroes - mind if I leave that as a follow up task for the Kibana App team?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs
Copy link
Contributor

Bargs commented Jun 18, 2019

@lukasolson @stacey-gammon I don't think this migration is required for saved searches and visualizations. afaik dashboard was the only app to store queries in the filters array like this.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

Had a couple questions but no blocking issues. Tested the functionality with an export from 5.6 and it worked.


import { SavedObjectReference } from '../../../../legacy/server/saved_objects/routes/types';

export interface SavedObjectAttributes {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these not just be the existing saved object types? (Same question for the other types.ts file under the dashboard directory)

/**
*
* @public
*/
export interface SavedObjectAttributes {
[key: string]: SavedObjectAttributes | string | number | boolean | null;
}
/**
*
* @public
*/
export interface SavedObject<T extends SavedObjectAttributes = any> {
id: string;
type: string;
version?: string;
updated_at?: string;
error?: {
message: string;
statusCode: number;
};
attributes: T;
references: SavedObjectReference[];
migrationVersion?: SavedObjectsMigrationVersion;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you these types could all be refactored/improved. It's just not super straightforward. I started going down many typescript rabbit holes with this endeavor (started trying to type all of migrations.js), but they quickly grew out of scope.

For instance, we have interface SavedObjectDashboard extends SavedObject. That's actually not the SavedObject here though, that's actually the SavedObjectAttributes (but also with id... so maybe it's actually the saved object after a post processing step). This extends SavedObject type is also different than the raw SavedObject.

Another type that gets in the mix is the one before and after the extractReferences call. It actually changes the raw saved object type by injecting the panel ids into the panelsJSON and removing the outer references.

So I think I'd prefer for this PR and just focus on the migration plus minimal typing support. Ideally we should typescript all of migrations and start a pattern of adding types for each iteration of saved object.

@lukasolson
Copy link
Member

@Bargs The advanced settings are still stored as part of queries in 5.x, though, which I thought was the intention of this migration (to remove those).

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon stacey-gammon merged commit 184111d into elastic:master Jun 19, 2019
@Bargs
Copy link
Contributor

Bargs commented Jun 19, 2019

@lukasolson ah yeah I suppose so, I thought the main point of this PR was to get the query out of the filters array, and removing the advanced settings was more of a side effect.

@stacey-gammon
Copy link
Contributor Author

The main point of the PR was actually to clean up dashboard logic :D both removing the query from the filters array and removing the advanced settings were side affects of that. :-P

stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Jun 19, 2019
* Migrate old query filters

* Null check instead of undefined for more completeness

* remove unnecessary undefined check

* Use good defaults, not undefined, for brand new dashboards.

* fix: typescript errors

* be explicit instead of matchinline snapshot.

* default to Kuery when there is no query given
stacey-gammon added a commit that referenced this pull request Jun 19, 2019
* Migrate old query filters

* Null check instead of undefined for more completeness

* remove unnecessary undefined check

* Use good defaults, not undefined, for brand new dashboards.

* fix: typescript errors

* be explicit instead of matchinline snapshot.

* default to Kuery when there is no query given
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported release_note:fix review Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre 6.0.0 dashboards apply old query.querystring parameters
5 participants