String slice issue v1 #981

asahasrabuddhe · 2019-12-08T07:57:45Z

Motivation

Fixes #790

The issue describes a bug where the default values of a Slice type flag persist even after the user explicitly sets the flag. Consider the following code:

package main

import (
	"fmt"
	"log"
	"os"

	"github.com/urfave/cli"
)

func main() {
	app := cli.NewApp()
	app.Flags = []cli.Flag{
		cli.StringSliceFlag{
			Name: "Hostnames, n",
			Usage: "`HOSTNAME`s to listen to.",
			Value: &cli.StringSlice{"localhost"},
		},
	}
	app.Action = func(c *cli.Context) error {
		if c.IsSet("Hostnames") {
			// do something
		}

		if c.IsSet("n") {
			// do something else
		}

		return nil
	}

	err := app.Run(os.Args)
	if err != nil {
		log.Fatal()
	}
}

Suppose, we execute this code using --Hostnames a.b.c --Hostnames e.f.g, we would expect the c.StringSlice("Hostnames") function to return a StringSlice with length equal to 2 and containing a.b.c and e.f.g. As of writing this PR, the call to c.StringSlice("Hostnames") returns a StringSlice with length equal to 3 and containing localhost, a.b.c and e.f.g. This behavior is incorrect.

The same behavior can be replicated in the IntSliceFlag and Int64SliceFlag also. This PR aims to fix this inconsistency.

Release Notes

[BUG FIX] String flag no longer persists the default value if the flag is explicitly initialized.

Changes

flag_int64_slice.go: Update lookupInt64Slice function to extract default values of the flag and then subsequently remove them.
flag_int_slice.go: Update lookupIntSlice function to extract default values of the flag and then subsequently remove them.
flag_string_slice.go: Update lookupStringSlice function to extract default values of the flag and then subsequently remove them.
flag_test.go: Add unit tests.

Testing

Unit tests are added to cover the following scenarios:

Flags are initialized with default values and are not explicitly initialized by the user.
Flags are initialized with default values and are explicitly initialized by the user.
Flags are initialized with default values and are explicitly initialized by the user with values the same as the default values.

codecov · 2019-12-08T08:07:26Z

Codecov Report

Merging #981 into v1 will increase coverage by 0.58%.
The diff coverage is 90.36%.

@@            Coverage Diff             @@
##               v1     #981      +/-   ##
==========================================
+ Coverage   71.65%   72.24%   +0.58%     
==========================================
  Files          30       30              
  Lines        2410     2490      +80     
==========================================
+ Hits         1727     1799      +72     
- Misses        577      581       +4     
- Partials      106      110       +4

Impacted Files	Coverage Δ
flag_int64_slice.go	`73.25% <86.2%> (+6.01%)`	⬆️
flag_string_slice.go	`75.94% <92%> (+6.85%)`	⬆️
flag_int_slice.go	`77.9% <93.1%> (+7.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebf9c3e...0f5ca21. Read the comment docs.

asahasrabuddhe · 2019-12-17T09:24:38Z

bump

rliebz

I'm pausing review here because I think this PR might not be approaching the fix in the ideal way. Right now, we'd be accepting that the new values are getting appended incorrectly, and then removing the default based on their string representation after the fact, rather than preventing them from getting incorrectly joined in the first place.

The bigger issue I see is that there's already code to handle removing the defaults—this is in master:

cli/flag_string_slice.go

Lines 22 to 26 in cae7b0c

    
           func (s *StringSlice) Set(value string) error { 
        
           	if !s.hasBeenSet { 
        
           		s.slice = []string{} 
        
           		s.hasBeenSet = true 
        
           	}

This means on the first setting of a slice flag, we should in theory be zeroing out the value. I'm not sure why that's not working (or no longer working, if it used to), but I think the better fix would be to address that issue at the root rather than patch it downstream on flag lookup.

flag_int64_slice.go

flag_string_slice.go

flag_int64_slice.go

rliebz · 2019-12-17T13:45:18Z

flag_int64_slice.go

 		return parsed
 	}
 	return nil
 }
+
+func removeFromInt64Slice(slice []int64, val int64) (newVal []int64) {


Nit: We shouldn't be using named returns unless the function signature is ambiguous, or we absolutely have to

There are caveats with the scope of a named variable that aren't always intuitive, and naked returns pretty much always obfuscate what code should be doing. It also encourages us to mutate an existing variable rather than returning directly.

I'd probably write the function like this:

func removeFromInt64Slice(slice []int64, val int64) ([]int64) { for i, v := range slice { if v == val { return slice[count+1:] } } return slice }

That said, there's an issue here, which I'm not sure if is intentional or not. This function removes all items up to and including the first occurrence of val, but the name implies that we would just be removing val. I don't think it manifests because of how this function is called, but we should either fix the implementation or rename the function to avoid future misuse of this helper.

I have updated the function to behave like it's named

func removeFromInt64Slice(slice []int64, val int64) []int64 { for i, v := range slice { if v == val { return append(slice[:i], slice[i+1:]...) } } return slice }

See it in action: https://play.golang.org/p/ZjJxp38tKNQ

asahasrabuddhe · 2020-01-26T12:21:29Z

I'm pausing review here because I think this PR might not be approaching the fix in the ideal way. Right now, we'd be accepting that the new values are getting appended incorrectly, and then removing the default based on their string representation after the fact, rather than preventing them from getting incorrectly joined in the first place.

The bigger issue I see is that there's already code to handle removing the defaults—this is in master:

cli/flag_string_slice.go

Lines 22 to 26 in cae7b0c

func (s *StringSlice) Set(value string) error {

if !s.hasBeenSet {

s.slice = []string{}

s.hasBeenSet = true

}

This means on the first setting of a slice flag, we should in theory be zeroing out the value. I'm not sure why that's not working (or no longer working, if it used to), but I think the better fix would be to address that issue at the root rather than patch it downstream on flag lookup.

I agree with your comment but unfortunately, this is right now not possible in the v1 branch because the StringSliceFlag is simply an aliased string slice. To make it work like you say, I would need to rewrite the Slice Flags to make them into structs and then include the has been set.

rliebz

Sorry for missing it earlier, I was definitely comparing this change to what we had in master rather than v1.

Looks pretty good, just a couple small notes.

flag_int_slice.go

flag_int64_slice.go

flag_int_slice.go

flag_test.go

flag_int64_slice.go

- Add check for blank default value - Reduce indentation of code

saschagrunert

LGTM

saschagrunert · 2020-01-30T13:20:11Z

flag_int64_slice.go

+
+func isInt64SliceEqual(newValue, defaultValue []int64) bool {
+	// If one is nil, the other must also be nil.
+	if (newValue == nil) != (defaultValue == nil) {


This is the first time I every see an expression like this in golang. 👀

Woooooooooah, love it!

Since urfave/cli v1.22.3, string flag no longer persists the default value if the flag is explicitly initialized. Please refer to urfave/cli#981. Signed-off-by: Derek Su <derek.su@suse.com>

asahasrabuddhe added 4 commits December 7, 2019 21:54

fix default value persisting after flag is initialized

c34587c

add tests

9a9e396

fix failing tests

a5a927c

add more scenarios

9b12ddb

asahasrabuddhe marked this pull request as ready for review December 8, 2019 08:10

asahasrabuddhe requested a review from a team as a code owner December 8, 2019 08:10

coilysiren assigned asahasrabuddhe Dec 8, 2019

rliebz reviewed Dec 17, 2019

View reviewed changes

coilysiren mentioned this pull request Dec 18, 2019

maint: add gofmt checking to CI #993

Closed

tadaskay mentioned this pull request Jan 13, 2020

discovery.type flag default value is not overridden mysteriumnetwork/node#1495

Closed

improve remove from slice functions

dfb55ca

asahasrabuddhe requested review from rliebz and coilysiren January 26, 2020 12:35

rliebz reviewed Jan 26, 2020

View reviewed changes

flag_int_slice.go Outdated Show resolved Hide resolved

flag_int64_slice.go Outdated Show resolved Hide resolved

flag_int_slice.go Outdated Show resolved Hide resolved

flag_test.go Show resolved Hide resolved

asahasrabuddhe added 2 commits January 26, 2020 20:00

update remove val from int slice function

94e50b3

handle potential panics

809a71d

asahasrabuddhe requested a review from rliebz January 26, 2020 14:47

rliebz previously approved these changes Jan 26, 2020

View reviewed changes

flag_int64_slice.go Outdated Show resolved Hide resolved

Update slice flags

0f5ca21

- Add check for blank default value - Reduce indentation of code

asahasrabuddhe dismissed rliebz’s stale review via 0f5ca21 January 27, 2020 05:40

asahasrabuddhe requested a review from rliebz January 27, 2020 05:42

rliebz approved these changes Jan 30, 2020

View reviewed changes

saschagrunert approved these changes Jan 30, 2020

View reviewed changes

coilysiren approved these changes Feb 26, 2020

View reviewed changes

coilysiren merged commit c354cec into v1 Feb 26, 2020

coilysiren deleted the string-slice-issue-v1 branch February 26, 2020 05:38

coilysiren mentioned this pull request Feb 26, 2020

Release 1.22.3 notes #1077

Merged

coilysiren mentioned this pull request Feb 26, 2020

StringSliceFlag will show it's value as default if failed in Before: #1076

Closed

3 tasks

dependabot bot mentioned this pull request Mar 16, 2021

go-tool: bump github.com/urfave/cli from 1.19.1 to 1.22.5 Workiva/frugal#1508

Merged

tonistiigi mentioned this pull request Feb 17, 2022

buildkitd: fix handling multiple addr flags moby/buildkit#2649

Merged

joeycumines mentioned this pull request Jun 7, 2022

IntSliceFlag and friends don't work correctly with destination (+ behavior quirks relating to value) #1410

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

String slice issue v1 #981

String slice issue v1 #981

asahasrabuddhe commented Dec 8, 2019

codecov bot commented Dec 8, 2019 •

edited

asahasrabuddhe commented Dec 17, 2019

rliebz left a comment

rliebz Dec 17, 2019

asahasrabuddhe Jan 26, 2020

asahasrabuddhe commented Jan 26, 2020

rliebz left a comment

saschagrunert left a comment

saschagrunert Jan 30, 2020

coilysiren Jan 31, 2020

	func (s *StringSlice) Set(value string) error {
	if !s.hasBeenSet {
	s.slice = []string{}
	s.hasBeenSet = true
	}

String slice issue v1 #981

String slice issue v1 #981

Conversation

asahasrabuddhe commented Dec 8, 2019

Motivation

Release Notes

Changes

Testing

codecov bot commented Dec 8, 2019 • edited

Codecov Report

asahasrabuddhe commented Dec 17, 2019

rliebz left a comment

Choose a reason for hiding this comment

rliebz Dec 17, 2019

Choose a reason for hiding this comment

asahasrabuddhe Jan 26, 2020

Choose a reason for hiding this comment

asahasrabuddhe commented Jan 26, 2020

rliebz left a comment

Choose a reason for hiding this comment

saschagrunert left a comment

Choose a reason for hiding this comment

saschagrunert Jan 30, 2020

Choose a reason for hiding this comment

coilysiren Jan 31, 2020

Choose a reason for hiding this comment

codecov bot commented Dec 8, 2019 •

edited