commons.bundle.js:3 Uncaught (in promise) TypeError: Cannot read property 'type' of undefined

[stacey-gammon]

Splitting out the errors in #41240

Kibana version: 7.3.0 BC4

Elasticsearch version: 7.3.0 BC4

Steps to reproduce:

1. Load sample data from load sample data kibana tutorial for logstash - create logstash-* index pattern https://www.elastic.co/guide/en/kibana/7.3/tutorial-build-dashboard.html#tutorial-load-sample-data
2. Import the attached json and associate the saved objects - machine_os horizontal_bard_chart response_code with logstash index pattern
3. Open the dashboard all_the_viz

bubbles.json.txt

Errors in browser console (if relevant):

commons.bundle.js:3 Uncaught (in promise) TypeError: Cannot read property 'type' of undefined
    at _callee$ (commons.bundle.js:3)
    at tryCatch (vendors.bundle.dll.js:541)
    at Generator.invoke [as _invoke] (vendors.bundle.dll.js:541)
    at Generator.prototype.<computed> [as next] (vendors.bundle.dll.js:541)
    at asyncGeneratorStep (commons.bundle.js:3)
    at _next (commons.bundle.js:3)
    at commons.bundle.js:3
    at new Promise (<anonymous>)
    at commons.bundle.js:3
    at mapPhrases (commons.bundle.js:3)

[elasticmachine]

Pinging @elastic/kibana-app-arch

[lizozom]

Copying this information from the original issue (not a solution, just summary of a debugging session):

@stacey-gammon @lukeelmers I debugged the issue, thinking this was a filter_manager issue, because the error is coming from filter manager (map_pharses).

However, the dashboard shouldn't have filters at all (it doesn't have them in 7.2). The problem happens when in 7.3, this dashboard decides it has a filter and then adds a misshaped one to $scope.filters. filter_state_manager picks in up, and calls setFilter and that's where the error in the console log is coming from.

I didn't figure out yet why the dashboard thought it had a filter though.

[stacey-gammon]

So I think this does actually have to do with the migration changes. In 7.3 I added migrations so I could get rid of some in place, ad hoc migration logic. This change was not 1:1.

So, in 7.2 we have this logic when creating filter objects from the saved object:

  static isQueryFilter(filter) {
    return filter.query && !filter.meta;
  }

  /**
   * Returns the filters for the dashboard that should appear in the filter bar area.
   * @param {SavedDashboard} dashboard
   * @return {Array.<Object>} Array of filters that should appear in the filter bar for the
   * given dashboard
   */
  static getFilterBarsForDashboard(dashboard) {
    return _.reject(this.getDashboardFilters(dashboard), this.isQueryFilter);
  }

The original migration logic closely mimicked this, but then we hit the bug with certain filters that didn't have meta but also didn't have query.query.query_string, this was fixed in https://github.com/elastic/kibana/pull/41245/files, and the new check for isQueryFilter became:

    filter.query && !(filter as Filter).meta && (filter as Pre600FilterQuery).query.query_string

So actually that type error in the console only showed up once I fixed the original bug in that first issue!

The original code in 7.2 had its own problems because it would still improperly detect a query filter, just like the original migration code did. I'm going to guess dashboard handled that improper query string filter okay, but migrations didn't.

So when we found that the dashboard saved object in 7.2 and the dashboard saved object in 7.3 looked the same, this is actually wrong. In 7.3 we should just throw that empty filter out. The logic should be something like:

• only move query filters to query if it passes the more thorough check, but
• remove any filters that don't have a meta property.

So I think the fix is to change migrations to remove filters that don't have a meta property, or once again put that logic back in the code.

[stacey-gammon]

What might need more investigation is how this is handled with the new async filter updates.

e.g. what happens if someone manually modifies the URL to include the bad filters? Will migrateAppState handle migrating these old urls or will the Filter manager hit the bug first? I think the latter, so another fix for that could be to initialize the queryFilter in dashboardApp to after initializing dashboardStateManager.

[stacey-gammon]

I tested the URL situation and you can hit the bug that way because the migrateAppState logic is not calling the moveFiltersToQuery logic. Luckily, we should not have any URLs we produced that look like that because the old code fixed up the URL before it was exposed to the user.

So we should actually be good re: URLs. If they manually modify the URL, we don't make any promises on it working.

Though, it would be really great if we had our migration system somehow hooked up to URLs too.