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

[kbn/server-route-repository] Add support for defining response validations via Zod #205486

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

awahab07
Copy link
Contributor

@awahab07 awahab07 commented Jan 3, 2025

Closes #198680

Summary

The PR adds responses route config property to CreateServerRouteFactory so that consumers can define response schemas while configuring routes. The schema is then used to generate OAS docs with response types as well as making route handler return payload type safe.

Testing

Consult #187562 to generate OAS for a particular route during dev.

Example route configuration:

const testOasGeneration = createObservabilityOnboardingServerRoute({
  endpoint: 'GET /api/test_os_generation',
  options: {
    tags: [],
    access: 'public',
  },
  params: z.object({
    query: z.object({
      start: z.string().optional().describe('Start index'),
    }),
  }),
  responses: {
    200: {
      description: 'Success response',
      body: z.object({
        changed: z.literal(true),
        data: z.array(z.object({ id: z.number() })),
      }),
      bodyContentType: 'application/json',
    },
    204: {
      description: 'No content',
      body: z.object({
        changed: z.literal(false),
      }),
      bodyContentType: 'application/json',
    },
  },
  async handler(resources) {
    const start = resources.params.query?.start;

    if (start && isNaN(parseInt(start, 10))) {
      throw Boom.badRequest('Invalid start index');
    }

    if (start) {
      return {
        changed: true as const,
        data: [{ id: 1 }, { id: 2 }],
      };
    } else {
      return { changed: false as const };
    }
  },
});

Type inference in client when using createRepositoryClient

    client
      .fetch('GET /api/test_os_generation', {
        params: {
          query: {
            start: '123',
          },
        },
      })
      .then((response) => {
        if (response.changed) {
          response.data.forEach((item) => {
            console.log(item);
          });
        } else {
          console.info('No content');
        }
      });

Generates OAS:

{
  "paths": {
    "/api/test_os_generation": {
      "get": {
        "summary": "",
        "tags": [],
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "anyOf": [
                  {
                    "type": "object",
                    "properties": {},
                    "additionalProperties": false
                  },
                  {
                    "enum": [
                      "null"
                    ],
                    "nullable": true
                  },
                  {
                    "not": {}
                  }
                ]
              }
            }
          }
        },
        "responses": {
          "200": {
            "description": "Success response",
            "content": {
              "application/json": {
                "schema": {
                  "type": "object",
                  "properties": {
                    "changed": {
                      "type": "boolean",
                      "enum": [
                        true
                      ]
                    },
                    "data": {
                      "type": "array",
                      "items": {
                        "type": "object",
                        "properties": {
                          "id": {
                            "type": "number"
                          }
                        },
                        "required": [
                          "id"
                        ],
                        "additionalProperties": false
                      }
                    }
                  },
                  "required": [
                    "changed",
                    "data"
                  ],
                  "additionalProperties": false
                }
              }
            }
          },
          "204": {
            "description": "No content",
            "content": {
              "application/json": {
                "schema": {
                  "type": "object",
                  "properties": {
                    "changed": {
                      "type": "boolean",
                      "enum": [
                        false
                      ]
                    }
                  },
                  "required": [
                    "changed"
                  ],
                  "additionalProperties": false
                }
              }
            }
          }
        },
        "parameters": [
          {
            "name": "start",
            "in": "query",
            "required": false,
            "schema": {
              "anyOf": [
                {
                  "not": {}
                },
                {
                  "type": "string"
                }
              ]
            },
            "description": "Start index"
          }
        ],
        "operationId": "get-test-os-generation"
      }
    }
  }
}

@awahab07 awahab07 added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Jan 15, 2025
@awahab07 awahab07 marked this pull request as ready for review January 15, 2025 10:44
@awahab07 awahab07 requested review from a team as code owners January 15, 2025 10:44
@awahab07 awahab07 changed the title [kbn/server-route-repository] Add support for defining responses validation via Zod schemas [kbn/server-route-repository] Add support for defining response validations via Zod Jan 15, 2025
@miltonhultgren
Copy link
Contributor

Some comments before I jump into the code:
Rather than responseValidation, let’s just call it responses?

From the example, I don’t understand how this inference connects with the response validation?
The existing behaviour is that any plain object return results in a 200 OK, does your PR change this to instead return a 400 in your example?
If so, how does that work with other 4xx or 5xx codes? In main, you need to use the KibanaResponseFactory to return other codes which I would then expect should align with the various validation schemas.

Do we try to infer 4xx/5xx in the client? If so, I’m not sure if that is a good idea since the Core HTTP client will throw for those codes so we will lose anything that we infer anyway.

} & Omit<VersionedRouteResponseValidation[number], 'body'>;
}

export type TRouteResponse = TRouteResponseStatusCodes & {
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 understand what this type achieves, can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a result of incrementally trying to get the union of all configured response body's. Without this TypeScript would short circuit to only the top response type, forcing the handler to always return the top response type.

Now that I go through it again, I've simplified the type into:

export type TRouteResponse = {
  [statusCode: number]: {
    body: z.ZodSchema<ServerRouteHandlerReturnType>;
  } & Omit<VersionedRouteResponseValidation[number], 'body'>;
} & Omit<VersionedRouteResponseValidation, number>;

export type ExtractResponseStatusBodyTypes<T extends TRouteResponse> = z.infer<
  T[Extract<keyof T, number>]['body']
>;

});
}

return { success: false as const, error: 'Example error' };
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is no code changes done in register routes, this will result in a HTTP 200 OK, so I don't think the 400 response schema will be used, what we should do here is return a 400 using the response factory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes using 400 here was incorrect. It was more like different shapes of 2xx responses. I've updated the test.

Comment on lines 482 to 485
assertType<{
success: false;
error: string;
}>(res);
Copy link
Contributor

Choose a reason for hiding this comment

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

This likely passes because indeed the type can be inferred to this but I don't believe the response validation for the 400 is running and since we're not actually returning a 400, this ends up in then while a real 400 should end up in a rejection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it shouldn't be 400. I've updated the test to use 2xx as currently error responses aren't unwrapped as with .ok response.

@awahab07
Copy link
Contributor Author

Rather than responseValidation, let’s just call it responses?

Done. I too was feeling it needs better naming.

From the example, I don’t understand how this inference connects with the response validation?
The existing behaviour is that any plain object return results in a 200 OK, does your PR change this to instead return a 400 in your example?

Using 400 in the example was misleading. It was more like being able to define multiple success response types e.g. for 200, 201 and other 2xx. I've updated the example.

If so, how does that work with other 4xx or 5xx codes? In main, you need to use the KibanaResponseFactory to return other codes which I would then expect should align with the various validation schemas.

That's right. Trying to type the errors requires to change types in the core and is likely to invalidate types of many consumer plugins. For now, there's no typing for error responses and it is assumed the handlers will throw any 4xx or 5xx (e.g. throw Boom.badRequest), bypassing the validation.

Do we try to infer 4xx/5xx in the client? If so, I’m not sure if that is a good idea since the Core HTTP client will throw for those codes so we will lose anything that we infer anyway.

No, the typings currently do not cater for error responses or thrown payload. Doing se needs some work and the types in KibanaErrorResponseFactory need adjustments.

@miltonhultgren
Copy link
Contributor

@awahab07 Can you just confirm for me, what happens if a handler does return kibanaResponseFactory.notFound()?
Does it run the schema validation for the 404 status code and what does the return type of the handler infer to?

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

Can we first try to get a better understanding of the impact of zod on TS responsiveness? And ideally we can make sure that the Zod types are only checked against the return type of the handler, and not stored in the inferred type in the Repository. type, as this will cause TypeScript to re-parse the zod schemas every time there is a change in any file that uses the Repository type

@awahab07
Copy link
Contributor Author

awahab07 commented Jan 27, 2025

Can you just confirm for me, what happens if a handler does return kibanaResponseFactory.notFound()? Does it run the schema validation for the 404 status code and what does the return type of the handler infer to?

@miltonhultgren
kibanaResponseFactory.notFound() resolves to IKibanaResponse (IKibanaResponse<any>) so the handler does not validate it. And in this case the inferred type of the handler includes IKibanaResponse.

If explicit error type is asserted for IKibanaResponse:

      return kibanaResponseFactory.notFound({
        body: { message: 'Not found' },
      }) as IKibanaResponse<{ message: string }>;

then the handler requires the type { message: string } to be included in any of the response status codes (it doesn't narrow the response types per status code).

@awahab07
Copy link
Contributor Author

zod vs io-ts performance.

I ran TS with tracing on and compared the runtime performance (by invoking type checking and auto completion in VSCode), against an example route. For each case, the route imported its params from io-ts and zod defined schema with four levels of nesting including a mix of object and array types.

Here's the summary of executed commands by TS with their average times:

io-ts zod
commandavg. Time elapsed (ms)countavg. Time elapsed (ms)count
completionEntryDetails 72.9786723.31296
completionInfo 298.40306256.11054
configure 0.193310.19861
documentHighlights 0.556210.50331
getApplicableRefactors 0.8880171.22169
getCodeFixes 13.8945521.15294
updateOpen 311.928117338.51916

@awahab07
Copy link
Contributor Author

And ideally we can make sure that the Zod types are only checked against the return type of the handler, and not stored in the inferred type in the Repository. type, as this will cause TypeScript to re-parse the zod schemas every time there is a change in any file that uses the Repository type

@dgieselaar I used a similar process as above to try to determine that. I imported route's params and response types from a separate zod file zod_types.ts, and changed another file where the repository type was imported. I tried to query repository's routes and other props. Unless I explicitly invoked the autocomplete for the handler for that route, zod_types.ts wasn't included in traces for any of the above commands.

@dgieselaar
Copy link
Member

@awahab07 thank you! Two things:

  • can you share the code for both types?
  • i wasnt particularly concerned with io-ts vs zod in this case (although this is useful and i would have expected io-ts to be faster just based on my own experience, so thanks!), but rather explicit types (ie, hand written) versus inferred types (from code using zod schemas). Unfortunately this is needed to keep the route repository types fast as it ends up in a very big type that needs to be re-evaluated every time there is a change

@awahab07
Copy link
Contributor Author

@dgieselaar you can see the zod schema used in PR. io-ts schemas are defined alongside.

For re-evaluation issue, and given this test here, do you have a suggestion to how to confirm that? Are we interested in evaluating zod explicit types vs. zod inferred types performance?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/server-route-repository-utils 28 30 +2

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/server-route-repository-utils 2 3 +1
Unknown metric groups

API count

id before after diff
@kbn/server-route-repository-utils 28 30 +2

History

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[kbn/server-route-repository] Add support for defining Zod schemas for responses
4 participants