feat(getFeatureVariableString): Add getFeatureVariableString API #71

yasirfolio3 · 2019-08-22T16:08:06Z

Summary

Add GetFeatureVariableString as a function in client.go
Add GetVariableByKey() function in datafileprojectconfig/config.go

Tests

Unit tests in client_test.go

Issues

OASIS-5006

mikeproeng37 · 2019-08-22T16:10:53Z

optimizely/client/client.go

+
+	var typeName = ""
+	var valueParsed interface{}
+	switch valueType.(type) {


Instead of passing in a variable for which we then get the type of, we can just pass in a string with the name of the type. This is less expensive to do.

mikeproeng37 · 2019-08-22T16:14:29Z

optimizely/client/client.go

+func (o *OptimizelyClient) GetFeatureVariableString(featureKey string, variableKey string, userContext entities.UserContext) (value string, err error) {
+	val, err := o.getFeatureVariable("", featureKey, variableKey, userContext)
+	if err == nil {
+		switch val.(type) {


Here we can

if returnValue, ok := val.(string); ok { value = returnValue }

Then at the end you return the named variables value, err

mikeproeng37 · 2019-08-22T16:16:55Z

optimizely/client/client.go

+}
+
+func (o *OptimizelyClient) getFeatureVariable(valueType interface{}, featureKey string, variableKey string, userContext entities.UserContext) (value interface{}, err error) {
+	if !o.isValid {


I feel this check, along with the project config check below can likely be collapsed into a isValid method that takes in the method name so it can print it out if not valid

mikeproeng37 · 2019-08-22T16:21:17Z

optimizely/interface.go

 	GetEventByKey(string) (entities.Event, error)
 	GetExperimentByKey(string) (entities.Experiment, error)
 	GetFeatureByKey(string) (entities.Feature, error)
+	GetFeatureFlagByKey(string) (datafileEntities.FeatureFlag, error)


This is wrong. Feature flags an d features are the same thing. You are bringing in the data file raw feature flags in here, which is not what we want.

mikeproeng37 · 2019-08-22T16:23:45Z

optimizely/config/datafileprojectconfig/entities/entities.go

 }

+// GetVariable returns variable for key from feature flag
+func (f *FeatureFlag) GetVariable(key string) (Variable, error) {


Instead of adding this functionality to the model, let's add this method to the project config itself to get a variable with key from the feature

mikeproeng37 · 2019-08-22T16:25:20Z

optimizely/config/datafileprojectconfig/config.go

 	experimentKeyToIDMap map[string]string
 	experimentMap        map[string]entities.Experiment
 	featureMap           map[string]entities.Feature
+	featureFlagMap       map[string]datafileEntities.FeatureFlag


See comment at bottom. This is incorrect. We should be using the entities.* which are public facing. The datafileEntities are internal models that we don't expose and are only needed so we can parse into entities.* models

mikeproeng37

Mostly good. I have a few more suggestions

mikeproeng37 · 2019-08-22T20:51:19Z

optimizely/config/datafileprojectconfig/mappers/experiment.go

 	}
+
+	for _, variable := range rawVariation.Variables {
+		variation.Variables = append(variation.Variables, entities.VariationVariable{ID: variable.ID, Value: variable.Value})


I think we can benefit from making Feature.Variables a map of string to variable for easier access. It also helps that we can easily iterate over maps in Go.

mikeproeng37 · 2019-08-22T20:54:08Z

optimizely/client/client_test.go

+		DefaultValue: "default",
+		ID:           "1",
+		Key:          "test_feature_flag_key",
+		Type:         "string"}


Please move the } to a new line

mikeproeng37 · 2019-08-22T20:54:24Z

optimizely/client/client_test.go

+		DefaultValue: "default",
+		ID:           "1",
+		Key:          "test_feature_flag_key",
+		Type:         "boolean"}


Move the } to a new line

mikeproeng37 · 2019-08-22T20:54:32Z

optimizely/client/client_test.go

+		DefaultValue: "defaultString",
+		ID:           "1",
+		Key:          "test_feature_flag_key",
+		Type:         "string"}


ditto above

mikeproeng37 · 2019-08-22T20:55:15Z

optimizely/config/datafileprojectconfig/entities/entities.go

-	Key            string `json:"key"`
-	FeatureEnabled bool   `json:"featureEnabled"`
+	ID             string              `json:"id"`
+	Variables      []VariationVariable `json:"variables"`


I think we can use a map here to make look-ups easier.

I have updated the public facing dataModel of Variation with a map for Variables.
https://github.com/optimizely/go-sdk/pull/71/files#diff-f55225b09df17f6c9183261f39659ee1R22

mikeproeng37 · 2019-08-22T20:55:36Z

optimizely/event/processor_test.go

 	assert.Equal(t, 1, processor.EventsCount())

-	time.Sleep(200 * time.Millisecond)
+	time.Sleep(2000 * time.Millisecond)


Is this because the test is flaky?

yes, i had instances where it failed, increasing the timeout made it more stable.

pawels-optimizely · 2019-08-23T18:25:24Z

optimizely/client/client.go

+	return value, err
+}
+
+func (o *OptimizelyClient) getFeatureVariable(valueType string, featureKey string, variableKey string, userContext entities.UserContext) (value interface{}, err error) {


I think conceptually it will be better if that function returns the string instead of interface and GetFeatureVariableString does the checking. That way we eliminate valueType parameter.

actually GetFeatureVariableString will not have to do any checking - just pass-through, but GetFeatureVariableBool, GetFeatureVariableFloat, etc will do some checking

That is a great point. I think we should do it that way even if it ends up duplicating some of the code at the bottom since strong typing does give us other kinds of guarantees @yasirfolio3

@pawels-optimizely we need valueType parameter for this condition
https://github.com/optimizely/go-sdk/pull/71/files#diff-b852811f33e54bd7b6e89917e41eefadR203

@mikeng13 yes we can move the checking part to each individual api such as GetFeatureVariableString, but we would still need valueType argument to check if variable.Type is the required valueType, any thoughts?

you are calling it as getFeatureVariable("string", featureKey, variableKey, userContext), that means variable "2" would be treated as a string in GetFeatureVariableString

also if you need the type, you can return a string and the type and check that in GetFeatureVariableString. try to avoid returning interface{} unless you really have to.

also if you need the type, you can return a string and the type and check that in GetFeatureVariableString

il go ahead with this idea!

yeah, sorry to be a pain - I am not 100% on this code yet, but it's kind of awkward flow when I read the code: 1. getting a string, 2. converting to interface, 3. returning the interface, 4. converting interface to string again - this is your overall flow. Just trying to avoid that.

no worries, can you review it now!

pawels-optimizely · 2019-08-23T20:59:25Z

optimizely/client/client.go

 }

+// GetFeatureVariableString returns string feature variable value
+func (o *OptimizelyClient) GetFeatureVariableString(featureKey string, variableKey string, userContext entities.UserContext) (value string, err error) {


nice !, much cleaner now, no hardcoded parameter, more readable, no unneeded interface. small change but cleaned a lot of things and eliminated confusion. lgtm

mikeproeng37

Just one more comment on it!

mikeproeng37 · 2019-08-23T21:32:17Z

optimizely/client/client.go

+		}
+	}
+
+	if featureValue == "" {


Is this still needed? Empty strings should be OK. I think we should revert the above flow where if err != nil then we return this error message

done! i'l do that for other getFeatureVariable API's, getFeatureVariableString doesn't require type-casting so i don't think we need it.

mikeproeng37

lgtm

mikeproeng37 · 2019-08-26T16:30:07Z

@yasirfolio3 can you please resolve merge conflicts?

yasirfolio3 · 2019-08-26T16:45:49Z

@yasirfolio3 can you please resolve merge conflicts?

yep, done!

mikeproeng37

lgtm

getFeatureVariableString API implemented.

8951915

yasirfolio3 requested a review from a team as a code owner August 22, 2019 16:08

jamesopti assigned yasirfolio3 Aug 22, 2019

yasirfolio3 mentioned this pull request Aug 22, 2019

feat(getFeatureVariable): Add getFeatureVariable API #70

Closed

mikeproeng37 suggested changes Aug 22, 2019

View reviewed changes

Addressed Recommended changes.

1acdb79

yasirfolio3 requested a review from mikeproeng37 August 22, 2019 19:20

mikeproeng37 suggested changes Aug 22, 2019

View reviewed changes

mikeproeng37 requested a review from pawels-optimizely August 22, 2019 21:39

Recommended changes made.

deff23b

yasirfolio3 requested a review from mikeproeng37 August 23, 2019 07:05

yasirfolio3 and others added 2 commits August 23, 2019 12:23

bug fixes.

6a60c9a

Merge branch 'go-alpha' into yasir/featurevariablestring

e6a2cda

pawels-optimizely reviewed Aug 23, 2019

View reviewed changes

yasirfolio3 added 2 commits August 24, 2019 00:36

Merge branch 'go-alpha' into yasir/featurevariablestring

cfc1d14

Addressed review.

75e592f

yasirfolio3 requested a review from pawels-optimizely August 23, 2019 20:54

pawels-optimizely reviewed Aug 23, 2019

View reviewed changes

mikeproeng37 suggested changes Aug 23, 2019

View reviewed changes

Addressed review.

8ff52ba

yasirfolio3 requested a review from mikeproeng37 August 23, 2019 21:50

mikeproeng37 approved these changes Aug 23, 2019

View reviewed changes

Merge branch 'go-alpha' into yasir/featurevariablestring

1679b4e

mikeproeng37 approved these changes Aug 26, 2019

View reviewed changes

mikeproeng37 merged commit 9d260d6 into go-alpha Aug 26, 2019

yasirfolio3 deleted the yasir/featurevariablestring branch August 27, 2019 18:26

feat(getFeatureVariableString): Add getFeatureVariableString API #71

feat(getFeatureVariableString): Add getFeatureVariableString API #71

Uh oh!

Conversation

yasirfolio3 commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tests

Issues

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yasirfolio3 Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yasirfolio3 Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawels-optimizely Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 commented Aug 26, 2019

yasirfolio3 commented Aug 22, 2019 •

edited

Loading

yasirfolio3 Aug 23, 2019 •

edited

Loading

yasirfolio3 Aug 23, 2019 •

edited

Loading

pawels-optimizely Aug 23, 2019 •

edited

Loading