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

[NP] Allow custom validations in HTTP Routes apart from @kbn/config-schema #51919

Merged
merged 44 commits into from
Dec 20, 2019

Conversation

afharo
Copy link
Member

@afharo afharo commented Nov 29, 2019

Summary

Allow the use of custom validation functions when defining the HTTP routes for those use cases where the use of @kbn/config-schema is not available (like in the browser).
Fixes #50179

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@afharo afharo added release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v8.0.0 v7.7.0 labels Nov 29, 2019
@afharo afharo requested a review from a team as a code owner November 29, 2019 18:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

To answer the main question, I think this is a valid approach and follows what we discussed at EAH.

I have to admit, I'm a little concerned about the growing types complexity in http route types, even if I don't have suggestion on how to reduce this.

Comment on lines 353 to 355
P extends ObjectType | ValidateFunction<unknown>,
Q extends ObjectType | ValidateFunction<unknown>,
B extends ObjectType | Type<Buffer> | Type<Stream> | ValidateFunction<unknown>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: (I know this is from a previous PR but,) why do we want to allow user to 'manually' handles body as raw buffer or stream again?

Copy link
Member Author

@afharo afharo Dec 2, 2019

Choose a reason for hiding this comment

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

It's a valid question. In the body options:

  • When parse: false, the body is returned as a Buffer
  • When output: 'stream', the body is returned as a Stream

So in the config-schema validation, the user needs to specify a valid type validation:

  • If they do schema.object({}, {allowUnknown: true}), it will not pass validation.
  • If they don't provide any validation, the body won't be passed through to them

Comment on lines 42 to 51
export type TypeOfFunctionReturn<T extends ValidateFunction<unknown>> = NonNullable<
ReturnType<T>['value']
>;

export type ValidateSpecs<T> = ObjectType | Type<T> | ValidateFunction<T>;
export type ValidatedType<T extends ValidateSpecs<unknown>> = T extends Type<unknown>
? TypeOf<T>
: T extends ValidateFunction<unknown>
? TypeOfFunctionReturn<T>
: never;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is some serious typescript we got there!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah... annoying as hell... To me, lines 49 and 51 should not exist, but TS is not that smart yet and in the right side of the conditional, it doesn't understand the exclusion of T not being Type<unknown> anymore :/

Comment on lines 144 to 147
function routeSchemasFromRouteConfig<
P extends ObjectType,
Q extends ObjectType,
B extends ObjectType | Type<Buffer> | Type<Stream>
P extends ObjectType | ValidateFunction<unknown>,
Q extends ObjectType | ValidateFunction<unknown>,
B extends ObjectType | Type<Buffer> | Type<Stream> | ValidateFunction<unknown>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot to change this a few lines after?

if (route.validate !== false) {
Object.entries(route.validate).forEach(([key, schema]) => {
if (!(schema instanceof Type)) {
throw new Error(
`Expected a valid schema declared with '@kbn/config-schema' package at key: [${key}].`
);
}
});
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! I thought I did! Maybe it got lost when "fixing" the conflicts... Thank you for spotting it!

Comment on lines 272 to 273
Q extends ObjectType | ValidateFunction<unknown>,
B extends ObjectType | Type<Buffer> | Type<Stream> | ValidateFunction<unknown>
Copy link
Contributor

Choose a reason for hiding this comment

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

given the number of usages in our code, I think type aliases for

  • ObjectType | ValidateFunction<unknown>
  • ObjectType | Type<Buffer> | Type<Stream> | ValidateFunction<unknown>

should be a good idea. Will also avoid heavy diffs in further validation type updates.

Comment on lines 124 to 125
ValidateFunctionReturn,
ValidateFunction,
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually prefix exports from a given core submodule to avoid collision, I think we want to do this here? RouteXXX probably? Also I think RouteValidationFunction maybe is a better name than RouteValidateFunction?

* Custom validate function (only if @kbn/config-schema is not a valid option in your plugin)
* @public
*/
export type ValidateFunction<T> = (data: any) => ValidateFunctionReturn<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can be a little more restrictive than any here? I think it's a pain as

const params =
routeSchemas.params === undefined
? {}
: routeSchemas.params.validate(req.params, {}, 'request params');
const query =
routeSchemas.query === undefined
? {}
: routeSchemas.query.validate(req.query, {}, 'request query');
const body =
routeSchemas.body === undefined
? {}
: routeSchemas.body.validate(req.payload, {}, 'request body');

req.params, req.query, req.payload have different types, but if possible I think at least something like data: reqParamsType | reqQueryType | reqBodyType is a little better (or is it just worse?)

Mega bonus point if we can context-type it depending on if its a query, params or body validation method, but not sure if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the best we can assume here is req.params and req.query to always be Record<string, string>. req.payload is a bit harder to assume any type before validating it (although we've assumed {} up until now and it looks like it didn't harm, especially after Hapi's parsing). I'll see if it can be done without overcomplicating (even more) the typings

@afharo
Copy link
Member Author

afharo commented Dec 2, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

}
| {
value?: undefined;
error: SchemaTypeError;
Copy link
Contributor

Choose a reason for hiding this comment

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

This task requires to decouple request validation from @kbm/config-schema. Here it still depends on the package. If '@kbn/config-schema' makes changes in the SchemaTypeError, all routes will have to adopt their validation. We should use an internal error type instead, that '@kbn/config-schema' should satisfy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense! I'll export a new type of Error

if (typeof validationSpec === 'function') {
return validateFunction(validationSpec, data, namespace);
} else {
return validationSpec.validate(data, {}, namespace);
Copy link
Contributor

Choose a reason for hiding this comment

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

so any object with validate method is also allowed?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, TS does a validation but we should add run-time validation.

P extends ObjectType,
Q extends ObjectType,
B extends ObjectType | Type<Buffer> | Type<Stream>,
P extends ObjectType | ValidateFunction<unknown>,
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to add at least one usage example. in integration tests, e.g.

interface Foo {
  foo?: number;
}
test('invalid params', async () => {
  const router = new Router('/foo', logger, enhanceWithContext);

  const validate = {
    params: (params: Foo) => {
       // TODO add an example with an error
      const value = typeof params.foo === 'number' ?  params.foo || 0;
      return { value };
    },
  };
  const handler: RequestHandler<typeof validate.params> = (context, req, res) => {
    console.log(req.body);
    return res.ok({ body: String(req.params.test) });
  };

  router.get(
    {
      path: '/path-1',
      validate,
    },
    handler
  );

  const validateKbnSchema = {
    params: schema.object({
      test: schema.string(),
    }),
  };
  const handlerKbnSchema: RequestHandler<typeof validateKbnSchema.params> = (context, req, res) => {
    console.log(req.body);
    return res.ok({ body: String(req.params.test) });
  };

  router.get(
    {
      path: '/path-1',
      validateKbnSchema,
    },
    handlerKbnSchema
  );

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ah... I overlooked it and wrote my test instead. stupid me 🤣

Q extends ObjectType,
B extends ObjectType | Type<Buffer> | Type<Stream>,
P extends ObjectType | ValidateFunction<unknown>,
Q extends ObjectType | ValidateFunction<unknown>,
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we fallback to = ValidateFunction<unknown> to make all types optional? it would simplify manual declaration:

const handler: RequestHandler<typeof validate.params> = (context, req, res) => { ... };

try {
result = validationSpec(data);
} catch (err) {
result = { error: new SchemaTypeError(err, []) };
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow what a desirable way to fail validation is: to throw an exception or return an exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

The desirable way is to return { value: validatedValue } or { error: RouteValidationError }. But since we don't have control over the validation method and it can throw an error, we don't want to break the process and throw a 500 error (or do we?).

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member Author

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Thanks for the comments. I'll fix the run-time validation and the fallback in the RequestHandler in a bit and will wait for your thoughts regarding the rest of the discussions.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@afharo
Copy link
Member Author

afharo commented Dec 2, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover
Copy link
Contributor

ack: will review today

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

This design is looking great! Let us know if you need us to take over wrapping this up since you moved teams 😄

Some additional testing I'd like to see:

  • Validation tests added to src/core/server/http/integration_tests/router.test.ts that verifies the 400 response code and error message.
  • Functional tests added to test/plugin_functional/test_suites/core_plugins/server_plugins.ts that verifies validation behavior for both config-schema validation and custom validator functions.

*
* @public
*/
export type RouteValidateFunction<T> = (data: any) => RouteValidateFunctionReturn<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice if this function was supplied a factory method for creating RouteValidatorError instances so plugins don't have to import that class manually. I think that'd make this a bit more ergonomic and consistent with the response factories provided to route handlers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah! Makes perfect sense! That even gave me the idea of providing a RouteValidationResolver that provides the ok and fail methods so the returned contract is maintained. An example of usage: https://github.com/afharo/kibana/blob/np-decouple-configSchema/docs/development/core/server/kibana-plugin-server.routevalidationfunction.md

Comment on lines 28 to 36
export type RouteValidateFunctionReturn<T> =
| {
value: T;
error?: never;
}
| {
value?: never;
error: RouteValidationError;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the intellisense would be improved if this was inlined in the RouteValidateFunction type. It's only referenced in one other place which can be easily replaced with a ReturnType generic.


/**
* Allowed property validation options: either @kbn/config-schema validations or custom validation functions
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*
* See {@link RouteValidateFunction} for custom validation.

export type RouteValidationSpec<T> = ObjectType | Type<T> | RouteValidateFunction<T>;

// Ugly as hell but we need this conditional typing to have proper type inference
type RouteValidatedValue<T extends RouteValidationSpec<any> | undefined> = NonNullable<
Copy link
Contributor

Choose a reason for hiding this comment

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

How about renaming to RouteValidationResultType?

/**
* Route validator class to define the validation logic for each new route.
*
* @private
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about the behavior differences, but we've been using @internal

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad! Well spotted!

return new RouteValidator({ params, query, body }, options);
}

constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

with the public static from above, maybe this should be private?

}

private customValidation<T>(
validationRule?: RouteValidationSpec<T>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can make this non-optional and remove the if undefined below

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing is the validationRule might be not set. Aka: some routes check body but not params (and the other way around), so you get query and params as undefined and the current logic simply returns those 2 properties as {}.

I might prefer not to return anything at all. But our contract is to always return those params, either the validated value or {} if no validation is provided. The current master implementation does it in a very repetitive way:

if (routeSchemas === undefined) {
return {
body: {},
params: {},
query: {},
};
}
const params =
routeSchemas.params === undefined
? {}
: routeSchemas.params.validate(req.params, {}, 'request params');
const query =
routeSchemas.query === undefined
? {}
: routeSchemas.query.validate(req.query, {}, 'request query');
const body =
routeSchemas.body === undefined
? {}
: routeSchemas.body.validate(req.payload, {}, 'request body');
return { query, params, body };

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, but I think the defined-ness is already checked in the function that calls this one. It doesn't appear that this method is ever called with an undefined validationRule.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! Eagle eye!

req: Request,
routeSchemas: RouteSchemas<P, Q, B> | undefined
routeSchemas: RouteValidator<P, Q, B> | undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: rename routeSchemas to routeValidator and maybe make optional with ? instead of | undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes a difference, ? allows you not to send the parameter. | undefined enforces you to send the parameter, but it accepts the parameter to be undefined. This way you don't forget to send it by mistake ;)

Nevermind, I found a better approach so we always can send the RouteValidator and we can remove the if undefined in it.

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

LGTM after these comments, thanks for the expanded test coverage!

Comment on lines 60 to 61
validationResolver: RouteValidationResolver,
data: any
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect these arguments to be in the reverse order to more closely match route handlers (where the input argument (request) comes before the response toolkit).

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it in that order to somehow enforce the user gets the param :)
But, in order to maintain our convention, I can switch them 👍

/**
* The custom validation function if @kbn/config-schema is not a valid solution for your specific plugin requirements.
*
* The validation should look something like:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add the @example tag above this line.

*
* @public
*/
export interface RouteValidationResolver {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we call this RouteValidationResultFactory to closer match the KibanaResponseFactory name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've also changed the interface so it returns the methods ok and badRequest (instead of fail) to be an even closer match to the response factory :)

…nterface to look more like the KibanaResponseFactory
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@afharo
Copy link
Member Author

afharo commented Dec 19, 2019

@joshdover thank you for your comments. I applied some final changes. If you could give it a final look for a sanity check, I'd appreciate it

@afharo afharo merged commit 3bdbcd0 into elastic:master Dec 20, 2019
@afharo afharo deleted the np-decouple-configSchema branch December 20, 2019 17:53
afharo added a commit to afharo/kibana that referenced this pull request Dec 20, 2019
…chema (elastic#51919)

* [NP] Allow custom validations in HTTP Routes apart from @kbn/config-schema

* API docs

* Allow validate function in the route handler (run-code validation)

* Prefix RouteXXX + Params and Body Validation Aliases

* Fix test broken by lodash

* Update API docs

* Add default types for simpler manual declaration

* Add run-time validation of the RouteValidateSpec

* Expose RouteValidationError instead of SchemaTypeError

* RouteValidator as a class to match config-schema interface

* Test for not-inline handler (need to check IRouter for elastic#47047)

* Add preValidation of the input for a safer custom validation

* Better types for RouteHandlers

* [NP] Move route validation to RouteValidator wrapper

* Use the class only internally but maintain the same API

* Fix types

* Ensure RouteValidator instance in KibanaRequest.from

* Fix validator.tests (Buffer.from instead of new Buffer)

* Default precheck should allow null values

* Also allow undefined in preChecks

* MR feedback fixes

* Provide RouteValidationResolver to the validation function

* Add functional tests

* Fix new functional tests

* Fix validator additional test

* Fix test with new resolver

* Remove unused import

* Rename ValidationResolver to ValidationResultFactory and change the interface to look more like the KibanaResponseFactory

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
afharo added a commit that referenced this pull request Dec 21, 2019
…chema (#51919) (#53714)

* [NP] Allow custom validations in HTTP Routes apart from @kbn/config-schema

* API docs

* Allow validate function in the route handler (run-code validation)

* Prefix RouteXXX + Params and Body Validation Aliases

* Fix test broken by lodash

* Update API docs

* Add default types for simpler manual declaration

* Add run-time validation of the RouteValidateSpec

* Expose RouteValidationError instead of SchemaTypeError

* RouteValidator as a class to match config-schema interface

* Test for not-inline handler (need to check IRouter for #47047)

* Add preValidation of the input for a safer custom validation

* Better types for RouteHandlers

* [NP] Move route validation to RouteValidator wrapper

* Use the class only internally but maintain the same API

* Fix types

* Ensure RouteValidator instance in KibanaRequest.from

* Fix validator.tests (Buffer.from instead of new Buffer)

* Default precheck should allow null values

* Also allow undefined in preChecks

* MR feedback fixes

* Provide RouteValidationResolver to the validation function

* Add functional tests

* Fix new functional tests

* Fix validator additional test

* Fix test with new resolver

* Remove unused import

* Rename ValidationResolver to ValidationResultFactory and change the interface to look more like the KibanaResponseFactory

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 8, 2020
…chema (elastic#51919)

* [NP] Allow custom validations in HTTP Routes apart from @kbn/config-schema

* API docs

* Allow validate function in the route handler (run-code validation)

* Prefix RouteXXX + Params and Body Validation Aliases

* Fix test broken by lodash

* Update API docs

* Add default types for simpler manual declaration

* Add run-time validation of the RouteValidateSpec

* Expose RouteValidationError instead of SchemaTypeError

* RouteValidator as a class to match config-schema interface

* Test for not-inline handler (need to check IRouter for elastic#47047)

* Add preValidation of the input for a safer custom validation

* Better types for RouteHandlers

* [NP] Move route validation to RouteValidator wrapper

* Use the class only internally but maintain the same API

* Fix types

* Ensure RouteValidator instance in KibanaRequest.from

* Fix validator.tests (Buffer.from instead of new Buffer)

* Default precheck should allow null values

* Also allow undefined in preChecks

* MR feedback fixes

* Provide RouteValidationResolver to the validation function

* Add functional tests

* Fix new functional tests

* Fix validator additional test

* Fix test with new resolver

* Remove unused import

* Rename ValidationResolver to ValidationResultFactory and change the interface to look more like the KibanaResponseFactory

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@joshdover joshdover added v7.6.0 and removed v7.7.0 labels Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple request validation from @kbn/config-schema
10 participants