Skip to content

Commit

Permalink
names match: tighten validation of inlining and metadata
Browse files Browse the repository at this point in the history
Because "" and "metadata" were excluded from checking, some incorrect
usage of those were not detected.

Also, naming ListMeta or ObjectMeta "listMeta" or "objectMeta" was allowed
because the names match, even though by convention the name should have
been "metadata".
  • Loading branch information
pohly committed Aug 26, 2024
1 parent 4fca853 commit 3d12da0
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 54 deletions.
38 changes: 21 additions & 17 deletions pkg/generators/rules/names_match.go
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.Underlying.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
45 changes: 8 additions & 37 deletions pkg/generators/rules/names_match_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,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 @@ -305,7 +276,7 @@ func TestNamesMatch(t *testing.T) {
},
},
},
expected: []string{},
expected: []string{"podSpec"},
},
{
name: "non_struct",
Expand Down Expand Up @@ -399,8 +370,8 @@ func TestNamesMatch(t *testing.T) {
},
},
expected: []string{
// "Int", TODO: should be reported!
// "IntPtr", TODO: should be reported!
"Int",
"IntPtr",
},
},
{
Expand All @@ -426,9 +397,9 @@ func TestNamesMatch(t *testing.T) {
},
},
expected: []string{
// "ListMeta", TODO: should be reported!
// "ObjectMeta", TODO: should be reported!
// "SomeStruct", TODO: should be reported!
"ListMeta",
"ObjectMeta",
"SomeStruct",
},
},
{
Expand All @@ -454,7 +425,7 @@ func TestNamesMatch(t *testing.T) {
},
},
expected: []string{
// "SomeStruct", TODO: should be reported!
"SomeStruct",
},
},
}
Expand Down

0 comments on commit 3d12da0

Please sign in to comment.