From 5b13d4000edb05d58a02f0b2c865af91a51effff Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Mon, 26 Aug 2024 11:34:41 +0200 Subject: [PATCH] names match: tighten validation of inlining and metadata 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". --- pkg/generators/rules/names_match.go | 38 +++++++------ pkg/generators/rules/names_match_test.go | 69 +++++++----------------- 2 files changed, 41 insertions(+), 66 deletions(-) diff --git a/pkg/generators/rules/names_match.go b/pkg/generators/rules/names_match.go index af30edc5e..d7655f0d9 100644 --- a/pkg/generators/rules/names_match.go +++ b/pkg/generators/rules/names_match.go @@ -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 @@ -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{} @@ -109,7 +100,7 @@ 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) } } @@ -117,6 +108,22 @@ func (n *NamesMatch) Validate(t *types.Type) ([]string, error) { 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 // @@ -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 } diff --git a/pkg/generators/rules/names_match_test.go b/pkg/generators/rules/names_match_test.go index 185167547..1e0954704 100644 --- a/pkg/generators/rules/names_match_test.go +++ b/pkg/generators/rules/names_match_test.go @@ -29,32 +29,32 @@ func TestNamesMatch(t *testing.T) { Kind: types.Struct, } someStructPtr := &types.Type{ - Name: types.Name{Name: "SomeStructPtr"}, - Kind: types.Pointer, - Underlying: someStruct, + Name: types.Name{Name: "SomeStructPtr"}, + Kind: types.Pointer, + Elem: someStruct, } intPtr := &types.Type{ - Name: types.Name{Name: "IntPtr"}, - Kind: types.Pointer, - Underlying: types.Int, + 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, - Underlying: listMeta, + 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, - Underlying: objectMeta, + Name: types.Name{Package: "k8s.io/apimachinery/pkg/apis/meta/v1", Name: "ObjectMetaPtr"}, + Kind: types.Pointer, + Elem: objectMeta, } tcs := []struct { @@ -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{ @@ -305,7 +276,7 @@ func TestNamesMatch(t *testing.T) { }, }, }, - expected: []string{}, + expected: []string{"podSpec"}, }, { name: "non_struct", @@ -399,8 +370,8 @@ func TestNamesMatch(t *testing.T) { }, }, expected: []string{ - // "Int", TODO: should be reported! - // "IntPtr", TODO: should be reported! + "Int", + "IntPtr", }, }, { @@ -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", }, }, { @@ -454,7 +425,7 @@ func TestNamesMatch(t *testing.T) { }, }, expected: []string{ - // "SomeStruct", TODO: should be reported! + "SomeStruct", }, }, }