Implement cache and replay #216
Conversation
|
A potential improvement is making the cached []byte value be an Entry struct, which would contain not only the serialized http.Response, but also the http.Request and other meta information, so it can be extended in the future, as well as aid in debugging. Let me know what you think. |
cache/cache_modifier.go
Outdated
|
|
||
| const ( | ||
| // CustomKey is the context key for setting a custom cache key for a request. | ||
| CustomKey = "cache.CustomKey" |
There was a problem hiding this comment.
does CustomKey need to be exported?
There was a problem hiding this comment.
Yes. This allows other modifiers to to set custom cache keys via setting it in the context. E.g. a modifier that looks for a specific header, or a modifier that looks for a specific URL param, etc. This allows flexibility in controlling how a cache key is generated via modifiers.
cache/cache_modifier.go
Outdated
| // If `replay` is true, the modifier will replay responses from its cache. | ||
| // If `hermetic` is true, the modifier will return error if it cannot replay a cached response, e.g. on cache miss or not replaying. | ||
| // Argument combinations that don't make sense will return error, e.g. replay=false and hermetic=true. | ||
| func NewModifier(filepath, bucket string, update, replay, hermetic bool) (martian.RequestResponseModifier, error) { |
There was a problem hiding this comment.
have this constructor only take the filepath - the rest should be reasonable defaults that can be modified by setters. By default, the modifier should be replay mode.
func NewModifier(filepath) (martian.RequestResponseModifier, error) {
func (m *modifier) SetBucketName(name string) {
etc
cache/cache_modifier.go
Outdated
| return nil, fmt.Errorf("cache.Modifier: bucket name cannot be empty if updating or replaying") | ||
| } | ||
|
|
||
| opt := &bolt.Options{ |
cache/cache_modifier.go
Outdated
| } | ||
| // TODO: Figure out how to close the db after use. | ||
|
|
||
| if bucket != "" { |
There was a problem hiding this comment.
we should probably have a default bucket name
cache/cache_modifier.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // ModifyRequest modifies the http.Request. |
cache/cache_modifier.go
Outdated
| } | ||
|
|
||
| // getCacheKey gets a cache key from the request context or generates a new one. | ||
| func getCacheKey(req *http.Request) ([]byte, error) { |
There was a problem hiding this comment.
What's the benefit of holding the key in the request context? Generating the key on the fly is pretty inexpensive, and this would only be helpful if we were generating it several times over the course of the request / response - and I only see us doing it twice (once on request, once on response).
There was a problem hiding this comment.
This is so any upstream request modifiers can set a custom cache key, instead of using the naive default one here. I think storing it in the context is better than in the HTTP header or URL param, so it doesn't pollute the HTTP request.
cache/cache_modifier.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // ModifyResponse modifies the http.Response. |
cache/cache_modifier.go
Outdated
| ctx := martian.NewContext(res.Request) | ||
| cached, ok := ctx.Get(cachedResponseCtxKey) | ||
| if ok { | ||
| *res = *cached.(*http.Response) |
There was a problem hiding this comment.
if cached, ok := ctx.Get(cachedResponseCtxKey); ok {
*res = *cached.(*http.Response)
return nil
}
and then you don't need else
cache/cache_modifier.go
Outdated
| ctx := martian.NewContext(res.Request) | ||
| cached, ok := ctx.Get(cachedResponseCtxKey) | ||
| if ok { | ||
| *res = *cached.(*http.Response) |
There was a problem hiding this comment.
should make res.Request the request that we're responding to - not the request associated with the cached response
rather than wholesale swapping out the response, might it be more clear to replace the body and the headers of res?
There was a problem hiding this comment.
Good catch about the res.Request. Will update it.
For the response, I swapped it out entirely, since the one we start with is a canned response anyway. Besides the body and headers, there's also the HTTP major/minor versions, and possible trailers. Is there any concern with just swapping it out? The only thing I can think of is if there is a response modifier upstream that had data it wanted to preserve. However, those modifiers should be downstream from the cache modifier, so they can modify the cached response.
|
PTAL. |
|
Is this in usable state atm? |
This implementation of cache and replay is backed by boltdb. #75
Features:
Future work: