Open DenisPalnitsky opened 2 years ago
@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 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 theembed.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 https://github.com/wI2L/fizz/pull/69/commits/6f9af58d5f376d13e3a69753bc297869a72d432f
If you add swagger.AddOpenApiUIHandler(f.Engine(),"swagger-ui", "/openapi.json" )
to the code the binary becomes twice bigger.
I tested that:
@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.
I think we should take a
fizz.RouterGroup
, so a user could pass afizz
instance to represent the "root" of their API, or afizz.RouterGroup
returned byfizz.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.
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.
Merging #69 (38708b8) into master (aaea309) will decrease coverage by
0.63%
. The diff coverage is75.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.
@wI2L did you have a chance to check my comments?
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".
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.
You're right. However, I stil dislike that the user has to pass the
fizz.Engine()
result. We could instead provide afizz
method that would be calledServeSwaggerUI(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
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 :)
@wI2L would you consider merging it?
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;
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:
If you consider merging it I'll add a couple of tests.