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

kv:bulk only accepts string values #571

Closed
nr-goby opened this issue Apr 18, 2021 · 10 comments · Fixed by #1024 or #1025
Closed

kv:bulk only accepts string values #571

nr-goby opened this issue Apr 18, 2021 · 10 comments · Fixed by #1024 or #1025
Assignees
Labels
enhancement New feature or request

Comments

@nr-goby
Copy link

nr-goby commented Apr 18, 2021

Currently the kv:bulk tool will only upload values that are strings. This is different from the KV web back end when you can upload json values. The behaviour seems to come from cloudflare::endpoints::workerskv::write_bulk::KeyValuePair. Would changing the type of KeyValuePair.value from String to json::JsonValue be sufficient to fix this? I'm not sure if this library is used for more than wrangler or not so it might be cumbersome to make API changes but it is a cloudflare library.

Or if there is some workaround that I am unaware of, this request could be quickly closed.

@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity in the last 180 days. It will be closed if no further activity occurs in the next week. Please feel free to comment if you'd like it to remain open, and thank you for your contributions.

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically closed because it has not had recent activity. You may re-open the issue if it is still relevant.

@stale stale bot closed this as completed Mar 2, 2022
@threepointone threepointone reopened this Mar 11, 2022
@threepointone threepointone transferred this issue from cloudflare/wrangler-legacy Mar 11, 2022
@threepointone threepointone moved this to Must-have in workers-sdk Mar 11, 2022
@petebacondarwin
Copy link
Contributor

I don't understand this request.

A Workers KV namespace is only able to store string values. The REST API and the dashboard, as far as I can tell, also only allow you to store strings. Of course one can JSON encode data and store it in the KV namespace, but this is the responsibility of the application.

Am I missing something?

@koeninger
Copy link

KV doesn't have to store strings, it can store arbitrary byte arrays or readable streams, https://developers.cloudflare.com/workers/runtime-apis/kv/#writing-key-value-pairs

The REST api will accept "Content-Type: application/json"

@petebacondarwin
Copy link
Contributor

petebacondarwin commented Mar 11, 2022

Ah thanks for that @koeninger - this wasn't clear from the API: https://api.cloudflare.com/#:~:text=A%20UTF%2D8%20encoded%20string%20to%20be%20stored%2C%20up%20to%2010%20MB%20in%20length.

So currently the bulk:kv put command (in Wrangler2) reads a text file and parses it as JSON of the form:

Array<{
  key: string;
  value: string;
  expiration?: number;
  expiration_ttl?: number;
  metadata?: object;
  base64?: boolean;
}>

But as it turns out Wrangler2 does not check that value is a string, and moreover when it does the PUT request it just JSON stringifies the array and does indeed use the { "Content-Type": "application/json" } header.

https://github.com/cloudflare/wrangler2/blob/bf2bee92d6ac37045a7ce04b0cf32fbd0edb9411/packages/wrangler/src/kv.tsx#L180-L187

So... I wonder if this is actually already fixed in Wrangler2?

@petebacondarwin
Copy link
Contributor

Hmm, it appears that the REST API for bulk uploads does not support non-string values.

when I try uploading the following (or any other non-string value)

[
  {
    "key": "test",
    "value": { "str": "a string", "num": 123 }
  }
]
✘ [ERROR] Received a bad response from the API

  could not parse array of key/value objects from request body: 'could not unmarshal KVPair into
  intermediate struct: 'json: cannot unmarshal object into Go struct field kvPairJSON.value of type
  string'' [code: 10012]

@petebacondarwin
Copy link
Contributor

So it looks like Wrangler2 supports this but the Edge Worker Config REST API does not.

@koeninger
Copy link

Ah, sorry, I was responding to the comment about KV being string only and didn't notice this was about the bulk api specifically. Yeah the bulk api isn't set up to take nested json for each value. Not sure what @nr-goby 's original ask was given that.

@caass caass self-assigned this Mar 14, 2022
@petebacondarwin petebacondarwin added this to the 2.0 milestone Mar 15, 2022
@petebacondarwin
Copy link
Contributor

Since the original poster has not actually commented in almost a year, this is definitely not a high priority.
And it also appears that the request is at odds with how KV works both in Wrangler and internally.
So let's push this back to the 2.1 milestone and decide later if it is something we should implement.

@petebacondarwin petebacondarwin modified the milestones: 2.0, 2.1 Mar 15, 2022
@petebacondarwin petebacondarwin added enhancement New feature or request and removed feature labels May 12, 2022
@petebacondarwin petebacondarwin modified the milestones: Selected for development, Backlog May 15, 2022
@petebacondarwin petebacondarwin moved this to Backlog in workers-sdk May 15, 2022
@petebacondarwin petebacondarwin removed this from the Backlog milestone May 15, 2022
@petebacondarwin petebacondarwin self-assigned this May 16, 2022
@petebacondarwin petebacondarwin moved this from Backlog to Selected for development in workers-sdk May 16, 2022
@threepointone threepointone moved this from Selected for development to In Progress in workers-sdk May 16, 2022
@threepointone
Copy link
Contributor

Now that some time has passed and we've been able to think about this, I'm of the opinion that we shouldn't do anything special for uploading non-string values. Further, we can and should apply client side validation, ensuring that only string values are being sent before uploading. I'll send a PR that does so.

threepointone added a commit that referenced this issue May 16, 2022
This adds client side validation for the paylod for `kv:bulk put`, importantly ensuring we're uploading only string key/value pairs (as well as validation for the other fields).

Fixes #571
@threepointone threepointone moved this from In Progress to In Review in workers-sdk May 16, 2022
threepointone added a commit that referenced this issue May 16, 2022
This adds client side validation for the paylod for `kv:bulk put`, importantly ensuring we're uploading only string key/value pairs (as well as validation for the other fields).

Fixes #571
threepointone added a commit that referenced this issue May 16, 2022
This adds client side validation for the paylod for `kv:bulk put`, importantly ensuring we're uploading only string key/value pairs (as well as validation for the other fields).

Fixes #571
threepointone added a commit that referenced this issue May 16, 2022
This adds client side validation for the paylod for `kv:bulk put`, importantly ensuring we're uploading only string key/value pairs (as well as validation for the other fields).

Fixes #571
Repository owner moved this from In Review to Done in workers-sdk May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
5 participants