[TSVB] Breaks requests when series id or metric id is purely digits

[timroes]

When the visState of a TSVB visualization contains series ids or metric ids that are purely containing digits, e.g. `"2"`` the request won't be built correctly.

We're using lodash.set to call something like: set(doc, aggs.${series.id}.aggs.timeseries.aggs.${metric.id}, bucket);. If metric.id or series.id is now an integer value (even if it's a string by type), lodash.set will interpret that as an array access, instead of a property name, and thus suddenly create arrays for aggs instead of an object, which will fail then with an error message like: [parsing_exception]: Expected [START_OBJECT] under [aggs], but got a [START_ARRAY] in [timeseries] when we send out that request.

We never generate purely numerical ids internally, but users might inject visualizations via scripts where they generate the ids in this format and cause a failure. We should try to be as fail-safe as possible towards those ids.

cc @stratoula @flash1293

[elasticmachine]

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

[flash1293]

This should be fixed by looking through the saved object during load and prefixing all series and metric ids parseable as number with a character to set will be able to handle them correctly. The existing updateOldState function can be extended for this https://github.com/elastic/kibana/blob/4584a8b570402aa07832cf3e5b520e5d2cfa7166/src/plugins/visualizations/public/legacy/vis_update_state.js#L1[…]91

• Run through all series and metrics
• If id parses as a number, prefix it with an x

Can be tested with this SO (should fail without the fix described above):

{"attributes":{"color":"#147c75","description":"","name":"tracker"},"coreMigrationVersion":"8.0.0","id":"01198630-ac3b-11eb-bd50-01ab2ee455fc","migrationVersion":{"tag":"8.0.0"},"references":[],"type":"tag","updated_at":"2021-10-01T01:33:34.488Z","version":"WzE5OCwxXQ=="}
{"attributes":{"description":"","kibanaSavedObjectMeta":{"searchSourceJSON":"{\"query\":{\"language\":\"kuery\",\"query\":\"\"},\"filter\":[]}"},"title":"recreation","uiStateJSON":"{}","version":"1","visState":"{\"title\":\"recreation\",\"type\":\"metrics\",\"aggs\":[],\"params\":{\"time_range_mode\":\"entire_time_range\",\"id\":\"2\",\"index_pattern\":\"\",\"time_field\":null,\"interval\":\"auto\",\"series\":[{\"id\":\"A\",\"label\":\"US\",\"metrics\":[{\"id\":\"1\",\"field\":\"select field\",\"percentiles\":null,\"value\":null,\"type\":\"count\"}],\"split_mode\":\"terms\",\"split_filters\":null,\"terms_field\":\"customer_id\",\"terms_include\":null,\"terms_exclude\":null,\"terms_direction\":\"desc\",\"terms_order_by\":\"_count\",\"terms_size\":\"30\",\"axis_max\":\"\",\"axis_min\":\"0\",\"axis_position\":\"left\",\"chart_type\":\"line\",\"color\":\"#68BC00\",\"fill\":0.5,\"filter\":{\"query\":\"hostname:tracker-web-* AND NOT http.response.status_code: (200 OR 204 OR 400 OR 404 OR 408) AND environment.country:us\",\"language\":\"kuery\"},\"formatter\":\"number\",\"hidden\":false,\"line_width\":1,\"point_size\":1,\"separate_axis\":0,\"split_color_mode\":\"gradient\",\"stacked\":\"none\",\"type\":\"timeseries\",\"value_template\":\"{{value}}\"},{\"id\":\"74a66e70-ac44-11eb-9865-6b616e971cf8\",\"label\":\"CA\",\"metrics\":[{\"id\":\"74a66e71-ac44-11eb-9865-6b616e971cf8\",\"field\":\"select field\",\"percentiles\":null,\"value\":null,\"type\":\"count\"}],\"split_mode\":\"terms\",\"split_filters\":null,\"terms_field\":\"customer_id\",\"terms_include\":null,\"terms_exclude\":null,\"terms_direction\":\"desc\",\"terms_order_by\":\"_count\",\"terms_size\":\"30\",\"axis_max\":\"\",\"axis_min\":\"0\",\"axis_position\":\"left\",\"chart_type\":\"line\",\"color\":\"rgba(211,96,134,1)\",\"fill\":0.5,\"filter\":{\"query\":\"hostname:tracker-web-* AND NOT http.response.status_code: (200 OR 204 OR 400 OR 404 OR 408) AND environment.country:\\\"ca\\\" \",\"language\":\"kuery\"},\"formatter\":\"number\",\"hidden\":false,\"line_width\":1,\"point_size\":1,\"separate_axis\":0,\"split_color_mode\":\"gradient\",\"stacked\":\"none\",\"type\":\"timeseries\",\"value_template\":\"{{value}}\"}],\"annotations\":[],\"axis_formatter\":\"number\",\"axis_max\":\"\",\"axis_min\":\"\",\"axis_position\":\"left\",\"axis_scale\":\"normal\",\"filter\":{\"language\":\"kuery\",\"query\":\"\"},\"isModelInvalid\":false,\"show_grid\":1,\"show_legend\":1,\"type\":\"timeseries\",\"tooltip_mode\":\"show_all\",\"use_kibana_indexes\":false,\"truncate_legend\":1,\"max_lines_legend\":1,\"drop_last_bucket\":0}}"},"coreMigrationVersion":"8.0.0","id":"8a2cfd90-ac44-11eb-b2d9-cf127792184b","migrationVersion":{"visualization":"7.14.0"},"references":[{"id":"01198630-ac3b-11eb-bd50-01ab2ee455fc","name":"tag-01198630-ac3b-11eb-bd50-01ab2ee455fc","type":"tag"}],"type":"visualization","updated_at":"2021-10-01T01:34:40.957Z","version":"WzIwMCwxXQ=="}
{"excludedObjects":[],"excludedObjectsCount":0,"exportedCount":2,"missingRefCount":0,"missingReferences":[]}

[timroes]

This was actually caused by us upgrading set-value (and worked in 7.14 and beforehand, though never tested, since we cannot end up with those ids by just using Kibana).

set-value version in 7.14:

set-value version in 7.15

We missed that during testing, since you cannot generate those IDs inside Kibana, and the library did not really have any changelogs ...

Fix stays the same, but I'll change this to a regression, since it worked beforehand.

[kobelb]

Hey, y'all! I'm super excited to see that we found a solution to the problem, and thank you 100x for solving it so quickly.

This bug was a weird one. Do you all have any thoughts on what we can do to prevent further bugs like this from occurring? Do we need stricter validation on import/creation? Do we need to rely less on "reflection-esque" design patterns? Did we just need more testing?

[flash1293]

@kobelb I think there are two perspectives here:

• Users auto-creating saved objects which are not exactly shaped like we expect - there are tons of ways to break it and I'm not sure whether it's feasible to maintain validation logic for every part of it. IMHO it's part of using the saved objects API to be able to break things this way. Happy to discuss though if you feel different about this.
• We did a major version bump of the set-value lib which actually triggered this bug Upgrade set-value to 4.1.0 #111988 To cite the PR:

The changelog for this dependency is quite lacking

Maybe we should be more thorough testing/validating package upgrades, especially for small libraries which are not optimally maintained. In the end all of them can become critical.

[kobelb]

IMHO it's part of using the saved objects API to be able to break things this way. Happy to discuss though if you feel different about this.

I completely agree. However, I think it's something we need to fix long-term. The Kibana core team has discussed the way that we can provide validation as part of this effort.

Users auto-creating saved objects which are not exactly shaped like we expect - there are tons of ways to break it and I'm not sure whether it's feasible to maintain validation logic for every part of i

This part scares me. Without the ability to know the various parameters that are used to render these visualizations, we're going to really struggle to figure out how to evolve these visualizations over time without creating breaking changes. Is this a remnant of the way that TSVB was architected or is it just inherent when dealing with this subject matter?

[flash1293]

This part scares me. Without the ability to know the various parameters that are used to render these visualizations, we're going to really struggle to figure out how to evolve these visualizations over time without creating breaking changes. Is this a remnant of the way that TSVB was architected or is it just inherent when dealing with this subject matter?

This is a tsvb specific problem - there was an attempt to type it already but we ran into tons of issues with legacy saved objects with unexpected shapes. However we are not fully guarded against these problems in other visualization types as well, mostly because it wasn’t a priority so far. If you already have an issue for this I can add details for the visualization use case (Lens in particular). Things to consider:

• everything typescript can express in terms of object shape (unions etc.) - probably covered by the existing validation logic but I didn’t check
• object internal references - this is used a lot and references have to stay in sync (for example the id of an axis is part of the definition of a series in an xy chart, but it’s defined somewhere else), but there’s no code validating whether they are in sync right now because it’s not possible to configure a saved object with broken axis references

In general I would love a mechanism making sure every aspect of the visualization is correct as it would help with the Lens embedded use case as well, happy to work towards it.

[kobelb]

Here's the issue where they're talking about adding per-type validation to saved objects: #104088