tests, improvements and documentation

Kavindu-Dodan · Kavindu-Dodan · commit ed7f8a896a06 · 2024-03-19T11:38:31.000-07:00
Signed-off-by: Kavindu Dodanduwa &lt;kavindudodanduwa@gmail.com&gt;
diff --git a/providers/ofrep/README.md b/providers/ofrep/README.md
@@ -2,7 +2,6 @@
 
 This is the Go implementation of the OFREP provider.
 The provider works by evaluating flags against OFREP single flag evaluation endpoint.
-And there is no caching layer in this version.
 
 ## Installation
 
diff --git a/providers/ofrep/internal/evaluate/flags.go b/providers/ofrep/internal/evaluate/flags.go
@@ -8,19 +8,18 @@ import (
 	of "github.com/open-feature/go-sdk/openfeature"
 )
 
-type resolver interface {
-	resolveSingle(ctx context.Context, key string, evalCtx map[string]interface{}) (*successDto, *of.ResolutionError)
-}
-
+// Flags is the flag evaluator implementation. It contains domain logic of the OpenFeature flag evaluation.
 type Flags struct {
 	resolver resolver
 }
 
-func NewFlagsEvaluator(cfg outbound.Configuration) *Flags {
-	client := outbound.NewHttp(cfg)
+type resolver interface {
+	resolveSingle(ctx context.Context, key string, evalCtx map[string]interface{}) (*successDto, *of.ResolutionError)
+}
 
+func NewFlagsEvaluator(cfg outbound.Configuration) *Flags {
 	return &Flags{
-		resolver: NewOutboundResolver(client),
+		resolver: NewOutboundResolver(cfg),
 	}
 }
 
diff --git a/providers/ofrep/internal/evaluate/flags_test.go b/providers/ofrep/internal/evaluate/flags_test.go
@@ -2,7 +2,6 @@ package evaluate
 
 import (
 	"context"
-	"fmt"
 	"reflect"
 	"testing"
 
@@ -302,19 +301,19 @@ func genericValidator[T knownTypes](test testDefinition[T], resolvedValue T, rea
 		}
 
 		if !reflect.DeepEqual(test.defaultValue, resolvedValue) {
-			t.Error(fmt.Sprintf("expected deafault value %v, but got %v", test.defaultValue, resolvedValue))
+			t.Errorf("expected deafault value %v, but got %v", test.defaultValue, resolvedValue)
 		}
 
 		if reason != of.ErrorReason {
-			t.Error(fmt.Sprintf("expected reason %v, but got %v", of.ErrorReason, reason))
+			t.Errorf("expected reason %v, but got %v", of.ErrorReason, reason)
 		}
 	} else {
 		if err != nil {
-			t.Fatal(fmt.Sprintf("expected no error, but got none nil error: %v", err))
+			t.Fatalf("expected no error, but got none nil error: %v", err)
 		}
 
 		if !reflect.DeepEqual(test.expect, resolvedValue) {
-			t.Error(fmt.Sprintf("expected value %v, but got %v", test.expect, resolvedValue))
+			t.Errorf("expected value %v, but got %v", test.expect, resolvedValue)
 		}
 	}
 }
diff --git a/providers/ofrep/internal/evaluate/resolver.go b/providers/ofrep/internal/evaluate/resolver.go
@@ -9,19 +9,22 @@ import (
 	"strconv"
 	"time"
 
+	"github.com/open-feature/go-sdk-contrib/providers/ofrep/internal/outbound"
 	of "github.com/open-feature/go-sdk/openfeature"
 )
 
-type Outbound interface {
-	PostSingle(ctx context.Context, key string, payload []byte) (*http.Response, error)
-}
-
+// OutboundResolver is responsible for resolving flags with outbound communications.
+// It contains domain logic of the OFREP specification.
 type OutboundResolver struct {
 	client Outbound
 }
 
-func NewOutboundResolver(client Outbound) *OutboundResolver {
-	return &OutboundResolver{client}
+type Outbound interface {
+	PostSingle(ctx context.Context, key string, payload []byte) (*http.Response, error)
+}
+
+func NewOutboundResolver(cfg outbound.Configuration) *OutboundResolver {
+	return &OutboundResolver{client: outbound.NewHttp(cfg)}
 }
 
 func (g *OutboundResolver) resolveSingle(ctx context.Context, key string, evalCtx map[string]interface{}) (
@@ -116,7 +119,7 @@ func parse429(rsp *http.Response) time.Duration {
 		return 0
 	}
 
-	return parsed.Sub(time.Now())
+	return time.Until(parsed)
 }
 
 func parseError500(body io.ReadCloser) *of.ResolutionError {
diff --git a/providers/ofrep/internal/evaluate/resolver_test.go b/providers/ofrep/internal/evaluate/resolver_test.go
@@ -5,7 +5,6 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"fmt"
 	"io"
 	"net/http"
 	"strings"
@@ -36,45 +35,49 @@ func TestSuccess200(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		client := mockOutbound{
+		resolver := OutboundResolver{client: mockOutbound{
 			rsp: http.Response{
 				StatusCode: http.StatusOK,
 				Body:       io.NopCloser(bytes.NewReader(successBytes)),
 			},
-		}
+		}}
 
-		successDto, resolutionError := NewOutboundResolver(client).resolveSingle(context.Background(), "", make(map[string]interface{}))
+		successDto, resolutionError := resolver.resolveSingle(context.Background(), "", make(map[string]interface{}))
 
 		if resolutionError != nil {
-			t.Error(fmt.Sprintf("expected no errors, but got error: %v", err))
+			t.Errorf("expected no errors, but got error: %v", err)
 		}
 
 		if successDto == nil {
 			t.Fatal("expected non empty success response")
 		}
 
 		if successDto.Value != success.Value {
-			t.Errorf(fmt.Sprintf("expected value %v, but got %v", success.Value, successDto.Value))
+			t.Errorf("expected value %v, but got %v", success.Value, successDto.Value)
 		}
 
 		if successDto.Variant != success.Variant {
-			t.Errorf(fmt.Sprintf("expected variant %v, but got %v", success.Variant, successDto.Variant))
+			t.Errorf("expected variant %v, but got %v", success.Variant, successDto.Variant)
 		}
 
 		if successDto.Reason != success.Reason {
-			t.Errorf(fmt.Sprintf("expected reason %s, but got %s", success.Reason, successDto.Reason))
+			t.Errorf("expected reason %s, but got %s", success.Reason, successDto.Reason)
+		}
+
+		if successDto.Metadata["key"] != "value" {
+			t.Errorf("expected key to contain value %s, but got %s", "value", successDto.Metadata["key"])
 		}
 	})
 
 	t.Run("invalid payload type results in general error", func(t *testing.T) {
-		client := mockOutbound{
+		resolver := OutboundResolver{client: mockOutbound{
 			rsp: http.Response{
 				StatusCode: http.StatusOK,
 				Body:       io.NopCloser(bytes.NewReader([]byte("some payload"))),
 			},
-		}
+		}}
+		success, resolutionError := resolver.resolveSingle(context.Background(), "", make(map[string]interface{}))
 
-		success, resolutionError := NewOutboundResolver(client).resolveSingle(context.Background(), "", make(map[string]interface{}))
 		validateErrorCode(success, resolutionError, of.GeneralCode, t)
 	})
 }
@@ -116,8 +119,9 @@ func TestResolveGeneralErrors(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			// when
-			success, resolutionError := NewOutboundResolver(test.client).resolveSingle(context.Background(), "key", map[string]interface{}{})
+			resolver := OutboundResolver{client: test.client}
+			success, resolutionError := resolver.resolveSingle(context.Background(), "key", map[string]interface{}{})
+
 			validateErrorCode(success, resolutionError, of.GeneralCode, t)
 		})
 	}
@@ -166,31 +170,27 @@ func TestEvaluationError4xx(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			client := mockOutbound{
+			resolver := OutboundResolver{client: mockOutbound{
 				rsp: http.Response{
 					StatusCode: http.StatusBadRequest,
 					Body:       io.NopCloser(bytes.NewReader(errBytes)),
 				},
-			}
-			// when
-			success, resolutionError := NewOutboundResolver(client).resolveSingle(context.Background(), "", make(map[string]interface{}))
+			}}
+			success, resolutionError := resolver.resolveSingle(context.Background(), "", make(map[string]interface{}))
 
-			// then
 			validateErrorCode(success, resolutionError, test.expectCode, t)
 		})
 	}
 }
 
 func TestFlagNotFound404(t *testing.T) {
-	client := mockOutbound{
+	resolver := OutboundResolver{client: mockOutbound{
 		rsp: http.Response{
 			StatusCode: http.StatusNotFound,
 		},
-	}
-	// when
-	success, resolutionError := NewOutboundResolver(client).resolveSingle(context.Background(), "", make(map[string]interface{}))
+	}}
+	success, resolutionError := resolver.resolveSingle(context.Background(), "", make(map[string]interface{}))
 
-	// then
 	validateErrorCode(success, resolutionError, of.FlagNotFoundCode, t)
 }
 
@@ -225,29 +225,24 @@ func Test429(t *testing.T) {
 				}
 			}
 
-			client := mockOutbound{
-				rsp: response,
-			}
+			resolver := OutboundResolver{client: mockOutbound{rsp: response}}
+			success, resolutionError := resolver.resolveSingle(context.Background(), "", make(map[string]interface{}))
 
-			// when
-			success, resolutionError := NewOutboundResolver(client).resolveSingle(context.Background(), "", make(map[string]interface{}))
 			validateErrorCode(success, resolutionError, of.GeneralCode, t)
 		})
 	}
 }
 
 func TestEvaluationError5xx(t *testing.T) {
 	t.Run("without body", func(t *testing.T) {
-		client := mockOutbound{
+		resolver := OutboundResolver{client: mockOutbound{
 			rsp: http.Response{
 				StatusCode: http.StatusInternalServerError,
 				Body:       io.NopCloser(bytes.NewReader([]byte{})),
 			},
-		}
-		// when
-		success, resolutionError := NewOutboundResolver(client).resolveSingle(context.Background(), "", make(map[string]interface{}))
+		}}
+		success, resolutionError := resolver.resolveSingle(context.Background(), "", make(map[string]interface{}))
 
-		// then
 		validateErrorCode(success, resolutionError, of.GeneralCode, t)
 	})
 
@@ -257,30 +252,26 @@ func TestEvaluationError5xx(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		client := mockOutbound{
+		resolver := OutboundResolver{client: mockOutbound{
 			rsp: http.Response{
 				StatusCode: http.StatusInternalServerError,
 				Body:       io.NopCloser(bytes.NewReader(errorBytes)),
 			},
-		}
-		// when
-		success, resolutionError := NewOutboundResolver(client).resolveSingle(context.Background(), "", make(map[string]interface{}))
+		}}
+		success, resolutionError := resolver.resolveSingle(context.Background(), "", make(map[string]interface{}))
 
-		// then
 		validateErrorCode(success, resolutionError, of.GeneralCode, t)
 	})
 
 	t.Run("with invalid body", func(t *testing.T) {
-		client := mockOutbound{
+		resolver := OutboundResolver{client: mockOutbound{
 			rsp: http.Response{
 				StatusCode: http.StatusInternalServerError,
 				Body:       io.NopCloser(bytes.NewReader([]byte("some error"))),
 			},
-		}
-		// when
-		success, resolutionError := NewOutboundResolver(client).resolveSingle(context.Background(), "", make(map[string]interface{}))
+		}}
+		success, resolutionError := resolver.resolveSingle(context.Background(), "", make(map[string]interface{}))
 
-		// then
 		validateErrorCode(success, resolutionError, of.GeneralCode, t)
 	})
 }
diff --git a/providers/ofrep/internal/outbound/http_test.go b/providers/ofrep/internal/outbound/http_test.go
@@ -9,17 +9,23 @@ import (
 
 func TestHttpOutbound(t *testing.T) {
 	// given
+	host := "localhost:18181"
 	key := "flag"
+
 	server := http.Server{
-		Addr: "localhost:18181",
+		Addr: host,
 		Handler: mockHandler{
 			t:   t,
 			key: key,
 		},
 	}
 
 	go func() {
-		server.ListenAndServe()
+		err := server.ListenAndServe()
+		if err != nil {
+			t.Logf("error starting mock server: %v", err)
+			return
+		}
 	}()
 
 	outbound := NewHttp(Configuration{
@@ -28,7 +34,7 @@ func TestHttpOutbound(t *testing.T) {
 				return "Authorization", "Token"
 			},
 		},
-		BaseURI: "http://localhost:18181",
+		BaseURI: fmt.Sprintf("http://%s", host),
 	})
 
 	// when
@@ -37,14 +43,16 @@ func TestHttpOutbound(t *testing.T) {
 		return
 	}
 
-	// then - expect a ok response
+	// then - expect an ok response
 	if response.StatusCode != http.StatusOK {
 		t.Errorf("expected 200, but got %d", response.StatusCode)
 	}
 
 	// cleanup
-	server.Close()
-	server.Shutdown(context.Background())
+	err = server.Shutdown(context.Background())
+	if err != nil {
+		t.Errorf("error shuttting down mock server: %v", err)
+	}
 }
 
 type mockHandler struct {
@@ -73,5 +81,4 @@ func (r mockHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
 	}
 
 	resp.WriteHeader(http.StatusOK)
-	return
 }
diff --git a/providers/ofrep/provider.go b/providers/ofrep/provider.go
@@ -16,7 +16,8 @@ type Provider struct {
 
 type option func(*outbound.Configuration)
 
-// NewProvider returns a provider configured with provided configuration
+// NewProvider returns an OFREP provider configured with provided configuration.
+// The only mandatory configuration is the baseUri, which is the base path of the OFREP service implementation.
 func NewProvider(baseUri string, options ...option) *Provider {
 	cfg := outbound.Configuration{
 		BaseURI: baseUri,
diff --git a/providers/ofrep/provider_test.go b/providers/ofrep/provider_test.go

Original file line number	Diff line number	Diff line change
`@@ -8,19 +8,18 @@ import (`
`8`	`8`	`of "github.com/open-feature/go-sdk/openfeature"`
`9`	`9`	`)`
`10`	`10`
`11`		`-type resolver interface {`
`12`		`- resolveSingle(ctx context.Context, key string, evalCtx map[string]interface{}) (successDto, of.ResolutionError)`
`13`		`-}`
`14`		`-`
	`11`	`+// Flags is the flag evaluator implementation. It contains domain logic of the OpenFeature flag evaluation.`
`15`	`12`	`type Flags struct {`
`16`	`13`	`resolver resolver`
`17`	`14`	`}`
`18`	`15`
`19`		`-func NewFlagsEvaluator(cfg outbound.Configuration) *Flags {`
`20`		`- client := outbound.NewHttp(cfg)`
	`16`	`+type resolver interface {`
	`17`	`+ resolveSingle(ctx context.Context, key string, evalCtx map[string]interface{}) (successDto, of.ResolutionError)`
	`18`	`+}`
`21`	`19`
	`20`	`+func NewFlagsEvaluator(cfg outbound.Configuration) *Flags {`
`22`	`21`	`return &Flags{`
`23`		`- resolver: NewOutboundResolver(client),`
	`22`	`+ resolver: NewOutboundResolver(cfg),`
`24`	`23`	`}`
`25`	`24`	`}`
`26`	`25`
Original file line number	Diff line number	Diff line change
`@@ -2,7 +2,6 @@ package evaluate`
`2`	`2`
`3`	`3`	`import (`
`4`	`4`	`"context"`
`5`		`- "fmt"`
`6`	`5`	`"reflect"`
`7`	`6`	`"testing"`
`8`	`7`
`@@ -302,19 +301,19 @@ func genericValidator[T knownTypes](test testDefinition[T], resolvedValue T, rea`
`302`	`301`	`}`
`303`	`302`
`304`	`303`	`if !reflect.DeepEqual(test.defaultValue, resolvedValue) {`
`305`		`- t.Error(fmt.Sprintf("expected deafault value %v, but got %v", test.defaultValue, resolvedValue))`
	`304`	`+ t.Errorf("expected deafault value %v, but got %v", test.defaultValue, resolvedValue)`
`306`	`305`	`}`
`307`	`306`
`308`	`307`	`if reason != of.ErrorReason {`
`309`		`- t.Error(fmt.Sprintf("expected reason %v, but got %v", of.ErrorReason, reason))`
	`308`	`+ t.Errorf("expected reason %v, but got %v", of.ErrorReason, reason)`
`310`	`309`	`}`
`311`	`310`	`} else {`
`312`	`311`	`if err != nil {`
`313`		`- t.Fatal(fmt.Sprintf("expected no error, but got none nil error: %v", err))`
	`312`	`+ t.Fatalf("expected no error, but got none nil error: %v", err)`
`314`	`313`	`}`
`315`	`314`
`316`	`315`	`if !reflect.DeepEqual(test.expect, resolvedValue) {`
`317`		`- t.Error(fmt.Sprintf("expected value %v, but got %v", test.expect, resolvedValue))`
	`316`	`+ t.Errorf("expected value %v, but got %v", test.expect, resolvedValue)`
`318`	`317`	`}`
`319`	`318`	`}`
`320`	`319`	`}`