From 429f2c67c8e21c7721bd40657e906717ca7dc9c7 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 19 Apr 2026 22:23:02 +0200 Subject: [PATCH] chore: dead-code removal * removed dead-code (added justification comments) * added unit tests (now using mocks) * removed misleading comments or "TODO" mentions that will never get done Signed-off-by: Frederic BIDON --- .codecov.yml | 8 + fixtures/goparsing/spec/api.go | 1 - internal/builders/items/typable.go | 2 +- internal/builders/parameters/parameters.go | 2 +- internal/builders/parameters/taggers.go | 19 +- internal/builders/resolvers/resolvers.go | 5 +- internal/builders/resolvers/resolvers_test.go | 287 ++++++++++++++++++ internal/builders/responses/typable.go | 8 - internal/builders/schema/schema.go | 187 +++--------- internal/logger/debug.go | 14 + internal/parsers/extensions_test.go | 68 +++++ internal/parsers/responses.go | 9 +- internal/parsers/validations.go | 2 +- internal/parsers/validations_test.go | 142 +++++++++ internal/scanner/index.go | 2 +- internal/scantest/golden.go | 2 +- 16 files changed, 580 insertions(+), 178 deletions(-) create mode 100644 .codecov.yml diff --git a/.codecov.yml b/.codecov.yml new file mode 100644 index 0000000..c8edfe2 --- /dev/null +++ b/.codecov.yml @@ -0,0 +1,8 @@ +coverage: + status: + patch: + default: + target: 80% +ignore: + - internal/scantest + - fixtures diff --git a/fixtures/goparsing/spec/api.go b/fixtures/goparsing/spec/api.go index 08e3cc9..5234744 100644 --- a/fixtures/goparsing/spec/api.go +++ b/fixtures/goparsing/spec/api.go @@ -82,5 +82,4 @@ type BookingResponse struct { // Responses: // 200: BookingResponse func bookings(w http.ResponseWriter, r *http.Request) { - } diff --git a/internal/builders/items/typable.go b/internal/builders/items/typable.go index 806059d..54b4d57 100644 --- a/internal/builders/items/typable.go +++ b/internal/builders/items/typable.go @@ -22,7 +22,7 @@ func NewTypable(items *oaispec.Items, level int, in string) Typable { } } -func (pt Typable) In() string { return pt.in } // TODO(fred): inherit from param +func (pt Typable) In() string { return pt.in } func (pt Typable) Level() int { return pt.level } diff --git a/internal/builders/parameters/parameters.go b/internal/builders/parameters/parameters.go index 151e188..1ee8be6 100644 --- a/internal/builders/parameters/parameters.go +++ b/internal/builders/parameters/parameters.go @@ -167,7 +167,7 @@ func (p *ParameterBuilder) buildFromField(fld *types.Var, tpe types.Type, typabl case *types.Named: return p.buildNamedField(ftpe, typable) case *types.Alias: - logger.DebugLogf(p.ctx.Debug(), "alias(parameters.buildFromField): got alias %v to %v", ftpe, ftpe.Rhs()) // TODO + logger.DebugLogf(p.ctx.Debug(), "alias(parameters.buildFromField): got alias %v to %v", ftpe, ftpe.Rhs()) return p.buildFieldAlias(ftpe, typable, fld, seen) default: return fmt.Errorf("unknown type for %s: %T: %w", fld.String(), fld.Type(), ErrParameters) diff --git a/internal/builders/parameters/taggers.go b/internal/builders/parameters/taggers.go index 891f4e5..273daa7 100644 --- a/internal/builders/parameters/taggers.go +++ b/internal/builders/parameters/taggers.go @@ -13,22 +13,15 @@ import ( ) func setupParamTaggers(param *oaispec.Parameter, name string, afld *ast.Field, skipExt, debug bool) ([]parsers.TagParser, error) { - if param.Ref.String() != "" { - return setupRefParamTaggers(param, skipExt, debug), nil - } - + // Parameter-level $ref (e.g. {$ref: "#/parameters/X"}) is not emitted by + // the scanner today — named struct fields become body params with a + // schema-level ref (ps.Schema.Ref), never ps.Ref. To support + // operation-level parameter refs, branch here on + // `param.Ref.String() != ""` and dispatch to a narrower tagger set + // (in, required, extensions only). return setupInlineParamTaggers(param, name, afld, skipExt, debug) } -// setupRefParamTaggers configures taggers for a parameter that is a $ref. -func setupRefParamTaggers(param *oaispec.Parameter, skipExt, debug bool) []parsers.TagParser { - return []parsers.TagParser{ - parsers.NewSingleLineTagParser("in", parsers.NewMatchParamIn(param)), - parsers.NewSingleLineTagParser("required", parsers.NewMatchParamRequired(param)), - parsers.NewMultiLineTagParser("Extensions", parsers.NewSetExtensions(spExtensionsSetter(param, skipExt), debug), true), - } -} - // baseInlineParamTaggers configures taggers for a fully-defined inline parameter. func baseInlineParamTaggers(param *oaispec.Parameter, skipExt, debug bool) []parsers.TagParser { return []parsers.TagParser{ diff --git a/internal/builders/resolvers/resolvers.go b/internal/builders/resolvers/resolvers.go index 68f24d0..c0a1e1f 100644 --- a/internal/builders/resolvers/resolvers.go +++ b/internal/builders/resolvers/resolvers.go @@ -27,6 +27,7 @@ const ( ) // SwaggerSchemaForType maps all Go builtin types that have Json representation to Swagger/Json types. +// // See https://golang.org/pkg/builtin/ and http://swagger.io/specification/ func SwaggerSchemaForType(typeName string, prop ifaces.SwaggerTypable) error { switch typeName { @@ -37,9 +38,9 @@ func SwaggerSchemaForType(typeName string, prop ifaces.SwaggerTypable) error { case "complex128", "complex64": return fmt.Errorf("unsupported builtin %q (no JSON marshaller): %w", typeName, ErrResolver) case "error": - // TODO: error is often marshalled into a string but not always (e.g. errors package creates + // Proposal for enhancement: error is often marshalled into a string but not always (e.g. errors package creates // errors that are marshalled into an empty object), this could be handled the same way - // custom JSON marshallers are handled (in future) + // custom JSON marshallers are handled (future) prop.Typed("string", "") case "float32": prop.Typed("number", "float") diff --git a/internal/builders/resolvers/resolvers_test.go b/internal/builders/resolvers/resolvers_test.go index 81834fd..5977e14 100644 --- a/internal/builders/resolvers/resolvers_test.go +++ b/internal/builders/resolvers/resolvers_test.go @@ -4,8 +4,14 @@ package resolvers import ( + "errors" + "go/ast" + "go/token" + "go/types" "testing" + "github.com/go-openapi/codescan/internal/ifaces" + "github.com/go-openapi/codescan/internal/scantest/mocks" oaispec "github.com/go-openapi/spec" "github.com/go-openapi/testify/v2/assert" "github.com/go-openapi/testify/v2/require" @@ -35,3 +41,284 @@ func TestAddExtension(t *testing.T) { AddExtension(ve, key3, value3, true) assert.Nil(t, ve.Extensions[key3]) } + +// typableCapture builds a MockSwaggerTypable whose Typed() records +// the (swaggerType, format) pair it receives. +func typableCapture() (*mocks.MockSwaggerTypable, *[2]string) { + got := new([2]string) + m := &mocks.MockSwaggerTypable{ + TypedFunc: func(swaggerType, format string) { + got[0] = swaggerType + got[1] = format + }, + } + return m, got +} + +func TestSwaggerSchemaForType(t *testing.T) { + t.Run("supported builtins map to swagger types", func(t *testing.T) { + cases := []struct { + in string + want [2]string + }{ + {"bool", [2]string{"boolean", ""}}, + {"byte", [2]string{"integer", "uint8"}}, + {"error", [2]string{"string", ""}}, + {"float32", [2]string{"number", "float"}}, + {"float64", [2]string{"number", "double"}}, + {"int", [2]string{"integer", "int64"}}, + {"int8", [2]string{"integer", "int8"}}, + {"int16", [2]string{"integer", "int16"}}, + {"int32", [2]string{"integer", "int32"}}, + {"int64", [2]string{"integer", "int64"}}, + {"rune", [2]string{"integer", "int32"}}, + {"string", [2]string{"string", ""}}, + {"uint", [2]string{"integer", "uint64"}}, + {"uint8", [2]string{"integer", "uint8"}}, + {"uint16", [2]string{"integer", "uint16"}}, + {"uint32", [2]string{"integer", "uint32"}}, + {"uint64", [2]string{"integer", "uint64"}}, + {"uintptr", [2]string{"integer", "uint64"}}, + {"object", [2]string{"object", ""}}, + } + + for _, tc := range cases { + t.Run(tc.in, func(t *testing.T) { + m, got := typableCapture() + require.NoError(t, SwaggerSchemaForType(tc.in, m)) + assert.EqualT(t, tc.want, *got) + }) + } + }) + + t.Run("complex64/128 returns ErrResolver", func(t *testing.T) { + for _, name := range []string{"complex64", "complex128"} { + t.Run(name, func(t *testing.T) { + m := &mocks.MockSwaggerTypable{ + TypedFunc: func(_, _ string) { t.Fatalf("Typed should not be called for %q", name) }, + } + err := SwaggerSchemaForType(name, m) + require.Error(t, err) + require.TrueT(t, errors.Is(err, ErrResolver)) + }) + } + }) + + t.Run("unknown type returns ErrResolver", func(t *testing.T) { + m := &mocks.MockSwaggerTypable{ + TypedFunc: func(_, _ string) { t.Fatal("Typed should not be called for unknown type") }, + } + err := SwaggerSchemaForType("bogus", m) + require.Error(t, err) + require.TrueT(t, errors.Is(err, ErrResolver)) + }) +} + +func TestUnsupportedBuiltin(t *testing.T) { + t.Run("nil Obj returns false", func(t *testing.T) { + m := &mocks.MockObjecter{ObjFunc: func() *types.TypeName { return nil }} + assert.FalseT(t, UnsupportedBuiltin(m)) + }) + + t.Run("unsafe package returns true", func(t *testing.T) { + pkg := types.NewPackage("unsafe", "unsafe") + tn := types.NewTypeName(token.NoPos, pkg, "Pointer", nil) + m := &mocks.MockObjecter{ObjFunc: func() *types.TypeName { return tn }} + assert.TrueT(t, UnsupportedBuiltin(m)) + }) + + t.Run("user package returns false", func(t *testing.T) { + pkg := types.NewPackage("example.com/foo", "foo") + tn := types.NewTypeName(token.NoPos, pkg, "Bar", nil) + m := &mocks.MockObjecter{ObjFunc: func() *types.TypeName { return tn }} + assert.FalseT(t, UnsupportedBuiltin(m)) + }) + + t.Run("builtin complex64 returns true", func(t *testing.T) { + tn := types.NewTypeName(token.NoPos, nil, "complex64", nil) + m := &mocks.MockObjecter{ObjFunc: func() *types.TypeName { return tn }} + assert.TrueT(t, UnsupportedBuiltin(m)) + }) + + t.Run("builtin int returns false", func(t *testing.T) { + tn := types.NewTypeName(token.NoPos, nil, "int", nil) + m := &mocks.MockObjecter{ObjFunc: func() *types.TypeName { return tn }} + assert.FalseT(t, UnsupportedBuiltin(m)) + }) +} + +func TestUnsupportedBuiltinType(t *testing.T) { + t.Run("Basic complex64 returns true", func(t *testing.T) { + assert.TrueT(t, UnsupportedBuiltinType(types.Typ[types.Complex64])) + }) + + t.Run("Basic int returns false", func(t *testing.T) { + assert.FalseT(t, UnsupportedBuiltinType(types.Typ[types.Int])) + }) + + t.Run("UnsafePointer Basic returns true", func(t *testing.T) { + assert.TrueT(t, UnsupportedBuiltinType(types.Typ[types.UnsafePointer])) + }) + + t.Run("Chan returns true", func(t *testing.T) { + ch := types.NewChan(types.SendRecv, types.Typ[types.Int]) + assert.TrueT(t, UnsupportedBuiltinType(ch)) + }) + + t.Run("Signature returns true", func(t *testing.T) { + sig := types.NewSignatureType(nil, nil, nil, types.NewTuple(), types.NewTuple(), false) + assert.TrueT(t, UnsupportedBuiltinType(sig)) + }) + + t.Run("TypeParam returns true", func(t *testing.T) { + pkg := types.NewPackage("example.com/foo", "foo") + tn := types.NewTypeName(token.NoPos, pkg, "T", nil) + tp := types.NewTypeParam(tn, types.NewInterfaceType(nil, nil)) + assert.TrueT(t, UnsupportedBuiltinType(tp)) + }) + + t.Run("Named delegates to UnsupportedBuiltin", func(t *testing.T) { + pkg := types.NewPackage("example.com/foo", "foo") + tn := types.NewTypeName(token.NoPos, pkg, "Foo", nil) + named := types.NewNamed(tn, types.Typ[types.Int], nil) + assert.FalseT(t, UnsupportedBuiltinType(named)) + }) + + t.Run("Map (default case) returns false", func(t *testing.T) { + m := types.NewMap(types.Typ[types.String], types.Typ[types.Int]) + assert.FalseT(t, UnsupportedBuiltinType(m)) + }) +} + +func TestIsFieldStringable(t *testing.T) { + t.Run("scalar Ident returns true", func(t *testing.T) { + for _, name := range []string{"int", "int8", "int64", "uint32", "float64", "string", "bool"} { + assert.TrueT(t, IsFieldStringable(ast.NewIdent(name)), "want true for %s", name) + } + }) + + t.Run("non-scalar Ident returns false", func(t *testing.T) { + for _, name := range []string{"any", "Foo", "float32"} { // float32 isn't in the stringable set + assert.FalseT(t, IsFieldStringable(ast.NewIdent(name)), "want false for %s", name) + } + }) + + t.Run("StarExpr to scalar returns true", func(t *testing.T) { + star := &ast.StarExpr{X: ast.NewIdent("int")} + assert.TrueT(t, IsFieldStringable(star)) + }) + + t.Run("StarExpr to non-scalar returns false", func(t *testing.T) { + star := &ast.StarExpr{X: ast.NewIdent("Foo")} + assert.FalseT(t, IsFieldStringable(star)) + }) + + t.Run("other AST expr returns false", func(t *testing.T) { + // SelectorExpr like pkg.Type — neither Ident nor StarExpr. + sel := &ast.SelectorExpr{X: ast.NewIdent("pkg"), Sel: ast.NewIdent("Type")} + assert.FalseT(t, IsFieldStringable(sel)) + + // ArrayType — same, hits the else-return-false branch. + arr := &ast.ArrayType{Elt: ast.NewIdent("int")} + assert.FalseT(t, IsFieldStringable(arr)) + }) +} + +func TestParseJSONTag(t *testing.T) { + ident := func(name string) *ast.Field { + return &ast.Field{ + Names: []*ast.Ident{ast.NewIdent(name)}, + Type: ast.NewIdent("int"), + } + } + + t.Run("no tag uses field name", func(t *testing.T) { + f := ident("Foo") + name, ignore, isString, omitEmpty, err := ParseJSONTag(f) + require.NoError(t, err) + assert.EqualT(t, "Foo", name) + assert.FalseT(t, ignore) + assert.FalseT(t, isString) + assert.FalseT(t, omitEmpty) + }) + + t.Run("json tag renames", func(t *testing.T) { + f := ident("Foo") + f.Tag = &ast.BasicLit{Value: "`json:\"foo,omitempty\"`"} + name, ignore, isString, omitEmpty, err := ParseJSONTag(f) + require.NoError(t, err) + assert.EqualT(t, "foo", name) + assert.FalseT(t, ignore) + assert.FalseT(t, isString) + assert.TrueT(t, omitEmpty) + }) + + t.Run("json:\"-\" marks ignored", func(t *testing.T) { + f := ident("Foo") + f.Tag = &ast.BasicLit{Value: "`json:\"-\"`"} + name, ignore, _, _, err := ParseJSONTag(f) + require.NoError(t, err) + assert.EqualT(t, "Foo", name) + assert.TrueT(t, ignore) + }) + + t.Run("json:\",string\" on scalar sets isString", func(t *testing.T) { + f := ident("Foo") + f.Tag = &ast.BasicLit{Value: "`json:\",string\"`"} + name, _, isString, _, err := ParseJSONTag(f) + require.NoError(t, err) + assert.EqualT(t, "Foo", name) + assert.TrueT(t, isString) + }) + + t.Run("whitespace-only tag value falls through", func(t *testing.T) { + f := ident("Foo") + // Backticks wrap a single space — TrimSpace of `" "` != empty (outer check + // passes), but Unquote yields " " which does TrimSpace to empty — hits + // the final fallthrough. + f.Tag = &ast.BasicLit{Value: "` `"} + name, ignore, isString, omitEmpty, err := ParseJSONTag(f) + require.NoError(t, err) + assert.EqualT(t, "Foo", name) + assert.FalseT(t, ignore) + assert.FalseT(t, isString) + assert.FalseT(t, omitEmpty) + }) + + t.Run("malformed tag returns Unquote error", func(t *testing.T) { + f := ident("Foo") + // Unquote requires surrounding backticks/quotes. Bare word is invalid. + f.Tag = &ast.BasicLit{Value: "not-a-quoted-tag"} + _, _, _, _, err := ParseJSONTag(f) + require.Error(t, err) + }) +} + +func TestMustNotBeABuiltinType(t *testing.T) { + t.Run("user type does not panic", func(t *testing.T) { + pkg := types.NewPackage("example.com/foo", "foo") + tn := types.NewTypeName(token.NoPos, pkg, "Foo", nil) + assert.NotPanics(t, func() { MustNotBeABuiltinType(tn) }) + }) + + t.Run("builtin panics wrapping ErrInternal", func(t *testing.T) { + tn := types.NewTypeName(token.NoPos, nil, "complex64", nil) + defer func() { + r := recover() + require.NotNil(t, r) + err, ok := r.(error) + require.TrueT(t, ok, "panic value should be an error") + require.TrueT(t, errors.Is(err, ErrInternal)) + }() + MustNotBeABuiltinType(tn) + }) +} + +func TestInternalError_Error(t *testing.T) { + // Exercises the internalError.Error method that satisfies the `error` + // interface used in fmt.Errorf("...%w", ErrInternal). + assert.EqualT(t, string(ErrInternal), ErrInternal.Error()) +} + +// Assert at compile time that our typable fixture satisfies the interface. +var _ ifaces.SwaggerTypable = (*mocks.MockSwaggerTypable)(nil) diff --git a/internal/builders/responses/typable.go b/internal/builders/responses/typable.go index f9c9cdb..f3feae3 100644 --- a/internal/builders/responses/typable.go +++ b/internal/builders/responses/typable.go @@ -57,14 +57,6 @@ func (ht responseTypable) Schema() *oaispec.Schema { return ht.response.Schema } -func (ht responseTypable) SetSchema(schema *oaispec.Schema) { - ht.response.Schema = schema -} - -func (ht responseTypable) CollectionOf(items *oaispec.Items, format string) { - ht.header.CollectionOf(items, format) -} - func (ht responseTypable) AddExtension(key string, value any) { ht.response.AddExtension(key, value) } diff --git a/internal/builders/schema/schema.go b/internal/builders/schema/schema.go index ec641e7..04eb405 100644 --- a/internal/builders/schema/schema.go +++ b/internal/builders/schema/schema.go @@ -123,23 +123,6 @@ func (s *Builder) buildFromDecl(_ *scanner.EntityDecl, schema *oaispec.Schema) e }() switch tpe := s.decl.ObjType().(type) { - // TODO(fredbi): we may safely remove all the cases here that are not Named or Alias - case *types.Basic: - logger.DebugLogf(s.ctx.Debug(), "basic: %v", tpe.Name()) - return nil - case *types.Struct: - return s.buildFromStruct(s.decl, tpe, schema, make(map[string]string)) - case *types.Interface: - return s.buildFromInterface(s.decl, tpe, schema, make(map[string]string)) - case *types.Array: - logger.DebugLogf(s.ctx.Debug(), "array: %v -> %v", s.decl.Ident.Name, tpe.Elem().String()) - return nil - case *types.Slice: - logger.DebugLogf(s.ctx.Debug(), "slice: %v -> %v", s.decl.Ident.Name, tpe.Elem().String()) - return nil - case *types.Map: - logger.DebugLogf(s.ctx.Debug(), "map: %v -> [%v]%v", s.decl.Ident.Name, tpe.Key().String(), tpe.Elem().String()) - return nil case *types.Named: logger.DebugLogf(s.ctx.Debug(), "named: %v", tpe) return s.buildDeclNamed(tpe, schema) @@ -148,17 +131,8 @@ func (s *Builder) buildFromDecl(_ *scanner.EntityDecl, schema *oaispec.Schema) e tgt := Typable{schema, 0, s.ctx.SkipExtensions()} return s.buildDeclAlias(tpe, tgt) - case *types.TypeParam: - log.Printf("WARNING: generic type parameters are not supported yet %[1]v (%[1]T). Skipped", tpe) - return nil - case *types.Chan: - log.Printf("WARNING: channels are not supported %[1]v (%[1]T). Skipped", tpe) - return nil - case *types.Signature: - log.Printf("WARNING: functions are not supported %[1]v (%[1]T). Skipped", tpe) - return nil default: - log.Printf("WARNING: missing parser for type %T, skipping model: %s\n", tpe, s.Name) + logger.UnsupportedTypeKind("buildFromDecl", tpe) return nil } } @@ -283,17 +257,13 @@ func (s *Builder) buildFromType(tpe types.Type, tgt ifaces.SwaggerTypable) error // a named alias, e.g. type X = {RHS type}. logger.DebugLogf(s.ctx.Debug(), "alias(schema.buildFromType): got alias %v to %v", titpe, titpe.Rhs()) return s.buildAlias(titpe, tgt) - case *types.TypeParam: - log.Printf("WARNING: generic type parameters are not supported yet %[1]v (%[1]T). Skipped", titpe) - return nil - case *types.Chan: - log.Printf("WARNING: channels are not supported %[1]v (%[1]T). Skipped", tpe) - return nil - case *types.Signature: - log.Printf("WARNING: functions are not supported %[1]v (%[1]T). Skipped", tpe) - return nil default: - panic(fmt.Errorf("ERROR: can't determine refined type %[1]v (%[1]T): %w", titpe, resolvers.ErrInternal)) + // Warn-and-skip for unsupported kinds (TypeParam, Chan, Signature, + // Union, or future go/types additions). The scanner runs on user + // code in uncontrolled environments, so panicking here would be a + // worse experience than producing a partial spec. + logger.UnsupportedTypeKind("buildFromType", titpe) + return nil } } @@ -392,21 +362,8 @@ func (s *Builder) buildNamedType(titpe *types.Named, tgt ifaces.SwaggerTypable) return s.makeRef(decl, tgt) } return nil - case *types.TypeParam: - log.Printf("WARNING: generic type parameters are not supported yet %[1]v (%[1]T). Skipped", utitpe) - return nil - case *types.Chan: - log.Printf("WARNING: channels are not supported %[1]v (%[1]T). Skipped", utitpe) - return nil - case *types.Signature: - log.Printf("WARNING: functions are not supported %[1]v (%[1]T). Skipped", utitpe) - return nil default: - log.Printf( - "WARNING: can't figure out object type for named type (%T): %v [alias: %t]", - titpe.Underlying(), titpe.Underlying(), titpe.Obj().IsAlias(), - ) - + logger.UnsupportedTypeKind("buildNamedType", utitpe) return nil } } @@ -463,6 +420,18 @@ func (s *Builder) buildNamedBasic(tio *types.TypeName, pkg *packages.Package, cm return resolvers.SwaggerSchemaForType(utitpe.String(), tgt) } +// buildNamedStruct emits a $ref to a named struct definition (or a strfmt +// override when `swagger:strfmt` is set on the type's doc). +// +// Preconditions established by the sole caller buildNamedType and not +// re-checked here: +// - tio is never time.Time — IsStdTime(tio) short-circuits upstream to +// {string, date-time} before the Underlying() switch runs. +// - parsers.TypeName(cmt) is never set — the upstream TypeName branch +// either resolves via SwaggerSchemaForType or delegates to +// buildFromType(Underlying), so neither outcome reaches this function. +// +// Re-adding either check here would be dead code. func (s *Builder) buildNamedStruct(tio *types.TypeName, cmt *ast.CommentGroup, tgt ifaces.SwaggerTypable) error { logger.DebugLogf(s.ctx.Debug(), "found struct: %s.%s", tio.Pkg().Path(), tio.Name()) @@ -472,25 +441,11 @@ func (s *Builder) buildNamedStruct(tio *types.TypeName, cmt *ast.CommentGroup, t return nil } - o := decl.Obj() - if resolvers.IsStdTime(o) { - tgt.Typed("string", "date-time") - return nil - } - if sfnm, isf := parsers.StrfmtName(cmt); isf { tgt.Typed("string", sfnm) return nil } - if tn, ok := parsers.TypeName(cmt); ok { - if err := resolvers.SwaggerSchemaForType(tn, tgt); err == nil { - return nil - } - // For unsupported swagger:type values, fall through to makeRef - // rather than silently returning an empty schema. - } - return s.makeRef(decl, tgt) } @@ -552,6 +507,20 @@ func (s *Builder) buildNamedSlice(tio *types.TypeName, cmt *ast.CommentGroup, el } // buildDeclAlias builds a top-level alias declaration. +// +// Note on LHS checks NOT performed here: IsAny(o) / IsStdError(o) / +// IsStdTime(o) on o := tpe.Obj() would all be false. o is the user's +// declared name (e.g. "X" in `type X = any`), so o.Pkg() is always the +// user's package and o.Name() is the user's identifier — neither +// matches the predeclared any/error (Pkg()==nil) nor stdlib time.Time +// (pkg "time", name "Time"). The live equivalents IsAny(ro) / +// IsStdError(ro) inside the `case *types.Alias:` branch of the RHS +// switch below do fire: they inspect the alias target, which for +// `type X = any` resolves to the predeclared any TypeName. +// +// For `type Timestamp = time.Time` the date-time format is currently +// lost — see Q3 in .claude/plans/observed-quirks.md. Any fix belongs +// in the RHS switch, not here. func (s *Builder) buildDeclAlias(tpe *types.Alias, tgt ifaces.SwaggerTypable) error { if resolvers.UnsupportedBuiltinType(tpe) { log.Printf("WARNING: skipped unsupported builtin type: %v", tpe) @@ -559,21 +528,7 @@ func (s *Builder) buildDeclAlias(tpe *types.Alias, tgt ifaces.SwaggerTypable) er } o := tpe.Obj() - if resolvers.IsAny(o) { - _ = tgt.Schema() // this is mutating tgt to create an empty schema - return nil - } - if resolvers.IsStdError(o) { - tgt.AddExtension("x-go-type", o.Name()) - return resolvers.SwaggerSchemaForType(o.Name(), tgt) - } resolvers.MustNotBeABuiltinType(o) - - if resolvers.IsStdTime(o) { - tgt.Typed("string", "date-time") - return nil - } - resolvers.MustHaveRightHandSide(tpe) rhs := tpe.Rhs() @@ -865,19 +820,8 @@ func (s *Builder) processEmbeddedType( fieldHasAllOf = true schema.AddToAllOf(aliasedSchema) } - case *types.Union: - log.Printf("WARNING: union type constraints are not supported yet %[1]v (%[1]T). Skipped", ftpe) - case *types.TypeParam: - log.Printf("WARNING: generic type parameters are not supported yet %[1]v (%[1]T). Skipped", ftpe) - case *types.Chan: - log.Printf("WARNING: channels are not supported %[1]v (%[1]T). Skipped", ftpe) - case *types.Signature: - log.Printf("WARNING: functions are not supported %[1]v (%[1]T). Skipped", ftpe) default: - log.Printf( - "WARNING: can't figure out object type for allOf named type (%T): %v", - ftpe, ftpe.Underlying(), - ) + logger.UnsupportedTypeKind("buildNamedInterface.allOf", ftpe) } logger.DebugLogf(s.ctx.Debug(), "got embedded interface: %v {%T}, fieldHasAllOf: %t", fld, fld, fieldHasAllOf) @@ -1222,18 +1166,9 @@ func (s *Builder) buildAllOf(tpe types.Type, schema *oaispec.Schema) error { logger.DebugLogf(s.ctx.Debug(), "allOf member is alias %v => %v", ftpe, ftpe.Rhs()) tgt := Typable{schema: schema, skipExt: s.ctx.SkipExtensions()} return s.buildAlias(ftpe, tgt) - case *types.TypeParam: - log.Printf("WARNING: generic type parameters are not supported yet %[1]v (%[1]T). Skipped", ftpe) - return nil - case *types.Chan: - log.Printf("WARNING: channels are not supported %[1]v (%[1]T). Skipped", ftpe) - return nil - case *types.Signature: - log.Printf("WARNING: functions are not supported %[1]v (%[1]T). Skipped", ftpe) - return nil default: - log.Printf("WARNING: missing allOf parser for a %T, skipping field", ftpe) - return fmt.Errorf("unable to resolve allOf member for: %v: %w", ftpe, ErrSchema) + logger.UnsupportedTypeKind("buildAllOf", ftpe) + return nil } } @@ -1276,23 +1211,9 @@ func (s *Builder) buildNamedAllOf(ftpe *types.Named, schema *oaispec.Schema) err } return s.buildFromInterface(decl, utpe, schema, make(map[string]string)) - case *types.TypeParam: - log.Printf("WARNING: generic type parameters are not supported yet %[1]v (%[1]T). Skipped", ftpe) - return nil - case *types.Chan: - log.Printf("WARNING: channels are not supported %[1]v (%[1]T). Skipped", ftpe) - return nil - case *types.Signature: - log.Printf("WARNING: functions are not supported %[1]v (%[1]T). Skipped", ftpe) - return nil default: - log.Printf( - "WARNING: can't figure out object type for allOf named type (%T): %v", - ftpe, utpe, - ) - return fmt.Errorf("unable to locate source file for allOf (%T): %v: %w", - ftpe, utpe, ErrSchema, - ) + logger.UnsupportedTypeKind("buildNamedAllOf", utpe) + return nil } } @@ -1308,20 +1229,8 @@ func (s *Builder) buildEmbedded(tpe types.Type, schema *oaispec.Schema, seen map logger.DebugLogf(s.ctx.Debug(), "embedded alias %v => %v", ftpe, ftpe.Rhs()) tgt := Typable{schema, 0, s.ctx.SkipExtensions()} return s.buildAlias(ftpe, tgt) - case *types.Union: // e.g. type X interface{ ~uint16 | ~float32 } - log.Printf("WARNING: union type constraints are not supported yet %[1]v (%[1]T). Skipped", ftpe) - return nil - case *types.TypeParam: - log.Printf("WARNING: generic type parameters are not supported yet %[1]v (%[1]T). Skipped", ftpe) - return nil - case *types.Chan: - log.Printf("WARNING: channels are not supported %[1]v (%[1]T). Skipped", ftpe) - return nil - case *types.Signature: - log.Printf("WARNING: functions are not supported %[1]v (%[1]T). Skipped", ftpe) - return nil default: - log.Printf("WARNING: Missing embedded parser for a %T, skipping model\n", ftpe) + logger.UnsupportedTypeKind("buildEmbedded", ftpe) return nil } } @@ -1362,22 +1271,8 @@ func (s *Builder) buildNamedEmbedded(ftpe *types.Named, schema *oaispec.Schema, return fmt.Errorf("can't find source file for struct: %s: %w", ftpe.String(), ErrSchema) } return s.buildFromInterface(decl, utpe, schema, seen) - case *types.Union: // e.g. type X interface{ ~uint16 | ~float32 } - log.Printf("WARNING: union type constraints are not supported yet %[1]v (%[1]T). Skipped", utpe) - return nil - case *types.TypeParam: - log.Printf("WARNING: generic type parameters are not supported yet %[1]v (%[1]T). Skipped", utpe) - return nil - case *types.Chan: - log.Printf("WARNING: channels are not supported %[1]v (%[1]T). Skipped", utpe) - return nil - case *types.Signature: - log.Printf("WARNING: functions are not supported %[1]v (%[1]T). Skipped", utpe) - return nil default: - log.Printf("WARNING: can't figure out object type for embedded named type (%T): %v", - ftpe, utpe, - ) + logger.UnsupportedTypeKind("buildNamedEmbedded", utpe) return nil } } diff --git a/internal/logger/debug.go b/internal/logger/debug.go index 0d5e8e4..3b2e50e 100644 --- a/internal/logger/debug.go +++ b/internal/logger/debug.go @@ -5,6 +5,7 @@ package logger import ( "fmt" + "go/types" "log" ) @@ -16,3 +17,16 @@ func DebugLogf(debug bool, format string, args ...any) { _ = log.Output(logCallerDepth, fmt.Sprintf(format, args...)) } } + +// UnsupportedTypeKind emits a uniform warning when a go/types kind +// cannot be translated to a Swagger 2.0 construct. +// +// The scanner runs on arbitrary user code in uncontrolled environments, +// so encountering an unsupported kind must not panic — we log and let +// the caller skip. `where` is the dispatcher site (typically the +// function name) so a future go/types evolution — e.g. a new kind we +// haven't modeled — surfaces one grep-able diagnostic instead of +// disappearing behind a silent default. +func UnsupportedTypeKind(where string, tpe types.Type) { + log.Printf("WARNING: %s: unsupported Go type kind %[2]T (%[2]v); skipping", where, tpe) +} diff --git a/internal/parsers/extensions_test.go b/internal/parsers/extensions_test.go index c07db43..c55bee4 100644 --- a/internal/parsers/extensions_test.go +++ b/internal/parsers/extensions_test.go @@ -194,3 +194,71 @@ func TestSetOpExtensions_Parse(t *testing.T) { assert.EqualT(t, "1", obj["a"]) }) } + +// TestSetOpExtensions_Parse_Malformed exercises the defensive guards in +// buildExtensionObjects against malformed extension blocks. Each case +// covers a branch that would otherwise be silently skipped with no test +// witness — losing data without warning would be a nasty silent-bug class. +// The contract these tests pin: malformed lines never panic, never +// corrupt prior extensions, and are simply dropped from the output. +func TestSetOpExtensions_Parse_Malformed(t *testing.T) { + t.Parallel() + + t.Run("line with empty key is skipped", func(t *testing.T) { + // Covers the `if key == "" { return }` guard in buildExtensionObjects: + // a line whose colon-prefix produces an empty trimmed key (e.g. leading + // colon). Must not panic; nothing should land in the output. + var got oaispec.Extensions + se := NewSetExtensions(func(ext *oaispec.Extensions) { got = *ext }, false) + + lines := []string{":just-a-value"} + require.NoError(t, se.Parse(lines)) + assert.Empty(t, got) + }) + + t.Run("list item at top level with no prior extension is dropped", func(t *testing.T) { + // Covers `if stack == nil || len(*stack) == 0 { return }` in the + // `len(kv) <= 1` branch — a bare word (no colon) with no preceding + // x- extension to attach it to. + var got oaispec.Extensions + se := NewSetExtensions(func(ext *oaispec.Extensions) { got = *ext }, false) + + lines := []string{"orphan-item"} + require.NoError(t, se.Parse(lines)) + assert.Empty(t, got) + }) + + t.Run("non-x key:value at top level with no prior extension is dropped", func(t *testing.T) { + // Covers the second `if stack == nil || len(*stack) == 0 { return }` + // guard further down: a plain key:value line whose key isn't an x- + // extension and there's no open extension to nest under. + var got oaispec.Extensions + se := NewSetExtensions(func(ext *oaispec.Extensions) { got = *ext }, false) + + lines := []string{"plain-key: plain-value"} + require.NoError(t, se.Parse(lines)) + assert.Empty(t, got) + }) + + t.Run("valid extension followed by malformed lines keeps valid output", func(t *testing.T) { + // Composition check: a good x- extension followed by each malformed + // shape. The good extension must survive; the junk lines must be + // dropped without disturbing it. + var got oaispec.Extensions + se := NewSetExtensions(func(ext *oaispec.Extensions) { got = *ext }, false) + + lines := []string{ + "x-good: keeper", + ":empty-key", + "orphan-item", + "plain-key: plain-value", + } + require.NoError(t, se.Parse(lines)) + + val, ok := got.GetString("x-good") + require.TrueT(t, ok) + assert.EqualT(t, "keeper", val) + _, hasPlain := got["plain-key"] + assert.FalseT(t, hasPlain, "non-x- key must not leak into output") + }) +} diff --git a/internal/parsers/responses.go b/internal/parsers/responses.go index 0957360..53373e4 100644 --- a/internal/parsers/responses.go +++ b/internal/parsers/responses.go @@ -144,7 +144,8 @@ func parseTags(line string) (modelOrResponse string, arrays int, isDefinitionRef tag = tagValList[0] value = tagValList[1] } else { - // TODO: Print a warning, and in the long term, do not support not tagged values + // Proposal for enhancement: print a warning, and in the long term, do not support untagged values + // // Add a default tag if none is supplied if i == 0 { tag = responseTag @@ -194,11 +195,13 @@ func parseTags(line string) (modelOrResponse string, arrays int, isDefinitionRef } else { err = fmt.Errorf("invalid tag: %s: %w", tag, ErrParser) } - // return error + + // Error case return modelOrResponse, arrays, isDefinitionRef, description, err } - // TODO: Maybe do, if !parsedModelOrResponse {return some error} + // Proposal for enhancement: maybe do, if !parsedModelOrResponse {return some error} + return modelOrResponse, arrays, isDefinitionRef, description, err } diff --git a/internal/parsers/validations.go b/internal/parsers/validations.go index 06fc3e9..1625d9e 100644 --- a/internal/parsers/validations.go +++ b/internal/parsers/validations.go @@ -34,7 +34,7 @@ func WithItemsPrefixLevel(level int) PrefixRxOption { // the expression is 1-index based not 0-index itemsPrefix := fmt.Sprintf(rxItemsPrefixFmt, level+1) return func(expr string) *regexp.Regexp { - return Rxf(expr, itemsPrefix) // TODO(fred): cache + return Rxf(expr, itemsPrefix) // Proposal for enhancement(fred): cache } } diff --git a/internal/parsers/validations_test.go b/internal/parsers/validations_test.go index 4cb6550..ed09c9d 100644 --- a/internal/parsers/validations_test.go +++ b/internal/parsers/validations_test.go @@ -4,8 +4,11 @@ package parsers import ( + "strings" "testing" + "github.com/go-openapi/codescan/internal/ifaces" + "github.com/go-openapi/codescan/internal/scantest/mocks" "github.com/go-openapi/testify/v2/assert" "github.com/go-openapi/testify/v2/require" @@ -606,3 +609,142 @@ func TestSetRequiredSchema_Matches(t *testing.T) { assert.TrueT(t, su.Matches("Required: false")) assert.FalseT(t, su.Matches("something else")) } + +// strictMockValidationBuilder returns a MockValidationBuilder whose Set* methods +// fail the test if called. Use this in tests that assert no mutation happened +// (empty-input tolerance, overflow errors, etc.). +func strictMockValidationBuilder(t *testing.T) *mocks.MockValidationBuilder { + t.Helper() + fail := func(name string) func(...any) { + return func(args ...any) { t.Fatalf("%s should not be called (args: %v)", name, args) } + } + m := &mocks.MockValidationBuilder{} + m.SetMaximumFunc = func(v float64, exclusive bool) { fail("SetMaximum")(v, exclusive) } + m.SetMinimumFunc = func(v float64, exclusive bool) { fail("SetMinimum")(v, exclusive) } + m.SetMultipleOfFunc = func(v float64) { fail("SetMultipleOf")(v) } + m.SetMaxItemsFunc = func(v int64) { fail("SetMaxItems")(v) } + m.SetMinItemsFunc = func(v int64) { fail("SetMinItems")(v) } + m.SetMaxLengthFunc = func(v int64) { fail("SetMaxLength")(v) } + m.SetMinLengthFunc = func(v int64) { fail("SetMinLength")(v) } + m.SetPatternFunc = func(v string) { fail("SetPattern")(v) } + m.SetUniqueFunc = func(v bool) { fail("SetUnique")(v) } + m.SetEnumFunc = func(v string) { fail("SetEnum")(v) } + m.SetDefaultFunc = func(v any) { fail("SetDefault")(v) } + m.SetExampleFunc = func(v any) { fail("SetExample")(v) } + return m +} + +// TestValidationParsers_EmptyInputTolerance pins the defensive-guard +// contract documented in the D.5 post-mortem: every Parse(lines) tolerates +// nil / empty-slice / single-empty-string input without panic and without +// mutating its target. Uses MockValidationBuilder (Set* funcs fail on call) +// to prove no side effect. +func TestValidationParsers_EmptyInputTolerance(t *testing.T) { + t.Parallel() + + emptyInputs := [][]string{nil, {}, {""}} + + cases := []struct { + name string + factory func(*testing.T) ifaces.ValueParser + }{ + {"SetMaximum", func(t *testing.T) ifaces.ValueParser { return NewSetMaximum(strictMockValidationBuilder(t)) }}, + {"SetMinimum", func(t *testing.T) ifaces.ValueParser { return NewSetMinimum(strictMockValidationBuilder(t)) }}, + {"SetMultipleOf", func(t *testing.T) ifaces.ValueParser { return NewSetMultipleOf(strictMockValidationBuilder(t)) }}, + {"SetMaxItems", func(t *testing.T) ifaces.ValueParser { return NewSetMaxItems(strictMockValidationBuilder(t)) }}, + {"SetMinItems", func(t *testing.T) ifaces.ValueParser { return NewSetMinItems(strictMockValidationBuilder(t)) }}, + {"SetMaxLength", func(t *testing.T) ifaces.ValueParser { return NewSetMaxLength(strictMockValidationBuilder(t)) }}, + {"SetMinLength", func(t *testing.T) ifaces.ValueParser { return NewSetMinLength(strictMockValidationBuilder(t)) }}, + {"SetPattern", func(t *testing.T) ifaces.ValueParser { return NewSetPattern(strictMockValidationBuilder(t)) }}, + {"SetUnique", func(t *testing.T) ifaces.ValueParser { return NewSetUnique(strictMockValidationBuilder(t)) }}, + {"SetExample", func(t *testing.T) ifaces.ValueParser { + scheme := &oaispec.SimpleSchema{Type: "string"} + return NewSetExample(scheme, strictMockValidationBuilder(t)) + }}, + {"SetCollectionFormat", func(t *testing.T) ifaces.ValueParser { + // OperationValidationBuilder — use the op-variant mock, fail-all. + m := &mocks.MockOperationValidationBuilder{ + SetCollectionFormatFunc: func(v string) { t.Fatalf("SetCollectionFormat should not be called (arg: %s)", v) }, + } + return NewSetCollectionFormat(m) + }}, + {"SetReadOnlySchema", func(_ *testing.T) ifaces.ValueParser { return NewSetReadOnlySchema(new(oaispec.Schema)) }}, + {"SetDiscriminator", func(_ *testing.T) ifaces.ValueParser { return NewSetDiscriminator(new(oaispec.Schema), "kind") }}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := tc.factory(t) + for _, in := range emptyInputs { + require.NoError(t, p.Parse(in)) + } + }) + } +} + +// TestValidationParsers_NumericOverflow pins the overflow defence we kept +// in D.5: the regex captures \p{N}+ (any-length digit string), which matches +// values beyond int64 / float64 range. strconv.ParseInt / ParseFloat returns +// ErrRange in those cases, and the parser must propagate the error without +// invoking the target builder. See .claude/plans/dead-code-cleanup.md D.5 +// post-mortem for the rationale. +func TestValidationParsers_NumericOverflow(t *testing.T) { + t.Parallel() + + // int64 max is 9223372036854775807 (19 digits); 20+ 9's overflows. + intOverflow := strings.Repeat("9", 25) + // float64 max is ~1.8e308 in magnitude; 400 9's in decimal notation + // overflows ParseFloat (returns +Inf, ErrRange). + floatOverflow := strings.Repeat("9", 400) + + cases := []struct { + name string + line string + newP func(*testing.T) ifaces.ValueParser + }{ + { + name: "SetMaximum float overflow", + line: "maximum: " + floatOverflow, + newP: func(t *testing.T) ifaces.ValueParser { return NewSetMaximum(strictMockValidationBuilder(t)) }, + }, + { + name: "SetMinimum float overflow", + line: "minimum: " + floatOverflow, + newP: func(t *testing.T) ifaces.ValueParser { return NewSetMinimum(strictMockValidationBuilder(t)) }, + }, + { + name: "SetMultipleOf float overflow", + line: "multiple of: " + floatOverflow, + newP: func(t *testing.T) ifaces.ValueParser { return NewSetMultipleOf(strictMockValidationBuilder(t)) }, + }, + { + name: "SetMaxItems int overflow", + line: "max items: " + intOverflow, + newP: func(t *testing.T) ifaces.ValueParser { return NewSetMaxItems(strictMockValidationBuilder(t)) }, + }, + { + name: "SetMinItems int overflow", + line: "min items: " + intOverflow, + newP: func(t *testing.T) ifaces.ValueParser { return NewSetMinItems(strictMockValidationBuilder(t)) }, + }, + { + name: "SetMaxLength int overflow", + line: "max length: " + intOverflow, + newP: func(t *testing.T) ifaces.ValueParser { return NewSetMaxLength(strictMockValidationBuilder(t)) }, + }, + { + name: "SetMinLength int overflow", + line: "min length: " + intOverflow, + newP: func(t *testing.T) ifaces.ValueParser { return NewSetMinLength(strictMockValidationBuilder(t)) }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := tc.newP(t) + require.TrueT(t, p.Matches(tc.line), "regex must match overflow input; otherwise the guard we're testing is dead") + err := p.Parse([]string{tc.line}) + require.Error(t, err, "expected ParseInt/ParseFloat ErrRange") + }) + } +} diff --git a/internal/scanner/index.go b/internal/scanner/index.go index ef40a8e..4a68828 100644 --- a/internal/scanner/index.go +++ b/internal/scanner/index.go @@ -342,7 +342,7 @@ func (a *TypeIndex) detectNodes(file *ast.File) (node, error) { return 0, err } case "strfmt", "name", "discriminated", "file", "enum", "default", "alias", "type": - // TODO: perhaps collect these and pass along to avoid lookups later on + // Proposal for enhancement: perhaps collect these and pass along to avoid lookups later on case "allOf": case "ignore": default: diff --git a/internal/scantest/golden.go b/internal/scantest/golden.go index 88a04e8..e3f1f10 100644 --- a/internal/scantest/golden.go +++ b/internal/scantest/golden.go @@ -20,7 +20,7 @@ import ( // // This is the regression-testing harness used to detect any behavior change // in the go-openapi/spec objects produced by the scanner, compared against -// a captured baseline. See .claude/plans/regression-strategy.md. +// a captured baseline. // // Golden files are named by content (fixture bundle + object kind + entity), // not by test name, so they survive test reshuffling.