Clean up TSVB turn Joi schema into kbn schema #65482
Conversation
| }, | ||
| }, | ||
| async (requestContext, request, response) => { | ||
| const { error: validationError } = visPayloadSchema.validate(request.body); |
There was a problem hiding this comment.
Thank you for removing it. I like your idea of using official way for validating! 👍
There was a problem hiding this comment.
@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.
| }, | ||
| }, | ||
| async (requestContext, request, response) => { | ||
| const { error: validationError } = visPayloadSchema.validate(request.body); |
There was a problem hiding this comment.
@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.
| import { schema } from '@kbn/config-schema'; | ||
| import { TypeOptions } from '@kbn/config-schema/target/types/types'; | ||
|
|
||
| const stringOptionalNullable = schema.nullable(schema.string()); |
There was a problem hiding this comment.
Looks like this should be schema.maybe(schema.nullable(schema.string()));
The current schema just gives string | null.
| .allow(null) | ||
| .optional(), | ||
| const backgroundColorRulesItems = schema.object({ | ||
| value: schema.nullable(schema.number()), |
There was a problem hiding this comment.
Looks like this needs a schema.maybe wrapper as well - in the Joi schema this was optional.
| .items(Joi.string().allow('', null)) | ||
| .allow(null) | ||
| .optional(), | ||
| values: schema.nullable(schema.arrayOf(schema.nullable(schema.string()))), |
| }).required(), | ||
| savedObjectId: Joi.string().optional(), | ||
| timerange: Joi.object({ | ||
| query: schema.maybe(schema.arrayOf(schema.nullable(queryObject))), |
There was a problem hiding this comment.
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))
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
DianaDerevyankina
added
v7.9.0
release_note:skip
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
* Clean up TSVB turn Joi schema into kbn schema Part of elastic#57342 * Return validationTelemerty and logging part back * Add schema.maybe() where it was missed
Part of #57342
Summary
Changed
Joischema to@kbn/config-schemaChecklist
Delete any items that are not applicable to this PR.
For maintainers