Merge pull request #504 from pohly/naming-convention

tighten validation of inlining and metadata

Author: k8s-ci-robot

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,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
 //
@@ -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
 	}

pkg/generators/rules/names_match_test.go

@@ -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
@@ -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{
@@ -272,7 +276,7 @@ func TestNamesMatch(t *testing.T) {
 					},
 				},
 			},
-			expected: []string{},
+			expected: []string{"podSpec"},
 		},
 		{
 			name: "non_struct",
@@ -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{}