Repository: open-feature/go-sdk
Pull request #178 (2023-05-23 09:37:01)

feat: add flag metadata field
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

- implement flag metadata: https://github.com/open-feature/spec/commit/74c373e089ad77bf8cac84f3d93c00c945ff3a8a

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

https://github.com/open-feature/go-sdk/issues/177

### Notes
<!-- any additional notes for this PR -->

If the flag metadata is not in the provider response, then the returned evaluation details will have a nil value, this can be updated to have an empty map, but IMO its a cleaner implementation to leave this value as nil ([nil slices have length 0](https://go.dev/play/p/_m0EwwCvJ65))

### Follow-up Tasks
<!-- anything that is related to this PR but not done here should be noted under this section -->
<!-- if there is a need for a new issue, please link it here -->

### How to test
<!-- if applicable, add testing instructions under this section -->



codecov[bot]: ## [Codecov](https://app.codecov.io/gh/open-feature/go-sdk/pull/178?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-feature) Report
> Merging [#178](https://app.codecov.io/gh/open-feature/go-sdk/pull/178?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-feature) (a018fd1) into [main](https://app.codecov.io/gh/open-feature/go-sdk/commit/e0f37d529f669295748b210c274e62c392c307be?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-feature) (e0f37d5) will **decrease** coverage by `3.98%`.
> The diff coverage is `2.43%`.

```diff
@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
- Coverage   71.25%   67.27%   -3.98%     
==========================================
  Files           8        8              
  Lines         668      709      +41     
==========================================
+ Hits          476      477       +1     
- Misses        174      214      +40     
  Partials       18       18              
```


| [Impacted Files](https://app.codecov.io/gh/open-feature/go-sdk/pull/178?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-feature) | Coverage Δ | |
|---|---|---|
| [pkg/openfeature/client.go](https://app.codecov.io/gh/open-feature/go-sdk/pull/178?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-feature#diff-cGtnL29wZW5mZWF0dXJlL2NsaWVudC5nbw==) | `66.13% <0.00%> (-5.73%)` | :arrow_down: |
| [pkg/openfeature/provider.go](https://app.codecov.io/gh/open-feature/go-sdk/pull/178?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-feature#diff-cGtnL29wZW5mZWF0dXJlL3Byb3ZpZGVyLmdv) | `100.00% <100.00%> (ø)` | |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-feature)

toddbaert: This all makes sense to me.

I'm particularly interested in other's opinions on this:

> If the flag metadata is not in the provider response, then the returned evaluation details will have a nil value, this can be updated to have an empty map, but IMO its a cleaner implementation to leave this value as nil ([nil slices have length 0](https://go.dev/play/p/_m0EwwCvJ65))

I'll defer to those with more familiarity with go semantics.
Kavindu-Dodan: > This all makes sense to me.
> 
> I'm particularly interested in other's opinions on this:
> 
> > If the flag metadata is not in the provider response, then the returned evaluation details will have a nil value, this can be updated to have an empty map, but IMO its a cleaner implementation to leave this value as nil ([nil slices have length 0](https://go.dev/play/p/_m0EwwCvJ65))
> 
> I'll defer to those with more familiarity with go semantics.

From pure spec point of view [1]

> If the flag metadata field in the flag resolution structure returned by the configured provider is set, the evaluation details structure's flag metadata field MUST contain that value. Otherwise, it MUST contain an empty record.

IMO, to be spec compliant, we should have an empty record here 🤔  

[1] - https://openfeature.dev/specification/sections/flag-evaluation/#requirement-1413
beeme1mr: How do we want to handle releasing a breaking change? If we're going to release this as a minor version, we need to make sure Release Please is configured properly and that the changes are clearly documented.
toddbaert: > How do we want to handle releasing a breaking change? If we're going to release this as a minor version, we need to make sure Release Please is configured properly and that the changes are clearly documented.

I didn't notice the `!` in the title. Is this actually breaking? I assumed from

> If the flag metadata is not in the provider response

That this was non-breaking? @james-milligan - can you clarify?
james-milligan: yes this is non breaking, apologies should have caught this sooner! 
toddbaert: I've updated the title so it reads a bit better in release notes. Approved!

Patch:
diff --git a/pkg/openfeature/client.go b/pkg/openfeature/client.go
index 47b07e5..28530b9 100644
--- a/pkg/openfeature/client.go
+++ b/pkg/openfeature/client.go
@@ -164,6 +164,78 @@ type ResolutionDetail struct {
 	Reason       Reason
 	ErrorCode    ErrorCode
 	ErrorMessage string
+	FlagMetadata FlagMetadata
+}
+
+// A structure which supports definition of arbitrary properties, with keys of type string, and values of type boolean, string, int64 or float64.
+//
+// This structure is populated by a provider for use by an Application Author (via the Evaluation API) or an Application Integrator (via hooks).
+type FlagMetadata map[string]interface{}
+
+// Fetch string value from FlagMetadata, returns an error if the key does not exist, or, the value is of the wrong type
+func (f FlagMetadata) GetString(key string) (string, error) {
+	v, ok := f[key]
+	if !ok {
+		return "", fmt.Errorf("key %s does not exist in FlagMetadata", key)
+	}
+	switch t := v.(type) {
+	case string:
+		return v.(string), nil
+	default:
+		return "", fmt.Errorf("wrong type for key %s, expected string, got %T", key, t)
+	}
+}
+
+// Fetch bool value from FlagMetadata, returns an error if the key does not exist, or, the value is of the wrong type
+func (f FlagMetadata) GetBool(key string) (bool, error) {
+	v, ok := f[key]
+	if !ok {
+		return false, fmt.Errorf("key %s does not exist in FlagMetadata", key)
+	}
+	switch t := v.(type) {
+	case bool:
+		return v.(bool), nil
+	default:
+		return false, fmt.Errorf("wrong type for key %s, expected bool, got %T", key, t)
+	}
+}
+
+// Fetch int64 value from FlagMetadata, returns an error if the key does not exist, or, the value is of the wrong type
+func (f FlagMetadata) GetInt(key string) (int64, error) {
+	v, ok := f[key]
+	if !ok {
+		return 0, fmt.Errorf("key %s does not exist in FlagMetadata", key)
+	}
+	switch t := v.(type) {
+	case int:
+		return int64(v.(int)), nil
+	case int8:
+		return int64(v.(int8)), nil
+	case int16:
+		return int64(v.(int16)), nil
+	case int32:
+		return int64(v.(int32)), nil
+	case int64:
+		return v.(int64), nil
+	default:
+		return 0, fmt.Errorf("wrong type for key %s, expected integer, got %T", key, t)
+	}
+}
+
+// Fetch float64 value from FlagMetadata, returns an error if the key does not exist, or, the value is of the wrong type
+func (f FlagMetadata) GetFloat(key string) (float64, error) {
+	v, ok := f[key]
+	if !ok {
+		return 0, fmt.Errorf("key %s does not exist in FlagMetadata", key)
+	}
+	switch t := v.(type) {
+	case float32:
+		return float64(v.(float32)), nil
+	case float64:
+		return v.(float64), nil
+	default:
+		return 0, fmt.Errorf("wrong type for key %s, expected float, got %T", key, t)
+	}
 }
 
 // Option applies a change to EvaluationOptions
diff --git a/pkg/openfeature/provider.go b/pkg/openfeature/provider.go
index 0e2c2fe..7bc9009 100644
--- a/pkg/openfeature/provider.go
+++ b/pkg/openfeature/provider.go
@@ -18,7 +18,7 @@ const (
 	StaticReason Reason = "STATIC"
 	// CachedReason - the resolved value was retrieved from cache
 	CachedReason Reason = "CACHED"
-	// UnknownReason - the reason for the resolved value could not be determined.	
+	// UnknownReason - the reason for the resolved value could not be determined.
 	UnknownReason Reason = "UNKNOWN"
 	// ErrorReason - the resolved value was the result of an error.
 	ErrorReason Reason = "ERROR"
@@ -54,14 +54,20 @@ type ProviderResolutionDetail struct {
 	ResolutionError ResolutionError
 	Reason          Reason
 	Variant         string
+	FlagMetadata    FlagMetadata
 }
 
 func (p ProviderResolutionDetail) ResolutionDetail() ResolutionDetail {
+	metadata := FlagMetadata{}
+	if p.FlagMetadata != nil {
+		metadata = p.FlagMetadata
+	}
 	return ResolutionDetail{
 		Variant:      p.Variant,
 		Reason:       p.Reason,
 		ErrorCode:    p.ResolutionError.code,
 		ErrorMessage: p.ResolutionError.message,
+		FlagMetadata: metadata,
 	}
 }
 


Test patch:
diff --git a/pkg/openfeature/client_test.go b/pkg/openfeature/client_test.go
index 465fc71..1b27cc0 100644
--- a/pkg/openfeature/client_test.go
+++ b/pkg/openfeature/client_test.go
@@ -630,6 +630,75 @@ func TestRequirement_1_4_12(t *testing.T) {
 	}
 }
 
+// Requirement_1_4_13
+// If the `flag metadata` field in the `flag resolution` structure returned by the configured `provider` is set,
+// the `evaluation details` structure's `flag metadata` field MUST contain that value. Otherwise,
+// it MUST contain an empty record.
+func TestRequirement_1_4_13(t *testing.T) {
+	client := NewClient("test-client")
+	flagKey := "flag-key"
+	evalCtx := EvaluationContext{}
+	flatCtx := flattenContext(evalCtx)
+
+	ctrl := gomock.NewController(t)
+	t.Run("No Metadata", func(t *testing.T) {
+		defer t.Cleanup(initSingleton)
+		mockProvider := NewMockFeatureProvider(ctrl)
+		defaultValue := true
+		mockProvider.EXPECT().Metadata().AnyTimes()
+		mockProvider.EXPECT().Hooks().AnyTimes()
+		mockProvider.EXPECT().BooleanEvaluation(context.Background(), flagKey, defaultValue, flatCtx).
+			Return(BoolResolutionDetail{
+				Value: true,
+				ProviderResolutionDetail: ProviderResolutionDetail{
+					FlagMetadata: nil,
+				},
+			}).Times(1)
+		SetProvider(mockProvider)
+
+		evDetails, err := client.BooleanValueDetails(context.Background(), flagKey, defaultValue, EvaluationContext{})
+		if err != nil {
+			t.Error(err)
+		}
+		if !reflect.DeepEqual(evDetails.FlagMetadata, FlagMetadata{}) {
+			t.Errorf(
+				"flag metadata is not as expected in EvaluationDetail, got %v, expected %v",
+				evDetails.FlagMetadata, FlagMetadata{},
+			)
+		}
+	})
+
+	t.Run("Metadata present", func(t *testing.T) {
+		defer t.Cleanup(initSingleton)
+		mockProvider := NewMockFeatureProvider(ctrl)
+		defaultValue := true
+		metadata := FlagMetadata{
+			"bing": "bong",
+		}
+		mockProvider.EXPECT().Metadata().AnyTimes()
+		mockProvider.EXPECT().Hooks().AnyTimes()
+		mockProvider.EXPECT().BooleanEvaluation(context.Background(), flagKey, defaultValue, flatCtx).
+			Return(BoolResolutionDetail{
+				Value: true,
+				ProviderResolutionDetail: ProviderResolutionDetail{
+					FlagMetadata: metadata,
+				},
+			}).Times(1)
+		SetProvider(mockProvider)
+
+		evDetails, err := client.BooleanValueDetails(context.Background(), flagKey, defaultValue, EvaluationContext{})
+		if err != nil {
+			t.Error(err)
+		}
+		if !reflect.DeepEqual(metadata, evDetails.FlagMetadata) {
+			t.Errorf(
+				"flag metadata is not as expected in EvaluationDetail, got %v, expected %v",
+				evDetails.FlagMetadata, metadata,
+			)
+		}
+	})
+}
+
 // Requirement_1_5_1
 // The `evaluation options` structure's `hooks` field denotes an ordered collection of hooks that the client MUST
 // execute for the respective flag evaluation, in addition to those already configured.
@@ -908,3 +977,117 @@ func TestObjectEvaluationShouldSupportNilValue(t *testing.T) {
 		t.Error("not supposed to have an error code")
 	}
 }
+
+func TestFlagMetadataAccessors(t *testing.T) {
+
+	t.Run("bool", func(t *testing.T) {
+		expectedValue := true
+		key := "bool"
+		key2 := "not-bool"
+		metadata := FlagMetadata{
+			key:  expectedValue,
+			key2: "12",
+		}
+		val, err := metadata.GetBool(key)
+		if err != nil {
+			t.Error("unexpected error value, expected nil", err)
+		}
+		if val != expectedValue {
+			t.Errorf("wrong value returned from FlagMetadata, expected %t, got %t", val, expectedValue)
+		}
+		_, err = metadata.GetBool(key2)
+		if err == nil {
+			t.Error("unexpected error value", err)
+		}
+		_, err = metadata.GetBool("not-in-map")
+		if err == nil {
+			t.Error("unexpected error value", err)
+		}
+	})
+
+	t.Run("string", func(t *testing.T) {
+		expectedValue := "string"
+		key := "string"
+		key2 := "not-string"
+		metadata := FlagMetadata{
+			key:  expectedValue,
+			key2: true,
+		}
+		val, err := metadata.GetString(key)
+		if err != nil {
+			t.Error("unexpected error value, expected nil", err)
+		}
+		if val != expectedValue {
+			t.Errorf("wrong value returned from FlagMetadata, expected %s, got %s", val, expectedValue)
+		}
+		_, err = metadata.GetString(key2)
+		if err == nil {
+			t.Error("unexpected error value", err)
+		}
+		_, err = metadata.GetString("not-in-map")
+		if err == nil {
+			t.Error("unexpected error value", err)
+		}
+	})
+
+	t.Run("int", func(t *testing.T) {
+		expectedValue := int64(12)
+		metadata := FlagMetadata{
+			"int":    int(12),
+			"int8":   int8(12),
+			"int16":  int16(12),
+			"int32":  int32(12),
+			"int164": int32(12),
+		}
+		for k := range metadata {
+			val, err := metadata.GetInt(k)
+			if err != nil {
+				t.Error("unexpected error value, expected nil", err)
+			}
+			if val != expectedValue {
+				t.Errorf("wrong value returned from FlagMetadata, expected %b, got %b", val, expectedValue)
+			}
+		}
+
+		metadata = FlagMetadata{
+			"not-int": true,
+		}
+		_, err := metadata.GetInt("not-int")
+		if err == nil {
+			t.Error("unexpected error value", err)
+		}
+		_, err = metadata.GetInt("not-in-map")
+		if err == nil {
+			t.Error("unexpected error value", err)
+		}
+	})
+
+	t.Run("float", func(t *testing.T) {
+		expectedValue := float64(12)
+		metadata := FlagMetadata{
+			"float32": float32(12),
+			"float64": float64(12),
+		}
+		for k := range metadata {
+			val, err := metadata.GetFloat(k)
+			if err != nil {
+				t.Error("unexpected error value, expected nil", err)
+			}
+			if val != expectedValue {
+				t.Errorf("wrong value returned from FlagMetadata, expected %b, got %b", val, expectedValue)
+			}
+		}
+
+		metadata = FlagMetadata{
+			"not-float": true,
+		}
+		_, err := metadata.GetInt("not-float")
+		if err == nil {
+			t.Error("unexpected error value", err)
+		}
+		_, err = metadata.GetInt("not-in-map")
+		if err == nil {
+			t.Error("unexpected error value", err)
+		}
+	})
+}