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

Serve swagger ui from embedded resources #69

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

DenisPalnitsky
Copy link
Contributor

I would like to suggest serving Swagger UI from Fizz.
In this PR I'm embedding Swagger UI static files and adjusting index.html to use the Open API specification file name provided by the user.

The only limitation is that it relies on go:embed that is available from 1.16

It will look as follows:
image

If you consider merging it I'll add a couple of tests.

@wI2L
Copy link
Owner

wI2L commented Jan 18, 2022

@DenisPalnitsky This sounds like a great feature, however, I feel like it should be conditional and off by default, especially because this will have an impact on the final binary size.
The only solution I can think of right now would be to use a build tag to include (or not) the embed directive. The code that use the embed.FS variable should be agnostic as to whether the Swagger UI assets are included or not, so it could be part of the public API without be dependent of the build tag.

@DenisPalnitsky
Copy link
Contributor Author

@DenisPalnitsky This sounds like a great feature, however, I feel like it should be conditional and off by default, especially because this will have an impact on the final binary size. The only solution I can think of right now would be to use a build tag to include (or not) the embed directive. The code that use the embed.FS variable should be agnostic as to whether the Swagger UI assets are included or not, so it could be part of the public API without be dependent of the build tag.

@wI2L That's a reasonable concern. We can also move the swagger code to a separate package. In that case, if user needs swagger ui then he imports the package and assets will be added to binary but if the user does not import that package then assets won't be embedded. Check this commit 6f9af58
If you add swagger.AddOpenApiUIHandler(f.Engine(),"swagger-ui", "/openapi.json" ) to the code the binary becomes twice bigger.
I tested that:
image

@wI2L
Copy link
Owner

wI2L commented Jan 19, 2022

@DenisPalnitsky

We can also move the swagger code to a separate package. In that case, if user needs swagger ui then he imports the package and assets will be added to binary but if the user does not import that package then assets won't be embedded. Check this commit 6f9af58

Yes, that's better indeed.
See some of my comments, regarding the public API and the internals.
Also, I was wondering if we could create a bash script that would fetch the latest UI assets from the official Swagger UI project to simplify the update process. Or could we use a git submodule? For the time being, I'm fine with static assets copied in the project tree.

@DenisPalnitsky
Copy link
Contributor Author

DenisPalnitsky commented Jan 21, 2022

I think we should take a fizz.RouterGroup, so a user could pass a fizz instance to represent the "root" of their API, or a fizz.RouterGroup returned by fizz.Group().

Is that what you mean?

g := f.Group("", "This is swaggerUi", "What goes here?")
swagger.AddUIHandler(g,"swagger-ui", "/openapi.json" )

Judging by my use-case, I would not want to be forced to create a group to host UI. Also, if it's part of the group then it will be visible in open-api spec, which is not desirable.

@DenisPalnitsky
Copy link
Contributor Author

Also, I was wondering if we could create a bash script that would fetch the latest UI assets from the official Swagger UI project to simplify the update process. Or could we use a git submodule? For the time being, I'm fine with static assets copied in the project tree.

There is a small catch. Every time we update those assets we need to change index.html to index.gohtml and add "/{{ .openApiJson }}" to it. We need this to be able to specify path to openapi.json in UI. That's why we have to go through all those troubles in fs_wrapper.go
Of cause, we can script all that, but UI does not change that often, so it may not be worth the hassle.

@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #69 (38708b8) into master (aaea309) will decrease coverage by 0.63%.
The diff coverage is 75.75%.

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage   94.91%   94.28%   -0.64%     
==========================================
  Files           7        9       +2     
  Lines         964      997      +33     
==========================================
+ Hits          915      940      +25     
- Misses         33       38       +5     
- Partials       16       19       +3     
Impacted Files Coverage Δ
swagger/swagger.go 60.00% <60.00%> (ø)
swagger/fs_wrapper.go 78.57% <78.57%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaea309...38708b8. Read the comment docs.

@DenisPalnitsky
Copy link
Contributor Author

@wI2L did you have a chance to check my comments?

swagger/fs_wrapper.go Show resolved Hide resolved
swagger/swagger.go Outdated Show resolved Hide resolved
swagger/fs_wrapper.go Outdated Show resolved Hide resolved
swagger/fs_wrapper.go Outdated Show resolved Hide resolved
swagger/fs_wrapper.go Outdated Show resolved Hide resolved
swagger/swagger-ui/index.gohtml Outdated Show resolved Hide resolved
swagger/fs_wrapper.go Outdated Show resolved Hide resolved
@wI2L
Copy link
Owner

wI2L commented Feb 7, 2022

did you have a chance to check my comments?

Sorry it took that long.
I added some comments regarding the code itself.

Judging by my use-case, I would not want to be forced to create a group to host UI. Also, if it's part of the group then it will be visible in open-api spec, which is not desirable.

You're right. However, I stil dislike that the user has to pass the fizz.Engine() result. We could instead provide a fizz method that would be called ServeSwaggerUI(path, specPath string) that would do the same under the hood without exposing the client to the underlying "machinery".

@wI2L
Copy link
Owner

wI2L commented Feb 7, 2022

There is a small catch. Every time we update those assets we need to change index.html to index.gohtml and add "/{{ .openApiJson }}" to it. We need this to be able to specify path to openapi.json in UI. That's why we have to go through all those troubles in fs_wrapper.go
Of cause, we can script all that, but UI does not change that often, so it may not be worth the hassle.

I don't know when or who will update the UI in the future, but since we simply need to download the latest Swagger UI bundle, unpack?, rename a file and replace a field value with a known template variable, I think it's sound easy enough, and might be worth to script it right away and forget about it.

@DenisPalnitsky
Copy link
Contributor Author

You're right. However, I stil dislike that the user has to pass the fizz.Engine() result. We could instead provide a fizz method that would be called ServeSwaggerUI(path, specPath string) that would do the same under the hood without exposing the client to the underlying "machinery".

We had fizz method in the initial version but if the method is in the same package then assets are always part of the resulting binary even if UI is not used
I'm open to suggestions though

@DenisPalnitsky
Copy link
Contributor Author

I don't know when or who will update the UI in the future, but since we simply need to download the latest Swagger UI bundle, unpack?, rename a file and replace a field value with a known template variable, I think it's sound easy enough, and might be worth to script it right away and forget about it.

UI does not change that much so I don't expect that updates will be often. We can stay with the current UI forever. Also, Swagger team can change the format and the script won't work anymore.
I agree that having a script would be preferable but I'm afraid I don't have the capacity for it right now. I added a manual script in form of Readme though :)

@DenisPalnitsky DenisPalnitsky requested a review from wI2L February 12, 2022 18:08
@DenisPalnitsky
Copy link
Contributor Author

@wI2L would you consider merging it?

@ccfish86
Copy link

We can use swaggo/gin-swagger to server the ui for openapi.json

import (
	swaggerfiles "github.com/swaggo/files"
	ginSwagger "github.com/swaggo/gin-swagger"

	"github.com/wI2L/fizz"
	"github.com/wI2L/fizz/openapi"
)

func InitRouter() {
	Router := gin.Default()
....
	Router.GET("/app/swagger/*any", ginSwagger.WrapHandler(swaggerfiles.Handler, func(c *ginSwagger.Config) {
		c.URL = "../openapi.json"
	}, ginSwagger.DefaultModelsExpandDepth(1)))
...
	f := fizz.NewFromEngine(engine)
	infos := &openapi.Info{
		Title:       "APP Api",
		Description: `This is app api server.`,
		Version:     "1.0.0",
	}

	// Create a new route that serve the OpenAPI spec.
	f.GET("/app/openapi.json", nil, fizz.OpenAPI(infos, "json"))

...
}

And it can works;

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants