Skip to content
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.

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

tighten validation of inlining and metadata #504

Merged
merged 2 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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.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
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
Loading