fix: core-cache headers case (#889)

Author: guybedford

integration-tests/js-compute/fixtures/app/src/cache-core.js

@@ -3786,3 +3786,145 @@ let error;
         });
     }
 }
+
+{
+
+    routes.set("/core-cache/transaction-lookup-transaction-insert-vary-works", async () => {
+        const key = `/core-cache/vary-works-${Date.now()}`
+        const animal = 'animal'
+        let entry = CoreCache.transactionLookup(key, {
+            headers: {
+                [animal]: 'cat'
+            }
+        })
+        let error = assert(entry.state().found(), false, `entry.state().found() === false`)
+        if (error) { return error }
+        let writer = entry.insert({
+            maxAge: 60_000 * 60,
+            vary: [animal],
+            headers: {
+                [animal]: 'cat'
+            }
+        })
+
+        writer.append('cat')
+        writer.close()
+        entry.close()
+        await sleep(1_000);
+
+        entry = CoreCache.transactionLookup(key, {
+            headers: {
+                [animal]: 'cat'
+            }
+        })
+        error = assert(entry.state().found(), true, `entry.state().found() === true`)
+        if (error) { return error }
+
+        error = assert(await streamToString(entry.body()), 'cat', `await streamToString(CoreCache.lookup(key).body())`)
+        if (error) { return error }
+        entry.close()
+
+        entry = CoreCache.transactionLookup(key, {
+            headers: {
+                [animal]: 'dog'
+            }
+        })
+        error = assert(entry.state().found(), false, `entry.state().found() == false`)
+        if (error) { return error }
+
+        writer = entry.insert({
+            maxAge: 60_000 * 60,
+            vary: [animal],
+            headers: {
+                [animal]: 'dog'
+            }
+        })
+
+        writer.append('dog')
+        writer.close()
+        entry.close()
+        await sleep(1_000);
+
+        entry = CoreCache.transactionLookup(key, {
+            headers: {
+                [animal]: 'dog'
+            }
+        })
+        error = assert(entry.state().found(), true, `entry.state().found() === true`)
+        if (error) { return error }
+
+        error = assert(await streamToString(entry.body()), 'dog', `await streamToString(CoreCache.lookup(key).body())`)
+        if (error) { return error }
+        entry.close()
+
+        return pass("ok")
+    });
+
+    routes.set("/core-cache/lookup-insert-vary-works", async () => {
+        const key = `/core-cache/vary-works-${Date.now()}`
+        const animal = 'animal'
+        let entry = CoreCache.lookup(key, {
+            headers: {
+                [animal]: 'cat'
+            }
+        })
+        let error = assert(entry, null, `entry == null`)
+        if (error) { return error }
+        let writer = CoreCache.insert(key, {
+            maxAge: 60_000 * 60,
+            vary: [animal],
+            headers: {
+                [animal]: 'cat'
+            }
+        })
+
+        writer.append('cat')
+        writer.close()
+        await sleep(1_000);
+
+        entry = CoreCache.lookup(key, {
+            headers: {
+                [animal]: 'cat'
+            }
+        })
+        error = assert(entry.state().found(), true, `entry.state().found() === true`)
+        if (error) { return error }
+
+        error = assert(await streamToString(entry.body()), 'cat', `await streamToString(CoreCache.lookup(key).body())`)
+        if (error) { return error }
+        entry.close()
+
+        entry = CoreCache.lookup(key, {
+            headers: {
+                [animal]: 'dog'
+            }
+        })
+        error = assert(entry, null, `entry == null`)
+        if (error) { return error }
+
+        writer = CoreCache.insert(key, {
+            maxAge: 60_000 * 60,
+            vary: [animal],
+            headers: {
+                [animal]: 'dog'
+            }
+        })
+
+        writer.append('dog')
+        writer.close()
+        await sleep(1_000);
+
+        entry = CoreCache.lookup(key, {
+            headers: {
+                [animal]: 'dog'
+            }
+        })
+        error = assert(entry.state().found(), true, `entry.state().found() === true`)
+        if (error) { return error }
+
+        error = assert(await streamToString(entry.body()), 'dog', `await streamToString(CoreCache.lookup(key).body())`)
+        if (error) { return error }
+
+        return pass("ok")
+    });
+}

integration-tests/js-compute/fixtures/app/tests.json

@@ -8703,5 +8703,27 @@
       "status": 200,
       "body": "ok"
     }
+  },
+  "GET /core-cache/transaction-lookup-transaction-insert-vary-works": {
+    "environments": ["compute"],
+    "downstream_request": {
+      "method": "GET",
+      "pathname": "/core-cache/transaction-lookup-transaction-insert-vary-works"
+    },
+    "downstream_response": {
+      "status": 200,
+      "body": "ok"
+    }
+  },
+  "GET /core-cache/lookup-insert-vary-works": {
+    "environments": ["compute"],
+    "downstream_request": {
+      "method": "GET",
+      "pathname": "/core-cache/lookup-insert-vary-works"
+    },
+    "downstream_response": {
+      "status": 200,
+      "body": "ok"
+    }
   }
 }

runtime/fastly/builtins/cache-core.cpp

@@ -30,6 +30,10 @@ api::Engine *ENGINE;
 JS::Result<host_api::CacheLookupOptions> parseLookupOptions(JSContext *cx,
                                                             JS::HandleValue options_val) {
   host_api::CacheLookupOptions options;
+  // options parameter is optional
+  // options is meant to be an object with an optional headers field,
+  // the headers field can be:
+  // Headers | string[][] | Record<string, string>;
   if (!options_val.isUndefined()) {
     if (!options_val.isObject()) {
       JS_ReportErrorASCII(cx, "options argument must be an object");
@@ -42,28 +46,29 @@ JS::Result<host_api::CacheLookupOptions> parseLookupOptions(JSContext *cx,
     }
     // headers property is optional
     if (!headers_val.isUndefined()) {
-      JS::RootedObject headersInstance(
-          cx, JS_NewObjectWithGivenProto(cx, &Headers::class_, Headers::proto_obj));
-      if (!headersInstance) {
+      if (!headers_val.isObject()) {
+        JS_ReportErrorASCII(
+            cx, "Failed to construct Headers object. If defined, the first argument must be either "
+                "a [ ['name', 'value'], ... ] sequence, or a { 'name' : 'value', ... } record.");
         return JS::Result<host_api::CacheLookupOptions>(JS::Error());
       }
-      auto headers = Headers::create(cx, headersInstance, Headers::Mode::Standalone, nullptr,
-                                     headers_val, true);
-      if (!headers) {
+      JS::RootedObject request_opts(cx, JS_NewPlainObject(cx));
+      if (!JS_SetProperty(cx, request_opts, "headers", headers_val)) {
         return JS::Result<host_api::CacheLookupOptions>(JS::Error());
       }
-      JS::RootedValue headers_val(cx, JS::ObjectValue(*headers));
       JS::RootedObject requestInstance(cx, Request::create_instance(cx));
       if (!requestInstance) {
         return JS::Result<host_api::CacheLookupOptions>(JS::Error());
       }
 
+      JS::RootedValue request_opts_val(cx, ObjectValue(*request_opts));
+
       // We need to convert the supplied HeadersInit in the `headers` property into a host-backed
       // Request which contains the same headers Request::create does exactly that
       // however, it also expects a fully valid URL for the Request. We don't ever use the Request
       // URL, so we hard-code a valid URL
       JS::RootedValue input(cx, JS::StringValue(JS_NewStringCopyZ(cx, "http://example.com")));
-      JS::RootedObject request(cx, Request::create(cx, requestInstance, input, headers_val));
+      JS::RootedObject request(cx, Request::create(cx, requestInstance, input, request_opts_val));
       options.request_headers = host_api::HttpReq(Request::request_handle(request));
     }
   }
@@ -326,28 +331,28 @@ JS::Result<host_api::CacheWriteOptions> parseInsertOptions(JSContext *cx,
   }
   // headers property is optional
   if (!headers_val.isUndefined()) {
-    JS::RootedObject headersInstance(
-        cx, JS_NewObjectWithGivenProto(cx, &Headers::class_, Headers::proto_obj));
-    if (!headersInstance) {
+    if (!headers_val.isObject()) {
+      JS_ReportErrorASCII(
+          cx, "Failed to construct Headers object. If defined, the first argument must be either "
+              "a [ ['name', 'value'], ... ] sequence, or a { 'name' : 'value', ... } record.");
       return JS::Result<host_api::CacheWriteOptions>(JS::Error());
     }
-    auto headers =
-        Headers::create(cx, headersInstance, Headers::Mode::Standalone, nullptr, headers_val, true);
-    if (!headers) {
+    JS::RootedObject request_opts(cx, JS_NewPlainObject(cx));
+    if (!JS_SetProperty(cx, request_opts, "headers", headers_val)) {
       return JS::Result<host_api::CacheWriteOptions>(JS::Error());
     }
-    JS::RootedValue headers_val(cx, JS::ObjectValue(*headers));
     JS::RootedObject requestInstance(cx, Request::create_instance(cx));
     if (!requestInstance) {
       return JS::Result<host_api::CacheWriteOptions>(JS::Error());
     }
+    JS::RootedValue request_opts_val(cx, ObjectValue(*request_opts));
 
     // We need to convert the supplied HeadersInit in the `headers` property into a host-backed
     // Request which contains the same headers Request::create does exactly that however,
     // it also expects a fully valid URL for the Request. We don't ever use the Request URL, so we
     // hard-code a valid URL
     JS::RootedValue input(cx, JS::StringValue(JS_NewStringCopyZ(cx, "http://example.com")));
-    JS::RootedObject request(cx, Request::create(cx, requestInstance, input, headers_val));
+    JS::RootedObject request(cx, Request::create(cx, requestInstance, input, request_opts_val));
     options.request_headers = host_api::HttpReq(Request::request_handle(request));
   }
   return options;
@@ -1055,49 +1060,11 @@ bool CoreCache::transactionLookup(JSContext *cx, unsigned argc, JS::Value *vp) {
     return false;
   }
 
-  host_api::CacheLookupOptions options;
-  auto options_val = args.get(1);
-  // options parameter is optional
-  // options is meant to be an object with an optional headers field,
-  // the headers field can be:
-  // Headers | string[][] | Record<string, string>;
-  if (!options_val.isUndefined()) {
-    if (!options_val.isObject()) {
-      JS_ReportErrorASCII(cx, "options argument must be an object");
-      return false;
-    }
-    JS::RootedObject options_obj(cx, &options_val.toObject());
-    JS::RootedValue headers_val(cx);
-    if (!JS_GetProperty(cx, options_obj, "headers", &headers_val)) {
-      return false;
-    }
-    // headers property is optional
-    if (!headers_val.isUndefined()) {
-      JS::RootedObject headersInstance(
-          cx, JS_NewObjectWithGivenProto(cx, &Headers::class_, Headers::proto_obj));
-      if (!headersInstance) {
-        return false;
-      }
-      auto headers = Headers::create(cx, headersInstance, Headers::Mode::Standalone, nullptr,
-                                     headers_val, true);
-      if (!headers) {
-        return false;
-      }
-      JS::RootedValue headers_val(cx, JS::ObjectValue(*headers));
-      JS::RootedObject requestInstance(cx, Request::create_instance(cx));
-      if (!requestInstance) {
-        return false;
-      }
-
-      // We need to convert the supplied HeadersInit in the `headers` property into a host-backed
-      // Request which contains the same headers Request::create does exactly that
-      // however, it also expects a fully valid URL for the Request. We don't ever use the Request
-      // URL, so we hard-code a valid URL
-      JS::RootedValue input(cx, JS::StringValue(JS_NewStringCopyZ(cx, "http://example.com")));
-      JS::RootedObject request(cx, Request::create(cx, requestInstance, input, headers_val));
-      options.request_headers = host_api::HttpReq(Request::request_handle(request));
-    }
+  auto options_result = parseLookupOptions(cx, args.get(1));
+  if (options_result.isErr()) {
+    return false;
   }
+  auto options = options_result.unwrap();
 
   auto res = host_api::CacheHandle::transaction_lookup(key, options);
   if (auto *err = res.to_err()) {