Issue-6642 Add http.send request attribute to ignore headers for caching key #6675
Conversation
Signed-off-by: Rudrakh Panigrahi <[email protected]>
rudrakhp
force-pushed
the
cache_ignored_headers
branch
from
f3accd4
to
1c9b35d
Compare
rudrakhp
force-pushed
the
cache_ignored_headers
branch
from
8a0052e
to
81c75e9
Compare
I would have to explore if there is any straight forward way to delete a value by path reference in the Request |
|
@rudrakhp just checking to see if you got a chance to explore the option of excluding certain request object params. |
|
@ashutosh-narkar was out last week, will raise an updated PR this week. |
|
This pull request has been automatically marked as stale because it has not had any activity in the last 30 days. |
There was a problem hiding this comment.
@rudrakhp the changes seem fine. More testing would be helpful. Also we need to update the docs for the builtin.
| func getKeyFromRequest(req ast.Object) (ast.Object, error) { | ||
| var cacheIgnoredHeaders []string | ||
| var allHeaders map[string]interface{} | ||
| cacheIgnoredHeadersTerm := req.Get(ast.StringTerm("cache_ignored_headers")) |
There was a problem hiding this comment.
Nit: We can do an early exit here
if cacheIgnoredHeadersTerm == nil {
return nil, nil
}| expectedReqCount: 1, | ||
| }, | ||
| { | ||
| note: "http.send GET cache miss different headers (force_cache enabled)", |
There was a problem hiding this comment.
Not sure what scenario in the changes is this test case trying to exercise.
| note: "http.send GET different cache_ignored_headers but still cached (force_cache enabled)", | ||
| ruleTemplate: `p = x { | ||
| r1 = http.send({"method": "get", "url": "%URL%", "force_json_decode": true, "headers": {"h1": "v1", "h2": "v2"}, "force_cache": true, "force_cache_duration_seconds": 300, "cache_ignored_headers": ["h2"]}) | ||
| r2 = http.send({"method": "get", "url": "%URL%", "force_json_decode": true, "headers": {"h1": "v1", "h2": "v2"}, "force_cache": true, "force_cache_duration_seconds": 300, "cache_ignored_headers": ["h2", "h3"]}) # cached |
There was a problem hiding this comment.
Just for testing we can actually have a h3 header in the headers object.
There was a problem hiding this comment.
Also a test for the scenario when the value of cache_ignored_headers and headers differs and we get a cache miss would be helpful
There was a problem hiding this comment.
One more case I can think of:
R1: {"headers": {"h1": "v1"}}
R2: {"headers": {"h1": "v1", "h2": "v2"}, "cache_ignored_headers": ["h2"]}
So here R1 and R2 are equivalent, correct?
There was a problem hiding this comment.
Another one:
R1: {"headers": {"h1": "v1"}, "cache_ignored_headers": []}
R2: {"headers": {"h1": "v1", "h2": "v2"}, "cache_ignored_headers": ["h2"]}
So here R1 and R2 are equivalent, correct?
Why the changes in this PR are needed?
Need to support exclusion of certain headers from
http.sendquery cache key.What are the changes in this PR?
cache_ignored_headersin thehttp.sendbuilt in request object. Enables policy authors to define an exclusion list for headers to ignore while caching.Notes to assist PR review:
cache_ignored_headerschanges but does not affect the final computed cache key, the results are still served from cache. This attribute is always excluded from the cache key.Further comments:
Resolves #6642