Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 50 additions & 5 deletions commands/deploy_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,23 +207,35 @@ var _ = Describe("DeployCommand", func() {
command.FileUrlReadTimeout = time.Second
})

// unknown flag - error
Context("with argument that is not a directory or MTA", func() {
// with argument that is not a directory or MTA and a valid and unknown flag (unknown flag - error)
Context("with argument that is not a directory or MTA and a valid and unknown flag", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"x", "-l"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flag")
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -l")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

// with argument that is a directory or MTA and with unknown flag (unknown flag - error)
Context("with argument that is a directory or MTA and with unknown flag", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{mtaArchivePath, "-l"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flag")
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -l")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

// with argument that is a directory or MTA and with unknown flags - some even duplicated (unknown flag - error)
Context("with argument that is a directory or MTA and with unknown flags - some even duplicated", func() {
It("should print incorrect usage (and print the duplicating flags only once), call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{mtaArchivePath, "-nonValidFlag", "-u", "-nonValidFlag", "-nonValidFlag", "--flagInvalid"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -nonValidFlag, --flagInvalid")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})
Expand Down Expand Up @@ -276,7 +288,40 @@ var _ = Describe("DeployCommand", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{mtaArchivePath, "--strategy"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flag")
ex.ExpectFailure(status, output, "Incorrect usage. Parsing of arguments has failed")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

// more than one unknown flag and one valid flag, which is a boolean, but its value is missing
Context("with argument that is a directory or MTA and with more than one unknown flag and one valid flag, which is a boolean, but its value is missing", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{mtaArchivePath, "--nonValidFlag", "--apply-namespace-app-names", "-notAValidOne", "-not-a-valid-one"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: --nonValidFlag, -notAValidOne, -not-a-valid-one")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

// more than one unknown flag and one valid flag, which is a boolean and its value is set
Context("with argument that is a directory or MTA and with more than one unknown flag and one valid flag, which is a boolean and its value is set", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{mtaArchivePath, "--nonValidFlag", "--apply-namespace-app-names", "true", "-notAValidOne", "-not-a-valid-one"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: --nonValidFlag, -notAValidOne, -not-a-valid-one")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

// more than one unknown flag and one valid flag that is not boolean and has a value set
Context("with argument that is a directory or MTA and with more than one unknown flag and one valid flag that is not boolean and has a value set", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{mtaArchivePath, "--nonValidFlag", "--apps-stage-timeout", "8000", "-notAValidOne", "-inavalid-flag"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: --nonValidFlag, -notAValidOne, -inavalid-flag")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})
Expand Down
13 changes: 12 additions & 1 deletion commands/download_mta_op_logs_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,18 @@ var _ = Describe("DownloadMtaOperationLogsCommand", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"-a"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flag")
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -a")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

// with an unknown flag and one valid flag
Context("with an unknown flag and one valid flag", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"-a", "--mta"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -a")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})
Expand Down
70 changes: 69 additions & 1 deletion commands/flags_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ func NewCommandFlagsParser(flag *flag.FlagSet, parser FlagsParser, validator Fla

// Parse parsing the args
func (p *CommandFlagsParser) Parse(args []string) error {
if unknownFlags := collectUnknownFlags(p.flag, args); len(unknownFlags) > 0 {
return fmt.Errorf("Unknown or wrong flags: %s", strings.Join(unknownFlags, ", "))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check going to be executed on each parse? Why not execute it only when the parsing fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the check is going to be executed for each new command's arguments. I thought it was best to have this check for unknown flags as early as possible, because I think its rather a big mistake having unsupported flags and if there are any - then it is better to directly exit with an error and not even go through the execution of the internal ParseFlags() method and after it is finished to make the check for unknown flags in the block after thats supposed to take care of errors returned by ParseFlags() method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach is cleaner code-wise since its clearly separated and fails earlier. Still, if we assume generally that errors with flags are more rare, I think that a check only when the flags.Parse() fails would be preferred.
Also there is the added risk of false-positives when checking before flags.Parse(), since were essentially adding more restrictions to the command execution with the new logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the same unknown flag is present more than once. How many times will it be printed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely more than once - there will be duplicates. Thanks for the suggestion- fixed!

err := p.parser.ParseFlags(p.flag, args)
if err != nil {
return err
Expand Down Expand Up @@ -88,7 +92,7 @@ func (p DefaultCommandFlagsParser) ParseFlags(flags *flag.FlagSet, args []string
// Parse the arguments
err := flags.Parse(args[positionalArgsCount:])
if err != nil {
return errors.New("Unknown or wrong flag")
return errors.New("Parsing of arguments has failed")
}

// Check for wrong arguments
Expand Down Expand Up @@ -124,3 +128,67 @@ func (v DefaultCommandFlagsValidator) ValidateParsedFlags(flags *flag.FlagSet) e

return nil
}

func collectUnknownFlags(flags *flag.FlagSet, args []string) []string {
var unknownFlags []string
alreadySeenUnknownFLags := make(map[string]int)

for i := 0; i < len(args); i++ {
currentArgument := args[i]

if !strings.HasPrefix(currentArgument, "-") {
continue
}

currentFlag := currentArgument
flagName := strings.TrimLeft(currentFlag, "-")

if flagName == "" {
continue
}

isFlagKnown := flags.Lookup(flagName)
if isFlagKnown != nil {
isBoolean := isBoolFlag(isFlagKnown)
if !isBoolean {
i = tryToGetNext(args, i)
}
continue
}

appendOnlyWhenCountIsOne(alreadySeenUnknownFLags, currentFlag, &unknownFlags)
i = tryToGetNext(args, i)
}

return unknownFlags
}

func isBoolFlag(flag *flag.Flag) bool {
type boolFlagInterface interface{ IsBoolFlag() bool }

boolFlag, isInterfaceImplemented := flag.Value.(boolFlagInterface)
if !isInterfaceImplemented {
return false
}

return boolFlag.IsBoolFlag()
}

func tryToGetNext(args []string, currentIndex int) int {
nextIndex := currentIndex + 1
if nextIndex < len(args) {
nextArgument := args[nextIndex]
nextHasPrefixDash := strings.HasPrefix(nextArgument, "-")
if !nextHasPrefixDash {
return nextIndex
}
}
return currentIndex
}

func appendOnlyWhenCountIsOne(alreadySeenUnknownFLags map[string]int, currentFlag string, unknownFlags *[]string) {
alreadySeenUnknownFLags[currentFlag]++
if alreadySeenUnknownFLags[currentFlag] == 1 {
*unknownFlags = append(*unknownFlags, currentFlag)
}
}
11 changes: 11 additions & 0 deletions commands/mta_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,17 @@ var _ = Describe("MtaCommand", func() {
})
})

// with unknown flags and one valid flag - error
Context("with unknown flags and one valid flag", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"--nonValidFlag", "--namespace", "-u"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: --nonValidFlag")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

// can't connect to backend - error
Context("when can't connect to backend", func() {
const host = "x"
Expand Down
16 changes: 14 additions & 2 deletions commands/mta_operations_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,29 @@ var _ = Describe("MtaOperationsCommand", func() {
command.InitializeAll(name, cliConnection, testutil.NewCustomTransport(200), clientFactory, testTokenFactory, deployServiceURLCalculator)
})

// with an unknown flag - error
Context("with an unknown flag", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"-a"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flag")
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -a")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

// // wrong arguments
// with an unknown flag and one valid flag
Context("with an unknown flag and one valid flag", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"-a", "--last"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -a")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

// wrong arguments
Context("with wrong arguments", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
Expand Down
15 changes: 13 additions & 2 deletions commands/mtas_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,24 @@ var _ = Describe("MtasCommand", func() {
command.InitializeAll(name, cliConnection, testutil.NewCustomTransport(200), clientFactory, testTokenFactory, deployServiceURLCalculator)
})

// unknown flag - error
// with an unknown flag - error
Context("with an unknown flag", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"-a"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flag")
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -a")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

// with an unknown flag and one valid flag
Context("with an unknown flag and one valid flag", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"-nonValidFlag", "-u"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -nonValidFlag")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})
Expand Down
12 changes: 11 additions & 1 deletion commands/purge_config_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,17 @@ var _ = Describe("PurgeConfigCommand", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"-a"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flag")
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -a")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})

Context("with an unknown flag and one valid flag", func() {
It("should print incorrect usage, call cf help, and exit with a non-zero status", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"-a", "-u"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -a")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})
Expand Down
2 changes: 1 addition & 1 deletion commands/undeploy_command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ var _ = Describe("UndeployCommand", func() {
output, status := oc.CaptureOutputAndStatus(func() int {
return command.Execute([]string{"test-mta-id", "-unknown-flag"}).ToInt()
})
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flag")
ex.ExpectFailure(status, output, "Incorrect usage. Unknown or wrong flags: -unknown-flag")
Expect(cliConnection.CliCommandArgsForCall(0)).To(Equal([]string{"help", name}))
})
})
Expand Down