logto-io / logto

🧑‍🚀 The better identity infrastructure for developers and the open-source alternative to Auth0.
https://logto.io
Mozilla Public License 2.0
8.5k stars 419 forks source link

feat: add `operationId` to HTTP methods on paths #6108

Closed mostafa closed 3 months ago

mostafa commented 3 months ago

Summary

This PR adds operationId to the each HTTP method on each endpoint. As per the specification, "operationId is an optional unique string used to identify an operation. If provided, these IDs must be unique among all operations described in your API". This greatly simplifies the creation of client SDKs in different languages, because it generates more meaningful function names instead of auto-generated ones, like the following examples:

- org, _, err := s.Client.OrganizationsAPI.ApiOrganizationsIdGet(ctx, req.GetId()).Execute()
+ org, _, err := s.Client.OrganizationsAPI.GetOrganization(ctx, req.GetId()).Execute()
- users, _, err := s.Client.OrganizationsAPI.ApiOrganizationsIdUsersGet(ctx, req.GetId()).Execute()
+ users, _, err := s.Client.OrganizationsAPI.ListOrganizationUsers(ctx, req.GetId()).Execute()

Note for the Reviewer

I tried to make a uniform set of names based on the method and path, yet certain paths were duplicated due to their composition. For these cases I opted for custom names in a map and tried to copy the phrase from https://openapi.logto.io. The naming can be improved using a 1:1 mapping, but that's probably not the desired way to do this. I am not aware of other ways to achieve this in Zod or your project, and would be happy to know more if one exists. Also, I wanted to use route names, but they map to anonymous functions.

Testing

Search for operationId in the http://localhost:3002/api/swagger.json.

Checklist

github-actions[bot] commented 3 months ago

COMPARE TO master

Total Size Diff :warning: :chart_with_upwards_trend: +11.21 KB

Diff by File |Name|Diff| |---|---| |.changeset/brown-cobras-know.md|:chart_with_upwards_trend: +950 Bytes| |packages/core/src/routes/swagger/index.test.ts|:chart_with_upwards_trend: +28 Bytes| |packages/core/src/routes/swagger/index.ts|:chart_with_upwards_trend: +558 Bytes| |packages/core/src/routes/swagger/utils/general.ts|:chart_with_upwards_trend: +98 Bytes| |packages/core/src/routes/swagger/utils/operation-id.test.ts|:chart_with_upwards_trend: +1.77 KB| |packages/core/src/routes/swagger/utils/operation-id.ts|:chart_with_upwards_trend: +7.7 KB| |packages/integration-tests/src/api/index.ts|:chart_with_upwards_trend: +16 Bytes| |packages/integration-tests/src/tests/api/swagger-check.test.ts|:chart_with_upwards_trend: +138 Bytes|
gao-sun commented 3 months ago

Thanks for contributing, I'll take a look soon

gao-sun commented 3 months ago

for record: unit tests are 100% covered for buildOperationId()

image
gao-sun commented 3 months ago

@logto-io/eng calling for another review

mostafa commented 3 months ago

@gao-sun I love your changes. 👌 Thank you!