[UI] Use the official OpenAPI specification to generate types. by ognis1205 · Pull Request #762 · unitycatalog/unitycatalog

ognis1205 · 2024-11-26T18:04:01Z

PR Checklist

A description of the changes is added to the description of this PR.
If there is a related issue, make sure it is linked to this PR.
If you've fixed a bug or added code that should be tested, add tests!
If you've added or modified a feature, documentation in docs is updated

Description of changes

Use openapi-typescript to generate types (URL paths, HTTP methods, requests, and responses)
Rationale
- The primary need for code generated from the OpenAPI schema is type information rather than client implementations. This allows for flexibility in selecting an HTTP client and supports gradual adaptation even when the schema YAML implementation is unstable.
- openapi-generator requires a Java runtime, while openapi-typescript does not depend on Java, node-gyp, or an API server.
- Even if the UI project becomes independent of the unitycatalog repository, it can still generate code from a remote schema without requiring a Java runtime.
Implement utility types and functions to handle generated artifacts type-safely
Install npm-run-all and enhance the build script

Will close #431 .

Signed-off-by: Shingo OKAWA <[email protected]>

ognis1205 · 2024-11-28T15:01:12Z

@jamieknight-db @yc-shawn
I have tested all CRUD operations and the login functionality (Google OAuth) in the local environment. Please review.

Signed-off-by: Shingo OKAWA <[email protected]>

ui/src/components/models/ModelVersionStatusDisplay.tsx

ui/src/context/control.ts

ui/src/context/catalog.ts

ui/src/hooks/functions.tsx

ui/src/hooks/models.tsx

ui/src/hooks/schemas.tsx

ui/src/hooks/tables.tsx

ui/src/hooks/user.tsx

ui/src/hooks/volumes.tsx

ui/src/pages/CatalogDetails.tsx

jamieknight-db · 2024-12-04T22:57:17Z

@ognis1205 this is really fantastic! Thank you for taking the lead on this. I have left some comments and suggestions. I still need to clone your branch and test it out, I will try to get that done tomorrow. Were you able to test out error states as well?

… union, and modify the arguments of the "route" function to maintain conventional notification messages. Signed-off-by: Shingo OKAWA <[email protected]>

ognis1205 · 2024-12-05T14:20:53Z

~~@jamieknight-db~~

Regarding the parameters in the hooks module (e.g., max_results, page_token, new_name), all of these are destructured parameters and can therefore be optional (i.e., they can be undefined, allowing users to omit them) as defined in the OpenAPI specification. I included these parameters for clarification purposes, not as strict requirements. I was concerned that removing them might make the specification less clear. However, since this is largely a matter of personal preference, I'll leave the final decision to you as the code owner. What are your thoughts?

ognis1205 · 2024-12-05T14:36:37Z

@ognis1205 this is really fantastic! Thank you for taking the lead on this. I have left some comments and suggestions. I still need to clone your branch and test it out, I will try to get that done tomorrow. Were you able to test out error states as well?

The current route function in the utils/openapi module is designed so that no processing is performed for error codes unless the expected error codes are explicitly specified in the hooks implementation and the OpenAPI specification (The current specification does not define error codes at all). As such, I am not conducting tests for specific error codes. However, in the event of a server error, the implementation ensures that the same notifications as the existing code are displayed. I have performed minimal checks, such as intentionally stopping the server to confirm that notifications are displayed. Regarding this, I plan to conduct thorough testing after resolving the following two issues:

Signed-off-by: Shingo OKAWA <[email protected]>

ognis1205 · 2024-12-09T01:55:37Z

~~Regarding the issue of instantiating two Axios clients, there are two main reasons:~~

- At the time I started implementing this PR, there were parts of the Catalog API client that utilized interceptors. Since these interceptors handled processes unnecessary for the Auth API client, a separate instance was deemed appropriate.

- Axios client instances are typically used to reuse common configurations. However, I couldn’t find any code where the configurations were shared between the Catalog and Auth clients. If the process involves overwriting the Catalog client’s base URL with the Auth client’s base URL for each request, I decided it would be better to create a separate client to enhance the symmetry between the code for the two clients.

ognis1205 · 2024-12-09T03:52:19Z

@jamieknight-db

Thank you for the detailed and thorough review. I have fixed the points that were straightforward and didn’t require further discussion. I’ve also left some comments, so when you have time, I’d appreciate it if you could review them again.

Signed-off-by: Shingo OKAWA <[email protected]>

…de to infer types. Signed-off-by: Shingo OKAWA <[email protected]>

Signed-off-by: Shingo OKAWA <[email protected]>

…nt.ts. Signed-off-by: Shingo OKAWA <[email protected]>

ognis1205 · 2024-12-18T06:32:11Z

@jamieknight-db

Consolidated /context/catalog.ts and /context/control.ts back into /context/client.ts, as they were previously.
Removed unused arguments from /hooks.
For error handling, I ensured it is handled at the type level, guaranteeing at compile-time that if (isError(response)) { ... } will not be executed. This preserves the same behavior as the existing implementation. One exception to note is the need to specifically handle the 401 error for useGetCurrentUser in /hooks/user.ts, but for all other cases, the behavior and error messages remain unchanged.

I’d appreciate it if you could review them again when you have time. Thanks!

Signed-off-by: Shingo OKAWA <[email protected]>

jamieknight-db

This all lgtm - I was not able to pull down the branch to test out CRUD operations myself but code looks good. Thank you for considering my feedback @ognis1205! I would like @yc-shawn and @dennyglee to take a look before merging.

ognis1205 · 2024-12-23T20:32:38Z

@jamieknight-db

Thank you for your thorough review. Please feel free to let me know if there are any issues or if anything needs to be addressed.

Signed-off-by: Shingo OKAWA <[email protected]>

ognis1205 · 2025-01-15T07:30:11Z

@yc-shawn @dennyglee

Just wanted to kindly follow up on this PR to see if you’ve had a chance to take another look. Let me know if there’s anything I can clarify or adjust to help with the review. Thank you!

Signed-off-by: Shingo OKAWA <[email protected]>

jamieknight-db · 2025-01-17T19:23:04Z

@ognis1205 does this PR need to be merged first? before this PR?

ognis1205 · 2025-01-19T01:30:08Z

@jamieknight-db

This PR and that PR are not dependent on each other, but since this PR has relatively fewer impacts and stakeholders, it might be better to merge this one first and wait a bit longer for feedback on the other PR. Thank you for the suggestion!

jamieknight-db · 2025-01-21T17:47:47Z

@dennyglee are you ok with me merging this?

dennyglee · 2025-01-21T18:28:40Z

@dennyglee are you ok with me merging this?

Absolutely, feel free to merge and/or I can take care of this later today, eh?! :)

…catalog#762) **PR Checklist** - [x] A description of the changes is added to the description of this PR. - [x] If there is a related issue, make sure it is linked to this PR. - [x] If you've fixed a bug or added code that should be tested, add tests! - [x] If you've added or modified a feature, documentation in `docs` is updated **Description of changes** - Use [openapi-typescript](https://openapi-ts.dev/) to generate types (URL paths, HTTP methods, requests, and responses) ***Rationale*** - The primary need for code generated from the OpenAPI schema is type information rather than client implementations. This allows for flexibility in selecting an HTTP client and supports gradual adaptation even when the schema YAML implementation is unstable. - openapi-generator requires a Java runtime, while openapi-typescript does not depend on Java, node-gyp, or an API server. - Even if the UI project becomes independent of the unitycatalog repository, it can still generate code from a remote schema without requiring a Java runtime. - Implement utility types and functions to handle generated artifacts type-safely - Install [npm-run-all](https://www.npmjs.com/package/npm-run-all) and enhance the build script Will close unitycatalog#431 . --------- Signed-off-by: Shingo OKAWA <[email protected]> Signed-off-by: Maksym Borodin <[email protected]>

ognis1205 added 4 commits November 27, 2024 01:27

Update catalog-related hooks.

0a42266

Signed-off-by: Shingo OKAWA <[email protected]>

Merge.

967a239

Signed-off-by: Shingo OKAWA <[email protected]>

Update user-related hooks.

bd45eee

Signed-off-by: Shingo OKAWA <[email protected]>

Merge.

f6637c8

Signed-off-by: Shingo OKAWA <[email protected]>

ognis1205 marked this pull request as ready for review November 28, 2024 14:28

ognis1205 requested review from jamieknight-db and yc-shawn as code owners November 28, 2024 14:28

ognis1205 changed the title ~~[WIP][UI] Use the official OpenAPI specification to generate types.~~ [UI] Use the official OpenAPI specification to generate types. Nov 28, 2024

ognis1205 added 3 commits December 4, 2024 14:06

Merge branch 'main' into feat/use-openapi-spec

5a95a3a

Signed-off-by: Shingo OKAWA <[email protected]>

Add MediaType type.

4c9a91e

Signed-off-by: Shingo OKAWA <[email protected]>

Format.

b68b37d

Signed-off-by: Shingo OKAWA <[email protected]>