From 5aa36394fa12687be7677de1a0b30a2a8e8fd2fe Mon Sep 17 00:00:00 2001 From: AdityaAtulTewari Date: Wed, 31 Jul 2024 12:56:28 -0500 Subject: [PATCH] Address nits --- src/workerd/api/http-test.js | 14 ++++++-------- src/workerd/api/http.c++ | 17 +++++------------ src/workerd/api/http.h | 6 ++++-- src/workerd/jsg/README.md | 3 ++- src/workerd/jsg/jsg.h | 11 +++++++++++ src/workerd/jsg/struct.h | 8 ++++++-- 6 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/workerd/api/http-test.js b/src/workerd/api/http-test.js index 480a47fe1dd..4186d345383 100644 --- a/src/workerd/api/http-test.js +++ b/src/workerd/api/http-test.js @@ -270,14 +270,12 @@ export const cacheMode = { assertFetchCacheRejectsError('no-transform'); assertFetchCacheRejectsError('unsupported'); } else { - { - 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'); - } + assertRequestCacheThrowsError('no-store', + 'TypeError', + "Unsupported cache mode: no-store"); + assertRequestCacheThrowsError('no-cache', + 'TypeError', + "Unsupported cache mode: no-cache"); assertRequestCacheThrowsError('no-transform', 'TypeError', "Unsupported cache mode: no-transform"); diff --git a/src/workerd/api/http.c++ b/src/workerd/api/http.c++ index 63667b75918..0e3eaae4c76 100644 --- a/src/workerd/api/http.c++ +++ b/src/workerd/api/http.c++ @@ -1198,14 +1198,13 @@ kj::Maybe Request::serializeCfBlobJson(jsg::Lock& js) { return cf.serialize(js); } -void RequestInitializerDict::validate() { +void RequestInitializerDict::validate(jsg::Lock& js) { KJ_IF_SOME(c, cache){ - // Check compatability - JSG_REQUIRE(Worker::Api::current().getFeatureFlags().getCacheOptionEnabled(), Error, kj::str( + // Check compatability flag + JSG_REQUIRE(FeatureFlags::get(js).getCacheOptionEnabled(), Error, kj::str( "The 'cache' field on 'RequestInitializerDict' is not implemented.")); - // Check that the string is a supported type - auto cacheStruct = getCacheModeFromName(c); + JSG_FAIL_REQUIRE(TypeError, kj::str("Unsupported cache mode: ", c)); } } @@ -1818,13 +1817,7 @@ jsg::Promise> fetchImplNoOutputLock( // 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) { - JSG_REQUIRE(FeatureFlags::get(js).getCacheOptionEnabled(), Error, kj::str( -+ "The 'cache' field on 'RequestInitializerDict' is not implemented.")); - return js.rejectedPromise>( - js.typeError(kj::str("Unsupported cache mode: ", - KJ_ASSERT_NONNULL(jsRequest->getCache(js))))); - } + KJ_ASSERT(jsRequest->getCacheMode() == Request::CacheMode::NONE); 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 76b2757b381..4f0f26178fa 100644 --- a/src/workerd/api/http.h +++ b/src/workerd/api/http.h @@ -752,8 +752,6 @@ struct RequestInitializerDict { // jsg::Optional priority; // TODO(conform): Might support later? - void validate(); - JSG_STRUCT(method, headers, body, redirect, fetcher, cf, mode, credentials, cache, referrer, referrerPolicy, integrity, signal); JSG_STRUCT_TS_OVERRIDE(RequestInit { @@ -761,6 +759,10 @@ struct RequestInitializerDict { body?: BodyInit | null; cf?: Cf; }); + + // This method is called within tryUnwrap() when the type is unpacked from v8. + // See jsg Readme for more details. + void validate(jsg::Lock&); }; class Request: public Body { diff --git a/src/workerd/jsg/README.md b/src/workerd/jsg/README.md index f1d3316d1fe..c9aa5187afa 100644 --- a/src/workerd/jsg/README.md +++ b/src/workerd/jsg/README.md @@ -642,12 +642,13 @@ included in the JavaScript object.) If the struct has a validate() method, it is called when the struct is unwrapped from v8. This is an opportunity for it to throw a TypeError based on some custom logic. +The signature for this method is `void validate(jsg::Lock&);` ```cpp struct ValdiatingFoo { kj::String abc; - void validate() { + void validate(jsg::Lock& lock) { JSG_REQUIRE(abc.size() != 0, TypeError, "Field 'abc' had no length in 'ValidatingFoo'."); } diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index 733abe9052a..b9bf69521e8 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -639,6 +639,17 @@ namespace { // // Note that if a `validate` function is provided, then it will be called after the struct is // unwrapped from v8. This would be an appropriate time to throw an error. +// Signature: void validate(jsg::Lock& js); +// Example: +// struct ValdiatingFoo { +// kj::String abc; +// void validate(jsg::Lock& js) { +// JSG_REQUIRE(abc.size() != 0, TypeError, "Field 'abc' had no length in 'ValidatingFoo'."); +// } +// JSG_STRUCT(abc); +// }; +// +// In this example the validate method would throw a `TypeError` if the size of the `abc` field was zero. // // Fields with a starting '$' will have that dollar sign prefix stripped in the JS binding. A // motivating example to enact that change was WebCrypto which has a field in a dictionary called diff --git a/src/workerd/jsg/struct.h b/src/workerd/jsg/struct.h index 77a76b3177a..6030e1cd48f 100644 --- a/src/workerd/jsg/struct.h +++ b/src/workerd/jsg/struct.h @@ -122,8 +122,12 @@ class StructWrapper, kj::_::Indexes(fields).unwrap(static_cast(*this), isolate, context, in)... }; - if constexpr (requires { t.validate(); }) { - t.validate(); + // Note that if a `validate` function is provided, then it will be called after the struct is + // unwrapped from v8. This would be an appropriate time to throw an error. + // Signature: void validate(jsg::Lock& js); + if constexpr (requires (jsg::Lock& js) { t.validate(js); }) { + jsg::Lock& js = jsg::Lock::from(isolate); + t.validate(js); } return t;