Skip to content

Fix overwrite by SetDefault for options that share Value #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 13, 2024
Merged

Fix overwrite by SetDefault for options that share Value #23

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5276b64
Fix overwrite by SetDefault for options that share Value
mafredri Nov 11, 2024
e053fda
fix yaml decoding bug unassigns value
mafredri Nov 13, 2024
457cbbe
add more test cases
mafredri Nov 13, 2024
ad8fe14
name the sort func
mafredri Nov 13, 2024
ee63bee
suggestions from pr
mafredri Nov 13, 2024
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
106 changes: 104 additions & 2 deletions command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,14 @@ func sampleCommand(t *testing.T) *serpent.Command {
Use: "root [subcommand]",
Options: serpent.OptionSet{
serpent.Option{
Name: "verbose",
Flag: "verbose",
Name: "verbose",
Flag: "verbose",
Default: "false",
Value: serpent.BoolOf(&verbose),
},
serpent.Option{
Name: "verbose-old",
Flag: "verbode-old",
Value: serpent.BoolOf(&verbose),
},
serpent.Option{
Expand Down Expand Up @@ -742,6 +748,12 @@ func TestCommand_DefaultsOverride(t *testing.T) {
Value: serpent.StringOf(&got),
YAML: "url",
},
{
Name: "url-deprecated",
Flag: "url-deprecated",
Env: "URL_DEPRECATED",
Value: serpent.StringOf(&got),
},
{
Name: "config",
Flag: "config",
Expand Down Expand Up @@ -790,6 +802,17 @@ func TestCommand_DefaultsOverride(t *testing.T) {
inv.Args = []string{"--config", fi.Name(), "--url", "good.com"}
})

test("EnvOverYAML", "good.com", func(t *testing.T, inv *serpent.Invocation) {
fi, err := os.CreateTemp(t.TempDir(), "config.yaml")
require.NoError(t, err)
defer fi.Close()

_, err = fi.WriteString("url: bad.com")
require.NoError(t, err)

inv.Environ.Set("URL", "good.com")
})

test("YAMLOverDefault", "good.com", func(t *testing.T, inv *serpent.Invocation) {
fi, err := os.CreateTemp(t.TempDir(), "config.yaml")
require.NoError(t, err)
Expand All @@ -800,4 +823,83 @@ func TestCommand_DefaultsOverride(t *testing.T) {

inv.Args = []string{"--config", fi.Name()}
})

test("AltFlagOverDefault", "good.com", func(t *testing.T, inv *serpent.Invocation) {
inv.Args = []string{"--url-deprecated", "good.com"}
})
}

func TestCommand_OptionsWithSharedValue(t *testing.T) {
t.Parallel()

var got string
makeCmd := func(def, altDef string) *serpent.Command {
got = ""
return &serpent.Command{
Options: serpent.OptionSet{
{
Name: "url",
Flag: "url",
Env: "URL",
Default: def,
Value: serpent.StringOf(&got),
},
{
Name: "alt-url",
Flag: "alt-url",
Env: "ALT_URL",
Default: altDef,
Value: serpent.StringOf(&got),
},
},
Handler: (func(i *serpent.Invocation) error {
return nil
}),
}
}

// Check proper value propagation.
err := makeCmd("def.com", "def.com").Invoke().Run()
require.NoError(t, err, "default values are same")
require.Equal(t, "def.com", got)

err = makeCmd("def.com", "").Invoke().Run()
require.NoError(t, err, "other default value is empty")
require.Equal(t, "def.com", got)

err = makeCmd("def.com", "").Invoke("--url", "sup").Run()
require.NoError(t, err)
require.Equal(t, "sup", got)

err = makeCmd("def.com", "").Invoke("--alt-url", "hup").Run()
require.NoError(t, err)
require.Equal(t, "hup", got)
Copy link
Member

Choose a reason for hiding this comment

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

Should we try to handle cases where both are set? Currently it defaults to the last one set. Should we at least make this deterministic, like by alphabetical order?

I have not tested if 1 is set with an env var, the other the flag.

// Passes
err = makeCmd("def.com", "").Invoke("--url", "sup", "--alt-url", "hup").Run()
require.NoError(t, err)
require.Equal(t, serpent.String("hup"), got)

// Fails
err = makeCmd("def.com", "").Invoke("--alt-url", "hup", "--url", "sup").Run()
require.NoError(t, err)
require.Equal(t, serpent.String("hup"), got)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think alphabetical order is going to be intuitive. The current behaviour of "last one wins" makes more sense to me.

Alternatively, we could say that specifying a non-default value for both is an error? That would remove any ambiguity, but would be a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Emyrk it's typical behavior for CLI programs to use the last value of any given flag, for example:

~/Work/Code/serpent mafredri/fix-shared-value-overwrite*
❯ git log -1 --format='%h'
e053fda

~/Work/Code/serpent mafredri/fix-shared-value-overwrite*
❯ git log -1 --format='%h' --format='%H'
e053fdac0b0dd01a0e6278dfbd703bbf7486a840

So I would say the failing test-case is working as intended, just the assertion that should change.

I added some more test-cases to cover this and also the env/flag priority one in 457cbbe.

@johnstcn In this PR we already handle the case where flags specify multiple defaults, and that's OK as long as:

  • Either only one specifies a default
  • Either one or more specify the same exact default
  • If two different defaults are detected, that's an error

Copy link
Member Author

Choose a reason for hiding this comment

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

This made me curious. In the case of envs, it seems to be "first wins". I tested with the following test-cases.

// Both envs are given, first wins.
inv = makeCmd("def.com", "").Invoke()
inv.Environ.Set("URL", "sup")
inv.Environ.Set("ALT_URL", "hup")
err = inv.Run()
require.NoError(t, err)
require.Equal(t, "sup", got)

// Both envs are given, first wins #2.
inv = makeCmd("def.com", "").Invoke()
inv.Environ.Set("ALT_URL", "hup")
inv.Environ.Set("URL", "sup")
err = inv.Run()
require.NoError(t, err)
require.Equal(t, "sup", got)

I'm not sure if we need to do anything about that, but just keep in mind that this behavior has not changed in this PR.


// Both flags are given, last wins.
err = makeCmd("def.com", "").Invoke("--url", "sup", "--alt-url", "hup").Run()
require.NoError(t, err)
require.Equal(t, "hup", got)

// Both flags are given, last wins #2.
err = makeCmd("", "def.com").Invoke("--alt-url", "hup", "--url", "sup").Run()
require.NoError(t, err)
require.Equal(t, "sup", got)

// Both flags are given, option type priority wins.
inv := makeCmd("def.com", "").Invoke("--alt-url", "hup")
inv.Environ.Set("URL", "sup")
err = inv.Run()
require.NoError(t, err)
require.Equal(t, "hup", got)

// Both flags are given, option type priority wins #2.
inv = makeCmd("", "def.com").Invoke("--url", "sup")
inv.Environ.Set("ALT_URL", "hup")
err = inv.Run()
require.NoError(t, err)
require.Equal(t, "sup", got)

// Catch invalid configuration.
err = makeCmd("def.com", "alt-def.com").Invoke().Run()
require.Error(t, err, "default values are different")
}
93 changes: 80 additions & 13 deletions option.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"encoding/json"
"os"
"slices"
"strings"

"github.com/hashicorp/go-multierror"
Expand All @@ -21,6 +22,14 @@ const (
ValueSourceDefault ValueSource = "default"
)

var valueSourcePriority = []ValueSource{
ValueSourceFlag,
ValueSourceEnv,
ValueSourceYAML,
ValueSourceDefault,
ValueSourceNone,
}

// Option is a configuration option for a CLI application.
type Option struct {
Name string `json:"name,omitempty"`
Expand Down Expand Up @@ -305,16 +314,12 @@ func (optSet *OptionSet) SetDefaults() error {

var merr *multierror.Error

for i, opt := range *optSet {
// Skip values that may have already been set by the user.
if opt.ValueSource != ValueSourceNone {
continue
}
Comment on lines -310 to -312
Copy link
Member Author

Choose a reason for hiding this comment

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

Review: This was removed in this refactor because we have a different way of handling values that have been set by the user.

However, this breaks one test in coder/coder: https://github.com/coder/coder/actions/runs/11782339194/job/32817210417?pr=15476#step:7:621

I could add a if opt.Value == nil && opt.ValueSource != ValueSourceNone { continue } here and it will fix the test. The only problem is, I don't understand the logic. If the value has been provided by the user and opt.Value == nil, that's OK. But if the value hasn't been provided, then we error out. Why?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't spent time to try and figure this out. This felt vaguely familiar, so I looked up when it was added.

It was added in this commit: coder/coder@7edea99

And this PR: coder/coder#6934

So it has to relate with YAML support somehow? I was hoping the commit when it was added would have more context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for looking and sharing, that was very helpful! It lead me to this: e053fda (#23).


if opt.Default == "" {
continue
}

// It's common to have multiple options with the same value to
// handle deprecation. We group the options by value so that we
// don't let other options overwrite user input.
groupByValue := make(map[pflag.Value][]*Option)
for i := range *optSet {
opt := &(*optSet)[i]
if opt.Value == nil {
merr = multierror.Append(
merr,
Expand All @@ -325,13 +330,75 @@ func (optSet *OptionSet) SetDefaults() error {
)
continue
}
(*optSet)[i].ValueSource = ValueSourceDefault
if err := opt.Value.Set(opt.Default); err != nil {
groupByValue[opt.Value] = append(groupByValue[opt.Value], opt)
}

sortOptionByValueSourcePriorityOrDefault := func(a, b *Option) int {
if a.ValueSource != b.ValueSource {
for _, vs := range valueSourcePriority {
if a.ValueSource == vs {
return -1
}
if b.ValueSource == vs {
return 1
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is totally fine, and an optimized solution. My morning brain just took a second to understand it.

Can you just throw a comment like, // Sorts by value source then a default value being set

I like using subtraction like this for compare functions.

if a.ValueSource != b.ValueSource {
    return slices.Index(valueSourcePriority, a.ValueSource) - slices.Index(valueSourcePriority, b.ValueSource)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

That's clean, thanks! 👍🏻

if a.Default != b.Default {
if a.Default == "" {
return 1
}
if b.Default == "" {
return -1
}
}
return 0
}
for _, opts := range groupByValue {
// Sort the options by priority and whether or not a default is
// set. This won't affect the value but represents correctness
// from whence the value originated.
slices.SortFunc(opts, sortOptionByValueSourcePriorityOrDefault)

// If the first option has a value source, then we don't need to
// set the default, but mark the source for all options.
if opts[0].ValueSource != ValueSourceNone {
for _, opt := range opts[1:] {
opt.ValueSource = opts[0].ValueSource
}
continue
}

var optWithDefault *Option
for _, opt := range opts {
if opt.Default == "" {
continue
}
if optWithDefault != nil && optWithDefault.Default != opt.Default {
merr = multierror.Append(
merr,
xerrors.Errorf(
"parse %q: multiple defaults set for the same value: %q and %q (%q)",
opt.Name, opt.Default, optWithDefault.Default, optWithDefault.Name,
),
)
continue
}
optWithDefault = opt
Comment on lines +368 to +381
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: this also looks like it could be extracted to its own function and tested independently

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see a benefit currently as this is pretty much "the meat" of this function and we don't need to call it elsewhere.

}
if optWithDefault == nil {
continue
}
if err := optWithDefault.Value.Set(optWithDefault.Default); err != nil {
merr = multierror.Append(
merr, xerrors.Errorf("parse %q: %w", opt.Name, err),
merr, xerrors.Errorf("parse %q: %w", optWithDefault.Name, err),
)
}
for _, opt := range opts {
opt.ValueSource = ValueSourceDefault
}
}

return merr.ErrorOrNil()
}

Expand Down
6 changes: 4 additions & 2 deletions yaml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,10 @@ func (o *Option) setFromYAMLNode(n *yaml.Node) error {
// We treat empty values as nil for consistency with other option
// mechanisms.
if len(n.Content) == 0 {
o.Value = nil
return nil
if o.Value == nil {
return nil
}
return o.Value.Set("")
}
return n.Decode(o.Value)
case yaml.MappingNode:
Expand Down
Loading