Skip to content

Commit

Permalink
Merge pull request #504 from pohly/naming-convention
Browse files Browse the repository at this point in the history 
tighten validation of inlining and metadata
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Sep 3, 2024
2 parents f7e401e + 5b13d40 commit 9e1beec
Show file tree
Hide file tree
Showing 2 changed files with 142 additions and 48 deletions.
38 changes: 21 additions & 17 deletions pkg/generators/rules/names_match.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,6 @@ var (
"-",
)

// Blacklist of JSON names that should skip match evaluation
jsonNameBlacklist = sets.NewString(
// Empty name is used for inline struct field (e.g. metav1.TypeMeta)
"",
// Special case for object and list meta
"metadata",
)

// List of substrings that aren't allowed in Go name and JSON name
disallowedNameSubstrings = sets.NewString(
// Underscore is not allowed in either name
Expand Down Expand Up @@ -73,12 +65,11 @@ is also considered matched.
HTTPJSONSpec httpjsonSpec true
NOTE: JSON names in jsonNameBlacklist should skip evaluation
NOTE: an empty JSON name is valid only for inlined structs or pointer to structs.
It cannot be empty for anything else because capitalization must be set explicitly.
true
podSpec true
podSpec - true
podSpec metadata true
NOTE: metav1.ListMeta and metav1.ObjectMeta by convention must have "metadata" as name.
Other fields may have that JSON name if the field name matches.
*/
type NamesMatch struct{}

Expand Down Expand Up @@ -109,14 +100,30 @@ func (n *NamesMatch) Validate(t *types.Type) ([]string, error) {
continue
}
jsonName := strings.Split(jsonTag, ",")[0]
if !namesMatch(goName, jsonName) {
if !nameIsOkay(m, jsonName) {
fields = append(fields, goName)
}
}
}
return fields, nil
}

func nameIsOkay(member types.Member, jsonName string) bool {
if jsonName == "" {
return member.Type.Kind == types.Struct ||
member.Type.Kind == types.Pointer && member.Type.Elem.Kind == types.Struct
}

typeName := member.Type.String()
switch typeName {
case "k8s.io/apimachinery/pkg/apis/meta/v1.ListMeta",
"k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta":
return jsonName == "metadata"
}

return namesMatch(member.Name, jsonName)
}

// namesMatch evaluates if goName and jsonName match the API rule
// TODO: Use an off-the-shelf CamelCase solution instead of implementing this logic. The following existing
//
Expand All @@ -129,9 +136,6 @@ func (n *NamesMatch) Validate(t *types.Type) ([]string, error) {
// about why they don't satisfy our need. What we need can be a function that detects an acronym at the
// beginning of a string.
func namesMatch(goName, jsonName string) bool {
if jsonNameBlacklist.Has(jsonName) {
return true
}
if !isAllowedName(goName) || !isAllowedName(jsonName) {
return false
}
Expand Down
152 changes: 121 additions & 31 deletions pkg/generators/rules/names_match_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,39 @@ import (
)

func TestNamesMatch(t *testing.T) {
someStruct := &types.Type{
Name: types.Name{Name: "SomeStruct"},
Kind: types.Struct,
}
someStructPtr := &types.Type{
Name: types.Name{Name: "SomeStructPtr"},
Kind: types.Pointer,
Elem: someStruct,
}
intPtr := &types.Type{
Name: types.Name{Name: "IntPtr"},
Kind: types.Pointer,
Elem: types.Int,
}
listMeta := &types.Type{
Name: types.Name{Package: "k8s.io/apimachinery/pkg/apis/meta/v1", Name: "ListMeta"},
Kind: types.Struct,
}
listMetaPtr := &types.Type{
Name: types.Name{Package: "k8s.io/apimachinery/pkg/apis/meta/v1", Name: "ListMetaPtr"},
Kind: types.Pointer,
Elem: listMeta,
}
objectMeta := &types.Type{
Name: types.Name{Package: "k8s.io/apimachinery/pkg/apis/meta/v1", Name: "ObjectMeta"},
Kind: types.Struct,
}
objectMetaPtr := &types.Type{
Name: types.Name{Package: "k8s.io/apimachinery/pkg/apis/meta/v1", Name: "ObjectMetaPtr"},
Kind: types.Pointer,
Elem: objectMeta,
}

tcs := []struct {
// name of test case
name string
Expand Down Expand Up @@ -231,36 +264,7 @@ func TestNamesMatch(t *testing.T) {
},
expected: []string{"PodSpec"},
},
// NOTE: JSON names in jsonNameBlacklist should skip evaluation
// {"", "", true},
{
name: "unspecified",
t: &types.Type{
Kind: types.Struct,
Members: []types.Member{
{
Name: "",
Tags: `json:""`,
},
},
},
expected: []string{},
},
// {"podSpec", "", true},
{
name: "blacklist_empty",
t: &types.Type{
Kind: types.Struct,
Members: []types.Member{
{
Name: "podSpec",
Tags: `json:""`,
},
},
},
expected: []string{},
},
// {"podSpec", "metadata", true},
// {"podSpec", "metadata", false},
{
name: "blacklist_metadata",
t: &types.Type{
Expand All @@ -272,7 +276,7 @@ func TestNamesMatch(t *testing.T) {
},
},
},
expected: []string{},
expected: []string{"podSpec"},
},
{
name: "non_struct",
Expand Down Expand Up @@ -338,6 +342,92 @@ func TestNamesMatch(t *testing.T) {
},
expected: []string{"Pod_Spec"},
},
{
name: "empty_JSON_name",
t: &types.Type{
Kind: types.Struct,
Members: []types.Member{
{
Name: "Int",
Tags: `json:""`, // Not okay!
Type: types.Int,
},
{
Name: "Struct",
Tags: `json:""`, // Okay, inlined.
Type: someStruct,
},
{
Name: "IntPtr",
Tags: `json:""`, // Not okay!
Type: intPtr,
},
{
Name: "StructPtr",
Tags: `json:""`, // Okay, inlined.
Type: someStructPtr,
},
},
},
expected: []string{
"Int",
"IntPtr",
},
},
{
name: "metadata_no_pointers",
t: &types.Type{
Kind: types.Struct,
Members: []types.Member{
{
Name: "ListMeta",
Tags: `json:"listMeta"`, // Not okay, should be "metadata"!
Type: listMeta,
},
{
Name: "ObjectMeta",
Tags: `json:"objectMeta"`, // Not okay, should be metadata"!
Type: objectMeta,
},
{
Name: "SomeStruct",
Tags: `json:"metadata"`, // Not okay, name mismatch!
Type: someStruct,
},
},
},
expected: []string{
"ListMeta",
"ObjectMeta",
"SomeStruct",
},
},
{
name: "metadata_pointers",
t: &types.Type{
Kind: types.Struct,
Members: []types.Member{
{
Name: "ListMeta",
Tags: `json:"listMeta"`, // Okay, convention only applies to struct.
Type: listMetaPtr,
},
{
Name: "ObjectMeta",
Tags: `json:"objectMeta"`, // Okay, convention only applies to struct.
Type: objectMetaPtr,
},
{
Name: "SomeStruct",
Tags: `json:"metadata"`, // Not okay, name mismatch!
Type: someStructPtr,
},
},
},
expected: []string{
"SomeStruct",
},
},
}

n := &NamesMatch{}
Expand Down

0 comments on commit 9e1beec

Please sign in to comment.