Replace go-swagger with huma #33685
Comments
TheFox0x7
added
the
type/proposal
|
It seems to be more user-friendly for Chi routers. |
Gitea isn't really using Chi, it is using a heavily customized router system. Actually we could completely drop Chi by adding some more code since Gitea's "router framework" has been complex enough. |
|
All integrations are just adapters for it so it isn't a big problem... I hope.
Oh. Oh that explains why it's almost like chi but different enough to be more confusing. And why I have trouble wrapping my head around it at times. And why it takes |
I really hope it can be used independently by other projects. |
|
off topic Out of curiosity - why? I find it very confusing compared to basic chi and besides port of the Maybe there's a killer feature which I don't see - hence the question. I'm not even suggesting to swap it, as I know it won't be a good use of anyone's time and won't be accepted. I'm mostly curious what's the big benefit I'm overlooking. |
|
That's really a long history, it can't simply say it's right or wrong. Some key points are like this:
ps: in my mind, |
yes. it is off topic, just ignore my comment. 😉 |
At the moment, there is a blocker reason: many middlewares and handlers have different "ctx" arguments: for example: APIContext, PrivateContext, ArtifactContext, (Web)Context. (ps: some of these contexts were already abused due to various reasons, I left some comments in code when I found them) Golang's generic type system is quite weak, so the router method functions have to accept "any" at the moment (I added some runtime validation during "init" stage in some refactoring PRs to make sure there won't be low-level mistakes) |
|
I meant my comment was off topic for the proposal but your comment got me interested in the subject as I get really confused around that area anyway :)
I'm not a fan either especially since they don't often have a reason to be part of the struct except for less func arguments. I think that More on topic - writing an adapter for gitea router with APIContext is possiblefunc New(r *web.Router, config huma.Config) huma.API {
return huma.NewAPI(config, &teaAdapter{router: r})
}
type teaAdapter struct {
router *web.Router
}
type subApiContext struct {
*context.APIContext
op *huma.Operation
}
// AppendHeader implements huma.Context.
func (s *subApiContext) AppendHeader(name string, value string) {
s.RespHeader().Add(name, value)
}
// BodyReader implements huma.Context.
func (s *subApiContext) BodyReader() io.Reader {
return s.Req.Body
}
// BodyWriter implements huma.Context.
func (s *subApiContext) BodyWriter() io.Writer {
return s.Resp
}
// Context implements huma.Context.
func (s *subApiContext) Context() goctx.Context {
return s
}
// EachHeader implements huma.Context.
func (s *subApiContext) EachHeader(cb func(name string, value string)) {
for name, values := range s.Req.Header {
for _, value := range values {
cb(name, value)
}
}
}
// GetMultipartForm implements huma.Context.
func (s *subApiContext) GetMultipartForm() (*multipart.Form, error) {
err := s.Req.ParseMultipartForm(8 * 1024) //Temp because I know it's there somewhere but it's a draft
return s.Req.MultipartForm, err
}
// Header implements huma.Context.
func (s *subApiContext) Header(name string) string {
return s.Req.Header.Get(name)
}
// Host implements huma.Context.
func (s *subApiContext) Host() string {
return s.Req.Host
}
// Method implements huma.Context.
func (s *subApiContext) Method() string {
return s.Req.Method
}
// Operation implements huma.Context.
func (s *subApiContext) Operation() *huma.Operation {
return s.op
}
// Param implements huma.Context.
func (s *subApiContext) Param(name string) string {
return s.Req.PathValue(name)
}
// Query implements huma.Context.
func (s *subApiContext) Query(name string) string {
return queryparam.Get(s.Req.URL.RawQuery, name)
}
// SetHeader implements huma.Context.
func (s *subApiContext) SetHeader(name string, value string) {
s.Resp.Header().Set(name, value)
}
// SetReadDeadline implements huma.Context.
func (s *subApiContext) SetReadDeadline(deadline time.Time) error {
return huma.SetReadDeadline(s.Resp, deadline)
}
// SetStatus implements huma.Context.
func (s *subApiContext) SetStatus(code int) {
s.Resp.WriteHeader(code)
}
// Status implements huma.Context.
// Subtle: this method shadows the method (APIContext).Status of subApiContext.APIContext.
func (s *subApiContext) Status() int {
return s.WrittenStatus()
}
// TLS implements huma.Context.
func (s *subApiContext) TLS() *tls.ConnectionState {
return s.Req.TLS
}
// URL implements huma.Context.
func (s *subApiContext) URL() url.URL {
return *s.Req.URL
}
// Version implements huma.Context.
func (s *subApiContext) Version() huma.ProtoVersion {
return huma.ProtoVersion{
Proto: s.Req.Proto,
ProtoMajor: s.Req.ProtoMajor,
ProtoMinor: s.Req.ProtoMinor,
}
}
// Handle implements huma.Adapter.
func (t *teaAdapter) Handle(op *huma.Operation, handler func(ctx huma.Context)) {
t.router.Methods(op.Method, op.Path, func(apiCtx *context.APIContext) {
handler(&subApiContext{op: op, APIContext: apiCtx})
})
}
// ServeHTTP implements huma.Adapter.
func (t *teaAdapter) ServeHTTP(w http.ResponseWriter, r *http.Request) {
t.ServeHTTP(w, r)
}
func RoutesV2() *web.Router {
router := web.NewRouter()
router.Use(securityHeaders())
if setting.CORSConfig.Enabled {
router.Use(cors.Handler(cors.Options{
AllowedOrigins: setting.CORSConfig.AllowDomain,
AllowedMethods: setting.CORSConfig.Methods,
AllowCredentials: setting.CORSConfig.AllowCredentials,
AllowedHeaders: append([]string{"Authorization", "X-Gitea-OTP"}, setting.CORSConfig.Headers...),
MaxAge: int(setting.CORSConfig.MaxAge.Seconds()),
}))
}
router.Use(context.APIContexter())
router.Use(apiAuth(buildAuthGroup()))
api := New(router, huma.DefaultConfig("Gitea API", setting.AppVer))
api.OpenAPI().Servers = []*huma.Server{
&huma.Server{URL: setting.AppURL + "api/v1/"},
}
huma.Get(api, "/version", misc.Version)
huma.Get(api, "/signing-key.gpg", misc.SigningKey)
huma.Get(api, "/gitignore/templates", misc.ListGitignoresTemplates)
huma.Get(api, "/gitignore/templates/{name}", misc.GetGitignoreTemplateInfo)
huma.Get(api, "/licenses", misc.ListLicenseTemplates)
huma.Get(api, "/licenses/{name}", misc.GetLicenseTemplateInfo)
huma.Get(api, "/label/templates", misc.ListLabelTemplates)
huma.Get(api, "/label/templates/{name}", misc.GetLabelTemplate)
settingsAPI := huma.NewGroup(api, "/settings")
huma.Get(settingsAPI, "/ui", settings.GetGeneralUISettings)
huma.Get(settingsAPI, "/api", settings.GetGeneralAPISettings)
huma.Get(settingsAPI, "/attachment", settings.GetGeneralAttachmentSettings)
huma.Get(settingsAPI, "/repository", settings.GetGeneralRepoSettings)
return router
} |
I think we shouldn't allow returning an "error" directly. It would be abused a lot (using "err" as user error has been abused a lot in current code base). The reason is that some error contains internal sensitive information, for example: database IP, git filesystem path, internal URL, etc. Golang also has a weak error system, we are not able to use any error directly for end users. For safety, we need to only process the errors we could handle. And
The approach looks pretty good. I am not familiar with huma so I can't comment too much for that part. For the API part, for the Req/Context part:
|
|
Don't look too much into the implementation - it's mainly a test if it could be done fairly simply and it's based on humachi wrapper which had a comment that it can be replaced with that function in go 1.22+. I wasn't looking into it that much and I'm sure there are more issues with it but it works as a demo.
Good question, middlewares in huma have 2 layers, global router level ones (so web.Router middleware) and huma layer ones (with PR I'm running which adds groups and group layer for huma). So some middlewares might need adoption (for example the auth one) func Demo() {
api := New(router, huma.DefaultConfig("Gitea API", setting.AppVer))
huma.Get(api, "/version", misc.Version) // No middleware adding by default, unless with func (o *huma.Operation){} which does have access to middlewares - but I don't think it's a correct approach.
// Alternatively:
operation := Operation{
OperationID: GenerateOperationID(http.MethodGet, "/version" , VersionOutput), //Version output is a wrapper for struct with version - see later example
Summary: GenerateSummary(http.MethodGet, "/version" , VersionOutput),
Method: http.MethodGet,
Path: "/version" ,
Middlewares: Middlewares{func(ctx huma.Context, next func(huma.Context)) {
// Some middleware, which has access to [huma.Context](https://pkg.go.dev/github.com/danielgtaylor/huma/v2#Context)
next(ctx)
}},
}
huma.Register(api, operation, misc.Version)
}Discussion about route specific middlewares
I don't think that's needed. It did start and work as expected so I think it does properly, though rewrite of actual handlers will be required, which is the main pain point. I probably should give a small example of such rewrites so we're on the same page and have something to look at: With a really simple version endpoint import (
"context" //huma uses stdlib context
"code.gitea.io/gitea/modules/setting"
)
type VersionOutput struct {
Body struct {
Version string `json:"version" example:"1.23" doc:"Returns the version of the Gitea application"`
} // Response body contains a json, with a string "version". Example for that string is provided with docs
}
// Version shows the version of the Gitea server
func Version(ctx context.Context, _ *struct{}) (*VersionOutput, error) {
// empty struct because endpoint does not accept input
resp := &VersionOutput{}
resp.Body.Version = setting.AppVer
return resp, nil
}With a bit more complex LicenseTemplateInfo type LicenseTemplateInfoOutput struct {
Body api.LicenseTemplateInfo // struct is already defined so we can reuse it. I haven't added tags to it
}
// Returns information about a gitignore template
func GetLicenseTemplateInfo(ctx context.Context, input *struct {
Name string `path:"name", example:"MIT"`
}) (*LicenseTemplateInfoOutput, error) {
// note that input now has data about where the parameter is (in path) and an example payload (MIT)
name := input.Name // not really needed
text, err := options.License(name)
if err != nil {
locale := ctx.Value("locale").(translation.Locale) // HACK.
return nil, huma.Error404NotFound(locale.TrString("error.not_found"), err) // given what you mentioned about errors it might not be the best idea all the time. I haven't looked into error response that much
}
response := api.LicenseTemplateInfo{
Key: name,
[openapi.json](https://github.com/user-attachments/files/18929640/openapi.json)
Name: name,
URL: fmt.Sprintf("%sapi/v1/licenses/%s", setting.AppURL, url.PathEscape(name)),
Body: string(text),
// This is for combatibilty with the GitHub API. This Text is for some reason added to each License response.
Implementation: "Create a text file (typically named LICENSE or LICENSE.txt) in the root of your source code and copy the text of the license into the file",
}
return &LicenseTemplateInfoOutput{Body: response}, nil
}generated json api (with a few more routes): openapi.json And version endpoint (with a different UI but they are swappable): |
Thank you for the demo. IMO we'd better to start from the most complex (hardest) part, including middlewares. If the most complex (hardest) part could be resolved, then the simple APIs could also be resolved. Vice versa is not the case, we might get stuck when handling the complex APIs. And by the way & FYI, it seems that some APIs were not well-designed, for example this newly found one: #33683 (comment) , |
|
Yeah, some will be more difficult than the rest - I got stuck on middlewares as I don't really understand how they are handled right now. As for that particular endpoint, I guess it's a choice between a custom schema with oneOf in response, or splitting the endpoint into a filtered list and and ref query, second one being breaking but more obvious for end user what will they get in response. |
|
I've tried to port the above mentioned endpoint and well... the experience isn't too great right now. I basically hacked my way up to it (inlining middlewares for a MVP) and here is a "report" of some conclusions. Currently the system doesn't mix well with APIContext and it's middlewaresThe actual handler is repoAssignment middleware and othersAfter more digging in code I finally found out where the repository is created and that system is my main point here. If you take a look at type GitRefsInput struct {
Owner string `path:"username" doc:"owner of the repo"` // Not actually used in handler itself
Repo string `path:"reponame" doc:"name of the repo"` // Not actually used in handler itself
Ref string `path:"*" doc:"part or full name of the ref"`
}I think handling of those should be moved to the handler instead of middleware, especially in API if we're to take full advantage of the autogenerated docs with input structs. While I can pull repo and owner/user (in this particular case) out of middleware - putting if it's a good idea aside - Doer is much harder to decouple (and it should actually be in the middleware I feel). example of adapted middleware - though I haven't tested it yet. func apiAuthv2(api huma.API, authMethod auth.Method) func(ctx huma.Context, next func(huma.Context)) {
return func(ctx huma.Context, next func(huma.Context)) {
apictx := giteahuma.Unwrap(ctx)
user, err := common.AuthShared(apictx.Base, nil, authMethod)
if err != nil {
huma.WriteErr(api, ctx, http.StatusUnauthorized, "token is required", err)
return
}
ctx = huma.WithValue(ctx, "user", user)
next(ctx)
}
}Honestly so far I don't see a lot of benefit of APIContext here except for locale and cache, both of which can exist detached from it - though I generally not a fan of this so I'm biased and I might still be missing something. endpoint port demotype GitRefsInput struct {
Owner string `path:"username" doc:"owner of the repo"`
Repo string `path:"reponame" doc:"name of the repo"`
Ref string `path:"*" doc:"part or full name of the ref"`
}
type GitRefsOutput struct {
Body []*api.Reference
}
// GetGitRefs get ref or an filteresd list of refs of a repository
func GetGitRefs(ctx go_context.Context, input *GitRefsInput) (*GitRefsOutput, error) {
// alternative hacky way of preparing it
// apiCtx := ctx.(*context.APIContext)
// repoAssignment()(apiCtx)
// context.ReferencesGitRepo(true)
// port of repoAssignment
owner, err := user_model.GetUserByName(ctx, input.Owner)
if err != nil {
if user_model.IsErrUserNotExist(err) {
if redirectUserID, err := user_model.LookupUserRedirect(ctx, input.Owner); err == nil {
// not ported case
} else if user_model.IsErrUserRedirectNotExist(err) {
return nil, huma.Error404NotFound("GetUserByName", err)
} else {
return nil, huma.Error500InternalServerError("Internal server error", err)
}
} else {
return nil, huma.Error500InternalServerError("Internal server error", err)
}
}
repo := &context.Repository{}
repo.Repository, err = repo_model.GetRepositoryByName(ctx, owner.ID, input.Repo)
if err != nil {
// Skipping more cases
return nil, huma.Error500InternalServerError("Internal server error", err)
}
git.OpenRepository(ctx, repo.Repository.RepoPath())
repo.GitRepo, err = gitrepo.OpenRepository(ctx, repo.Repository)
if err != nil {
return nil, huma.Error500InternalServerError("Internal server error", err)
}
refs, lastMethodName, err := GetGitRefs2(ctx, repo.GitRepo, input.Ref)
if err != nil {
return nil, huma.Error500InternalServerError(lastMethodName, err)
}
if len(refs) == 0 {
return nil, huma.Error404NotFound("Not found (TODO locale)")
}
apiRefs := make([]*api.Reference, len(refs))
for i := range refs {
apiRefs[i] = &api.Reference{
Ref: refs[i].Name,
URL: repo.Repository.APIURL() + "/git/" + util.PathEscapeSegments(refs[i].Name),
Object: &api.GitObject{
SHA: refs[i].Object.String(),
Type: refs[i].Type,
URL: repo.Repository.APIURL() + "/git/" + url.PathEscape(refs[i].Type) + "s/" + url.PathEscape(refs[i].Object.String()),
},
}
}
return &GitRefsOutput{Body: apiRefs}, nil
}
// Only difference is context type and new arg
func GetGitRefs2(ctx go_context.Context, gitrepo *git.Repository, filter string) ([]*git.Reference, string, error) {
if gitrepo == nil {
return nil, "", fmt.Errorf("no open git repo found in context")
}
if len(filter) > 0 {
filter = "refs/" + filter
}
refs, err := gitrepo.GetRefsFiltered(filter)
return refs, "GetRefsFiltered", err
}For a small conclusion - I still think this will bring benefits ultimately. Though it will be way harder to port at places then I thought. I'm open for better ideas though. |
|
To be honest, it doesn't look good to me. It makes the code cumbersome, and "port of repoAssignment" causes unnecessary duplicate code. That's just my opinion, I don't know what others think. |
|
I'm not arguing with that - It doesn't look good at all. Ultimately it would go to a separate function but the underlying issue remains as the alternative isn't much better IMO. func GetGitRefs(ctx go_context.Context, input *GitRefsInput) (*GitRefsOutput, error) {
apiCtx := ctx.(*context.APIContext) // this bothers me
// theoretically in middleware
repoAssignment()(apiCtx)
context.ReferencesGitRepo(true)
// end
refs, lastMethodName, err := GetGitRefs(apiCtx, input.Ref)
if err != nil {
return nil, huma.Error500InternalServerError(lastMethodName, err)
}
if len(refs) == 0 {
return nil, huma.Error404NotFound(apiCtx.Locale("error.404)") // don't remember the exact syntax but you get the picture
}
apiRefs := make([]*api.Reference, len(refs))
for i := range refs {
apiRefs[i] = &api.Reference{
Ref: refs[i].Name,
URL: repo.Repository.APIURL() + "/git/" + util.PathEscapeSegments(refs[i].Name),
Object: &api.GitObject{
SHA: refs[i].Object.String(),
Type: refs[i].Type,
URL: repo.Repository.APIURL() + "/git/" + url.PathEscape(refs[i].Type) + "s/" + url.PathEscape(refs[i].Object.String()),
},
}
}
return &GitRefsOutput{Body: apiRefs}, nil
}Though if memory serves me right as I don't have code at hand right now - repoAssignment is private func for api router so reworking it might work. Auth is more of an issue for me. |
|
Just to chime in with my opinion: I do support migrating to openapi 3.x, ideally 3.1. it's a huge step forward from swagger 2.0. Generating API specs from code instead of comments is also a big improvement as it eliminates a whole class of errors. |
|
Is it necessary to do that at the runtime? Is it possible to use a tool like |
|
No idea how/if validation is done currently, but I imagine having the openapi schema available at runtime will be very beneficial and necessary for any JSON schema-based validator to work. The nice thing about openapi 3.1 is that its schemas are compatible with the JSON schema standard, and there are a lot of validators for JSON schemas available. |
Not with this system to my knowledge.
I don't think this would integrate with gitea better. If needed it should be possible to add a command to setup all routes and fetch it without actually starting the server - but I don't think it's needed for anything besides documentation. I can't see a usecase where admin/user needs openapi file but doesn't want a running server - especially since parts of it can be toggled on and off so schema may vary instance to instance. |
|
After some more tinkering/reading/etc I have some thoughts/conclusions on the idea:
type ExampleOutput struct{
Body any
}which negates main benefit of using huma in the first place - automatic input/output schema generation. It's obviously better to have an API endpoint limited to a single type but it is a limitation that forces manually written schema (at least in golang not yaml so that's an upside) anyway. I see following options on how to proceed further: Rewrite api to not rely on and drop APIContextPros:
Cons:
My main concern here is that (unsurprisingly) I don't see anyone here too happy about this idea so it might be a waste of time. Force integration with current middlewarePros:
Cons:
Drop the migration and validation for now and just use it to generate OpenAPI documentPros:
Cons:
Any other ideas? If we'd like to stay with code comments - swaggo/swag should support openapi 3 spec from v2 version (which is yet to be fully released) I guess the fastest to some meaningful results is use huma just for OpenApi generation (third option) and this option still provides docs for future. |
To be honest, I do not have much time on this task at the moment:
There are too many existing problems and pending PRs -- some of them have much higher priority to me. Somewhat off-topic (to explain my concern), share some other proposals, for example:
(To avoid misunderstanding: I don't mean I disagree, actually I also agree while I just would like to discuss about "how to make refactoring succeed") These two proposals seem simpler than the API handler refactoring, and they are able to be done step by step, but still are not able to achieve an ideal status in time. So I don't know what would happen if we start the API handler refactoring now without a plan that can succeed. Please forgive my bluntness. 😄 |
|
I'd help with frontend but my skills in that area aren't good (and that's assuming their existence). If they were I'd probably propose a PR for notifications by now but I can't get it to work.
I didn't mean to rush or anything - everyone has their own priorities and things that they can take on. I'm just sharing what I have and what I got to know. About the concerts about refactoring - fair. In this case I think the least clashing with current design is option 3 (writing operation spec for handler and generating openapi doc from them). Rough proof of concept here - tedious but it does generate the spec. But you're correct - swagger works and this can wait for a bit. Until a proper plan for this emerges - do we stay with comments, go with code or swap to codegen with spec first.
I'll do you better and say I like it and appreciate it :) |
|
Posting this here so I don't forget about this in between jumping through topics: Note: this is a loose thought and might be a terrible idea. And we're nowhere near the state to introduce this and nothing is preventing us from using existing structs for the same purpose. |
Feature Description
Disclaimer: I'm working on it currently to see if it's doable and so far I think it is. If this gets accepted I'd like to author it.
I'd like to suggest dropping go-swagger based generation in favor of automatically generated docs from code with huma
This would bring following benefits:
Downsides:
func (context.Context, *struct) (*struct, error), context being the std lib one.1Screenshots
No response
Footnotes
Semi related: I find current router system with multiple different functions and contexts being accepted really confusing so maybe it's an area that can be simplified a bit? ↩
The text was updated successfully, but these errors were encountered: