Fix overwrite by SetDefault for options that share Value #23

mafredri · 2024-11-11T14:30:24Z

While working on tests for coder/internal#209 I noticed we don't properly handle shared values between options.

Perhaps this isn't the intended use-case, but it's definitely something that happens in the real world. Consider this example:

	emailFrom := serpent.Option{
		Name:        "Email: From Address",
		Description: "The sender's address to use.",
		Flag:        "email-from",
		Env:         "CODER_EMAIL_FROM",
		Value:       &c.Notifications.SMTP.From,
		Group:       &deploymentGroupEmail,
		YAML:        "from",
	}
	// ...
		{
			Name:        "Notifications: Email: From Address",
			Description: "The sender's address to use.",
			Flag:        "notifications-email-from",
			Env:         "CODER_NOTIFICATIONS_EMAIL_FROM",
			Value:       &c.Notifications.SMTP.From,
			Group:       &deploymentGroupNotificationsEmail,
			YAML:        "from",
			UseInstead:  serpent.OptionSet{emailFrom},
		},

See how both options reference c.Notifications.SMTP.From? This is still OK, but if the user adds a Default to either (or both) of these, the behavior will be very unexpected.

This PR fixes this in a way that handles this use-case. A simpler approach would be to return an error if two flags reference the same value, but I feel that would be restrictive and go against common Go patterns with regard to flag parsing (i.e. sharing the same var).

mafredri · 2024-11-11T16:51:51Z

option.go

-		if opt.ValueSource != ValueSourceNone {
-			continue
-		}


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?

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.

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

Emyrk · 2024-11-12T22:51:12Z

command_test.go

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


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)

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.

@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

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.

option.go

johnstcn · 2024-11-13T09:31:36Z

option.go

+			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


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

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.

johnstcn · 2024-11-13T09:33:56Z

command_test.go

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


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.

Emyrk

I updated my go mod to this branch, and that failing you mentioned test is passing locally.

Emyrk · 2024-11-13T14:35:55Z

option.go

+		if a.ValueSource != b.ValueSource {
+			for _, vs := range valueSourcePriority {
+				if a.ValueSource == vs {
+					return -1
+				}
+				if b.ValueSource == vs {
+					return 1
+				}
+			}
+		}


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) }

That's clean, thanks! 👍🏻

johnstcn

Thanks for doing this Mathias!

ammario · 2024-11-13T18:30:40Z

My instinct would be to error if the value is the same as the semantics around priority are not immediately obvious and perhaps better for clients to decide with their own logic. It seems like you have a lot of eyes here already so feel free to merge at will.

mafredri · 2024-11-13T22:08:13Z

My instinct would be to error if the value is the same as the semantics around priority are not immediately obvious and perhaps better for clients to decide with their own logic. It seems like you have a lot of eyes here already so feel free to merge at will.

Yeah, I was a bit torn myself between the two. In the end settled on this approach because it makes maintenance of deprecated flags a lot easier, especially if all you're doing is renaming or reorganizing.

If we enforce the use of two different unique values, then there must always exist logic to check both or merge them at an appropriate time, and that seems like a recipe for bugs.

Emyrk · 2024-11-13T22:35:47Z

If we enforce the use of two different unique values, then there must always exist logic to check both or merge them at an appropriate time, and that seems like a recipe for bugs.

Agreed. I was sold when @johnstcn mentioned flag order already overrides. And it is the existing behavior.

Fix overwrite by SetDefault for options that share Value

5276b64

mafredri requested review from ethanndickson and ammario November 11, 2024 14:38

mafredri mentioned this pull request Nov 11, 2024

fix(cli): add backwards compat for old telemetry env and tests coder/coder#15476

Merged

mafredri commented Nov 11, 2024

View reviewed changes

mafredri self-assigned this Nov 12, 2024

Emyrk reviewed Nov 12, 2024

View reviewed changes

johnstcn reviewed Nov 13, 2024

View reviewed changes

DanielleMaywood mentioned this pull request Nov 13, 2024

fix: remove defaults for CODER_EMAIL_ options coder/coder#15482

Closed

mafredri added 3 commits November 13, 2024 13:00

fix yaml decoding bug unassigns value

e053fda

add more test cases

457cbbe

name the sort func

ad8fe14

Emyrk approved these changes Nov 13, 2024

View reviewed changes

johnstcn approved these changes Nov 13, 2024

View reviewed changes

suggestions from pr

ee63bee

mafredri merged commit 09f1207 into main Nov 13, 2024
1 check passed

mafredri deleted the mafredri/fix-shared-value-overwrite branch November 13, 2024 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix overwrite by SetDefault for options that share Value #23

Fix overwrite by SetDefault for options that share Value #23

mafredri commented Nov 11, 2024 •

edited

Loading

mafredri Nov 11, 2024

Emyrk Nov 12, 2024

mafredri Nov 13, 2024

Emyrk Nov 12, 2024

johnstcn Nov 13, 2024

mafredri Nov 13, 2024

mafredri Nov 13, 2024

johnstcn Nov 13, 2024

mafredri Nov 13, 2024

johnstcn Nov 13, 2024

Emyrk left a comment

Emyrk Nov 13, 2024

mafredri Nov 13, 2024

johnstcn left a comment

ammario commented Nov 13, 2024

mafredri commented Nov 13, 2024

Emyrk commented Nov 13, 2024

Fix overwrite by SetDefault for options that share Value #23

Fix overwrite by SetDefault for options that share Value #23

Conversation

mafredri commented Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emyrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

ammario commented Nov 13, 2024

mafredri commented Nov 13, 2024

Emyrk commented Nov 13, 2024

mafredri commented Nov 11, 2024 •

edited

Loading