Alanxtl commented on code in PR #3280: URL: https://github.com/apache/dubbo-go/pull/3280#discussion_r2992078990
########## protocol/triple/server.go: ########## Review Comment: In `buildMethodInfoWithReflection`, the PR records `response.type` as `methodType.Type.Out(0)` whenever there is at least one return value, while `MethodInfo.Type` is still hardcoded as unary. This is okay for the common `(resp, error)` case, but it is not robust for non-unary or special signatures. That can produce misleading docs: * methods returning only `error` still get a synthetic object-like response, * non-unary/stream-like signatures may get the wrong response schema, * docs may imply a standard request/response JSON shape even when runtime semantics differ. I’d suggest limiting OpenAPI generation to cases where request/response types can be determined reliably, and explicitly skipping unsupported shapes instead of generating inaccurate docs. ########## protocol/triple/openapi/integration.go: ########## Review Comment: The new OpenAPI integration uses package-level globals plus sync.Once in `protocol/triple/openapi/integration.go`, and `NewHTTPHandler()` reads from those globals. That means the first initialized Triple server effectively owns the OpenAPI service for the whole process. Later servers cannot really apply their own OpenAPI config, path, groups, or settings. This is problematic when: multiple Triple servers run in the same process, different services want different OpenAPI paths/settings, tests run in the same process and pollute each other. I think the OpenAPI service/handlers should be bound to a specific `tri.Server` instance rather than stored in package globals. Right now `tri.NewServer(...)` creates a new server instance, but the OpenAPI state is still shared globally, which breaks isolation. ########## protocol/triple/openapi/definition_resolver.go: ########## @@ -0,0 +1,252 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package openapi + +import ( + "reflect" + "strings" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/common" + "dubbo.apache.org/dubbo-go/v3/global" + "dubbo.apache.org/dubbo-go/v3/protocol/triple/openapi/model" +) + +type DefinitionResolver struct { + config *global.OpenAPIConfig +} + +func NewDefinitionResolver(cfg *global.OpenAPIConfig, _ *SchemaResolver) *DefinitionResolver { + return &DefinitionResolver{ + config: cfg, + } +} + +func (r *DefinitionResolver) Resolve(interfaceName string, info *common.ServiceInfo) *model.OpenAPI { + openAPI := model.NewOpenAPI() + schemaResolver := NewSchemaResolver(r.config) + + openAPI.Info.Title = r.config.InfoTitle + openAPI.Info.Version = r.resolveVersion() + openAPI.Info.Description = r.config.InfoDescription + + seenMethods := make(map[string]bool) + for _, method := range info.Methods { + methodName := method.Name + if seenMethods[strings.ToLower(methodName)] { + continue + } + seenMethods[strings.ToLower(methodName)] = true + + httpMethods := r.determineHttpMethods() + for _, httpMethod := range httpMethods { + op := r.resolveOperation(method, httpMethod, interfaceName, schemaResolver) + path := r.buildPath(interfaceName, methodName) + pathItem := openAPI.GetOrAddPath(path) + pathItem.SetOperation(strings.ToUpper(httpMethod), op) + } + + reqSchema := r.resolveRequestSchema(method, schemaResolver) + if reqSchema != nil && reqSchema.Ref != "" { + if openAPI.Components == nil { + openAPI.Components = model.NewComponents() + } + schemaName := schemaResolver.GetSchemaName(reqSchema) + schemaDef := schemaResolver.GetSchemaDefinition(reqSchema) + if schemaName != "" && schemaDef != nil { + openAPI.Components.AddSchema(schemaName, schemaDef) + } + } + + respSchema := r.resolveResponseSchema(method, schemaResolver) + if respSchema != nil && respSchema.Ref != "" { + if openAPI.Components == nil { + openAPI.Components = model.NewComponents() + } + schemaName := schemaResolver.GetSchemaName(respSchema) + schemaDef := schemaResolver.GetSchemaDefinition(respSchema) + if schemaName != "" && schemaDef != nil { + openAPI.Components.AddSchema(schemaName, schemaDef) + } + } + } + + allSchemas := schemaResolver.GetSchemas() + if len(allSchemas) > 0 { + if openAPI.Components == nil { + openAPI.Components = model.NewComponents() + } + for name, schema := range allSchemas { + openAPI.Components.AddSchema(name, schema) + } + } + + return openAPI +} + +func (r *DefinitionResolver) resolveOperation(method common.MethodInfo, httpMethod string, tagName string, schemaResolver *SchemaResolver) *model.Operation { + op := model.NewOperation() + op.SetOperationId(method.Name) + op.SetGoMethod(method.Name) + op.SetHttpMethod(strings.ToUpper(httpMethod)) + op.AddTag(tagName) + + reqSchema := r.resolveRequestSchema(method, schemaResolver) + if reqSchema != nil { + mediaTypes := r.config.DefaultConsumesMediaTypes + if len(mediaTypes) == 0 { + mediaTypes = []string{"application/json"} + } + + reqBody := model.NewRequestBody() + for _, mt := range mediaTypes { + content := reqBody.GetOrAddContent(mt) + content.SetSchema(reqSchema) + example := schemaResolver.GenerateExample(reqSchema) + if example != nil { + content.SetExample(example) + } + } + op.SetRequestBody(reqBody) + } + + statusCodes := r.config.DefaultHttpStatusCodes + if len(statusCodes) == 0 { + statusCodes = []string{"200", "400", "500"} + } + + mediaTypes := r.config.DefaultProducesMediaTypes + if len(mediaTypes) == 0 { + mediaTypes = []string{"application/json"} + } + + for _, code := range statusCodes { + resp := op.GetOrAddResponse(code) + resp.Description = r.getStatusDescription(code) + if code == "200" { + respSchema := r.resolveResponseSchema(method, schemaResolver) + for _, mt := range mediaTypes { + content := resp.GetOrAddContent(mt) + if respSchema != nil { + content.SetSchema(respSchema) + example := schemaResolver.GenerateExample(respSchema) + if example != nil { + content.SetExample(example) + } + } + } + } + } + + return op +} + +func (r *DefinitionResolver) resolveRequestSchema(method common.MethodInfo, schemaResolver *SchemaResolver) *model.Schema { + if method.Meta != nil { + if reqType, ok := method.Meta["request.type"]; ok { + if t, ok := reqType.(reflect.Type); ok { + if t.Kind() == reflect.Ptr { + t = t.Elem() + } + return schemaResolver.Resolve(t) + } + } + } + + if method.ReqInitFunc == nil { + return nil + } + + req := method.ReqInitFunc() + if req == nil { + return nil + } + + reqType := reflect.TypeOf(req) + if reqType == nil { + return nil + } + + if reqType.Kind() == reflect.Slice { + elemType := reqType.Elem() + if elemType.Kind() == reflect.Interface { + return model.NewSchema().SetType(model.SchemaTypeObject) + } + if elemType.Kind() == reflect.Ptr { + elemType = elemType.Elem() + } + if elemType.Kind() == reflect.Struct { + return schemaResolver.Resolve(elemType) + } + return schemaResolver.Resolve(elemType) + } + + return schemaResolver.Resolve(reqType) +} + +func (r *DefinitionResolver) resolveResponseSchema(method common.MethodInfo, schemaResolver *SchemaResolver) *model.Schema { + if method.Meta != nil { + if respType, ok := method.Meta["response.type"]; ok { + if t, ok := respType.(reflect.Type); ok { + if t.Kind() == reflect.Ptr { + t = t.Elem() + } + return schemaResolver.Resolve(t) + } + } + } + + return model.NewSchema().SetType(model.SchemaTypeObject) +} + +func (r *DefinitionResolver) determineHttpMethods() []string { + return []string{"POST"} +} + +func (r *DefinitionResolver) buildPath(interfaceName, methodName string) string { + parts := strings.Split(interfaceName, ".") + serviceName := parts[len(parts)-1] + return "/" + serviceName + "/" + methodName +} Review Comment: There are several places where the PR uses weak identifiers, which can easily collide. #### Service registration is effectively keyed only by interface name `saveServiceInfo` still stores service info by `interfaceName`, and there is already a TODO saying interface name alone is not enough because group/version should also be considered. The new OpenAPI registration follows the same direction. This can overwrite/mix docs for services that share the same interface name across different groups/versions. #### Generated OpenAPI paths only use the short service name `buildPath(interfaceName, methodName)` strips the package and keeps only the last segment, so both `com.foo.UserService` and `org.bar.UserService` become `/UserService/Method`. That creates path collisions when multiple services with the same short name are exported. #### Component schema names only use `t.Name()` In `SchemaResolver`, schema/component naming is based on bare type names. Types like `Request`, `Response`, `User` from different packages will collide in `components.schemas`. I think this needs a more stable naming strategy: * registry key should include at least interface + group + version, * generated path should avoid collapsing to only the short service name, * schema registry should use a package-qualified internal key and only then derive a stable public schema name. ########## protocol/triple/openapi/constants.go: ########## Review Comment: move this file to common/constant ########## protocol/triple/openapi/model/openapi.go: ########## @@ -0,0 +1,207 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package model + +const ( + OpenAPIVersion30 = "3.0.1" +) + +type Info struct { + Title string `json:"title,omitempty"` + Description string `json:"description,omitempty"` + Version string `json:"version,omitempty"` +} + +func NewInfo() *Info { + return &Info{} +} + +func (i *Info) SetTitle(title string) *Info { + i.Title = title + return i +} + +func (i *Info) SetDescription(desc string) *Info { + i.Description = desc + return i +} + +func (i *Info) SetVersion(version string) *Info { + i.Version = version + return i +} + +type PathItem struct { + Get *Operation `json:"get,omitempty"` + Put *Operation `json:"put,omitempty"` + Post *Operation `json:"post,omitempty"` + Delete *Operation `json:"delete,omitempty"` + Options *Operation `json:"options,omitempty"` + Head *Operation `json:"head,omitempty"` + Patch *Operation `json:"patch,omitempty"` + Trace *Operation `json:"trace,omitempty"` +} + +func NewPathItem() *PathItem { + return &PathItem{} +} + +func (p *PathItem) SetOperation(method string, op *Operation) *PathItem { + switch method { + case "GET": + p.Get = op + case "PUT": + p.Put = op + case "POST": + p.Post = op + case "DELETE": + p.Delete = op + case "OPTIONS": + p.Options = op + case "HEAD": + p.Head = op + case "PATCH": + p.Patch = op + case "TRACE": + p.Trace = op + } + return p +} + +func (p *PathItem) GetOperation(method string) *Operation { + switch method { + case "GET": + return p.Get + case "PUT": + return p.Put + case "POST": + return p.Post + case "DELETE": + return p.Delete + case "OPTIONS": + return p.Options + case "HEAD": + return p.Head + case "PATCH": + return p.Patch + case "TRACE": + return p.Trace + } + return nil +} + +func (p *PathItem) GetOperations() map[string]*Operation { + ops := make(map[string]*Operation) + if p.Get != nil { + ops["GET"] = p.Get + } + if p.Put != nil { + ops["PUT"] = p.Put + } + if p.Post != nil { + ops["POST"] = p.Post + } + if p.Delete != nil { + ops["DELETE"] = p.Delete + } + if p.Options != nil { + ops["OPTIONS"] = p.Options + } + if p.Head != nil { + ops["HEAD"] = p.Head + } + if p.Patch != nil { + ops["PATCH"] = p.Patch + } + if p.Trace != nil { + ops["TRACE"] = p.Trace + } + return ops +} + +type Components struct { + Schemas map[string]*Schema `json:"schemas,omitempty"` Review Comment: Will this data structure have concurrency issues? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
