Skip to content

Find filters in map visualizations#5

Merged
jsoriano merged 1 commit intoelastic:mainfrom
jsoriano:filter-embedded-objects
May 14, 2024
Merged

Find filters in map visualizations#5
jsoriano merged 1 commit intoelastic:mainfrom
jsoriano:filter-embedded-objects

Conversation

@jsoriano
Copy link
Copy Markdown
Member

@jsoriano jsoriano commented May 8, 2024

What

Support detection of filters in map visualizations. This detection is used by package-spec and elastic-package to check that dashboards in packages include filters.

How

Maps visualizations keep most of their definitions in the mapStateJSON object, including filters. This change adds the path to this object to the list of paths considered when looking for filters in HasFilter().

Fix also the paths used to decode embedded JSON, so the fix mentioned also works for map panels whose content is encoded.

How to test

Export a dashboard containing a maps visualization stored by value, that includes a filter. HasFilters() for this visualization should return true.

Related issues

Look for filters also in mapStateJSON objects, where maps store their
filters.
@jsoriano jsoriano self-assigned this May 8, 2024
@jsoriano jsoriano requested review from a team as code owners May 8, 2024 17:49
@jsoriano jsoriano requested a review from a team May 8, 2024 17:49
Comment thread kbncontent.go

for _, path := range paths {
if m.Get(path).IsStr() {
if m.Get(path).IsStr() && m.Get(path).Str() != "" {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Without this change this seems to return the first path it founds, even if it is empty. There are cases where the first path is empty, but there are others with values.

Comment thread kbncontent.go
Comment on lines -181 to -184
if v.SavedObjectType != "visualization" {
return ""
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is there a reason to return a title only for visualizations?

Comment thread kbncontent.go
"embeddableConfig.attributes.kibanaSavedObjectMeta.searchSourceJSON",
"embeddableConfig.attributes.mapStateJSON",
"embeddableConfig.attributes.uiStateJSON",
"embeddableConfig.attributes.visState",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Adding the paths with the embeddableConfig prefix. Actually in all the test cases these paths are prefixed with embeddableConfig, so not sure if we need the others 🤔

Comment thread kbncontent.go
Comment on lines +214 to +215
"attributes.layerListJSON",
"embeddableConfig.attributes.layerListJSON",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

layerListJSON is an slice, and fails to be decoded with FromJSON, we need to use FromJSONSlice for them.

Comment thread testdata/dashboard.json
"panelIndex": "d9771ad7-cec0-4e4b-a51e-bbc880a8af0d",
"title": "Unique IPs map encoded",
"type": "map",
"version": "8.10.2"
Copy link
Copy Markdown
Member Author

@jsoriano jsoriano May 8, 2024

Choose a reason for hiding this comment

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

This one is a real example from the Apache package. I had to add a validation exception because we could not find a filter, but the filter is actually there.

The previous example is the same, but decoded. We don't support decoding maps yet in elastic-package, adding support in elastic/elastic-package#1825

@nreese
Copy link
Copy Markdown

nreese commented May 8, 2024

Can you add some more background information in the description? What problem is this trying to solve? Where/how can we view the existing problem? How can we test the PR? Thanks

@jsoriano
Copy link
Copy Markdown
Member Author

jsoriano commented May 8, 2024

Can you add some more background information in the description? What problem is this trying to solve? Where/how can we view the existing problem? How can we test the PR? Thanks

Added more info to the description. For testing I have added test cases to the unit test directly taken from real world examples. Let me know if I should prepare a build of elastic-package including this fix for further manual testing.

Thanks!

@jsoriano jsoriano changed the title Find filters in maps Find filters in map visualizations May 8, 2024
Comment thread kbncontent.go
queryPaths := []string{
"attributes.kibanaSavedObjectMeta.searchSourceJSON.query.query",
"attributes.state.query.query",
"embeddableConfig.attributes.mapStateJSON.query.query",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should you also add the path embeddableConfig.attributes.mapStateJSON.filters?

Maps uses Kibana's unified search, which applies filtering from 2 locations. query captures the contents search bar while filters captures the contents of the filter bar. Both should be checked to know if a map panel applies a panel level filtering.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added it in https://github.com/elastic/kbncontent/pull/5/files#diff-922b6d6bcd00ced7f244a57230a1075d65e42bae8fe3e8c8ac18ce9b82d03d9cR154.

The structure for queries and filters are different, so there are two different checks, each one with its own list of paths.

Copy link
Copy Markdown

@nreese nreese left a comment

Choose a reason for hiding this comment

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

kibana-presentation changes LGTM
code review only

Copy link
Copy Markdown

@mrodm mrodm left a comment

Choose a reason for hiding this comment

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

LGTM code changes

@jsoriano jsoriano merged commit b763922 into elastic:main May 14, 2024
@jsoriano jsoriano deleted the filter-embedded-objects branch May 14, 2024 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants