Merge pull request #20 from StephanHCB/issue-19-playback-recording-im…

…provements

Issue 19 playback recording improvements

.github/workflows/go.yml

@@ -20,7 +20,7 @@ jobs:
     - name: Set up Go
       uses: actions/setup-go@v2
       with:
-        go-version: 1.17
+        go-version: '1.18'
 
     - name: Build
       run: go build -v ./...

go.mod

@@ -1,16 +1,17 @@
 module github.com/StephanHCB/go-autumn-restclient
 
-go 1.17
+go 1.18
 
 require (
 	github.com/StephanHCB/go-autumn-logging v0.3.0
 	github.com/go-http-utils/headers v0.0.0-20181008091004-fed159eddc2a
-	github.com/stretchr/testify v1.8.1
-	github.com/tidwall/tinylru v1.1.0
+	github.com/stretchr/testify v1.8.4
+	github.com/tidwall/tinylru v1.2.1
 )
 
 require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
+	github.com/stretchr/objx v0.5.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -9,13 +9,19 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
+github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
 github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo=
 github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk=
 github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
+github.com/stretchr/testify v1.8.2/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
+github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
+github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
 github.com/tidwall/tinylru v1.1.0 h1:XY6IUfzVTU9rpwdhKUF6nQdChgCdGjkMfLzbWyiau6I=
 github.com/tidwall/tinylru v1.1.0/go.mod h1:3+bX+TJ2baOLMWTnlyNWHh4QMnFyARg2TLTQ6OFbzw8=
+github.com/tidwall/tinylru v1.2.1 h1:VgBr72c2IEr+V+pCdkPZUwiQ0KJknnWIYbhxAVkYfQk=
+github.com/tidwall/tinylru v1.2.1/go.mod h1:9bQnEduwB6inr2Y7AkBP7JPgCkyrhTV/ZpX0oOOpBI4=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

implementation/playback/playback.go

@@ -14,84 +14,98 @@ import (
 const PlaybackRewritePathEnvVariable = "GO_AUTUMN_RESTCLIENT_PLAYBACK_REWRITE_PATH"
 
 type PlaybackImpl struct {
-	RecorderPath        string
-	RecorderRewritePath string
-	// Now is exposed so tests can fixate the time by overwriting this field
-	Now func() time.Time
+	RecorderPath                string
+	RecorderRewritePath         string
+	ConstructFilenameCandidates []aurestrecorder.ConstructFilenameFunction
+	Now                         func() time.Time
+}
+
+type PlaybackOptions struct {
+	// ConstructFilenameCandidates contains filename constructor functions.
+	//
+	// The first one is considered "canonical", for all others, a log entry is printed that instructs the user
+	// to rename the file.
+	ConstructFilenameCandidates []aurestrecorder.ConstructFilenameFunction
+	NowFunc                     func() time.Time
 }
 
 // New builds a new http client simulator based on playback.
 //
 // Use this in your tests.
-func New(recorderPath string) aurestclientapi.Client {
+//
+// You can optionally add a PlaybackOptions instance to your call. The ... is really just so it's an optional argument.
+func New(recorderPath string, additionalOptions ...PlaybackOptions) aurestclientapi.Client {
+	filenameCandidates := []aurestrecorder.ConstructFilenameFunction{
+		aurestrecorder.ConstructFilenameV3WithBody,
+		aurestrecorder.ConstructFilenameWithBody,
+		aurestrecorder.ConstructFilenameV2WithBody,
+	}
+	nowFunc := time.Now
+	for _, o := range additionalOptions {
+		if len(o.ConstructFilenameCandidates) > 0 {
+			filenameCandidates = o.ConstructFilenameCandidates
+		}
+		if o.NowFunc != nil {
+			nowFunc = o.NowFunc
+		}
+	}
 	return &PlaybackImpl{
-		RecorderPath:        recorderPath,
-		RecorderRewritePath: os.Getenv(PlaybackRewritePathEnvVariable),
-		Now:                 time.Now,
+		RecorderPath:                recorderPath,
+		RecorderRewritePath:         os.Getenv(PlaybackRewritePathEnvVariable),
+		ConstructFilenameCandidates: filenameCandidates,
+		Now:                         nowFunc,
 	}
 }
 
-func (c *PlaybackImpl) Perform(ctx context.Context, method string, requestUrl string, _ interface{}, response *aurestclientapi.ParsedResponse) error {
-	filename, err := aurestrecorder.ConstructFilenameV3(method, requestUrl)
-	if err != nil {
-		return err
-	}
+func (c *PlaybackImpl) Perform(ctx context.Context, method string, requestUrl string, requestBody interface{}, response *aurestclientapi.ParsedResponse) error {
+	canonicalFilename := ""
+	var originalError error
+	for i, constructFilenameCandidate := range c.ConstructFilenameCandidates {
+		filename, err := constructFilenameCandidate(method, requestUrl, requestBody)
+		if err != nil {
+			return err
+		}
+		if i == 0 {
+			canonicalFilename = filename
+		}
 
-	jsonBytes, err := os.ReadFile(filepath.Join(c.RecorderPath, filename))
-	if err != nil {
-		// try old filename for compatibility (cannot fail if ConstructFilenameV2 didn't)
-		filenameOldV1, _ := aurestrecorder.ConstructFilename(method, requestUrl)
+		jsonBytes, err := os.ReadFile(filepath.Join(c.RecorderPath, filename))
+		if err != nil {
+			if i == 0 {
+				originalError = err
+			}
+		} else {
+			// successfully read
+			if i > 0 {
+				aulogging.Logger.Ctx(ctx).Info().Printf("use of deprecated recorder filename '%s', please move to '%s'", filename, canonicalFilename)
+			}
 
-		jsonBytesOldV1, errWithOldFilenameV1 := os.ReadFile(filepath.Join(c.RecorderPath, filenameOldV1))
-		if errWithOldFilenameV1 != nil {
-			// try old filename for compatibility (cannot fail if ConstructFilenameV2 didn't)
-			filenameOldV2, _ := aurestrecorder.ConstructFilenameV2(method, requestUrl)
+			_ = c.rewriteFileIfConfigured(ctx, filename, canonicalFilename)
 
-			jsonBytesOldV2, errWithOldFilenameV2 := os.ReadFile(filepath.Join(c.RecorderPath, filenameOldV2))
-			if errWithOldFilenameV2 != nil {
-				// but return original error if that also fails
+			recording := aurestrecorder.RecorderData{}
+			err = json.Unmarshal(jsonBytes, &recording)
+			if err != nil {
 				return err
-			} else {
-				aulogging.Logger.Ctx(ctx).Info().Printf("use of deprecated recorder filename (v2) '%s', please move to '%s'", filenameOldV2, filename)
+			}
 
-				_ = c.rewriteFileIfConfigured(ctx, filenameOldV2, filename)
+			response.Header = recording.ParsedResponse.Header
+			response.Status = recording.ParsedResponse.Status
+			response.Time = c.Now()
 
-				filename = filenameOldV2
-				jsonBytes = jsonBytesOldV2
+			// cannot just assign the body, need to re-parse into the existing pointer - using a json round trip
+			bodyJsonBytes, err := json.Marshal(recording.ParsedResponse.Body)
+			if err != nil {
+				return err
+			}
+			err = json.Unmarshal(bodyJsonBytes, response.Body)
+			if err != nil {
+				return err
 			}
-		} else {
-			aulogging.Logger.Ctx(ctx).Info().Printf("use of deprecated recorder filename (v1) '%s', please move to '%s'", filenameOldV1, filename)
-
-			_ = c.rewriteFileIfConfigured(ctx, filenameOldV1, filename)
 
-			filename = filenameOldV1
-			jsonBytes = jsonBytesOldV1
+			return recording.Error
 		}
-	} else {
-		_ = c.rewriteFileIfConfigured(ctx, filename, filename)
 	}
-
-	recording := aurestrecorder.RecorderData{}
-	err = json.Unmarshal(jsonBytes, &recording)
-	if err != nil {
-		return err
-	}
-
-	response.Header = recording.ParsedResponse.Header
-	response.Status = recording.ParsedResponse.Status
-	response.Time = c.Now()
-
-	// cannot just assign the body, need to re-parse into the existing pointer - using a json round trip
-	bodyJsonBytes, err := json.Marshal(recording.ParsedResponse.Body)
-	if err != nil {
-		return err
-	}
-	err = json.Unmarshal(bodyJsonBytes, response.Body)
-	if err != nil {
-		return err
-	}
-
-	return recording.Error
+	return originalError
 }
 
 func (c *PlaybackImpl) rewriteFileIfConfigured(ctx context.Context, fileNameFrom string, fileNameTo string) error {

implementation/recorder/recorder.go

@@ -12,30 +12,46 @@ import (
 	"strings"
 )
 
+type ConstructFilenameFunction func(method string, requestUrl string, requestBody interface{}) (string, error)
+
 type RecorderImpl struct {
-	Wrapped      aurestclientapi.Client
-	RecorderPath string
+	Wrapped               aurestclientapi.Client
+	RecorderPath          string
+	ConstructFilenameFunc ConstructFilenameFunction
 }
 
 const RecorderPathEnvVariable = "GO_AUTUMN_RESTCLIENT_RECORDER_PATH"
 
+type RecorderOptions struct {
+	ConstructFilenameFunc ConstructFilenameFunction
+}
+
 // New builds a new http recorder.
 //
 // Insert this into your stack just above the actual http client.
 //
 // Normally it does nothing, but if you set the environment variable RecorderPathEnvVariable to a path to a directory,
 // it will write response recordings for your requests that you can then play back using aurestplayback.PlaybackImpl
 // in your tests.
-func New(wrapped aurestclientapi.Client) aurestclientapi.Client {
+//
+// You can optionally add a RecorderOptions instance to your call. The ... is really just so it's an optional argument.
+func New(wrapped aurestclientapi.Client, additionalOptions ...RecorderOptions) aurestclientapi.Client {
 	recorderPath := os.Getenv(RecorderPathEnvVariable)
 	if recorderPath != "" {
 		if !strings.HasSuffix(recorderPath, "/") {
 			recorderPath += "/"
 		}
 	}
+	filenameFunc := ConstructFilenameV3WithBody
+	for _, o := range additionalOptions {
+		if o.ConstructFilenameFunc != nil {
+			filenameFunc = o.ConstructFilenameFunc
+		}
+	}
 	return &RecorderImpl{
-		Wrapped:      wrapped,
-		RecorderPath: recorderPath,
+		Wrapped:               wrapped,
+		RecorderPath:          recorderPath,
+		ConstructFilenameFunc: filenameFunc,
 	}
 }
 
@@ -50,7 +66,7 @@ type RecorderData struct {
 func (c *RecorderImpl) Perform(ctx context.Context, method string, requestUrl string, requestBody interface{}, response *aurestclientapi.ParsedResponse) error {
 	responseErr := c.Wrapped.Perform(ctx, method, requestUrl, requestBody, response)
 	if c.RecorderPath != "" {
-		filename, err := ConstructFilenameV3(method, requestUrl)
+		filename, err := c.ConstructFilenameFunc(method, requestUrl, requestBody)
 		if err == nil {
 			recording := RecorderData{
 				Method:         method,
@@ -88,6 +104,10 @@ func ConstructFilename(method string, requestUrl string) (string, error) {
 	return filename, nil
 }
 
+func ConstructFilenameWithBody(method string, requestUrl string, _ interface{}) (string, error) {
+	return ConstructFilename(method, requestUrl)
+}
+
 func ConstructFilenameV2(method string, requestUrl string) (string, error) {
 	parsedUrl, err := url.Parse(requestUrl)
 	if err != nil {
@@ -112,6 +132,10 @@ func ConstructFilenameV2(method string, requestUrl string) (string, error) {
 	return filename, nil
 }
 
+func ConstructFilenameV2WithBody(method string, requestUrl string, _ interface{}) (string, error) {
+	return ConstructFilenameV2(method, requestUrl)
+}
+
 func ConstructFilenameV3(method string, requestUrl string) (string, error) {
 	parsedUrl, err := url.Parse(requestUrl)
 	if err != nil {
@@ -135,3 +159,7 @@ func ConstructFilenameV3(method string, requestUrl string) (string, error) {
 	filename := fmt.Sprintf("request_%s_%s_%s.json", m, p, q)
 	return filename, nil
 }
+
+func ConstructFilenameV3WithBody(method string, requestUrl string, _ interface{}) (string, error) {
+	return ConstructFilenameV3(method, requestUrl)
+}