EW-9327 Implement connection string override support#6742
EW-9327 Implement connection string override support#6742
Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds host and port properties to Fetcher to support connection string override via a connect-override mechanism similar to Hyperdrive.
Issues found (ranked by severity):
- [HIGH] New Fetcher properties shadow JS RPC wildcard and are not compat-flag-gated —
hostandportare added unconditionally to allFetcherinstances, includingDurableObjectstubs. The comment at line 453 explicitly warns about this. - [HIGH]
self->portisjsg::Optional<uint16_t>but passed un-unwrapped tokj::str()— The connect override lambda will stringify akj::Mayberather than auint16_t. - [MEDIUM]
getPort()lacksKJ_LIFETIMEBOUND— minor, but noted per review checklist for methods returning views.
This review was generated by an AI assistant and may contain errors. Please verify all suggestions before applying.
| JSG_LAZY_READONLY_INSTANCE_PROPERTY(host, getHost); | ||
| JSG_LAZY_READONLY_INSTANCE_PROPERTY(port, getPort); |
There was a problem hiding this comment.
[HIGH] Missing compat flag gate — shadows JS RPC wildcard property.
The comment right above (lines 453–459) warns:
WARNING: New JSG_METHODs on Fetcher must be gated via compatibility flag to prevent conflicts with JS RPC methods (implemented via the wildcard property).
This applies equally to properties. JSG_LAZY_READONLY_INSTANCE_PROPERTY registers concrete property names (host, port) that will shadow JSG_WILDCARD_PROPERTY(getRpcMethod). Since DurableObject extends Fetcher, a DO stub whose RPC target exposes a host or port property/method will silently get undefined (the Fetcher property with no port set) instead of making the RPC call.
These should be gated behind a compat flag, or alternatively moved to a subclass that only the connection-string-override Fetcher uses (not all Fetchers globally).
| // fetcher. | ||
| auto& globalScope = IoContext::current().getCurrentLock().getGlobalScope(); | ||
| globalScope.setConnectOverride(kj::str(KJ_ASSERT_NONNULL(randomHost), ":", getPort()), | ||
| [self = JSG_THIS](jsg::Lock& js) mutable { return self->connect(js, kj::str(KJ_ASSERT_NONNULL(self->randomHost), ":", self->port), kj::none); }); |
There was a problem hiding this comment.
[HIGH] self->port is jsg::Optional<uint16_t> — stringifying a kj::Maybe instead of the unwrapped value.
self->port has type jsg::Optional<uint16_t> (which is kj::Maybe<uint16_t>). Passing it to kj::str() will not produce the numeric string you want — it will format the Maybe wrapper. Compare with the Hyperdrive implementation where getPort() returns a plain uint16_t.
| [self = JSG_THIS](jsg::Lock& js) mutable { return self->connect(js, kj::str(KJ_ASSERT_NONNULL(self->randomHost), ":", self->port), kj::none); }); | |
| [self = JSG_THIS](jsg::Lock& js) mutable { return self->connect(js, kj::str(KJ_ASSERT_NONNULL(self->randomHost), ":", KJ_ASSERT_NONNULL(self->port)), kj::none); }); |
| // cloudflare:sockets's connect() will route connections to this magic hostname through the | ||
| // fetcher. | ||
| auto& globalScope = IoContext::current().getCurrentLock().getGlobalScope(); | ||
| globalScope.setConnectOverride(kj::str(KJ_ASSERT_NONNULL(randomHost), ":", getPort()), |
There was a problem hiding this comment.
[LOW] Consider using getPort() rather than accessing port field directly for consistency.
The Hyperdrive implementation calls getPort() for both the setConnectOverride key and the lambda body. Here you mix getPort() (line 2091) and direct self->port access (line 2092). Using self->getPort() in the lambda would also need unwrapping but would be more consistent.
Also: this line calls getPort() which returns jsg::Optional<uint16_t> — kj::str() with a Maybe will produce unexpected output here too. Should unwrap:
| globalScope.setConnectOverride(kj::str(KJ_ASSERT_NONNULL(randomHost), ":", getPort()), | |
| globalScope.setConnectOverride(kj::str(KJ_ASSERT_NONNULL(randomHost), ":", KJ_ASSERT_NONNULL(port)), |
|
I'm Bonk, and I've done a quick review of your PR. This PR adds
|
draft/proof-of-concept, not yet ready for review – some more internal discussion is needed. See the downstream PR for more context.