Add advanced filtering #32

klauspost · 2018-10-19T15:17:48Z

When running tests where multiple request types are used, it if quite difficult to set up proper filtering.

To help this, I made it possible to chain multiple filters and check URL and method.

Furthermore the URL can now be modified.

For future-proofing the Request and Response structs can now be extended with more stuff without breaking compatibility.

If you like this, I can add some tests for the new functionality.

When running tests where multiple request types are used, it if quite difficult to set up proper filtering. To help this, I made it possible to chain multiple filters and check URL and method. Furthermore the URL can now be modified. For future-proofing the Request and Response structs can now be extended with more stuff without breaking compatibility. If you like this, I can add some tests for the new functionality.

Fix documentation.

klauspost · 2018-10-22T13:43:55Z

After doing some examples I've decided to re-write some of the stuff...

It is a bit too clunky to use now.

Add body replacement example.

klauspost · 2018-10-22T16:06:38Z

@seborama I have added examples for using the specific filtering. If there is more I can do, please let me know.

I think it is now reasonably easy to use and allows for much flexibility. Adding some unit tests to the filters should be fairly easy if you like it overall.

seborama · 2018-10-22T17:25:28Z

Hi @klauspost,

Thanks for contributing.
I need a little more context on your use case. Could you expand?
Do you have a working example to illustrate?

Regards

seborama · 2018-10-22T18:43:03Z

I just noticed you have also posted some examples...

klauspost · 2018-10-22T18:52:22Z

To give the overall picture, I need some tests to test functionality that can do multiple calls to the same endpoint.

The endpoint will return unique IDs, so I need to be able to flexibly adjust content of the returned bodies. URLs typically have IDs as well, so I need to be able to still match content even though IDs change.

A huge pain point was that it wasn't possible to do URL/method matching, so transforms would only happen when specific combinations were hit. This will record requests with request transformations applied, so "transformed" requests would be stored and allow for more flexible matching.

The unmodified request is given to the response filters, so they can still operate on the full information given.

seborama · 2018-10-22T21:33:18Z

govcr.go

+// Original filter is unmodified.
+func (r RequestFilter) OnPath(pathRegEx string) RequestFilter {
+	if pathRegEx == "" {
+		pathRegEx = "*"


This isn't a valid regex.
Use "." or ".*"

seborama · 2018-10-22T21:34:41Z

govcr.go

+// Original filter is unmodified.
+func (r ResponseFilter) OnPath(pathRegEx string) ResponseFilter {
+	if pathRegEx == "" {
+		pathRegEx = "*"


"*" is not a valid regex.
use "." or ".*"

seborama · 2018-10-22T21:44:39Z

govcr.go

+	*r = v
+}
+
+// First one or more filters before the current ones.


Prepend rather?

seborama · 2018-10-22T21:44:48Z

govcr.go

+}
+
+// First one or more filters before the current ones.
+func (r ResponseFilters) First(filters ...ResponseFilter) ResponseFilters {


Prepend rather?

seborama · 2018-10-22T21:46:06Z

govcr.go

@@ -270,8 +532,23 @@ type RequestFilterFunc func(http.Header, []byte) (*http.Header, *[]byte)
 // Return values:
 //  - value 1 - Response's amended header
 //  - value 2 - Response's amended body
+// Deprecated: Use ResponseFilterFunc instead.


If we're going to go down this road, let's get rid off ResponseFilterFunc altogether.
This would form v3 of govcr.

👍 Just didn't want to break it unless you were fine with it.

seborama · 2018-10-22T21:46:38Z

govcr.go

@@ -28,12 +30,21 @@ const defaultCassettePath = "./govcr-fixtures/"
 type VCRConfig struct {
 	Client            *http.Client
 	ExcludeHeaderFunc ExcludeHeaderFunc
+
+	// Deprecated, use RequestFilter


If we're going to go down this road, let's get rid of RequestFilterFunc altogether.
This would form v3 of govcr.

seborama · 2018-10-22T21:46:45Z

govcr.go

 	RequestFilterFunc RequestFilterFunc

 	// ResponseFilterFunc can be used to modify the header of the response.
 	// This is useful when a fingerprint is exchanged and expected to match between request and response.
+	// Deprecated, use ResponseFilter


If we're going to go down this road, let's get rid of ResponseFilterFunc altogether.
This would form v3 of govcr.

seborama · 2018-10-22T22:07:19Z

examples/example6.go

+	}
+
+	// print outcome.
+	fmt.Println("Status code:", resp.StatusCode, " (should be 404 on real and 202 on replay)")


This should form the basis of a formal go example test (as in the root folder xxx_examplen_test.go and some unit tests too.

seborama · 2018-10-22T22:12:49Z

examples/example6.go

+
+		// Add header if method was "GET"
+		govcr.ResponseFilter(func(resp govcr.Response) govcr.Response {
+			resp.Header.Add("method-was-get", "true")


best to use a different value such as present or added etc
when using 'true', this is converted to a boolean by the http client which then cannot be displayed by Println().

I am not sure what you mean, it prints fine:

method-was-post: true (should be true on replay on POST)

Ha, odd! On my environment the word 'true' is missing (go 1.11.1)

seborama · 2018-10-22T22:12:55Z

examples/example6.go

+
+		// Add header if method was "POST"
+		govcr.ResponseFilter(func(resp govcr.Response) govcr.Response {
+			resp.Header.Add("method-was-post", "true")


best to use a different value such as present or added etc
when using 'true', this is converted to a boolean by the http client which then cannot be displayed by Println().

seborama · 2018-10-22T22:18:29Z

govcr.go

+// ResponseFilters is a slice of ResponseFilter
+type ResponseFilters []ResponseFilter
+
+// ResponseContext provides the response parameters.


Not a ResponseContext

seborama · 2018-10-22T22:20:01Z

govcr.go

+	}
+}
+
+// AddHeaderValue will add or overwrite a header to the request


Method says RequestAddHeaderValue?

seborama · 2018-10-22T22:20:19Z

govcr.go

+	}
+}
+
+// DeleteHeaderKeys will delete one or more header keys on the request


Method says RequestDeleteHeaderKeys

seborama · 2018-10-22T22:20:49Z

govcr.go

+	}
+}
+
+// ResponseDeleteHeader will delete one or more headers on the response when returned from vcr playback.


Method says ResponseDeleteHeaderKeys

seborama · 2018-10-22T22:21:30Z

govcr.go

+	}
+}
+
+// Chain one or more filters after the current one and return as single filter.


Invalid comment: method name is Append

seborama · 2018-10-22T22:23:51Z

Hi,

Great contribution! I think this PR should take us straight to the long outstanding v3 of govcr.

I have some comments:

the units tests have not been written - ahem...
new example tests (as in go xxx_example.go in the root of the project) are needed and some of the visual checks in particular in examples/example6.go can be migrated there.
the doc needs updating to reflect the removal of FilterFunc and introduction of Filters
I've also posted a few more comments on the PR
I'd probably move the RequestFilter and ResponseFilter receiver functions each in their own separate go source file.

On a more philosophical note, I am concerned with examples/example6.go. It shows a deep transfiguration of the recorded track at replay time. This is likely to lead to complex test code that will require a degree maintenance and incur a steep learning curve for new joiners. (This was the original reason behind the imposed usage restrictions.)

Regards.

klauspost · 2018-10-24T14:30:00Z

the units tests have not been written - ahem...

Indeed. Didn't want to spend the time upfront if you didn't like the concept.

new example tests (as in go xxx_example.go in the root of the project) are needed and some of the visual checks in particular in examples/example6.go can be migrated there.

👍

the doc needs updating to reflect the removal of FilterFunc and introduction of Filters
👍

I'd probably move the RequestFilter and ResponseFilter receiver functions each in their own separate go source file.

👍

It shows a deep transfiguration of the recorded track at replay time.

Technically it is not much that couldn't be achieved before, just in a more available interface.

Things like testing external interfaces often includes complex logic, and having it available for advanced needs can be a positive.

Fix documentation.

seborama · 2018-10-24T15:08:59Z

Sounds good. I realise there's quite a bit in my comments. Don't feel coerced into doing all. I can pick up some of it if you like.

Remove Append and make Prepend operate on pointer and not return new slice.

klauspost · 2018-10-24T16:48:37Z

Sounds good. I realise there's quite a bit in my comments. Don't feel coerced into doing all. I can pick up some of it if you like.

No problem. I have added tests for all new functions and added the examples as tests as well.

Made a few tweaks. I removed "Append" and changed "Prepend" to not return the updated slice - seems to just be a way of forgetting to re-assign the returned value.

I can't really tell if ExcludeHeaderFunc is the same as RequestDeleteHeaderKeys. As far as I can tell it is. If so, I guess it can just be removed.

I don't have plans for more changes unless you ask me to :)

seborama · 2018-10-24T18:20:35Z

Thanks. Let me know when you're happy with the PR and I'll merge it. It would be nice if at least the docs were updated 😄

seborama · 2018-10-24T19:48:08Z

Sorry I'm reading this on my mobile and I seem to consistently fail to read your messages in whole. I just noticed you said you're done. I'll merge when I get a chance.

Thanks for your time and for sharing your contributions. I'm sure many people will be grateful too.

Bonne chance!

klauspost added 4 commits October 19, 2018 17:17

Add Example, update documentation.

26537d4

Fix documentation.

Merge branch 'master' into add-advanced-filtering

674962d

Update example description.

cfdb75c

klauspost added 2 commits October 22, 2018 17:51

Refactor filter chains to something simpler.

dcc4249

Add body replacement example.

Neat some stuff up

c05a457

seborama reviewed Oct 22, 2018

View reviewed changes

Update wildcard regex.

e3dfbb3

First -> Prepend

ac6e1d4

klauspost added 3 commits October 24, 2018 16:39

Remove RequestFilterFunc/ResponseFilterFunc - move to separate files.

78a0570

Add example 6&7 tests

46264ca

Fix documentation.

Fix comment.

adf837d

klauspost added 2 commits October 24, 2018 18:31

Add unit tests.

d4e5701

Remove Append and make Prepend operate on pointer and not return new slice.

Minimal documentation changes.

edf78cc

klauspost added 3 commits October 24, 2018 18:50

Fix two more places of FilterFunc

7627f3c

Formatting

a97f5d5

Comment formatting

88cd8ed

seborama merged commit a744811 into seborama:master Oct 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add advanced filtering #32

Add advanced filtering #32

klauspost commented Oct 19, 2018

klauspost commented Oct 22, 2018

klauspost commented Oct 22, 2018 •

edited

Loading

seborama commented Oct 22, 2018 •

edited

Loading

seborama commented Oct 22, 2018

klauspost commented Oct 22, 2018 •

edited

Loading

seborama Oct 22, 2018

seborama Oct 22, 2018

seborama Oct 22, 2018

seborama Oct 22, 2018

seborama Oct 22, 2018

klauspost Oct 24, 2018

seborama Oct 22, 2018 •

edited

Loading

seborama Oct 22, 2018 •

edited

Loading

seborama Oct 22, 2018

seborama Oct 22, 2018

klauspost Oct 24, 2018

seborama Oct 24, 2018

seborama Oct 22, 2018

seborama Oct 22, 2018

seborama Oct 22, 2018

seborama Oct 22, 2018

seborama Oct 22, 2018

seborama Oct 22, 2018

seborama commented Oct 22, 2018 •

edited

Loading

klauspost commented Oct 24, 2018

seborama commented Oct 24, 2018

klauspost commented Oct 24, 2018 •

edited

Loading

seborama commented Oct 24, 2018

seborama commented Oct 24, 2018

Add advanced filtering #32

Add advanced filtering #32

Conversation

klauspost commented Oct 19, 2018

klauspost commented Oct 22, 2018

klauspost commented Oct 22, 2018 • edited Loading

seborama commented Oct 22, 2018 • edited Loading

seborama commented Oct 22, 2018

klauspost commented Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seborama Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

seborama Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seborama commented Oct 22, 2018 • edited Loading

klauspost commented Oct 24, 2018

seborama commented Oct 24, 2018

klauspost commented Oct 24, 2018 • edited Loading

seborama commented Oct 24, 2018

seborama commented Oct 24, 2018

klauspost commented Oct 22, 2018 •

edited

Loading

seborama commented Oct 22, 2018 •

edited

Loading

klauspost commented Oct 22, 2018 •

edited

Loading

seborama Oct 22, 2018 •

edited

Loading

seborama Oct 22, 2018 •

edited

Loading

seborama commented Oct 22, 2018 •

edited

Loading

klauspost commented Oct 24, 2018 •

edited

Loading