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?

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

This is some serious typescript we got there!

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 :/

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

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

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.

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?

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

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.

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

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.

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

so any object with validate method is also allowed?

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

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
  );

do you mean this? https://github.com/elastic/kibana/pull/51919/files#diff-51730cbab06a430a940f78fc9e710bd7R290

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

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) => { ... };

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

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?).

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.

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

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.

How about renaming to RouteValidationResultType?

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

My bad! Well spotted!

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

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

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:

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.

Oh! Eagle eye!

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

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.

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).

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 👍

nit: add the @example tag above this line.