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.

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

consolidate default value type comparisons into local bucketing code #247

Draft
wants to merge 3 commits into
base: remove-indirection
Choose a base branch
from
Draft

consolidate default value type comparisons into local bucketing code #247

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04df209
consolidate default value type comparisons into local bucketing code
ajwootto Apr 22, 2024
20933f1
fix import
ajwootto Apr 22, 2024
587ae3f
handle error
ajwootto Apr 22, 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
18 changes: 16 additions & 2 deletions bucketing/bucketing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

import (
"errors"
"reflect"
"time"

"github.com/devcyclehq/go-server-sdk/v2/api"
"github.com/devcyclehq/go-server-sdk/v2/util"
"github.com/devcyclehq/go-server-sdk/v2/variable-utils"
)

// Max value of an unsigned 32-bit integer, which is what murmurhash returns
Expand Down Expand Up @@ -212,7 +214,9 @@
}, nil
}

func VariableForUser(sdkKey string, user api.PopulatedUser, variableKey string, expectedVariableType string, eventQueue *EventQueue, clientCustomData map[string]interface{}) (variableType string, variableValue any, err error) {
func VariableForUser(sdkKey string, user api.PopulatedUser, variableKey string, defaultValue interface{}, eventQueue *EventQueue, clientCustomData map[string]interface{}) (variableType string, variableValue any, err error) {
expectedVariableType, err := variable_utils.VariableTypeFromValue(variableKey, defaultValue, true)

Check failure on line 218 in bucketing/bucketing.go

View workflow job for this annotation

GitHub Actions / lint 

ineffectual assignment to err (ineffassign)
Copy link
Member

Choose a reason for hiding this comment

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

check the error result here


variableType, variableValue, featureId, variationId, err := generateBucketedVariableForUser(sdkKey, user, variableKey, clientCustomData)
if err != nil {
eventErr := eventQueue.QueueVariableDefaultedEvent(variableKey, BucketResultErrorToDefaultReason(err))
Expand All @@ -222,9 +226,17 @@
return "", nil, err
}

if !isVariableTypeValid(variableType, expectedVariableType) && expectedVariableType != "" {
typeFieldMismatch := !isVariableTypeValid(variableType, expectedVariableType) && expectedVariableType != ""
Copy link
Member

Choose a reason for hiding this comment

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

This kind of one liner is discouraged - and should be done in the if statement to make it more transparent.

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 found it so much harder to read the if statement condition when i inlined it all

Copy link
Member

Choose a reason for hiding this comment

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

if you really want to make it oneliner'd - move the explicit variable call checks first to make it more visible.

valueFieldMismatch := defaultValue != nil && !variable_utils.CompareTypes(variableValue, defaultValue)

if typeFieldMismatch || valueFieldMismatch {
err = ErrInvalidVariableType
eventErr := eventQueue.QueueVariableDefaultedEvent(variableKey, BucketResultErrorToDefaultReason(err))
util.Warnf("Type mismatch for variable %s. Expected type %s, got %s",
variableKey,
reflect.TypeOf(defaultValue).String(),
reflect.TypeOf(variableValue).String(),
)
if eventErr != nil {
util.Warnf("Failed to queue variable defaulted event: %s", eventErr)
}
Expand Down Expand Up @@ -303,6 +315,8 @@
return "USER_NOT_TARGETED"
case ErrInvalidVariableType:
return "INVALID_VARIABLE_TYPE"
case variable_utils.ErrInvalidDefaultValue:
return "INVALID_DEFAULT_VALUE"
Comment on lines +326 to +327
Copy link
Member

Choose a reason for hiding this comment

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

We should make these accessible constants still.

default:
return "Unknown"
}
Expand Down
78 changes: 4 additions & 74 deletions client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import (
"fmt"
"github.com/devcyclehq/go-server-sdk/v2/api"
"github.com/devcyclehq/go-server-sdk/v2/util"
"github.com/devcyclehq/go-server-sdk/v2/variable-utils"
"os"
"reflect"
"regexp"
"runtime"
"strings"
Expand Down Expand Up @@ -232,8 +232,8 @@ func (c *Client) Variable(userdata User, key string, defaultValue interface{}) (
return Variable{}, errors.New("invalid key provided for call to Variable")
}

convertedDefaultValue := convertDefaultValueType(defaultValue)
variableType, err := variableTypeFromValue(key, convertedDefaultValue, c.IsLocalBucketing())
convertedDefaultValue := variable_utils.ConvertDefaultValueType(defaultValue)
variableType, err := variable_utils.VariableTypeFromValue(key, convertedDefaultValue, c.IsLocalBucketing())

if err != nil {
return Variable{}, err
Expand All @@ -255,23 +255,7 @@ func (c *Client) Variable(userdata User, key string, defaultValue interface{}) (
return c.cloudClient.Variable(userdata, key, defaultValue)
}

bucketedVariable, err := c.localBucketing.Variable(userdata, key, variableType)

sameTypeAsDefault := compareTypes(bucketedVariable.Value, convertedDefaultValue)
if bucketedVariable.Value != nil && (sameTypeAsDefault || defaultValue == nil) {
variable.Type_ = bucketedVariable.Type_
variable.Value = bucketedVariable.Value
variable.IsDefaulted = false
} else {
if !sameTypeAsDefault && bucketedVariable.Value != nil {
util.Warnf("Type mismatch for variable %s. Expected type %s, got %s",
key,
reflect.TypeOf(defaultValue).String(),
reflect.TypeOf(bucketedVariable.Value).String(),
)
}
}
return variable, err
return c.localBucketing.Variable(userdata, key, convertedDefaultValue)
}

func (c *Client) AllVariables(user User) (map[string]ReadOnlyVariable, error) {
Expand Down Expand Up @@ -393,60 +377,6 @@ func (c *Client) hasConfig() bool {
return c.configManager.HasConfig()
}

func compareTypes(value1 interface{}, value2 interface{}) bool {
return reflect.TypeOf(value1) == reflect.TypeOf(value2)
}

func convertDefaultValueType(value interface{}) interface{} {
switch value := value.(type) {
case int:
return float64(value)
case int8:
return float64(value)
case int16:
return float64(value)
case int32:
return float64(value)
case int64:
return float64(value)
case uint:
return float64(value)
case uint8:
return float64(value)
case uint16:
return float64(value)
case uint32:
return float64(value)
case uint64:
return float64(value)
case float32:
return float64(value)
default:
return value
}
}

var ErrInvalidDefaultValue = errors.New("the default value for variable is not of type Boolean, Number, String, or JSON")

func variableTypeFromValue(key string, value interface{}, allowNil bool) (varType string, err error) {
switch value.(type) {
case float64:
return "Number", nil
case string:
return "String", nil
case bool:
return "Boolean", nil
case map[string]any:
return "JSON", nil
case nil:
if allowNil {
return "", nil
}
}

return "", fmt.Errorf("%w: %s", ErrInvalidDefaultValue, key)
}

// Change base path to allow switching to mocks
func (c *Client) ChangeBasePath(path string) {
c.cfg.BasePath = path
Expand Down
7 changes: 4 additions & 3 deletions client_cloud_bucketing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"github.com/devcyclehq/go-server-sdk/v2/util"
"github.com/devcyclehq/go-server-sdk/v2/variable-utils"
"github.com/matryer/try"
"io"
"math"
Expand Down Expand Up @@ -80,8 +81,8 @@ func (c *cloudClient) Variable(userdata User, key string, defaultValue interface
return Variable{}, errors.New("invalid key provided for call to Variable")
}

convertedDefaultValue := convertDefaultValueType(defaultValue)
variableType, err := variableTypeFromValue(key, convertedDefaultValue, false)
convertedDefaultValue := variable_utils.ConvertDefaultValueType(defaultValue)
variableType, err := variable_utils.VariableTypeFromValue(key, convertedDefaultValue, false)

if err != nil {
return Variable{}, err
Expand Down Expand Up @@ -127,7 +128,7 @@ func (c *cloudClient) Variable(userdata User, key string, defaultValue interface
// If we succeed, return the data, otherwise pass on to decode error.
err = decode(&localVarReturnValue, body, r.Header.Get("Content-Type"))
if err == nil && localVarReturnValue.Value != nil {
if compareTypes(localVarReturnValue.Value, convertedDefaultValue) {
if variable_utils.CompareTypes(localVarReturnValue.Value, convertedDefaultValue) {
variable.Value = localVarReturnValue.Value
variable.IsDefaulted = false
} else {
Expand Down
16 changes: 12 additions & 4 deletions client_native_bucketing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package devcycle

import (
"fmt"
"github.com/devcyclehq/go-server-sdk/v2/variable-utils"
"sync"
"time"

Expand Down Expand Up @@ -83,20 +84,27 @@ func (n *NativeLocalBucketing) SetClientCustomData(customData map[string]interfa
return nil
}

func (n *NativeLocalBucketing) Variable(user User, variableKey string, variableType string) (Variable, error) {
func (n *NativeLocalBucketing) Variable(user User, variableKey string, defaultValue interface{}) (Variable, error) {
variableType, err := variable_utils.VariableTypeFromValue(variableKey, defaultValue, true)

if err != nil {
return Variable{}, err
}

defaultVar := Variable{
BaseVariable: api.BaseVariable{
Key: variableKey,
Type_: variableType,
Value: nil,
Value: defaultValue,
},
DefaultValue: nil,
DefaultValue: defaultValue,
IsDefaulted: true,
}

clientCustomData := bucketing.GetClientCustomData(n.sdkKey)
populatedUser := user.GetPopulatedUserWithTime(n.platformData, DEFAULT_USER_TIME)
resultVariableType, resultValue, err := bucketing.VariableForUser(n.sdkKey, populatedUser, variableKey, variableType, n.eventQueue, clientCustomData)
resultVariableType, resultValue, err := bucketing.VariableForUser(n.sdkKey, populatedUser, variableKey, defaultValue, n.eventQueue, clientCustomData)

if err != nil {
return defaultVar, nil
}
Expand Down
61 changes: 61 additions & 0 deletions variable-utils/variable_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package variable_utils

import (
"errors"
"fmt"
"reflect"
)

Copy link
Member

Choose a reason for hiding this comment

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

You could add the default reasons 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.

feels like they should go in the same place as the error definitions that cause them, which is all in the bucketing package

Copy link
Member

Choose a reason for hiding this comment

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

really it doesn't matter where - they can be imported from anywhere

var ErrInvalidDefaultValue = errors.New("the default value for variable is not of type Boolean, Number, String, or JSON")

func ConvertDefaultValueType(value interface{}) interface{} {
switch value := value.(type) {
case int:
return float64(value)
case int8:
return float64(value)
case int16:
return float64(value)
case int32:
return float64(value)
case int64:
return float64(value)
case uint:
return float64(value)
case uint8:
return float64(value)
case uint16:
return float64(value)
case uint32:
return float64(value)
case uint64:
return float64(value)
case float32:
return float64(value)
default:
return value
}
}

func VariableTypeFromValue(key string, value interface{}, allowNil bool) (varType string, err error) {
switch value.(type) {
case float64:
return "Number", nil
case string:
return "String", nil
case bool:
return "Boolean", nil
case map[string]any:
return "JSON", nil
case nil:
if allowNil {
return "", nil
}
}

return "", fmt.Errorf("%w: %s", ErrInvalidDefaultValue, key)
}

func CompareTypes(value1 interface{}, value2 interface{}) bool {
return reflect.TypeOf(value1) == reflect.TypeOf(value2)
}
Loading