cloudflare
Add baseline enforcement within the runtime
Middleware might restrict the size of the URL that would get generate. Set a sane unrealistic upper bound that's guaranteed to get through to the KV worker. This should make error messages reliably clear when a name provided is too long.
Isn't this a breaking change unless it's literally exactly the same error text that's currently thrown by the KV worker for keys in the 512b - 32k range?
I had a mild concern that the message would be different (hence why I initially chose 8 kib - the probability of you seeing it is minimized unless you're really going crazy). If this is a concern I'd rather go back to 8 kib or even 32 kib limit here rather than trying to massage the error message to be identical as that seems harder to accomplish / harder to test for. What's not clear to me is how it's a breaking change - an error is an error and we have never bothered with error message stability as a back compat guarantee.
Thinking about this some more, the bigger breaking change concern I have here is that this:
NAMESPACE.get(<very long string>)
will now throw whereas before you'd need to await because it was coming from the KV worker:
NAMESPACE.get(<very long string>).catch(...)
I can add a separate js.evalNow to wrap the entrypoint functions because it looks like the KV runtime hadn't been doing that whereas @kentonv had indicated all entrypoint methods returning a promise should be doing that, but I want to get confirmation that we want to go ahead and do that. Not sure the consequence of that as it will subtly change some of the exception generation but historically IIRC we've just gone ahead and done that without a compat flag.
What's not clear to me is how it's a breaking change - an error is an error and we have never bothered with error message stability as a back compat guarantee.
Dunno what to say, we've just been careful about it in KV. For better or worse the message includes an HTTP status code and there is definitely code in the wild that string matches on them. I do agree that it'd be pretty weird for someone to catch long key lengths, when they could easily check that in JS before calling KV methods, but :shrug:
But yeah, throwing synchronously and not after an await feels like a bigger change.
I think I've addressed the comments.
- Extracted out constant to a standalone variable.
- Mimic the error response to be similar to what it would look like if FL rejected it.
- Wrap the code in
js::evalNowto fix the preexisting issue that exceptions were thrown at the call site instead of within the promise. - Internal PR adds a test for the new error message.
similar to what it would look like if FL rejected it.
It's not FL rejecting it that's the common case for 512b < size < 32k, so it's sgw's error message that we care about being the same as, right?
FYI, there is actually a compat flag which causes JSG to convert synchronous exceptions into async exceptions across all APIs that return promises, which became default 2022-10-31. So the introduction of evalNow() in this PR is a noop for anyone who has that flag set.
