Skip to content

add FieldName API #79

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 8, 2025
Merged

add FieldName API #79

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6384e29
add FieldName API
sivchari May 2, 2025
ac3e5e2
add nil check
sivchari May 3, 2025
b2f3683
add nil check
sivchari May 8, 2025
ec40bd7
hadnle StartExpr
sivchari May 7, 2025
90cf8be
add FieldName test case
sivchari May 8, 2025
c17423e
use ginkgo
sivchari May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions pkg/analysis/commentstart/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
)

const name = "commentstart"
Expand Down Expand Up @@ -58,10 +59,8 @@ func checkField(pass *analysis.Pass, field *ast.Field, tagInfo extractjsontags.F
return
}

var fieldName string
if len(field.Names) > 0 {
fieldName = field.Names[0].Name
} else {
fieldName := utils.FieldName(field)
if fieldName == "" {
fieldName = types.ExprString(field.Type)
}

Expand Down
10 changes: 2 additions & 8 deletions pkg/analysis/helpers/inspector/analyzer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
)

func TestInspector(t *testing.T) {
Expand All @@ -49,14 +50,7 @@ func run(pass *analysis.Pass) (any, error) {
}

inspect.InspectFields(func(field *ast.Field, stack []ast.Node, jsonTagInfo extractjsontags.FieldTagInfo, markersAccess markers.Markers) {
var fieldName string
if len(field.Names) > 0 {
fieldName = field.Names[0].Name
} else if ident, ok := field.Type.(*ast.Ident); ok {
fieldName = ident.Name
}

pass.Reportf(field.Pos(), "field: %v", fieldName)
pass.Reportf(field.Pos(), "field: %v", utils.FieldName(field))

if jsonTagInfo.Name != "" {
pass.Reportf(field.Pos(), "json tag: %v", jsonTagInfo.Name)
Expand Down
11 changes: 6 additions & 5 deletions pkg/analysis/jsontags/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
"sigs.k8s.io/kube-api-linter/pkg/config"

"golang.org/x/tools/go/analysis"
Expand Down Expand Up @@ -75,13 +76,13 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) {
}

func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, tagInfo extractjsontags.FieldTagInfo) {
var prefix string
if len(field.Names) > 0 && field.Names[0] != nil {
prefix = fmt.Sprintf("field %s", field.Names[0].Name)
} else if ident, ok := field.Type.(*ast.Ident); ok {
prefix = fmt.Sprintf("embedded field %s", ident.Name)
prefix := "field %s"
if len(field.Names) == 0 || field.Names[0] == nil {
prefix = "embedded field %s"
}

prefix = fmt.Sprintf(prefix, utils.FieldName(field))

if tagInfo.Missing {
pass.Reportf(field.Pos(), "%s is missing json tag", prefix)
return
Expand Down
4 changes: 2 additions & 2 deletions pkg/analysis/maxlength/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,11 @@ func run(pass *analysis.Pass) (any, error) {
}

func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers) {
if len(field.Names) == 0 || field.Names[0] == nil {
fieldName := utils.FieldName(field)
if fieldName == "" {
return
}

fieldName := field.Names[0].Name
prefix := fmt.Sprintf("field %s", fieldName)

checkTypeExpr(pass, field.Type, field, nil, markersAccess, prefix, markers.KubebuilderMaxLengthMarker, needsStringMaxLength)
Expand Down
7 changes: 4 additions & 3 deletions pkg/analysis/nomaps/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
"sigs.k8s.io/kube-api-linter/pkg/config"
)

Expand Down Expand Up @@ -81,7 +82,7 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field) {
}

if a.policy == config.NoMapsEnforce {
report(pass, field.Pos(), field.Names[0].Name)
report(pass, field.Pos(), utils.FieldName(field))
return
}

Expand All @@ -90,7 +91,7 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field) {
return
}

report(pass, field.Pos(), field.Names[0].Name)
report(pass, field.Pos(), utils.FieldName(field))
}

if a.policy == config.NoMapsIgnore {
Expand All @@ -104,7 +105,7 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field) {
return
}

report(pass, field.Pos(), field.Names[0].Name)
report(pass, field.Pos(), utils.FieldName(field))
}
}

Expand Down
3 changes: 2 additions & 1 deletion pkg/analysis/nophase/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"golang.org/x/tools/go/ast/inspector"
kalerrors "sigs.k8s.io/kube-api-linter/pkg/analysis/errors"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
)

const name = "nophase"
Expand Down Expand Up @@ -68,7 +69,7 @@ func run(pass *analysis.Pass) (any, error) {
return
}

fieldName := field.Names[0].Name
fieldName := utils.FieldName(field)

// First check if the struct field name contains 'phase'
if strings.Contains(strings.ToLower(fieldName), "phase") {
Expand Down
11 changes: 6 additions & 5 deletions pkg/analysis/optionalorrequired/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector"
markershelper "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
"sigs.k8s.io/kube-api-linter/pkg/config"
"sigs.k8s.io/kube-api-linter/pkg/markers"
)
Expand Down Expand Up @@ -107,13 +108,13 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, fieldMarker
return
}

var prefix string
if len(field.Names) > 0 && field.Names[0] != nil {
prefix = fmt.Sprintf("field %s", field.Names[0].Name)
} else if ident, ok := field.Type.(*ast.Ident); ok {
prefix = fmt.Sprintf("embedded field %s", ident.Name)
prefix := "field %s"
if len(field.Names) == 0 || field.Names[0] == nil {
prefix = "embedded field %s"
}

prefix = fmt.Sprintf(prefix, utils.FieldName(field))

hasPrimaryOptional := fieldMarkers.Has(a.primaryOptionalMarker)
hasPrimaryRequired := fieldMarkers.Has(a.primaryRequiredMarker)

Expand Down
6 changes: 3 additions & 3 deletions pkg/analysis/requiredfields/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector"
markershelper "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
"sigs.k8s.io/kube-api-linter/pkg/config"
"sigs.k8s.io/kube-api-linter/pkg/markers"
)
Expand Down Expand Up @@ -71,12 +72,11 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) {
}

func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, fieldMarkers markershelper.MarkerSet, fieldTagInfo extractjsontags.FieldTagInfo) {
if field == nil || len(field.Names) == 0 {
fieldName := utils.FieldName(field)
if fieldName == "" {
return
}

fieldName := field.Names[0].Name

if !fieldMarkers.Has(markers.RequiredMarker) && !fieldMarkers.Has(markers.KubebuilderRequiredMarker) {
// The field is not marked required, so we don't need to check it.
return
Expand Down
4 changes: 2 additions & 2 deletions pkg/analysis/utils/type_check.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ func (t *typeChecker) CheckNode(pass *analysis.Pass, node ast.Node) {
}

func (t *typeChecker) checkField(pass *analysis.Pass, field *ast.Field) {
if field == nil || len(field.Names) == 0 || field.Names[0] == nil {
fieldName := FieldName(field)
if fieldName == "" {
return
}

fieldName := field.Names[0].Name
prefix := fmt.Sprintf("field %s", fieldName)

t.checkTypeExpr(pass, field.Type, field, prefix)
Expand Down
20 changes: 20 additions & 0 deletions pkg/analysis/utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,26 @@ func LookupTypeSpec(pass *analysis.Pass, ident *ast.Ident) (*ast.TypeSpec, bool)
return nil, false
}

// FieldName returns the name of the field. If the field has a name, it returns that name.
// If the field is embedded and it can be converted to an identifier, it returns the name of the identifier.
// If it doesn't have a name and can't be converted to an identifier, it returns an empty string.
func FieldName(field *ast.Field) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the expected fieldName for pointer embedded fields? e.g.

  *StringPtrField `json:"stringPtrField"`

Under current implementation, it would be "". Should the output be *StringPtrField?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thanks, fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we create some test cases for this function that demonstrate it's functionality? Make sure if we make updates here in the future we don't forget why we are handling StarExpr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test cases

if len(field.Names) > 0 && field.Names[0] != nil {
return field.Names[0].Name
}

switch typ := field.Type.(type) {
case *ast.Ident:
return typ.Name
case *ast.StarExpr:
if ident, ok := typ.X.(*ast.Ident); ok {
return ident.Name
}
}

return ""
}

func getFilesForType(pass *analysis.Pass, ident *ast.Ident) (*token.File, *ast.File) {
namedType, ok := pass.TypesInfo.TypeOf(ident).(*types.Named)
if !ok {
Expand Down
67 changes: 67 additions & 0 deletions pkg/analysis/utils/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package utils_test

Check failure on line 1 in pkg/analysis/utils/utils_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

Missed header for check (goheader)

import (
"go/ast"
"testing"

"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
)

func TestFieldName(t *testing.T) {
t.Parallel()

cases := map[string]struct {
field *ast.Field
want string
}{
"field has Names": {
field: &ast.Field{
Names: []*ast.Ident{
{
Name: "foo",
},
},
},
want: "foo",
},
"filed has no Names, but is an Ident": {
field: &ast.Field{
Type: &ast.Ident{
Name: "foo",
},
},
want: "foo",
},
"field has no Names, but is a StarExpr with an Ident": {
field: &ast.Field{
Type: &ast.StarExpr{
X: &ast.Ident{
Name: "foo",
},
},
},
want: "foo",
},
"field has no Names, and is not an Ident or StarExpr": {
field: &ast.Field{
Type: &ast.ArrayType{
Elt: &ast.Ident{
Name: "foo",
},
},
},
want: "",
},
}

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
t.Parallel()

got := utils.FieldName(tc.field)
if got != tc.want {
t.Errorf("got %q, want %q", got, tc.want)
}
})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use ginkgo? Since other tests are using ginkgo already. The example below would show how this would look

func TestUtils(t *testing.T) {
	RegisterFailHandler(Fail)
	RunSpecs(t, "Utils")
}

var _ = Describe("FieldName", func() {
  type fieldNameTestCase struct {
    field *ast.Field
    want string
  }

  DescribeTable("Should extract the field name", func(tc fieldNameTestCase) {
    Expect(utils.FieldName(tc.field)).To(Equal(tc.want), "expected the extracted field name to match)
  },
    Entry("field has Name", fieldNameTestCase{
      field: &ast.Field{
        Names: []*ast.Ident{
          {
            Name: "foo",
          },
        },
      },
      want: "foo",
    }),
    ...
  )
})

The actual TestUtils conventionally goes in a utils_suite_test.go file on its own

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I rewrote the test case'

Loading