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

feat(catalog-client): Support generating a front end client based on an OpenAPI spec #17470

Merged
merged 9 commits into from
Nov 28, 2023

Conversation

sennyeya
Copy link
Contributor

@sennyeya sennyeya commented Apr 20, 2023

Hey, I just made a Pull Request!

Summary

Another step for #2566. There's a lot of code with this PR, the basic idea is to support using openapi-generator with some overrides to create a "closer to the spec" API client.

What's in this PR?

The primary work is in the packages/repo-tools/templates which is a set of overrides to the Mustache templates that openapi-generator's typescript generator uses under the hood. There are some unfortunate requirements there, namely the .model.ts and .client.ts. My goal here was to reuse the upstream template as much as possible.

I also adjusted the current CatalogClient to use the generated code under the hood instead of creating URLs and requests. The packages/catalog-client/src/generated folder is from the custom generator.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@sennyeya sennyeya requested review from a team and backstage-service as code owners April 20, 2023 15:35
@sennyeya sennyeya requested a review from freben April 20, 2023 15:35
@github-actions github-actions bot added the area:catalog Related to the Catalog Project Area label Apr 20, 2023
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Apr 20, 2023

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/catalog-client packages/catalog-client minor v1.4.6
@backstage/cli packages/cli minor v0.24.1-next.0
@backstage/repo-tools packages/repo-tools minor v0.4.0
@backstage/plugin-catalog-backend plugins/catalog-backend patch v1.15.1-next.0

@sennyeya
Copy link
Contributor Author

@freben @taras I was able to get a custom generator set up from openapi-generator that resembles most of the CatalogClient today. It's in Java and a lot of the code is based off of TypescriptClientCodegen.java. I can take a look at openapi-generator-plus but openapi-generator is definitely more standard and has support for more cases like server stubs and clients down the line.

That being said, I have 2 asks,

  1. Can you take a look at the output types (in catalog-client/src/generated/models/*) and the output API (in catalog-client/src/generated/api/DefaultApi.ts) and see if they're helpful?
  2. I pulled in the existing serialization code from their Typescript generator, which makes a lot of the methods much more verbose, does this make sense to add? It feels like it adds a fair amount of safety to our API calls that we didn't have before.

@taras
Copy link
Member

taras commented Apr 20, 2023

@sennyeya, awesome work on this. Thank you for pushing it forward.

Can you take a look at the output types (in catalog-client/src/generated/models/*) and the output API (in catalog-client/src/generated/api/DefaultApi.ts) and see if they're helpful?

generated/api/DefaultAPI.ts has Promise<Response> as return for all methods. This was a problem that I saw previously with openapi-generator. We need the return to be typed, but it's not (at least from what I can see).

catalog-client/src/generated/models/* is very java-ish. It generates a serializer class for something that could be a type. It doesn't use that class to type the response.

I pulled in the existing serialization code from their Typescript generator, which makes a lot of the methods much more verbose, does this make sense to add?

I'm not sure, maybe @freben has a better idea.

I can take a look at openapi-generator-plus but openapi-generator is definitely more standard and has support for more cases like server stubs and clients down the line.

Just a heads up, I did look at them previously. One of the challenge with those libraries is that their maintainers don't have much time. I opened a few issues a year ago karlvr/openapi-generator-plus-generators#32 and karlvr/openapi-generator-plus-generators#33. We could add eslint ignore, but just a heads up.

@sennyeya
Copy link
Contributor Author

@taras Thanks!

generated/api/DefaultAPI.ts has Promise as return for all methods. This was a problem that I saw previously with openapi-generator. We need the return to be typed, but it's not (at least from what I can see).

This was something that I was trying to hit an in between state with, the CatalogClient currently handles different endpoints separately (see the requestRequired, requestOptional and requestIgnored internal methods). We know the return types in the template but have to decide how to parse them and if we want to handle errors (ideally the errors are pushed to the caller and they handle them).

catalog-client/src/generated/models/* is very java-ish. It generates a serializer class for something that could be a type. It doesn't use that class to type the response.

Similar to above, I kept the serializer class from their implementation as an easier bridge from their typescript client, we can definitely remove (see template-openapi-client-plugin/generator/src/main/resources/typescript-backstage/model.mustache, but figured it would be useful for correctly serializing FormData/bodies.

Just a heads up, I did look at them previously. One of the challenge with those libraries is that their maintainers don't have much time. I opened a few issues a year ago karlvr/openapi-generator-plus-generators#32 and karlvr/openapi-generator-plus-generators#33. We could add eslint ignore, but just a heads up.

Yeah, that was one of my concerns, that library seems to have just 1 or 2 maintainers vs openapi-generator which has quite a few and a wider range of templates. My main issue with it is we have to write Java to customize the template if we want to maintain a custom generator (which I think we should). Not sure where this project sits on adding Java code, which is probably a question for @freben.

@sennyeya sennyeya marked this pull request as draft April 20, 2023 16:34
@sennyeya sennyeya mentioned this pull request Apr 21, 2023
20 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2023

Uffizzi Ephemeral Environment deployment-39136

☁️ https://app.uffizzi.com/github.com/backstage/backstage/pull/17470

📄 View Application Logs etc.

What is Uffizzi? Learn more!

@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2023

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label May 1, 2023
@freben freben removed the stale label May 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label May 9, 2023
@freben freben removed the stale label May 9, 2023
@sennyeya sennyeya marked this pull request as ready for review May 9, 2023 13:12
@sennyeya sennyeya requested a review from a team as a code owner May 9, 2023 13:12
@sennyeya sennyeya requested a review from nuritizra May 9, 2023 13:12
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label May 16, 2023
@sennyeya sennyeya added the will-fix We will fix this at some point label May 16, 2023
@freben freben removed the stale label May 16, 2023
@sennyeya
Copy link
Contributor Author

Did a little more investigation on the generated types and support, the openapi-generator typescript generator doesn't have support for oneOf or allOf, or additionalProperties: true. That's causing an issue from the lint step where the Entity from @backstage/catalog-models and the autogenerated Entity from the spec don't match. (There's work in the future to combine the two of those). Basically, because openapi-generator doesn't create a [key:string]: any; in addition to known properties on the interface, Typscript can't merge the types and we fail validation.

I looked into autorest to see if it handles those cases better and it handles them as expected. I'm not at all versed in ts-morph, but will start taking a look this week as openapi-generator probably isn't the direction anymore.

@freben
Copy link
Member

freben commented Oct 26, 2023

Sorry I haven't reviewed yet - but the changesets are missing!

@sennyeya
Copy link
Contributor Author

@freben Added!

@github-actions github-actions bot added the stale label Nov 2, 2023
@freben freben removed the stale label Nov 2, 2023
@backstage backstage deleted a comment from github-actions bot Nov 7, 2023
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Nice 😎

Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to have this injected or runtime or smth like that instead? Be nice to not have any new files in the repo root

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to remove this one, however there will probably need to be one in packages/repo-tools. It could be injected at runtime, but it serves as a version lock file that can hold additional config (we're not using that yet, but could in future).

package.json Outdated Show resolved Hide resolved
packages/catalog-client/openapitools.json Outdated Show resolved Hide resolved
packages/catalog-client/package.json Outdated Show resolved Hide resolved
packages/catalog-client/package.json Outdated Show resolved Hide resolved
packages/repo-tools/openapitools.json Outdated Show resolved Hide resolved
packages/repo-tools/src/commands/index.ts Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Nov 10, 2023

Uffizzi Ephemeral Environment - Virtual Cluster

Your cluster pr-17470 was successfully created. Learn more about Uffizzi virtual clusters
To connect to this cluster, follow these steps:

  1. Download and install the Uffizzi CLI from https://docs.uffizzi.com/install
  2. Login to Uffizzi, then select the backstage account and project:
uffizzi login
Select an account: 
  ‣ backstage
    jdoe

Select a project or create a new project: 
  ‣ backstage-6783521
  1. Update your kubeconfig: uffizzi cluster update-kubeconfig pr-17470 --kubeconfig=[PATH_TO_KUBECONFIG]
    After updating your kubeconfig, you can manage your cluster with kubectl, kustomize, helm, and other tools that use kubeconfig files: kubectl get namespace --kubeconfig [PATH_TO_KUBECONFIG]

Access the backstage endpoint at https://backstage-default-pr-17470-c1838.uclusters.app.uffizzi.com

Signed-off-by: Aramis Sennyey <sennyeya@amazon.com>
Signed-off-by: Aramis Sennyey <sennyeyaramis@gmail.com>
Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
@sennyeya sennyeya requested a review from Rugvip November 20, 2023 15:03
Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
Copy link
Contributor

github-actions bot commented Nov 22, 2023

Uffizzi Cluster pr-17470 was deleted.

Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
Signed-off-by: Aramis Sennyey <aramiss@spotify.com>
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

NIce! 😁

I think we're good to ship this and keep iterating. In particular figuring out the error responses, I agree defaulting to @backstage/errors would be nice, as well as cleaning up the lint rules and avoid the extra dep.

: {};
const response = await this.fetchApi.fetch(url, { method, headers });

private async requestIgnored(response: Response): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

I think we could tweak the naming of these now that they're not executing the request and just handling the response. Feeling we can leave that for a followup though

Comment on lines +173 to +174
'unused-imports/no-unused-imports': 'error',
'unused-imports/no-unused-vars': [
Copy link
Member

Choose a reason for hiding this comment

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

Hold up, why are these added here? @typescript-eslint is what we use for unused vars. In general I feel there's really no point in enabling any additional rules for generated code?

Copy link
Contributor Author

@sennyeya sennyeya Nov 28, 2023

Choose a reason for hiding this comment

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

Ah, yeah let me take another look at this. This is the library I've used in the past. Without this, we're getting a tsc error for unused variables if we keep this as is. The @typescript-eslint rule adds the check, but the unused-imports plugin adds an autofix rule, unless something changed in @typescript-eslint recently?

Copy link
Member

Choose a reason for hiding this comment

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

Ah so it trims away things to make tsc work? Makes sense then, let's :shipit:

@sennyeya sennyeya merged commit ce3f7d4 into backstage:master Nov 28, 2023
4 checks passed
Copy link
Contributor

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.21.0 release, scheduled for Tue, 19 Dec 2023.

@taras
Copy link
Member

taras commented Nov 28, 2023

Amazing work @sennyeya thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:catalog Related to the Catalog Project Area area:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants