Skip to content

Commit

Permalink
Revert "Update Request to support cache option (#2073)"
Browse files Browse the repository at this point in the history 
This reverts commit bb8c8cd.

This is a temporary fix for #2116. We plan to re-add this feature behind a compatibility flag.
  • Loading branch information
@harrishancock
harrishancock committed May 12, 2024
1 parent 0219e89 commit c1fd8ca
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 96 deletions.
34 changes: 0 additions & 34 deletions src/workerd/api/http-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ export const inspect = {
});
assert.strictEqual(util.inspect(request),
`Request {
cache: undefined,
keepalive: false,
integrity: '',
cf: undefined,
Expand Down Expand Up @@ -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',
});
}
};
44 changes: 1 addition & 43 deletions src/workerd/api/http.c++
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<kj::StringPtr> 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<jsg::ByteString, jsg::ByteString> dict)
Expand Down Expand Up @@ -905,13 +890,6 @@ jsg::Ref<Request> Request::coerce(
: Request::constructor(js, kj::mv(input), kj::mv(init));
}

jsg::Optional<kj::StringPtr> Request::getCache(jsg::Lock& js) {
return getCacheModeName(cacheMode);
}
Request::CacheMode Request::getCacheMode() {
return cacheMode;
}

jsg::Ref<Request> Request::constructor(
jsg::Lock& js,
Request::Info input,
Expand All @@ -924,7 +902,6 @@ jsg::Ref<Request> Request::constructor(
CfProperty cf;
kj::Maybe<Body::ExtractedBody> body;
Redirect redirect = Redirect::FOLLOW;
CacheMode cacheMode = CacheMode::NONE;

KJ_SWITCH_ONEOF(input) {
KJ_CASE_ONEOF(u, kj::String) {
Expand Down Expand Up @@ -974,7 +951,6 @@ jsg::Ref<Request> Request::constructor(
body = Body::ExtractedBody((oldJsBody)->detach(js), oldRequest->getBodyBuffer(js));
}
}
cacheMode = oldRequest->getCacheMode();
redirect = oldRequest->getRedirectEnum();
fetcher = oldRequest->getFetcher();
signal = oldRequest->getSignal();
Expand Down Expand Up @@ -1051,10 +1027,6 @@ jsg::Ref<Request> 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
Expand All @@ -1069,7 +1041,6 @@ jsg::Ref<Request> Request::constructor(
KJ_CASE_ONEOF(otherRequest, jsg::Ref<Request>) {
method = otherRequest->method;
redirect = otherRequest->redirect;
cacheMode = otherRequest->cacheMode;
fetcher = otherRequest->getFetcher();
signal = otherRequest->getSignal();
headers = jsg::alloc<Headers>(*otherRequest->headers);
Expand All @@ -1091,7 +1062,7 @@ jsg::Ref<Request> Request::constructor(
// TODO(conform): If `init` has a keepalive flag, pass it to the Body constructor.
return jsg::alloc<Request>(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> Request::clone(jsg::Lock& js) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1780,15 +1747,6 @@ jsg::Promise<jsg::Ref<Response>> 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<jsg::Ref<Response>>(
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());

Expand Down
35 changes: 16 additions & 19 deletions src/workerd/api/http.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<kj::String> 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
Expand Down Expand Up @@ -765,21 +764,13 @@ class Request: public Body {
};
static kj::Maybe<Redirect> 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> headers, kj::Maybe<jsg::Ref<Fetcher>> fetcher,
kj::Maybe<jsg::Ref<AbortSignal>> signal, CfProperty&& cf,
kj::Maybe<Body::ExtractedBody> body, CacheMode cacheMode = CacheMode::NONE)
kj::Maybe<Body::ExtractedBody> 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
Expand Down Expand Up @@ -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<kj::StringPtr> 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
Expand Down Expand Up @@ -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);

Expand All @@ -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);

Expand All @@ -939,7 +939,6 @@ class Request: public Body {
readonly cf?: Cf;
});
}
JSG_READONLY_PROTOTYPE_PROPERTY(cache, getCache);
}

void serialize(
Expand Down Expand Up @@ -968,8 +967,6 @@ class Request: public Body {
kj::Maybe<jsg::Ref<Fetcher>> fetcher;
kj::Maybe<jsg::Ref<AbortSignal>> 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
Expand Down

0 comments on commit c1fd8ca

Please sign in to comment.