Skip to content
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.

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

fix: variant fallback usage #171

Merged
merged 8 commits into from
Dec 21, 2023
Merged

fix: variant fallback usage #171

merged 8 commits into from
Dec 21, 2023

Conversation

thomasheartman
Copy link
Contributor

@thomasheartman thomasheartman commented Dec 18, 2023

This PR fixes a bug in how we use fallback variants in this SDK.

The fallback should be used when there is no flag, when the flag doesn't have any variants, and if the flag has variants, but is disabled. However, prior to this, it was only used if the flag didn't exist.

It addresses the issues in and closes #160.

Discussion points

Overriding FeatureEnabled

Comparing this to the Node SDK, I noticed one thing that we don't seem to do: setting the FeatureEnabled property based on the flag's enabled state.

Because the fallback you pass in also has FeatureEnabled as a property, you can in theory override this. However, that means you can get into situations where the flag is actually enabled, but the variant says FeatureEnabled is false because that's what the fallback says. That seems incorrect to me, so I've added in some handling for that, but I'd like to get a second opinion on it.

Code quality

This may be my first time writing Go, so it's very possible that I've missed some idioms or similar. Please feel free to correct as many things as you want.

This includes the very verbose test setup which accounts for ~85% of this PR.

Comment on lines -432 to -433
if !f.Enabled {
return defaultVariant
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know when we ever hit this? I couldn't find a way to trigger it. One of the tests uses a default strategy (which is always enabled), but with f.Enabled set to false. This seems to still hit the previous strategyResult.enabled, though, so I suspect this isn't used anymore, but I'll leave it in combined with missing flags regardless.

client_test.go Outdated Show resolved Hide resolved
Copy link
Member

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

If you were afraid of non idiomatic go, I might not be the best reviewer, but this certainly looks good to me. I understand what you're doing and I like the added tests.

👍

@@ -214,16 +214,20 @@ type variantOption struct {
// VariantOption provides options for querying if a variant is found or not.
type VariantOption func(*variantOption)

// WithVariantFallback specifies what the value should be if the variant is not found on the
// unleash service.
// WithVariantFallback specifies what the value should be if the

This comment was marked as resolved.

client_test.go Outdated

fallbackVariant := api.Variant{
Name: "fallback-variant",
FeatureEnabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Spacing is a bit weird 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.

That's probably a tabs/spaces thing. Weird that my editor changed to using whatever the other is. Anyway, that line is gone now, so that's not an issue anymore.


variantWithFallbackFunc := client.GetVariant(feature, WithVariantFallbackFunc(fallbackFunc))

assert.Equal(fallbackVariant, *variantWithFallbackFunc)
Copy link
Contributor

Choose a reason for hiding this comment

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

While I get that the FeatureEnabled field is being changed in the previous call to GetVariant that is a bit confusing here looking at the test. I think it might be better if you did:

	fallbackVariant = api.Variant{
		Name: "fallback-variant",
    FeatureEnabled: true,
	}
	fallbackFunc := func(feature string, ctx *context.Context) *api.Variant {
		return &fallbackVariant
	}

then later did the same

	assert.False(variantWithFallbackFunc.Enabled)

	assert.False(variantWithFallbackFunc.FeatureEnabled)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed changing the feature enabled in 251d167 as per your suggestion. With that change, is there still anything here you'd like to address?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, lgtm


variantWithFallbackFunc := client.GetVariant(feature, WithVariantFallbackFunc(fallbackFunc))

assert.Equal(fallbackVariant, *variantWithFallbackFunc)

This comment was marked as resolved.


variantWithFallbackFunc := client.GetVariant(feature, WithVariantFallbackFunc(fallbackFunc))

assert.Equal(fallbackVariant, *variantWithFallbackFunc)

This comment was marked as resolved.

@jameshartig
Copy link
Contributor

I left a few comments. I do think the FeatureEnabled overriding makes sense but since it is a pointer it's a bit risky. There definitely needs to be something in those method docs that makes it very clear that the pointer will be modified.

@jameshartig
Copy link
Contributor

There could actually be a race here where a caller passes in the same pointer to multiple calls to GetVariant and that ends up triggering a race when setting that field. Because of this possible race I'm actually leaning the other way now and maybe think we should just return it as-is (without touching the variant) at least until we have a solution for the race. @thomasheartman @chriswk thoughts?

@thomasheartman
Copy link
Contributor Author

@jameshartig Appreciate the review! I'll address your comments. As for touching / not touching the FeatureEnabled field. Yeah, that's a good point, which is why I was a little wary of it. Because we're dealing with pointers here (as far as I can tell), it makes it trickier to return a struct with just that field overridden. I'm happy to leave out changing the FeatureEnabled field (but I think that should also be clarified in the function comments and the docs).

@thomasheartman
Copy link
Contributor Author

I changed the FeatureEnabled handling in 251d167 and added some extra tests for that explicitly in bc87bd3. Does that seem sensible to you @jameshartig? Oh, and I'd be very interested to hear your thoughts on that handling as well, @chriswk ☺️

@jameshartig
Copy link
Contributor

I changed the FeatureEnabled handling in 251d167 and added some extra tests for that explicitly in bc87bd3. Does that seem sensible to you @jameshartig? Oh, and I'd be very interested to hear your thoughts on that handling as well, @chriswk ☺️

Yep! It looks good now. I do agree that it's unfortunate the field isn't being automatically set but that might be something we can solve down the road in a v5.

@thomasheartman thomasheartman merged commit 4081ef8 into v4 Dec 21, 2023
8 checks passed
@thomasheartman thomasheartman deleted the fix/variant-fallbacks branch December 21, 2023 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

WithVariantFallback and WithVariantFallbackFunc are called when feature isn't found
3 participants