Repository: urfave/cli
Pull request #1805 (2023-08-19 03:32:51)

fix: check duplicated sub command name and alias

## What type of PR is this?
- bug

## What this PR does / why we need it:

Fix https://github.com/urfave/cli/issues/785.

## Which issue(s) this PR fixes:

Fix https://github.com/urfave/cli/issues/785


## Special notes for your reviewer:

This fix is only for v2. Do we need to fix it in other versions?

## Testing

- I test it in unit test.
- Also, I write a dummy program and see the output:

```golang
package main

import (
	"fmt"

	"github.com/urfave/cli/v2"
)

func main() {
	app := &cli.App{
		Name: "linrl3",
		Commands: []*cli.Command{
			{Name: "sub1"},
			{Name: "sub1"},
		},
	}
	if err := app.Run([]string{"linrl3", "sub1"}); err != nil {
		fmt.Println(err)
	}
}
```
result: 
```
result:
➜  cli git:(fix/v2_duplicate_subcommand) ✗ go run cmd/dummyrun/main.go
parent command [linrl3] has duplicated subcommand name or alias: sub1
```
<!--
  Describe how you tested this change.
-->

## Release Notes

_(REQUIRED)_
<!--
  If this PR makes user facing changes, please describe them here. This
  description will be copied into the release notes/changelog, whenever the
  next version is released. Keep this section short, and focus on high level
  changes.

  Put your text between the block. To omit notes, use NONE within the block.
-->

```release-note
- fix: if sub-commands in the same level have same name or alias, an error would return
```


abitrolly: On the surface LGTM. Need somebody to get into deeper logic, like using new `error` module for v2 branch.
linrl3: > On the surface LGTM. Need somebody to get into deeper logic, like using new `error` module for v2 branch.

@abitrolly Who do you think should review the code? 

Patch:
diff --git a/app.go b/app.go
index 19b76c9..e07dd7b 100644
--- a/app.go
+++ b/app.go
@@ -329,6 +329,9 @@ func (a *App) RunContext(ctx context.Context, arguments []string) (err error) {
 	a.rootCommand = a.newRootCommand()
 	cCtx.Command = a.rootCommand
 
+	if err := checkDuplicatedCmds(a.rootCommand); err != nil {
+		return err
+	}
 	return a.rootCommand.Run(cCtx, arguments...)
 }
 
diff --git a/command.go b/command.go
index 69a0fdf..c318da9 100644
--- a/command.go
+++ b/command.go
@@ -149,6 +149,9 @@ func (c *Command) Run(cCtx *Context, arguments ...string) (err error) {
 
 	if !c.isRoot {
 		c.setup(cCtx)
+		if err := checkDuplicatedCmds(c); err != nil {
+			return err
+		}
 	}
 
 	a := args(arguments)
@@ -404,3 +407,16 @@ func hasCommand(commands []*Command, command *Command) bool {
 
 	return false
 }
+
+func checkDuplicatedCmds(parent *Command) error {
+	seen := make(map[string]struct{})
+	for _, c := range parent.Subcommands {
+		for _, name := range c.Names() {
+			if _, exists := seen[name]; exists {
+				return fmt.Errorf("parent command [%s] has duplicated subcommand name or alias: %s", parent.Name, name)
+			}
+			seen[name] = struct{}{}
+		}
+	}
+	return nil
+}


Test patch:
diff --git a/app_test.go b/app_test.go
index 981d074..d07d2c0 100644
--- a/app_test.go
+++ b/app_test.go
@@ -3087,3 +3087,92 @@ func TestFlagAction(t *testing.T) {
 		})
 	}
 }
+
+func TestDuplicateSubcommand(t *testing.T) {
+	var testdata = []struct {
+		app           *App
+		expectNoError bool
+	}{
+		{&App{
+			Name: "p1",
+		}, true},
+		{&App{
+			Name:     "p2",
+			Commands: []*Command{},
+		}, true},
+		{&App{
+			Name:     "p3",
+			Commands: []*Command{{Name: "sub1"}},
+		}, true},
+		{&App{
+			Name:     "p4",
+			Commands: []*Command{{Name: "sub1"}, {Name: "sub1"}},
+		}, false},
+		{&App{
+			Name:     "p5",
+			Commands: []*Command{{Name: "sub1"}, {Name: "sub2", Aliases: []string{"sub1"}}},
+		}, false},
+		{&App{
+			Name:     "p6",
+			Commands: []*Command{{Name: "sub1"}, {Name: "sub2"}},
+		}, true},
+	}
+	for _, tt := range testdata {
+		err := tt.app.Run([]string{})
+		if tt.expectNoError {
+			expect(t, err, nil)
+		} else {
+			expectNotEqual(t, err, nil)
+		}
+
+		err = checkDuplicatedCmds(tt.app.rootCommand)
+		if tt.expectNoError {
+			expect(t, err, nil)
+		} else {
+			expectNotEqual(t, err, nil)
+		}
+	}
+
+	var testNested = []struct {
+		app               *App
+		subcommandTocheck string
+		expectNoError     bool
+	}{
+		{
+			&App{
+				Name: "nested-0",
+				Commands: []*Command{
+					{Name: "sub1",
+						Subcommands: []*Command{
+							{Name: "sub1_a"},
+							{Name: "sub1_b"},
+						},
+					},
+					{Name: "sub2"}},
+			},
+			"sub1",
+			true},
+		{&App{
+			Name: "nested-1",
+			Commands: []*Command{
+				{Name: "sub1",
+					Subcommands: []*Command{
+						{Name: "sub1_a"},
+						{Name: "sub1_a"},
+					},
+				},
+				{Name: "sub2"}},
+		},
+			"sub1",
+			false},
+	}
+
+	for _, tt := range testNested {
+		err := tt.app.Run([]string{tt.app.Name, tt.subcommandTocheck})
+		if tt.expectNoError {
+			expect(t, err, nil)
+		} else {
+			expectNotEqual(t, err, nil)
+		}
+	}
+}
diff --git a/helpers_test.go b/helpers_test.go
index 9ecd8e1..30981e5 100644
--- a/helpers_test.go
+++ b/helpers_test.go
@@ -17,3 +17,11 @@ func expect(t *testing.T, a interface{}, b interface{}) {
 		t.Errorf("Expected %v (type %v) - Got %v (type %v)", b, reflect.TypeOf(b), a, reflect.TypeOf(a))
 	}
 }
+
+func expectNotEqual(t *testing.T, a interface{}, b interface{}) {
+	t.Helper()
+
+	if reflect.DeepEqual(a, b) {
+		t.Errorf("Expected not equal, but got: %v (type %v), %v (type %v) ", b, reflect.TypeOf(b), a, reflect.TypeOf(a))
+	}
+}