From e2b00e18aaaffd6bf21db319d3adbf0a5f8f7a26 Mon Sep 17 00:00:00 2001 From: visca Date: Sat, 7 May 2022 12:50:46 +0700 Subject: [PATCH] fix: query result cache key is not stable --- caching.go | 2 +- caching_handler.go | 16 ++++------- caching_plan.go | 14 ++++----- caching_rule.go | 2 +- caching_test.go | 8 ++---- caching_vary.go | 4 +-- caching_vary_test.go | 4 +-- caddyfile_caching.go | 32 ++++----------------- handler_test.go | 67 ++++++++++++++++++++++++++++++++++++++++++++ 9 files changed, 90 insertions(+), 59 deletions(-) diff --git a/caching.go b/caching.go index 177d7a9..c8bcdda 100644 --- a/caching.go +++ b/caching.go @@ -117,7 +117,7 @@ func (c *Caching) Provision(ctx caddy.Context) error { func (c *Caching) Validate() error { for ruleName, rule := range c.Rules { - for vary := range rule.Varies { + for _, vary := range rule.Varies { if _, ok := c.Varies[vary]; !ok { return fmt.Errorf("caching rule %s, configured vary: %s does not exist", ruleName, vary) } diff --git a/caching_handler.go b/caching_handler.go index 5e44c1e..912216b 100644 --- a/caching_handler.go +++ b/caching_handler.go @@ -140,22 +140,16 @@ func (c *Caching) addCachingResponseHeaders(s CachingStatus, r *cachingQueryResu return } - uniqueVaries := make(map[string]struct{}) - - for name := range p.VaryNames { - for v := range c.Varies[name].Headers { - uniqueVaries[v] = struct{}{} + for _, name := range p.VaryNames { + for _, v := range c.Varies[name].Headers { + h.Add("vary", v) } - for v := range c.Varies[name].Cookies { - uniqueVaries[fmt.Sprintf("cookie:%s", v)] = struct{}{} + for _, v := range c.Varies[name].Cookies { + h.Add("vary", fmt.Sprintf("cookie:%s", v)) } } - for vary := range uniqueVaries { - h.Add("vary", vary) - } - if s == CachingStatusHit { age := int64(r.Age().Seconds()) maxAge := int64(time.Duration(r.MaxAge).Seconds()) diff --git a/caching_plan.go b/caching_plan.go index fc57a7f..85c8439 100644 --- a/caching_plan.go +++ b/caching_plan.go @@ -24,7 +24,7 @@ var ( type cachingPlan struct { MaxAge caddy.Duration Swr caddy.Duration - VaryNames map[string]struct{} + VaryNames []string Types map[string]struct{} RulesHash uint64 VariesHash uint64 @@ -158,7 +158,7 @@ func (p *cachingPlanner) savePlan(plan *cachingPlan) error { func (p *cachingPlanner) computePlan() (*cachingPlan, error) { types := make(map[string]struct{}) - varyNames := make(map[string]struct{}) + var varyNames []string plan := &cachingPlan{ Passthrough: true, } @@ -191,9 +191,7 @@ func (p *cachingPlanner) computePlan() (*cachingPlan, error) { plan.Swr = rule.Swr } - for vary := range rule.Varies { - varyNames[vary] = struct{}{} - } + varyNames = append(varyNames, rule.Varies...) if rule.Types == nil { types = nil @@ -242,14 +240,14 @@ func (p *cachingPlanner) calcQueryResultCacheKey(plan *cachingPlan) (string, err r := p.request.httpRequest - for name := range plan.VaryNames { + for _, name := range plan.VaryNames { vary, ok := p.caching.Varies[name] if !ok { return "", fmt.Errorf("setting of vary %s does not exist in varies list given", vary) } - for name := range vary.Headers { + for _, name := range vary.Headers { buffString := fmt.Sprintf("header:%s=%s;", name, r.Header.Get(name)) if _, err := hash.Write([]byte(buffString)); err != nil { @@ -257,7 +255,7 @@ func (p *cachingPlanner) calcQueryResultCacheKey(plan *cachingPlan) (string, err } } - for name := range vary.Cookies { + for _, name := range vary.Cookies { var value string cookie, err := r.Cookie(name) diff --git a/caching_rule.go b/caching_rule.go index ff8eb23..b54ee35 100644 --- a/caching_rule.go +++ b/caching_rule.go @@ -23,7 +23,7 @@ type CachingRule struct { // Varies name apply to query results that match the rule types. // If not set query results will cache public. - Varies map[string]struct{} `json:"varies,omitempty"` + Varies []string `json:"varies,omitempty"` } type CachingRules map[string]*CachingRule diff --git a/caching_test.go b/caching_test.go index 48be230..1b1eaeb 100644 --- a/caching_test.go +++ b/caching_test.go @@ -51,9 +51,7 @@ func TestCaching_Validate(t *testing.T) { Rules: CachingRules{ "default": &CachingRule{ MaxAge: 1, - Varies: map[string]struct{}{ - "test": {}, - }, + Varies: []string{"test"}, }, }, }, @@ -72,9 +70,7 @@ func TestCaching_Validate(t *testing.T) { Rules: CachingRules{ "default": &CachingRule{ MaxAge: 1, - Varies: map[string]struct{}{ - "test": {}, - }, + Varies: []string{"test"}, }, }, }, diff --git a/caching_vary.go b/caching_vary.go index 1ae81aa..17b2b86 100644 --- a/caching_vary.go +++ b/caching_vary.go @@ -9,10 +9,10 @@ import ( // CachingVary using to compute query result cache key by http request cookies and headers. type CachingVary struct { // Headers names for identifier query result cache key. - Headers map[string]struct{} `json:"headers,omitempty"` + Headers []string `json:"headers,omitempty"` // Cookies names for identifier query result cache key. - Cookies map[string]struct{} `json:"cookies,omitempty"` + Cookies []string `json:"cookies,omitempty"` } type CachingVaries map[string]*CachingVary diff --git a/caching_vary_test.go b/caching_vary_test.go index 0af5e96..56ad0d6 100644 --- a/caching_vary_test.go +++ b/caching_vary_test.go @@ -15,9 +15,7 @@ func TestCachingVariesHash(t *testing.T) { varies = CachingVaries{ "default": &CachingVary{ - Cookies: map[string]struct{}{ - "session": {}, - }, + Cookies: []string{"session"}, }, } diff --git a/caddyfile_caching.go b/caddyfile_caching.go index d1810f1..d8389d6 100644 --- a/caddyfile_caching.go +++ b/caddyfile_caching.go @@ -129,17 +129,7 @@ func (c *Caching) unmarshalCaddyfileRules(d *caddyfile.Dispenser) error { return d.ArgErr() } - varies := make(map[string]struct{}) - - for _, arg := range args { - if _, exists := varies[arg]; exists { - return d.Errf("duplicate vary: %s", arg) - } - - varies[arg] = struct{}{} - } - - rule.Varies = varies + rule.Varies = args default: return d.Errf("unrecognized subdirective %s", d.Val()) } @@ -188,8 +178,8 @@ func (c *Caching) unmarshalCaddyfileVaries(d *caddyfile.Dispenser) error { for d.NextBlock(0) { name := d.Val() vary := &CachingVary{ - Headers: make(map[string]struct{}), - Cookies: make(map[string]struct{}), + Headers: []string{}, + Cookies: []string{}, } for subNesting := d.Nesting(); d.NextBlock(subNesting); { @@ -201,13 +191,7 @@ func (c *Caching) unmarshalCaddyfileVaries(d *caddyfile.Dispenser) error { return d.ArgErr() } - for _, arg := range args { - if _, exists := vary.Headers[arg]; exists { - return d.Errf("duplicate header: %s", arg) - } - - vary.Headers[arg] = struct{}{} - } + vary.Headers = args case "cookies": args := d.RemainingArgs() @@ -215,13 +199,7 @@ func (c *Caching) unmarshalCaddyfileVaries(d *caddyfile.Dispenser) error { return d.ArgErr() } - for _, arg := range args { - if _, exists := vary.Cookies[arg]; exists { - return d.Errf("duplicate cookie: %s", arg) - } - - vary.Cookies[arg] = struct{}{} - } + vary.Cookies = args default: return d.Errf("unrecognized subdirective %s", d.Val()) } diff --git a/handler_test.go b/handler_test.go index 32438c6..02f772e 100644 --- a/handler_test.go +++ b/handler_test.go @@ -745,6 +745,73 @@ caching { } } +func (s *HandlerIntegrationTestSuite) TestCombinedCachingVaries() { + const payload = `{"query": "query { users { name } }"}` + const config = ` +caching { + rules { + test { + max_age 60ms + swr 60ms + varies first second + } + } + varies { + first { + headers x-first-header-1 x-first-header-2 + cookies x-first-cookie-1 x-first-cookie-2 + } + second { + headers x-second-header-1 x-second-header-2 + cookies x-second-cookie-1 x-second-cookie-2 + } + } +} +` + tester := caddytest.NewTester(s.T()) + tester.InitServer(pureCaddyfile, "caddyfile") + tester.InitServer(fmt.Sprintf(caddyfilePattern, config), "caddyfile") + + // test cache hit for 20 times + for i := 0; i <= 20; i++ { + expectedCachingStatus := CachingStatusHit + expectedHitTimes := strconv.Itoa(i) + expectedBody := `{"data":{"users":[{"name":"A"},{"name":"B"},{"name":"C"}]}}` + if i == 0 { + expectedCachingStatus = CachingStatusMiss + expectedHitTimes = "" + } + + r, _ := http.NewRequest( + "POST", + "http://localhost:9090/graphql", + strings.NewReader(payload), + ) + r.Header.Add("content-type", "application/json") + + r.Header.Set("x-first-header-1", "x-first-header-1") + r.Header.Set("x-first-header-2", "x-first-header-2") + r.Header.Set("x-second-header-1", "x-second-header-1") + r.Header.Set("x-second-header-2", "x-second-header-2") + + r.AddCookie(&http.Cookie{Name: "x-first-cookie-1", Value: "x-first-cookie-1"}) + r.AddCookie(&http.Cookie{Name: "x-first-cookie-2", Value: "x-first-cookie-2"}) + r.AddCookie(&http.Cookie{Name: "x-second-cookie-1", Value: "x-second-cookie-1"}) + r.AddCookie(&http.Cookie{Name: "x-second-cookie-2", Value: "x-second-cookie-2"}) + + resp := tester.AssertResponseCode(r, http.StatusOK) + respBody, _ := io.ReadAll(resp.Body) + actualStatus := resp.Header.Get("x-cache") + actualHitTimes := resp.Header.Get("x-cache-hits") + + s.Require().Equalf(expectedBody, string(respBody), "unexpected payload") + s.Require().Equalf(string(expectedCachingStatus), actualStatus, "unexpected status") + s.Require().Equalf(expectedHitTimes, actualHitTimes, "unexpected hit times") + + resp.Body.Close() + } +} + func (s *HandlerIntegrationTestSuite) TestEnabledPlaygrounds() { tester := caddytest.NewTester(s.T()) tester.InitServer(pureCaddyfile, "caddyfile")