feat: add new option for bulk gets in KV #3628
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
teresalves
force-pushed
the
talves/bulk-gets-for-kv
branch
from
6e63cdc to
44f5025
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
| const decoder = new TextDecoder(); // UTF-8 by default | ||
| let r = ""; | ||
| while (true) { | ||
| const { done, value } = await reader.read(); |
There was a problem hiding this comment.
nit - any particular reason for doing this in stream, instead of just doing request.arrayBuffer()
There was a problem hiding this comment.
Tried some other ways and it was not reading the stream properly. Both here and in the type "stream" test below.
| } catch ({ name, message }){ | ||
| // assert(message.includes("invalid")) // this message is not processed, should it? | ||
| assert.ok(true); | ||
| } |
There was a problem hiding this comment.
what about adding the following test cases:
- a key is not found
- the array of keys is empty (or some of the key names are empty strings)
- fetching the maximum number of keys that we will allow
There was a problem hiding this comment.
- added
- added but I am returning an empty object and not throwing an error in that case - confirming that this is the intended behaviour and getting back to you
- have not added that one since that will be validated in the endpoint and not here. Should it be in both? I get it so, but wondering if it might be redundant
| } | ||
| // requested type is invalid for bulk get | ||
| try{ | ||
| var response = await env.KV.get(["key-not-json", "key2"], "arrayBuffer"); |
There was a problem hiding this comment.
Does this even get compiled?
There was a problem hiding this comment.
I am sending the type that is invalid here. But the format itself is acceptable, so it compiles but throws an error.
teresalves
force-pushed
the
talves/bulk-gets-for-kv
branch
from
b44d460 to
ff830c5
Compare
teresalves
force-pushed
the
talves/bulk-gets-for-kv
branch
from
ff830c5 to
39f063b
Compare
Creating a new part in the binding for new bulk gets, along with tests for regular gets and the bulk gets
We consider we have a POST endpoint in SGW and we send the keys in a json format as such:
However, we decided that the binding here will only be a passthrough and everything will processed in the endpoint. On top of that, since everything is processed in the binding and comes as a single response, we don't need extra logic to process metadata, we just need to make sure that the binding responds what we what. Because of these two things, we added two more parameters in the request body:
We added tests for:
Regular gets:
Bulk Gets:
Note that we do not test for 400s because we will not have them. If none of the keys exist, we return a 200 but with empty values. Example: