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(booter-lb3app): process generated OpenApiSpec of lb3 app #3623

Merged
merged 1 commit into from
Sep 10, 2019

Conversation

derdeka
Copy link
Contributor

@derdeka derdeka commented Aug 30, 2019

While migration from lb3 to lb4 I had several issues with the converted openapi spec of my lb3app, e.g.:

  • incomplete return types which results in unuseable angular model interfaces
  • renaming of models in the spec to avoid name collisions between lb3 and lb4 implementations

We solved this by post processing the generated openapi spec before it gets published into lb4.
If you think this is the way to go, we can add some tests.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@raymondfeng
Copy link
Contributor

@derdeka Thank you for the patch!

incomplete return types which results in unusable angular model interfaces
renaming of models in the spec to avoid name collisions between lb3 and lb4 implementations

Do these fixes apply to most use cases? If so, we should fix them in the booter implementation itself.

I'm open to the idea to allow postProcessOpenApiSpec option.

Please add some tests.

@bajtos
Copy link
Member

bajtos commented Sep 2, 2019

Thank you @derdeka for the pull request.

incomplete return types which results in unuseable angular model interfaces

I agree with what Raymond wrote above - it would be best to fix the code that's generating OpenAPI spec, so that no further post-processing is needed. It makes me wonder - are you using Swagger with your LB3 application? Is that swagger spec correct?

  • If yes, then the problem could be in the conversion from Swagger to OpenAPI - see swagger2openapi.
  • If not, then we should fix loopback-swagger to emit correct Swagger spec in the first place!

renaming of models in the spec to avoid name collisions between lb3 and lb4 implementations

I am not sure how easy would be to support this use case at loopback-swagger and/or the booter level. Your proposal to allow applications to contribute a custom post-processing function (handler) looks like a great solution 👍

Can we find a shorter name for processOpenApiSpec option? How about specTransformer or something similar?

@derdeka derdeka force-pushed the derdeka/lb3app-process-spec branch from 3f29d8d to d8e4a84 Compare September 6, 2019 23:39
@derdeka
Copy link
Contributor Author

derdeka commented Sep 6, 2019

I've renamed and added a simple test.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Nice 👍

@raymondfeng I'd like to get your approval too. Could you please land this patch if it looks good to you too?

@@ -128,6 +132,7 @@ export interface Lb3AppBooterOptions {
path: string;
mode: 'fullApp' | 'restRouter';
restApiRoot: string;
specTransformer?: (spec: OpenApiSpec) => OpenApiSpec;
Copy link
Contributor

@raymondfeng raymondfeng Sep 10, 2019

Choose a reason for hiding this comment

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

I'm good to have specTransformer to be synchronous function as a starting point. We should consider generalizing the idea:

  1. Allow asynchronous transformation
  2. Create an extension point for spec transformers/processors to transform OpenAPI specs generated from controller metadata. For example, our authentication/authorization packages may want to add more information to the spec.

Copy link
Member

Choose a reason for hiding this comment

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

Please note this transformer is applied only for LB3 applications imported to LB4.

@bajtos bajtos merged commit 8a51aa1 into loopbackio:master Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants