From c1fd8cac5e95e1a46ccef5fe0c05eb47c84703e0 Mon Sep 17 00:00:00 2001 From: Harris Hancock Date: Sun, 12 May 2024 16:25:29 +0100 Subject: [PATCH] Revert "Update Request to support cache option (#2073)" This reverts commit bb8c8cd75e6a85dc88a1905553eb1ce4e1e58576. This is a temporary fix for #2116. We plan to re-add this feature behind a compatibility flag. --- src/workerd/api/http-test.js | 34 ---------------------------- src/workerd/api/http.c++ | 44 +----------------------------------- src/workerd/api/http.h | 35 +++++++++++++--------------- 3 files changed, 17 insertions(+), 96 deletions(-) diff --git a/src/workerd/api/http-test.js b/src/workerd/api/http-test.js index eb231c773d4..58701b97995 100644 --- a/src/workerd/api/http-test.js +++ b/src/workerd/api/http-test.js @@ -126,7 +126,6 @@ export const inspect = { }); assert.strictEqual(util.inspect(request), `Request { - cache: undefined, keepalive: false, integrity: '', cf: undefined, @@ -206,36 +205,3 @@ export const inspect = { await messagePromise; } }; - -export const cacheMode = { - async test() { - { - const req = new Request('https://example.org', { }); - assert.strictEqual(req.cache, undefined); - } - { - const req = new Request('https://example.org', { cache: 'no-store' }); - assert.strictEqual(req.cache, 'no-store'); - } - { - const req = new Request('https://example.org', { cache: 'no-cache' }); - assert.strictEqual(req.cache, 'no-cache'); - } - assert.throws(() => { - new Request('https://example.org', { cache: 'unsupported' }); - }, { - name: 'TypeError', - message: 'Unsupported cache mode: unsupported', - }); - - // Any value other than undefined is currently not supported - // TODO(soon): The no-store and no-cache values will be supported - // soon, at which time this test will need to be updated. - await assert.rejects((async () => { - await fetch('http://example.org', { cache: 'no-store' }); - })(), { - name: 'TypeError', - message: 'Unsupported cache mode: no-store', - }); - } -}; diff --git a/src/workerd/api/http.c++ b/src/workerd/api/http.c++ index f3f86413e5e..6cc3ce79a82 100644 --- a/src/workerd/api/http.c++ +++ b/src/workerd/api/http.c++ @@ -112,21 +112,6 @@ void requireValidHeaderValue(kj::StringPtr value) { } } -Request::CacheMode getCacheModeFromName(kj::StringPtr value) { - if (value == "no-store") return Request::CacheMode::NOSTORE; - if (value == "no-cache") return Request::CacheMode::NOCACHE; - JSG_FAIL_REQUIRE(TypeError, kj::str("Unsupported cache mode: ", value)); -} - -jsg::Optional getCacheModeName(Request::CacheMode mode) { - switch (mode) { - case (Request::CacheMode::NONE): return kj::none; - case (Request::CacheMode::NOCACHE): return "no-cache"_kj; - case (Request::CacheMode::NOSTORE): return "no-store"_kj; - } - KJ_UNREACHABLE; -} - } // namespace Headers::Headers(jsg::Dict dict) @@ -905,13 +890,6 @@ jsg::Ref Request::coerce( : Request::constructor(js, kj::mv(input), kj::mv(init)); } -jsg::Optional Request::getCache(jsg::Lock& js) { - return getCacheModeName(cacheMode); -} -Request::CacheMode Request::getCacheMode() { - return cacheMode; -} - jsg::Ref Request::constructor( jsg::Lock& js, Request::Info input, @@ -924,7 +902,6 @@ jsg::Ref Request::constructor( CfProperty cf; kj::Maybe body; Redirect redirect = Redirect::FOLLOW; - CacheMode cacheMode = CacheMode::NONE; KJ_SWITCH_ONEOF(input) { KJ_CASE_ONEOF(u, kj::String) { @@ -974,7 +951,6 @@ jsg::Ref Request::constructor( body = Body::ExtractedBody((oldJsBody)->detach(js), oldRequest->getBodyBuffer(js)); } } - cacheMode = oldRequest->getCacheMode(); redirect = oldRequest->getRedirectEnum(); fetcher = oldRequest->getFetcher(); signal = oldRequest->getSignal(); @@ -1051,10 +1027,6 @@ jsg::Ref Request::constructor( "response status code)."); } - KJ_IF_SOME(c, initDict.cache) { - cacheMode = getCacheModeFromName(c); - } - if (initDict.method != kj::none || initDict.body != kj::none) { // We modified at least one of the method or the body. In this case, we enforce the // spec rule that GET/HEAD requests cannot have bodies. (On the other hand, if neither @@ -1069,7 +1041,6 @@ jsg::Ref Request::constructor( KJ_CASE_ONEOF(otherRequest, jsg::Ref) { method = otherRequest->method; redirect = otherRequest->redirect; - cacheMode = otherRequest->cacheMode; fetcher = otherRequest->getFetcher(); signal = otherRequest->getSignal(); headers = jsg::alloc(*otherRequest->headers); @@ -1091,7 +1062,7 @@ jsg::Ref Request::constructor( // TODO(conform): If `init` has a keepalive flag, pass it to the Body constructor. return jsg::alloc(method, url, redirect, KJ_ASSERT_NONNULL(kj::mv(headers)), kj::mv(fetcher), kj::mv(signal), - kj::mv(cf), kj::mv(body), cacheMode); + kj::mv(cf), kj::mv(body)); } jsg::Ref Request::clone(jsg::Lock& js) { @@ -1206,10 +1177,6 @@ void Request::serialize( .cf = cf.getRef(js), - .cache = getCacheModeName(cacheMode).map([](kj::StringPtr name) -> kj::String { - return kj::str(name); - }), - // .mode is unimplemented // .credentials is unimplemented // .cache is unimplemented @@ -1780,15 +1747,6 @@ jsg::Promise> fetchImplNoOutputLock( kj::HttpHeaders headers(ioContext.getHeaderTable()); jsRequest->shallowCopyHeadersTo(headers); - // If the jsRequest has a CacheMode, we need to handle that here. - // Currently, the only cache mode we support is undefined, but we will soon support - // no-cache and no-store. These additional modes will be hidden behind an autogate. - if (jsRequest->getCacheMode() != Request::CacheMode::NONE) { - return js.rejectedPromise>( - js.typeError(kj::str("Unsupported cache mode: ", - KJ_ASSERT_NONNULL(jsRequest->getCache(js))))); - } - kj::String url = uriEncodeControlChars( urlList.back().toString(kj::Url::HTTP_PROXY_REQUEST).asBytes()); diff --git a/src/workerd/api/http.h b/src/workerd/api/http.h index e4e01925631..51f36d76b6f 100644 --- a/src/workerd/api/http.h +++ b/src/workerd/api/http.h @@ -689,10 +689,9 @@ struct RequestInitializerDict { jsg::WontImplement credentials; // In browsers this controls the local browser cache. For Cloudflare Workers it could control the - // Cloudflare edge cache. While the standard defines a number of values for this property, our - // implementation supports only three: undefined (identifying the default caching behavior that - // has been implemented by the runtime), "no-store", and "no-cache". - jsg::Optional cache; + // Cloudflare edge cache. Note that this setting is different from using the `Cache-Control` + // header since `Cache-Control` would be forwarded to the origin. + jsg::Unimplemented cache; // These control how the `Referer` and `Origin` headers are initialized by the browser. // Browser-side JavaScript is normally not permitted to set these headers, because servers @@ -765,21 +764,13 @@ class Request: public Body { }; static kj::Maybe tryParseRedirect(kj::StringPtr redirect); - enum class CacheMode { - // CacheMode::NONE is set when cache is undefined. It represents the dafault cache - // mode that workers has supported. - NONE, - NOSTORE, - NOCACHE, - }; - Request(kj::HttpMethod method, kj::StringPtr url, Redirect redirect, jsg::Ref headers, kj::Maybe> fetcher, kj::Maybe> signal, CfProperty&& cf, - kj::Maybe body, CacheMode cacheMode = CacheMode::NONE) + kj::Maybe body) : Body(kj::mv(body), *headers), method(method), url(kj::str(url)), redirect(redirect), headers(kj::mv(headers)), fetcher(kj::mv(fetcher)), - cacheMode(cacheMode), cf(kj::mv(cf)) { + cf(kj::mv(cf)) { KJ_IF_SOME(s, signal) { // If the AbortSignal will never abort, assigning it to thisSignal instead ensures // that the cancel machinery is not used but the request.signal accessor will still @@ -874,8 +865,15 @@ class Request: public Body { // TODO(conform): Won't implement? // The cache mode determines how HTTP cache is used with the request. - jsg::Optional getCache(jsg::Lock& js); - CacheMode getCacheMode(); + // We currently do not fully implement this. Currently we will explicitly + // throw in the Request constructor if the option is set. For the accessor + // we want it to always just return undefined while it is not implemented. + // The spec does not provide a value to indicate "unimplemented" and all + // of the other values would imply semantics we do not follow. In discussion + // with other implementers with the same issues, it was decided that + // simply returning undefined for these was the best option. + // jsg::JsValue getCache(jsg::Lock& js) { return js.v8Undefined(); } + // TODO(conform): Won't implement? // We do not implement integrity checking at all. However, the spec says that // the default value should be an empty string. When the Request object is @@ -905,6 +903,7 @@ class Request: public Body { // JSG_READONLY_PROTOTYPE_PROPERTY(duplex, getDuplex); // JSG_READONLY_PROTOTYPE_PROPERTY(mode, getMode); // JSG_READONLY_PROTOTYPE_PROPERTY(credentials, getCredentials); + // JSG_READONLY_PROTOTYPE_PROPERTY(cache, getCache); JSG_READONLY_PROTOTYPE_PROPERTY(integrity, getIntegrity); JSG_READONLY_PROTOTYPE_PROPERTY(keepalive, getKeepalive); @@ -930,6 +929,7 @@ class Request: public Body { // JSG_READONLY_INSTANCE_PROPERTY(duplex, getDuplex); // JSG_READONLY_INSTANCE_PROPERTY(mode, getMode); // JSG_READONLY_INSTANCE_PROPERTY(credentials, getCredentials); + // JSG_READONLY_INSTANCE_PROPERTY(cache, getCache); JSG_READONLY_INSTANCE_PROPERTY(integrity, getIntegrity); JSG_READONLY_INSTANCE_PROPERTY(keepalive, getKeepalive); @@ -939,7 +939,6 @@ class Request: public Body { readonly cf?: Cf; }); } - JSG_READONLY_PROTOTYPE_PROPERTY(cache, getCache); } void serialize( @@ -968,8 +967,6 @@ class Request: public Body { kj::Maybe> fetcher; kj::Maybe> signal; - CacheMode cacheMode = CacheMode::NONE; - // The fetch spec definition of Request has a distinction between the "signal" (which is // an optional AbortSignal passed in with the options), and "this' signal", which is an // AbortSignal that is always available via the request.signal accessor. When signal is