-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(plugins): Caddy config cache_keys json parse error #460
base: master
Are you sure you want to change the base?
Conversation
IMHO that's not an issue caused by the JSON parser. |
plugins/caddy/httpcache_test.go
Outdated
resp1, _ := tester.AssertGetResponse(`http://localhost:9080/query-string?query=string`, 200, "Hello, query string!") | ||
if resp1.Header.Get("Cache-Status") != "Souin; fwd=uri-miss; stored; key=GET-http-localhost:9080-/query-string" { | ||
t.Errorf("unexpected Cache-Status header %v", resp1.Header) | ||
} | ||
resp2, _ := tester.AssertGetResponse(`http://localhost:9080/query-string?foo=bar`, 200, "Hello, query string!") | ||
if resp2.Header.Get("Cache-Status") != "Souin; fwd=uri-miss; stored; key=GET-http-localhost:9080-/query-string?foo=bar" { | ||
t.Errorf("unexpected Cache-Status header %v", resp2.Header) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you disable query within the cache_key then you can expect two stored responses because it should generate a key like that GET-http-localhost:9080-/query-string
for both so the first one will receive a response that is stored and the second one will get a cached response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick reply.
If I run the new test case alone with go test -run TestCacheKeys
, then it passes, but without the run
filter the same test fails. Could be a persistent state between tests?
I expected the new test case to be valid because the query=
regexp matches the first request and modifies the key generation to exclude the query string, and the key becomes GET-http-localhost:9080-/query-string
. The second request does not match the regexp so the key generation should include the query string and the key becomes GET-http-localhost:9080-/query-string?foo=bar
. Since it is a different key its a miss and store.
At least this was my understanding of the docs. What is the expected behavior in this case?
I changed the path in the test url to be unique among all tests, now running all tests passes. |
You were right I read to fast your PR, thank you 🙏. I still have a concern about the JSON parser. |
I'm not sure, how the
cache_keys
with regexp should exactly work by specification, but the following example in theCaddyfile
did not work.The json parser of the CacheKeys struct shifted directives up and the regexp become the directive form inside the block.