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.

Sign up for GitHub

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

Clean up TSVB turn Joi schema into kbn schema #65482

Merged
merged 5 commits into from
May 8, 2020
Merged

Clean up TSVB turn Joi schema into kbn schema #65482

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d0a59e
Clean up TSVB turn Joi schema into kbn schema
DianaDerevyankina May 6, 2020
8eb5bc9
Return validationTelemerty and logging part back
DianaDerevyankina May 6, 2020
d230c17
Merge branch 'master' into 57342
DianaDerevyankina May 7, 2020
507a21b
Add schema.maybe() where it was missed
DianaDerevyankina May 7, 2020
90ebf9e
Merge branch 'DianaDerevyankina' into 57342
DianaDerevyankina May 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
208 changes: 102 additions & 106 deletions src/plugins/vis_type_timeseries/server/routes/post_vis_schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,59 +17,65 @@
* under the License.
*/

import Joi from 'joi';
const stringOptionalNullable = Joi.string()
.allow('', null)
.optional();
const stringRequired = Joi.string()
.allow('')
.required();
const arrayNullable = Joi.array().allow(null);
const numberIntegerOptional = Joi.number()
.integer()
.optional();
const numberIntegerRequired = Joi.number()
.integer()
.required();
const numberOptional = Joi.number().optional();
const queryObject = Joi.object({
language: Joi.string().allow(''),
query: Joi.string().allow(''),
import { schema } from '@kbn/config-schema';
import { TypeOptions } from '@kbn/config-schema/target/types/types';

const stringOptionalNullable = schema.nullable(schema.string());
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this should be schema.maybe(schema.nullable(schema.string()));

The current schema just gives string | null.


const stringRequired = schema.string();

const arrayNullable = schema.arrayOf(schema.nullable(schema.any()));

const validateInteger: TypeOptions<number>['validate'] = value => {
if (!Number.isInteger(value)) {
return `${value} is not an integer`;
}
};
const numberIntegerOptional = schema.maybe(schema.number({ validate: validateInteger }));
const numberIntegerRequired = schema.number({ validate: validateInteger });

const numberOptional = schema.maybe(schema.number());

const queryObject = schema.object({
language: schema.string(),
query: schema.string(),
});
const stringOrNumberOptionalNullable = Joi.alternatives([stringOptionalNullable, numberOptional]);
const numberOptionalOrEmptyString = Joi.alternatives(numberOptional, Joi.string().valid(''));
const stringOrNumberOptionalNullable = schema.nullable(
schema.oneOf([stringOptionalNullable, numberOptional])
);
const numberOptionalOrEmptyString = schema.maybe(
schema.oneOf([numberOptional, schema.literal('')])
);

const annotationsItems = Joi.object({
const annotationsItems = schema.object({
color: stringOptionalNullable,
fields: stringOptionalNullable,
hidden: Joi.boolean().optional(),
hidden: schema.maybe(schema.boolean()),
icon: stringOptionalNullable,
id: stringOptionalNullable,
ignore_global_filters: numberIntegerOptional,
ignore_panel_filters: numberIntegerOptional,
index_pattern: stringOptionalNullable,
query_string: queryObject.optional(),
query_string: schema.maybe(queryObject),
template: stringOptionalNullable,
time_field: stringOptionalNullable,
});

const backgroundColorRulesItems = Joi.object({
value: Joi.number()
.allow(null)
.optional(),
const backgroundColorRulesItems = schema.object({
value: schema.nullable(schema.number()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this needs a schema.maybe wrapper as well - in the Joi schema this was optional.

id: stringOptionalNullable,
background_color: stringOptionalNullable,
color: stringOptionalNullable,
});

const gaugeColorRulesItems = Joi.object({
const gaugeColorRulesItems = schema.object({
gauge: stringOptionalNullable,
text: stringOptionalNullable,
id: stringOptionalNullable,
operator: stringOptionalNullable,
value: Joi.number(),
value: schema.number(),
});
const metricsItems = Joi.object({
const metricsItems = schema.object({
field: stringOptionalNullable,
id: stringRequired,
metric_agg: stringOptionalNullable,
Expand All @@ -84,82 +90,83 @@ const metricsItems = Joi.object({
beta: numberOptional,
gamma: numberOptional,
period: numberOptional,
multiplicative: Joi.boolean(),
multiplicative: schema.maybe(schema.boolean()),
window: numberOptional,
function: stringOptionalNullable,
script: stringOptionalNullable,
variables: Joi.array()
.items(
Joi.object({
variables: schema.maybe(
schema.arrayOf(
schema.object({
field: stringOptionalNullable,
id: stringRequired,
name: stringOptionalNullable,
})
)
.optional(),
percentiles: Joi.array()
.items(
Joi.object({
),
percentiles: schema.maybe(
schema.arrayOf(
schema.object({
id: stringRequired,
field: stringOptionalNullable,
mode: Joi.string().allow('line', 'band'),
shade: Joi.alternatives(numberOptional, stringOptionalNullable),
value: Joi.alternatives(numberOptional, stringOptionalNullable),
mode: schema.oneOf([schema.literal('line'), schema.literal('band')]),
shade: schema.oneOf([numberOptional, stringOptionalNullable]),
value: schema.oneOf([numberOptional, stringOptionalNullable]),
percentile: stringOptionalNullable,
})
)
.optional(),
),
type: stringRequired,
value: stringOptionalNullable,
values: Joi.array()
.items(Joi.string().allow('', null))
.allow(null)
.optional(),
values: schema.nullable(schema.arrayOf(schema.nullable(schema.string()))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

});

const splitFiltersItems = Joi.object({
const splitFiltersItems = schema.object({
id: stringOptionalNullable,
color: stringOptionalNullable,
filter: Joi.object({
language: Joi.string().allow(''),
query: Joi.string().allow(''),
}).optional(),
filter: schema.maybe(
schema.object({
language: schema.string(),
query: schema.string(),
})
),
label: stringOptionalNullable,
});

const seriesItems = Joi.object({
const seriesItems = schema.object({
aggregate_by: stringOptionalNullable,
aggregate_function: stringOptionalNullable,
axis_position: stringRequired,
axis_max: stringOrNumberOptionalNullable,
axis_min: stringOrNumberOptionalNullable,
chart_type: stringRequired,
color: stringRequired,
color_rules: Joi.array()
.items(
Joi.object({
color_rules: schema.maybe(
schema.arrayOf(
schema.object({
value: numberOptional,
id: stringRequired,
text: stringOptionalNullable,
operator: stringOptionalNullable,
})
)
.optional(),
),
fill: numberOptionalOrEmptyString,
filter: Joi.alternatives(
Joi.object({
query: stringRequired,
language: stringOptionalNullable,
}).optional(),
Joi.string().valid('')
filter: schema.maybe(
schema.oneOf([
schema.object({
query: stringRequired,
language: stringOptionalNullable,
}),
schema.literal(''),
])
),
formatter: stringRequired,
hide_in_legend: numberIntegerOptional,
hidden: Joi.boolean().optional(),
hidden: schema.maybe(schema.boolean()),
id: stringRequired,
label: stringOptionalNullable,
line_width: numberOptionalOrEmptyString,
metrics: Joi.array().items(metricsItems),
metrics: schema.arrayOf(metricsItems),
offset_time: stringOptionalNullable,
override_index_pattern: numberOptional,
point_size: numberOptionalOrEmptyString,
Expand All @@ -170,9 +177,7 @@ const seriesItems = Joi.object({
series_interval: stringOptionalNullable,
series_drop_last_bucket: numberIntegerOptional,
split_color_mode: stringOptionalNullable,
split_filters: Joi.array()
.items(splitFiltersItems)
.optional(),
split_filters: schema.maybe(schema.arrayOf(splitFiltersItems)),
split_mode: stringRequired,
stacked: stringRequired,
steps: numberIntegerOptional,
Expand All @@ -189,38 +194,34 @@ const seriesItems = Joi.object({
var_name: stringOptionalNullable,
});

export const visPayloadSchema = Joi.object({
export const visPayloadSchema = schema.object({
filters: arrayNullable,
panels: Joi.array().items(
Joi.object({
annotations: Joi.array()
.items(annotationsItems)
.optional(),
panels: schema.arrayOf(
schema.object({
annotations: schema.maybe(schema.arrayOf(annotationsItems)),
axis_formatter: stringRequired,
axis_position: stringRequired,
axis_scale: stringRequired,
axis_min: stringOrNumberOptionalNullable,
axis_max: stringOrNumberOptionalNullable,
bar_color_rules: arrayNullable.optional(),
bar_color_rules: schema.maybe(arrayNullable),
background_color: stringOptionalNullable,
background_color_rules: Joi.array()
.items(backgroundColorRulesItems)
.optional(),
background_color_rules: schema.maybe(schema.arrayOf(backgroundColorRulesItems)),
default_index_pattern: stringOptionalNullable,
default_timefield: stringOptionalNullable,
drilldown_url: stringOptionalNullable,
drop_last_bucket: numberIntegerOptional,
filter: Joi.alternatives(
stringOptionalNullable,
Joi.object({
language: stringOptionalNullable,
query: stringOptionalNullable,
})
filter: schema.nullable(
schema.oneOf([
stringOptionalNullable,
schema.object({
language: stringOptionalNullable,
query: stringOptionalNullable,
}),
])
),
gauge_color_rules: Joi.array()
.items(gaugeColorRulesItems)
.optional(),
gauge_width: [stringOptionalNullable, numberOptional],
gauge_color_rules: schema.maybe(schema.arrayOf(gaugeColorRulesItems)),
gauge_width: schema.nullable(schema.oneOf([stringOptionalNullable, numberOptional])),
gauge_inner_color: stringOptionalNullable,
gauge_inner_width: stringOrNumberOptionalNullable,
gauge_style: stringOptionalNullable,
Expand All @@ -230,7 +231,7 @@ export const visPayloadSchema = Joi.object({
ignore_global_filter: numberOptional,
index_pattern: stringRequired,
interval: stringRequired,
isModelInvalid: Joi.boolean().optional(),
isModelInvalid: schema.maybe(schema.boolean()),
legend_position: stringOptionalNullable,
markdown: stringOptionalNullable,
markdown_scrollbars: numberIntegerOptional,
Expand All @@ -242,9 +243,7 @@ export const visPayloadSchema = Joi.object({
pivot_label: stringOptionalNullable,
pivot_type: stringOptionalNullable,
pivot_rows: stringOptionalNullable,
series: Joi.array()
.items(seriesItems)
.required(),
series: schema.arrayOf(seriesItems),
show_grid: numberIntegerRequired,
show_legend: numberIntegerRequired,
time_field: stringOptionalNullable,
Expand All @@ -253,22 +252,19 @@ export const visPayloadSchema = Joi.object({
})
),
// general
query: Joi.array()
.items(queryObject)
.allow(null)
.required(),
state: Joi.object({
sort: Joi.object({
column: stringRequired,
order: Joi.string()
.valid(['asc', 'desc'])
.required(),
}).optional(),
}).required(),
savedObjectId: Joi.string().optional(),
timerange: Joi.object({
query: schema.maybe(schema.arrayOf(schema.nullable(queryObject))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the query is required, so the maybe should be removed here. Also when I'm reading the Joi schema correctly, query is allowed to be null, not the items within the array. That's not the same thing, Joi would allow query: null but fail on query: [null] while the new version would do the exact opposite.

So it should be schema.nullable(schema.arrayOf(queryObject))

state: schema.object({
sort: schema.maybe(
schema.object({
column: stringRequired,
order: schema.oneOf([schema.literal('asc'), schema.literal('desc')]),
})
),
}),
savedObjectId: schema.maybe(schema.string()),
timerange: schema.object({
timezone: stringRequired,
min: stringRequired,
max: stringRequired,
}).required(),
}),
});
8 changes: 5 additions & 3 deletions src/plugins/vis_type_timeseries/server/routes/vis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,18 @@ export const visDataRoutes = (
},
},
async (requestContext, request, response) => {
const { error: validationError } = visPayloadSchema.validate(request.body);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for removing it. I like your idea of using official way for validating! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@dziyanadzeraviankina Unfortunately we can't remove this part just yet. The telemetry is reporting that there are still some problems left in the validation which means working tsvb visualizations would start to fail when we are rolling this out now. The plan is to let the "soft validation" currently in place running for at least another minor.

The new kbn/schema should be used in the same way the joi schema is used right now.

if (validationError) {
try {
visPayloadSchema.validate(request.body);
} catch (error) {
logFailedValidation();
const savedObjectId =
(typeof request.body === 'object' && (request.body as any).savedObjectId) ||
'unavailable';
framework.logger.warn(
`Request validation error: ${validationError.message} (saved object id: ${savedObjectId}). This most likely means your TSVB visualization contains outdated configuration. You can report this problem under https://github.com/elastic/kibana/issues/new?template=Bug_report.md`
`Request validation error: ${error.message} (saved object id: ${savedObjectId}). This most likely means your TSVB visualization contains outdated configuration. You can report this problem under https://github.com/elastic/kibana/issues/new?template=Bug_report.md`
);
}

try {
const results = await getVisData(
requestContext,
Expand Down