Skip to content

Commit dec9ae4

Browse files
authored
fix: Do not convert string variant results into bool (#87)
The words "true" and "false" are valid variant keys. It's incorrect to convert them to `true` and `false`. This could result in a case where `isEnabled` returns `false` when in fact, it's `true` but the variant selected is named "false". Added a test to confirm this change.
1 parent 5e4cd82 commit dec9ae4

File tree

4 files changed

+73
-14
lines changed

4 files changed

+73
-14
lines changed

feature_flags_test.go

+69-2
Original file line numberDiff line numberDiff line change
@@ -947,7 +947,7 @@ func TestGetFeatureFlagPayload(t *testing.T) {
947947

948948
func TestGetRemoteConfigPayload(t *testing.T) {
949949
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
950-
w.Write([]byte(fixture("test-remote-config.json")))
950+
w.Write([]byte(fixture("test-remote-config.json")))
951951
}))
952952

953953
defer server.Close()
@@ -971,7 +971,6 @@ func TestGetRemoteConfigPayload(t *testing.T) {
971971
}
972972
}
973973

974-
975974
func TestFlagWithVariantOverrides(t *testing.T) {
976975

977976
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -4646,3 +4645,71 @@ func TestFetchFlagsFails(t *testing.T) {
46464645
t.Error("Expected to be called", expectedCalls, "times but got", actualCalls)
46474646
}
46484647
}
4648+
4649+
func TestFeatureFlagWithFalseVariant(t *testing.T) {
4650+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
4651+
if strings.HasPrefix(r.URL.Path, "/api/feature_flag/local_evaluation") {
4652+
w.Write([]byte(`{
4653+
"flags": [{
4654+
"id": 719,
4655+
"name": "",
4656+
"key": "test-flag",
4657+
"filters": {
4658+
"groups": [
4659+
{
4660+
"properties": [],
4661+
"rollout_percentage": 100
4662+
}
4663+
],
4664+
"multivariate": {
4665+
"variants": [
4666+
{"key": "false", "rollout_percentage": 50},
4667+
{"key": "true", "rollout_percentage": 50}
4668+
]
4669+
}
4670+
},
4671+
"deleted": false,
4672+
"active": true,
4673+
"is_simple_flag": false,
4674+
"rollout_percentage": null
4675+
}]
4676+
}`))
4677+
}
4678+
}))
4679+
defer server.Close()
4680+
4681+
client, _ := NewWithConfig("Csyjlnlun3OzyNJAafdlv", Config{
4682+
PersonalApiKey: "some very secret key",
4683+
Endpoint: server.URL,
4684+
})
4685+
defer client.Close()
4686+
4687+
// Test GetFeatureFlag - should return the variant name "false"
4688+
variant, err := client.GetFeatureFlag(
4689+
FeatureFlagPayload{
4690+
Key: "test-flag",
4691+
DistinctId: "test-id",
4692+
},
4693+
)
4694+
if err != nil {
4695+
t.Error("Unexpected error:", err)
4696+
}
4697+
if variant != "false" {
4698+
t.Errorf("Expected variant 'false', got: %v", variant)
4699+
}
4700+
4701+
// Test IsFeatureEnabled - should return the variant name "false"
4702+
// This will fail with the current implementation because it incorrectly converts "false" to false
4703+
isEnabled, err := client.IsFeatureEnabled(
4704+
FeatureFlagPayload{
4705+
Key: "test-flag",
4706+
DistinctId: "test-id",
4707+
},
4708+
)
4709+
if err != nil {
4710+
t.Error("Unexpected error:", err)
4711+
}
4712+
if isEnabled != "false" {
4713+
t.Errorf("Expected variant 'false', got: %v", isEnabled)
4714+
}
4715+
}

featureflags.go

+2-4
Original file line numberDiff line numberDiff line change
@@ -1009,10 +1009,8 @@ func (poller *FeatureFlagsPoller) getFeatureFlagVariant(featureFlag FeatureFlag,
10091009
}
10101010

10111011
for flagKey, flagValue := range featureFlagVariants.FeatureFlags {
1012-
flagValueString := fmt.Sprintf("%v", flagValue)
1013-
if key == flagKey && flagValueString != "false" {
1014-
result = flagValueString
1015-
break
1012+
if key == flagKey {
1013+
return flagValue, nil
10161014
}
10171015
}
10181016
return result, nil

posthog.go

-6
Original file line numberDiff line numberDiff line change
@@ -287,12 +287,6 @@ func (c *client) IsFeatureEnabled(flagConfig FeatureFlagPayload) (interface{}, e
287287
return nil, err
288288
}
289289

290-
if result == "false" {
291-
result = false
292-
} else if result == "true" {
293-
result = true
294-
}
295-
296290
return result, nil
297291
}
298292

posthog_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -842,7 +842,7 @@ func TestIsFeatureEnabled(t *testing.T) {
842842
DistinctId: "user789",
843843
},
844844
mockResponse: `{"featureFlags": {"test-flag": "true"}}`,
845-
expectedResult: true,
845+
expectedResult: "true",
846846
},
847847
{
848848
name: "Feature flag is a string 'false'",
@@ -851,7 +851,7 @@ func TestIsFeatureEnabled(t *testing.T) {
851851
DistinctId: "user101",
852852
},
853853
mockResponse: `{"featureFlags": {"test-flag": "false"}}`,
854-
expectedResult: false,
854+
expectedResult: "false",
855855
},
856856
{
857857
name: "Feature flag is a variant string",

0 commit comments

Comments
 (0)