Query string validation is limited

[azasypkin]

The query part of the Hapi request is always an object. It's just doesn't have any keys when no query string parameters are defined. Because of that it's not possible to define schema like this:

router.delete(
  {
    path,
    validate: {
      query: schema.maybe(
        schema.object({ required: schema.string(), optional: schema.maybe(schema.string()) })
      ),
    },
  },
  async () => {}
);

The proper and inferred type of the query is { required: string; optional?: string} | undefined while in reality it's treated as { required: string; optional?: string} since query is always an object. To overcome this issue we have to do a workaround like this:

router.delete(
  {
    path,
    validate: {
      query: schema.maybe(
        schema.object(
          { required: schema.maybe(schema.string()), optional: schema.maybe(schema.string()) },
          {
            validate(value) {
              if (typeof value?.required !== 'string' && Object.keys(value).length > 0) {
                return `[request query.required]: expected value of type [string] but got [${typeDetect(
                  value?.required
                )}]`;
              }
            },
          }
        )
      ),
    },
  },
  async () => {}
);

The use case we're trying to cover is that the API caller can either omit query string parameters completely or they always have to specify required parameter in addition to any number of optional ones.

/cc @restrry

STATUS

PR: #96025

Waiting on (non exhaustive atm)

• ML: [ML] Fix route handlers type violations #96034

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[pgayvallet]

Yea, I already encountered that one, and agree that it can be highly misleading.

HAPI has always been taking some 'interesting' opinionated decision, and the fact that the parsed query is always an object even when empty is one of them (which is very pragmatic when using the 'base' HAPI api imho, as it avoid null-check when accessing the query's properties)

We probably can't change HAPI's behavior here. We could adapt our wrapper to check for 'empty' objects and convert them to undefined, but I'm afraid of what we may be breaking in existing route handlers by doing so.

Also note that a workaround a little more elegant than what you did could be:

query: schema.oneOf([
    schema.object({}),
    schema.object({ required: schema.string(), optional: schema.maybe(schema.string()) })
]),

Still not ideal, but way more maintainable when multiple require properties are present.

[azasypkin]

but I'm afraid of what we may be breaking in existing route handlers by doing so.

Yeah, that's a fair point. But at the same time I see a number of query: schema.maybe(....) in master that can potentially lead to more runtime bugs due to discrepancy between TS type we get (assumes undefined for query) and the real value (can never be undefined). The risk is definitely very low, but it's still non-zero.

Still not ideal, but way more maintainable when multiple require properties are present.

Thanks! Although it looks nicer "on paper" it's a bit less usable in practice since the resulting type would require some sort of type-guard (e.g. if ('required' in request.query) {....}) and error message is a way too confusing for a public API (hopefully we can improve it in #84264) 🙂

expect(() => querySchema.validate({ optional: 'basic1' }))
  .toThrowErrorMatchingInlineSnapshot(`
  "types that failed validation:
  - [0.optional]: definition for this key is missing
  - [1.required]: expected value of type [string] but got [undefined]"
`);

[joshdover]

We probably can't change HAPI's behavior here. We could adapt our wrapper to check for 'empty' objects and convert them to undefined, but I'm afraid of what we may be breaking in existing route handlers by doing so.

We probably would break some things, but I think we need to be comfortable making this sorts of changes or else the Core API will just turn into a ball of gotchas. It also helps us uncover who isn't testing their code correctly 😄

[pgayvallet]

We probably would break some things, but I think we need to be comfortable making this sorts of changes

I'm fine doing it, but I'm still afraid of the impact on routes with only optional parameters tbh.

  router.get(
    {
      path: '/foo',
      validate: {
        query: schema.object({
          param: schema.maybe(schema.string()),
          param2: schema.maybe(schema.string()),
        }),
      },
    },
    (ctx, req, res) => {
      const { param, param2 } = req.query; // wget /foo => what is the value of `req.query` ? `{}` or `undefined`?
    }
  );

Need some real testing, but I think this example may fail with the suggested changes?

Not sure what this snippet actually do:

schema.object({
    param: schema.maybe(schema.string()),
   param2: schema.maybe(schema.string()),
}).validate(undefined)

Does that return undefined or {} (or even fails validation)?

EDIT:

test('foo', () => {
  expect(
    schema
      .object({
        param: schema.maybe(schema.string()),
        param2: schema.maybe(schema.string()),
      })
      .validate(undefined)
  ).toEqual({});
});

test('foo 2', () => {
  expect(
    schema
      .maybe(
        schema.object({
          param: schema.maybe(schema.string()),
          param2: schema.maybe(schema.string()),
        })
      )
      .validate(undefined)
  ).toEqual(undefined);
});

both tests are passing, so I guess we're good to go.

[pgayvallet]

So, I opened #96025, but as expected, this has some serious effects on the existing codebase, and breaks quite a lot of functional tests (I won't even speak of the risk of non-covered code and features)

e.g x-pack/plugins/ml/server/routes/job_service.ts (special mention to the fullLicenseAPIGuard HOF loosing types on the request's generics) cc @elastic/ml-ui

Are we still alright with the bold statement?

We probably would break some things, but I think we need to be comfortable making this sorts of changes or else the Core API will just turn into a ball of gotchas

Should I proceed and try to adapt the codebase to handle this new behavior?

[pgayvallet]

Going to do that iteratively I guess. Created #96034 for starter.

[pgayvallet]

I'm going to close this, the level of effort isn't just worth it.