Handle empty alias group case

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

@@ -832,6 +832,11 @@ var ownersAliases = map[string][]byte{
 	"collaborators": []byte(`aliases:
   foo-reviewers:
   - alice
+`),
+	"emptyAliasGroupOwnersAliases": []byte(`aliases:
+  foo-reviewers:
+  - alice
+  empty-aliases-group: {}
 `),
 }
 
@@ -1435,3 +1440,109 @@ 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 additions 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,42 @@ 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 {
+				members = append(members, member.(string))
+			}
+			if len(members) == 0 {
+				return result, fmt.Errorf("alias group '%s' is empty", alias)
+			}
+			result[github.NormLogin(alias)] = NormLogins(members)
+		case map[interface{}]interface{}:
+			// Handle empty braces {} as an empty list
+			if len(v) == 0 {
+				return result, fmt.Errorf("alias group '%s' is empty", alias)
+			}
+		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 nested map[string]interface{} as an empty list
+			if len(v) == 0 {
+				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,63 @@ 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
+		errContains 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: "empty alias group with braces",
+            input: `
+aliases:
+  empty-alias-group-with-braces: {}
+`,
+            expected: RepoAliases{},
+            wantErr:  true,
+			errContains: "alias group is empty",
+        },
+        {
+            name: "empty alias group with empty string",
+            input: `
+aliases:
+  empty-alias-group-with-empty-string:
+`,
+            expected: RepoAliases{},
+            wantErr:  true,
+			errContains: "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 !reflect.DeepEqual(got, tt.expected) {
+                t.Errorf("ParseAliasesConfig() = %v, want %v", got, tt.expected)
+            }
+        })
+    }
+}