[Dashboard] Log warning if filter and query state are malformed

[nickpeihl]

Fixes #230054

Summary

Logs a warning instead of throwing an error if filter and query state can not be read or written due to missing or malformed references.

Also removes unnecessary Promises in the Dashboard server search and get methods.

[elasticmachine]

Pinging @elastic/kibana-presentation (Team:Presentation)

[nreese]

I have a question about what to do with errors. I think dashboard API should just store what its given and return what its given in the event of the error. This will allow the UI the chance to display problems so filters and queries just don't magically disappear.

[nreese]

You will loss the filter state if you return an empty object. How about returning JSON.stringify(searchSource)?

[nreese]

how about returning un-injected state. That way the client can display a usable error message and maybe let the user fix the problem?

[nickpeihl]

Good idea. But should we also handle an error when the searchSourceJSON can not be parsed into an object? Should we throw an error here or log a warning and return the stringified JSON?

[nreese]

If searchSourceJSON parse fails, then return an empty object for filters and query. I almost wonder if we should include a warnings array in the response to capture failures like these and then display something client side. What do you think?

[nickpeihl]

If searchSourceJSON parse fails, then return an empty object for filters and query.

Sounds good.

I almost wonder if we should include a warnings array in the response to capture failures like these and then display something client side. What do you think?

I'm not opposed to it. Do you see the warnings array as a top-level item in the response?

for example, something like this?

{
  "id": "some-uuid",
  "type": "dashboard",
  "data": {
    "kibanaSavedObjectMeta": { ... },
    "panels": [],
    ...
  },
  "warnings": [{ 
    "title": "Reference error", 
    "detail": "Could not find reference for kibanaSavedObjectMeta.searchSourceJSON.filter[0].meta.index" 
  }],
}

[nreese]

Yes, something like that would allow the UI to display a warning toast if there are loading issues. I wonder if title is needed. I think dashboard should just display a single toast with bullets for each warning. But I am not against additional context. We could always just not display titles in the client

[nickpeihl]

@nreese I was thinking about this over the weekend and would like more time to consider if/how we expose different messages between the client and the server. A read-only dashboard user might be confused by a toast message warning about missing references. However, logging these kinds of warnings on the server might have more utility.

I have created this issue for adding a warnings array to the response. But it might be blocked by #196609 since I think the warnings should go in the read-only meta property that will be introduced by that issue.

Also, this PR is fixing a bug in main and serverless and I'd like to prioritized getting this merged before we start exposing warnings in the response and UI. So do you mind if we merge this PR and consider #230314 a separate effort?

[nreese]

Instead of nested try/catch statements how about just

let parsedSearchSource;
try {
  parsedSearchSource = parseSearchSourceJSON(searchSourceJSON);
} catch (parseError) {
  logger.warn(`Unable to parse searchSourceJSON. Error: ${parseError.message}`);
  return { searchSource: {} };
}

let searchSource;
try {
  searchSource = injectReferences(parsedSearchSource, references);
} catch (injectError) {
  logger.warn(
    `Unable to transform filter and query state on read. Error: ${injectError.message}`
  );
  // fallback to parsed if injection fails
  searchSource = parsedSearchSource;
}

try {
  const filters = cleanFiltersForSerialize(searchSource.filter);
  const query = searchSource.query ? migrateLegacyQuery(searchSource.query) : undefined;
  return { searchSource: { filters, query } };
} catch (error) {
  logger.warn(
    `Unexpected error transforming filter and query state on read. Error: ${error.message}`
  );
  return { 
    searchSource: {
      filters: searchSource.filter,
      query: searchSource.query,
    } 
  };
}

[nreese]

This should return

searchSource: {
      filters: searchSource.filter,
      query: searchSource.query,
    }

[nreese]

kibana-presentation changes LGTM
code review only

[elasticmachine]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 66bee0b

Failed CI Steps

• FTR Configs #129

Test Failures

• [job] [logs] FTR Configs #129 / Fleet Endpoints fleet policy secrets fleet server version requirements "after each" hook for "should not store secrets if there are no fleet servers"

Metrics [docs]

✅ unchanged

History

• 💚 Build #326261 succeeded b899ae0
• 💛 Build #325700 was flaky 2468e70
• 💛 Build #325325 was flaky fdae60a