From 80bb85321933dffbc9ce58f1be154087ea072bb5 Mon Sep 17 00:00:00 2001
From: Ashutosh Narkar <anarkar4387@gmail.com>
Date: Thu, 8 Oct 2020 23:53:10 -0700
Subject: [PATCH] topdown/http: validate force_cache parameters

Earlier the validation of the force_cache parameters was performed
after creating the inter-query cache object. So if 'force_cache' was
set and 'force_cache_duration_seconds' was not provided, OPA in server mode
would correctly return an error but OPA running in repl mode would not
return an error and simply would fall back to using the intra-query cache.

This commit updates how the 'force_cache' parameters are validated so that
consistent behavior is seen in both repl and server modes.

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
---
 topdown/http.go      | 29 +++++++++++++----------------
 topdown/http_test.go |  7 +------
 2 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/topdown/http.go b/topdown/http.go
index d13d63051d..4e8fff7bbc 100644
--- a/topdown/http.go
+++ b/topdown/http.go
@@ -936,13 +936,13 @@ type httpRequestExecutor interface {
 // newHTTPRequestExecutor returns a new HTTP request executor that wraps either an inter-query or
 // intra-query cache implementation
 func newHTTPRequestExecutor(bctx BuiltinContext, key ast.Object) (httpRequestExecutor, error) {
-	useInterQueryCache, forceCache, err := useInterQueryCache(key)
+	useInterQueryCache, forceCacheParams, err := useInterQueryCache(key)
 	if err != nil {
 		return nil, handleHTTPSendErr(bctx, err)
 	}
 
 	if useInterQueryCache && bctx.InterQueryBuiltinCache != nil {
-		return newInterQueryCache(bctx, key, forceCache)
+		return newInterQueryCache(bctx, key, forceCacheParams)
 	}
 	return newIntraQueryCache(bctx, key)
 }
@@ -953,12 +953,11 @@ type interQueryCache struct {
 	httpReq          *http.Request
 	httpClient       *http.Client
 	forceJSONDecode  bool
-	forceCache       bool
 	forceCacheParams *forceCacheParams
 }
 
-func newInterQueryCache(bctx BuiltinContext, key ast.Object, forceCache bool) (*interQueryCache, error) {
-	return &interQueryCache{bctx: bctx, key: key, forceCache: forceCache}, nil
+func newInterQueryCache(bctx BuiltinContext, key ast.Object, forceCacheParams *forceCacheParams) (*interQueryCache, error) {
+	return &interQueryCache{bctx: bctx, key: key, forceCacheParams: forceCacheParams}, nil
 }
 
 // CheckCache checks the cache for the value of the key set on this object
@@ -975,13 +974,6 @@ func (c *interQueryCache) CheckCache() (ast.Value, error) {
 		return nil, handleHTTPSendErr(c.bctx, err)
 	}
 
-	if c.forceCache {
-		c.forceCacheParams, err = newForceCacheParams(c.key)
-		if err != nil {
-			return nil, handleHTTPSendErr(c.bctx, err)
-		}
-	}
-
 	resp, err := checkHTTPSendInterQueryCache(c.bctx, c.key, c.httpReq, c.httpClient, c.forceJSONDecode, c.forceCacheParams)
 
 	// fallback to the intra-query cache if response not found in the inter-query cache or inter-query cache look-up results
@@ -1051,18 +1043,23 @@ func (c *intraQueryCache) ExecuteHTTPRequest() (*http.Response, error) {
 	return executeHTTPRequest(httpReq, httpClient)
 }
 
-func useInterQueryCache(req ast.Object) (bool, bool, error) {
+func useInterQueryCache(req ast.Object) (bool, *forceCacheParams, error) {
 	value, err := getBoolValFromReqObj(req, ast.StringTerm("cache"))
 	if err != nil {
-		return false, false, err
+		return false, nil, err
 	}
 
 	valueForceCache, err := getBoolValFromReqObj(req, ast.StringTerm("force_cache"))
 	if err != nil {
-		return false, false, err
+		return false, nil, err
 	}
 
-	return value || valueForceCache, valueForceCache, nil
+	if valueForceCache {
+		forceCacheParams, err := newForceCacheParams(req)
+		return true, forceCacheParams, err
+	}
+
+	return value, nil, nil
 }
 
 type forceCacheParams struct {
diff --git a/topdown/http_test.go b/topdown/http_test.go
index a93f0d64a3..7b1d354ff7 100644
--- a/topdown/http_test.go
+++ b/topdown/http_test.go
@@ -1448,12 +1448,7 @@ func TestInterQueryCheckCacheError(t *testing.T) {
 	input := ast.MustParseTerm(`{"force_cache": true}`)
 	inputObj := input.Value.(ast.Object)
 
-	cache, err := newInterQueryCache(BuiltinContext{Context: context.Background()}, inputObj, true)
-	if err != nil {
-		t.Fatalf("unexpected error %v", err)
-	}
-
-	_, err = cache.CheckCache()
+	_, err := newHTTPRequestExecutor(BuiltinContext{Context: context.Background()}, inputObj)
 	if err == nil {
 		t.Fatal("expected error but got nil")
 	}