Handle empty alias group case

pkg/plugins/verify-owners/verify-owners_test.go

@@ -832,6 +832,16 @@ var ownersAliases = map[string][]byte{
 	"collaborators": []byte(`aliases:
   foo-reviewers:
   - alice
+`),
+	"emptyAliasGroupOwnersAliasesWithEmptyBraces": []byte(`aliases:
+  foo-reviewers:
+  - alice
+  empty-aliases-group: {}
+`),
+	"emptyAliasGroupOwnersAliases": []byte(`aliases:
+  foo-reviewers:
+  - alice
+  empty-aliases-group:
 `),
 }
 
@@ -1435,3 +1445,119 @@ func testOwnersRemoval(clients localgit.Clients, t *testing.T) {
 func TestOwnersRemovalV2(t *testing.T) {
 	testOwnersRemoval(localgit.NewV2, t)
 }
+
+func TestHandleParseAliasesConfigErrors(t *testing.T) {
+	testHandleParseAliasesConfigErrors(localgit.NewV2, t)
+}
+
+func testHandleParseAliasesConfigErrors(clients localgit.Clients, t *testing.T) {
+	var tests = []struct {
+		name                 string
+		filesChanged         []string
+		ownersFile           string
+		ownersAliasesFile    string
+		shouldError          bool
+		expectedErrorMessage string
+		skipTrustedUserCheck bool
+	}{
+		{
+			name:                 "empty alias group with braces in OWNERS_ALIASES file",
+			filesChanged:         []string{"OWNERS_ALIASES"},
+			ownersFile:           "collaboratorsWithAliases",
+			ownersAliasesFile:    "emptyAliasGroupOwnersAliasesWithEmptyBraces",
+			shouldError:          true,
+			expectedErrorMessage: "error parsing aliases config for OWNERS_ALIASES file",
+			skipTrustedUserCheck: true,
+		},
+		{
+			name:                 "empty alias group in OWNERS_ALIASES file",
+			filesChanged:         []string{"OWNERS_ALIASES"},
+			ownersFile:           "collaboratorsWithAliases",
+			ownersAliasesFile:    "emptyAliasGroupOwnersAliases",
+			shouldError:          true,
+			expectedErrorMessage: "error parsing aliases config for OWNERS_ALIASES file",
+			skipTrustedUserCheck: true,
+		},
+	}
+
+	lg, c, err := clients()
+	if err != nil {
+		t.Fatalf("Making localgit: %v", err)
+	}
+	defer func() {
+		if err := lg.Clean(); err != nil {
+			t.Errorf("Cleaning up localgit: %v", err)
+		}
+		if err := c.Clean(); err != nil {
+			t.Errorf("Cleaning up client: %v", err)
+		}
+	}()
+	if err := lg.MakeFakeRepo("org", "repo"); err != nil {
+		t.Fatalf("Making fake repo: %v", err)
+	}
+	for i, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			pr := i + 1
+			// make sure we're on master before branching
+			if err := lg.Checkout("org", "repo", defaultBranch); err != nil {
+				t.Fatalf("Switching to master branch: %v", err)
+			}
+			if err := lg.CheckoutNewBranch("org", "repo", fmt.Sprintf("pull/%d/head", pr)); err != nil {
+				t.Fatalf("Checking out pull branch: %v", err)
+			}
+			pullFiles := map[string][]byte{}
+
+			for _, file := range test.filesChanged {
+				if strings.Contains(file, "OWNERS_ALIASES") {
+					pullFiles[file] = ownersAliases[test.ownersAliasesFile]
+				}
+			}
+
+			if err := lg.AddCommit("org", "repo", pullFiles); err != nil {
+				t.Fatalf("Adding PR commit: %v", err)
+			}
+			sha, err := lg.RevParse("org", "repo", "HEAD")
+			if err != nil {
+				t.Fatalf("Getting commit SHA: %v", err)
+			}
+			pre := &github.PullRequestEvent{
+				PullRequest: github.PullRequest{
+					User: github.User{Login: "author"},
+					Base: github.PullRequestBranch{
+						Ref: defaultBranch,
+					},
+					Head: github.PullRequestBranch{
+						SHA: sha,
+					},
+				},
+			}
+			fghc := newFakeGitHubClient(emptyPatch(test.filesChanged), nil, pr)
+
+			fghc.PullRequests = map[int]*github.PullRequest{}
+			fghc.PullRequests[pr] = &github.PullRequest{
+				Base: github.PullRequestBranch{
+					Ref: fakegithub.TestRef,
+				},
+			}
+
+			froc := makeFakeRepoOwnersClient()
+
+			prInfo := info{
+				org:          "org",
+				repo:         "repo",
+				repoFullName: "org/repo",
+				number:       pr,
+			}
+
+			if test.shouldError {
+				if err := handle(fghc, c, froc, logrus.WithField("plugin", PluginName), &pre.PullRequest, prInfo, []string{labels.Approved, labels.LGTM}, plugins.Trigger{}, test.skipTrustedUserCheck, &fakePruner{}, ownersconfig.FakeResolver); err != nil {
+					if !strings.Contains(err.Error(), test.expectedErrorMessage) {
+						t.Errorf("expected error message to contain %q, but got %q", test.expectedErrorMessage, err.Error())
+					}
+				} else {
+					t.Fatalf("Expected error but got none")
+				}
+			}
+		})
+	}
+}

pkg/repoowners/repoowners.go

@@ -663,14 +663,49 @@ func ParseAliasesConfig(b []byte) (RepoAliases, error) {
 	result := make(RepoAliases)
 
 	config := &struct {
-		Data map[string][]string `json:"aliases,omitempty"`
+		Data map[string]interface{} `json:"aliases,omitempty"`
 	}{}
 	if err := yaml.Unmarshal(b, config); err != nil {
 		return result, err
 	}
 
 	for alias, expanded := range config.Data {
-		result[github.NormLogin(alias)] = NormLogins(expanded)
+		switch v := expanded.(type) {
+		case []interface{}:
+			// Convert []interface{} to []string
+			var members []string
+			for _, member := range v {
+				memberAsString, ok := member.(string)
+				if !ok {
+					return result, fmt.Errorf("unexpected type for alias group member: %T", member)
+				}
+				members = append(members, memberAsString)
+			}
+			if len(members) == 0 {
+				return result, fmt.Errorf("alias group '%s' is empty", alias)
+			}
+			result[github.NormLogin(alias)] = NormLogins(members)
+		case string:
+			// Handle empty string as an empty list
+			if v == "" {
+				return result, fmt.Errorf("alias group '%s' is empty", alias)
+			}
+		case map[string]interface{}:
+			// Handle Flow Style Mapping (Inline Dictionary/Object Syntax). Example - aliases: { alias-group: { alias1, alias2 } }
+			if len(v) == 0 {
+				return result, fmt.Errorf("alias group '%s' is empty", alias)
+			}
+			var members []string
+            for key := range v {
+				members = append(members, key)
+            }
+            result[github.NormLogin(alias)] = NormLogins(members)
+		case nil:
+			// Handle empty alias group as an empty list. Examples - aliases: { alias-group: }
+			return result, fmt.Errorf("alias group '%s' is empty", alias)
+		default:
+			return result, fmt.Errorf("unexpected type for alias group: %T", v)
+		}
 	}
 	return result, nil
 }

pkg/repoowners/repoowners_test.go

@@ -1427,3 +1427,80 @@ func TestRepoOwners_AllReviewers(t *testing.T) {
 		t.Errorf("Expected reviewers: %v\tFound reviewers: %v ", expectedReviewers, sets.List(foundReviewers))
 	}
 }
+
+func TestParseAliasesConfig(t *testing.T) {
+	tests := []struct {
+		name        string
+		input       string
+		expected    RepoAliases
+		wantErr     bool
+		errContainsRegexp string
+	}{
+		{
+			name: "valid aliases",
+			input: `
+aliases:
+  team1:
+    - alice
+    - bob
+  team2:
+    - nikhilnishad
+`,
+			expected: RepoAliases{
+				"team1": sets.New[string]("alice", "bob"),
+				"team2": sets.New[string]("nikhilnishad"),
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid aliases in flow style mapping syntax",
+			input: `
+aliases:
+  team1: {alice, bob}
+`,
+			expected: RepoAliases{
+				"team1": sets.New[string]("alice", "bob"),
+			},
+			wantErr: false,
+		},
+		{
+			name: "empty alias group with braces",
+			input: `
+aliases:
+  empty-alias-group-with-braces: {}
+`,
+			expected:    RepoAliases{},
+			wantErr:     true,
+			errContainsRegexp: "alias group '.*' is empty",
+		},
+		{
+			name: "empty alias group",
+			input: `
+aliases:
+  empty-alias-group-with-empty-string:
+`,
+			expected:    RepoAliases{},
+			wantErr:     true,
+			errContainsRegexp: "alias group '.*' is empty",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ParseAliasesConfig([]byte(tt.input))
+			if (err != nil) != tt.wantErr {
+				t.Errorf("ParseAliasesConfig() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if err != nil {
+				matched, _ := regexp.MatchString(tt.errContainsRegexp, err.Error())
+				if !matched {
+					t.Errorf("ParseAliasesConfig() error = %v, expected to contain %v", err, tt.errContainsRegexp)
+				}
+			}
+			if !reflect.DeepEqual(got, tt.expected) {
+				t.Errorf("ParseAliasesConfig() = %v, want %v", got, tt.expected)
+			}
+		})
+	}
+}