From b598d78a21687804681a528aeba5a871a32d4028 Mon Sep 17 00:00:00 2001
From: Marc Khouzam <marc.khouzam@gmail.com>
Date: Fri, 21 Oct 2022 15:51:17 -0400
Subject: [PATCH] Check for group presence after full initialization

Fixes #1831

By moving the check for help group existence to "ExecuteC()" we no
longer need groups to be added before AddCommand() is called.  This
provides more flexibility to developers and works better with the use
of "init()" for command creation.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
---
 command.go      | 21 +++++++++++++---
 command_test.go | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 4 deletions(-)

diff --git a/command.go b/command.go
index 9d5e9cf5eb..03862a200c 100644
--- a/command.go
+++ b/command.go
@@ -998,6 +998,10 @@ func (c *Command) ExecuteC() (cmd *Command, err error) {
 	// initialize completion at the last point to allow for user overriding
 	c.InitDefaultCompletionCmd()
 
+	// Now that all commands have been created, let's make sure all	 groups
+	// are properly created also
+	c.checkCommandGroups()
+
 	args := c.args
 
 	// Workaround FAIL with "go test -v" or "cobra.test -test.v", see #155
@@ -1092,6 +1096,19 @@ func (c *Command) ValidateRequiredFlags() error {
 	return nil
 }
 
+// checkCommandGroups checks if a command has been added to a group that does not exists.
+// If so, we panic because it indicates a coding error that should be corrected.
+func (c *Command) checkCommandGroups() {
+	for _, sub := range c.commands {
+		// if Group is not defined let the developer know right away
+		if sub.GroupID != "" && !c.ContainsGroup(sub.GroupID) {
+			panic(fmt.Sprintf("group id '%s' is not defined for subcommand '%s'", sub.GroupID, sub.CommandPath()))
+		}
+
+		sub.checkCommandGroups()
+	}
+}
+
 // InitDefaultHelpFlag adds default help flag to c.
 // It is called automatically by executing the c or by calling help and usage.
 // If c already has help flag, it will do nothing.
@@ -1218,10 +1235,6 @@ func (c *Command) AddCommand(cmds ...*Command) {
 			panic("Command can't be a child of itself")
 		}
 		cmds[i].parent = c
-		// if Group is not defined let the developer know right away
-		if x.GroupID != "" && !c.ContainsGroup(x.GroupID) {
-			panic(fmt.Sprintf("Group id '%s' is not defined for subcommand '%s'", x.GroupID, cmds[i].CommandPath()))
-		}
 		// update max lengths
 		usageLen := len(x.Use)
 		if usageLen > c.commandsMaxUseLen {
diff --git a/command_test.go b/command_test.go
index 0adec93de7..3c536d7b34 100644
--- a/command_test.go
+++ b/command_test.go
@@ -1862,6 +1862,72 @@ func TestAddGroup(t *testing.T) {
 	checkStringContains(t, output, "\nTest group\n  cmd")
 }
 
+func TestMissingGroupFirstLevel(t *testing.T) {
+	var rootCmd = &Command{Use: "root", Short: "test", Run: emptyRun}
+
+	rootCmd.AddGroup(&Group{ID: "group", Title: "Test group"})
+	// Use the wrong group ID
+	rootCmd.AddCommand(&Command{Use: "cmd", GroupID: "wrong", Run: emptyRun})
+
+	defer func() {
+		if recover() == nil {
+			t.Errorf("The code should have panicked due to a missing group")
+		}
+	}()
+	executeCommand(rootCmd, "--help")
+}
+
+func TestMissingGroupNestedLevel(t *testing.T) {
+	var rootCmd = &Command{Use: "root", Short: "test", Run: emptyRun}
+	var childCmd = &Command{Use: "child", Run: emptyRun}
+	rootCmd.AddCommand(childCmd)
+
+	childCmd.AddGroup(&Group{ID: "group", Title: "Test group"})
+	// Use the wrong group ID
+	childCmd.AddCommand(&Command{Use: "cmd", GroupID: "wrong", Run: emptyRun})
+
+	defer func() {
+		if recover() == nil {
+			t.Errorf("The code should have panicked due to a missing group")
+		}
+	}()
+	executeCommand(rootCmd, "child", "--help")
+}
+
+func TestMissingGroupForHelp(t *testing.T) {
+	var rootCmd = &Command{Use: "root", Short: "test", Run: emptyRun}
+	var childCmd = &Command{Use: "child", Run: emptyRun}
+	rootCmd.AddCommand(childCmd)
+
+	rootCmd.AddGroup(&Group{ID: "group", Title: "Test group"})
+	// Use the wrong group ID
+	rootCmd.SetHelpCommandGroupID("wrong")
+
+	defer func() {
+		if recover() == nil {
+			t.Errorf("The code should have panicked due to a missing group")
+		}
+	}()
+	executeCommand(rootCmd, "--help")
+}
+
+func TestMissingGroupForCompletion(t *testing.T) {
+	var rootCmd = &Command{Use: "root", Short: "test", Run: emptyRun}
+	var childCmd = &Command{Use: "child", Run: emptyRun}
+	rootCmd.AddCommand(childCmd)
+
+	rootCmd.AddGroup(&Group{ID: "group", Title: "Test group"})
+	// Use the wrong group ID
+	rootCmd.SetCompletionCommandGroupID("wrong")
+
+	defer func() {
+		if recover() == nil {
+			t.Errorf("The code should have panicked due to a missing group")
+		}
+	}()
+	executeCommand(rootCmd, "--help")
+}
+
 func TestSetOutput(t *testing.T) {
 	c := &Command{}
 	c.SetOutput(nil)