Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add new option for bulk gets for kv #3744

Merged
merged 1 commit into from
Mar 20, 2025

Conversation

teresalves
Copy link
Contributor

@teresalves teresalves commented Mar 18, 2025

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:

{
   "keys": ["key1", "key2"]
}

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:

{
   "keys": ["key1", "key2"],
   "type": "text",
   "withMetadata": true
}

We added tests for:
Regular gets:

  • 400s
  • 500s
  • 200s with text, json, stream and arrayBuffer

Bulk Gets:

  • 500s
  • 200s with text and json, as these will be the only supported types for now.

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:

{
   "key1": null,
   "key2": null,
 }

Original PR: #3628

@teresalves teresalves requested review from a team as code owners March 18, 2025 14:27
@teresalves teresalves requested review from mikea and tewaro March 18, 2025 14:27
Copy link

github-actions bot commented Mar 18, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teresalves
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@teresalves teresalves force-pushed the teresalves/get-bulk-for-kv branch from d153ecb to e9a9d10 Compare March 18, 2025 14:29
@teresalves teresalves force-pushed the teresalves/get-bulk-for-kv branch from e9a9d10 to 00bb983 Compare March 18, 2025 14:30
@teresalves
Copy link
Contributor Author

recheck

@teresalves teresalves merged commit 8234e92 into main Mar 20, 2025
26 of 33 checks passed
@teresalves teresalves deleted the teresalves/get-bulk-for-kv branch March 20, 2025 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants