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

[CM] Soften response validation #167152

Closed
Dosant opened this issue Sep 25, 2023 · 1 comment · Fixed by #166919
Closed

[CM] Soften response validation #167152

Dosant opened this issue Sep 25, 2023 · 1 comment · Fixed by #166919
Assignees
Labels
Feature:Content Management User generated content (saved objects) management impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)

Comments

@Dosant
Copy link
Contributor

Dosant commented Sep 25, 2023

We started using content management for backward-compatibility reasons in appex analytics saved objects
Apart from other things the content management validate the responses against the schema, for example, for Discover's search:

const savedSearchAttributesSchema = schema.object(
{
title: schema.string(),
sort: schema.oneOf([sortSchema, schema.arrayOf(sortSchema)]),
columns: schema.arrayOf(schema.string()),
description: schema.string(),
grid: schema.object({
columns: schema.maybe(
schema.recordOf(
schema.string(),
schema.object({
width: schema.maybe(schema.number()),
})
)
),
}),
hideChart: schema.maybe(schema.boolean()),
isTextBasedQuery: schema.maybe(schema.boolean()),
usesAdHocDataView: schema.maybe(schema.boolean()),
kibanaSavedObjectMeta: schema.object({
searchSourceJSON: schema.string(),
}),
viewMode: schema.maybe(
schema.oneOf([schema.literal('documents'), schema.literal('aggregated')])
),
hideAggregatedPreview: schema.maybe(schema.boolean()),
rowHeight: schema.maybe(schema.number()),
hits: schema.maybe(schema.number()),
timeRestore: schema.maybe(schema.boolean()),
timeRange: schema.maybe(
schema.object({
from: schema.string(),
to: schema.string(),
})
),
refreshInterval: schema.maybe(
schema.object({
pause: schema.boolean(),
value: schema.number(),
})
),
rowsPerPage: schema.maybe(schema.number()),
breakdownField: schema.maybe(schema.string()),
version: schema.maybe(schema.number()),
},
{ unknowns: 'forbid' }
);

In case the response doesn't match the schema Discover and other appex analytics content storages throw an error:

if (resultError) {
throw Boom.badRequest(`Invalid response. ${resultError.message}`);
}

We added schemas and response validation, but there are ways for saved objects to get into the system in a “unexpected” format, for example, we recently fixed a bug with discover's search sort field #166886. We fixed that particular bug, but there might be other similar bugs that are hard to find.

We agreed we should keep the validation error in dev and write warning in console in prod. Here is a possible approach we could take: #166919

cc @davismcphee

@Dosant Dosant added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Feature:Content Management User generated content (saved objects) management labels Sep 25, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@Dosant Dosant self-assigned this Sep 25, 2023
Dosant added a commit that referenced this issue Sep 28, 2023
## Summary

Close #167152

Log a warning instead of throwing an error in
`saved_object_content_storage` when response validation failed.

We decided to do this as a precaution and as a follow up to an issue
found in saved search #166886
where storage started failing because of too strict validation.

As of this PR the saved_object_content_storage covers and this change
cover:

- `search`
- `index_pattern`
- `dashboard`
- `lens`
- `maps`

For other types we agreed with @dej611 that instead of applying the same
change for other types (visualization, graph, annotation) the team would
look into migrating their types to also use
`saved_object_content_storage`
#167421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Content Management User generated content (saved objects) management impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants