From 5928effb0faa3bb94726a6535c405ff15ca64e26 Mon Sep 17 00:00:00 2001 From: Kenton Varda Date: Wed, 24 Jul 2024 09:58:04 -0500 Subject: [PATCH] Revert "Redoing "#2073: Update Request to support cache option" with compat flags" --- src/workerd/api/http-test.js | 34 ------------------ src/workerd/api/http-test.wd-test | 2 +- src/workerd/api/http.c++ | 47 +------------------------ src/workerd/api/http.h | 35 +++++++++--------- src/workerd/io/compatibility-date.capnp | 6 ---- 5 files changed, 18 insertions(+), 106 deletions(-) diff --git a/src/workerd/api/http-test.js b/src/workerd/api/http-test.js index 3f8d297063c..e77dd7c19c2 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-test.wd-test b/src/workerd/api/http-test.wd-test index 629ab38e36b..2d147e52bb7 100644 --- a/src/workerd/api/http-test.wd-test +++ b/src/workerd/api/http-test.wd-test @@ -11,7 +11,7 @@ const unitTests :Workerd.Config = ( ( name = "SERVICE", service = "http-test" ) ], compatibilityDate = "2023-08-01", - compatibilityFlags = ["nodejs_compat", "service_binding_extra_handlers", "cache_option_enabled"], + compatibilityFlags = ["nodejs_compat", "service_binding_extra_handlers"], ) ), ], diff --git a/src/workerd/api/http.c++ b/src/workerd/api/http.c++ index 2dd1463bd2d..ec82b28933d 100644 --- a/src/workerd/api/http.c++ +++ b/src/workerd/api/http.c++ @@ -113,21 +113,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) @@ -913,13 +898,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, @@ -932,7 +910,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) { @@ -998,7 +975,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(); @@ -1075,13 +1051,6 @@ jsg::Ref Request::constructor( "response status code)."); } - KJ_IF_SOME(c, initDict.cache) { - JSG_REQUIRE(FeatureFlags::get(js).getCacheOptionEnabled(), TypeError, kj::str( - "Unsupported cache mode: ", c, - "\nYou may need to enable the cache_option_enabled compatability flag.")); - 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 @@ -1096,7 +1065,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); @@ -1118,7 +1086,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) { @@ -1233,10 +1201,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 @@ -1807,15 +1771,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 67564f69d50..4394a223847 100644 --- a/src/workerd/api/http.h +++ b/src/workerd/api/http.h @@ -694,10 +694,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 @@ -770,21 +769,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 @@ -879,8 +870,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 @@ -910,9 +908,9 @@ 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); - JSG_READONLY_PROTOTYPE_PROPERTY(cache, getCache); JSG_TS_OVERRIDE(> { constructor(input: RequestInfo, init?: RequestInit); @@ -936,6 +934,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); @@ -973,8 +972,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 diff --git a/src/workerd/io/compatibility-date.capnp b/src/workerd/io/compatibility-date.capnp index 6c858b3cba2..9573ea607ab 100644 --- a/src/workerd/io/compatibility-date.capnp +++ b/src/workerd/io/compatibility-date.capnp @@ -504,10 +504,4 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef { $compatDisableFlag("legacy_module_registry") $experimental; # Enables of the new module registry implementation. - - cacheOptionEnabled @53 :Bool - $compatEnableFlag("cache_option_enabled") - $compatDisableFlag("cache_option_disabled") - $experimental; - # Enables the use of no-cache and no-store headers from requests }