Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Stop assuming KV values are UTF8 text #1878

Merged
merged 1 commit into from
Apr 26, 2021
Merged

Stop assuming KV values are UTF8 text #1878

merged 1 commit into from
Apr 26, 2021

Conversation

koeninger
Copy link
Contributor

@koeninger koeninger commented Apr 26, 2021

Implementation of kv:key get is calling text() on the result of the api call, so it's replacing non-utf8 values with ef bf bd, the unicode REPLACEMENT CHAR. KV values can be arbitrary bytes, we shouldn't assume they're UTF8 text, so this patch just writes bytes to stdout. Probably won't work on Windows, but I don't know of a better option, and this at least isn't worse than we are today.

Testing on 1k of random data:

bash-3.2$ hexdump -C /tmp/1k_via_curl | head -n1
00000000  ed ff 79 19 cb 06 6e cf  d6 92 72 d6 89 8b 4f ac  |..y...n...r...O.|

bash-3.2$ hexdump -C /tmp/1k_via_wrangler | head -n1
00000000  ef bf bd ef bf bd 79 19  ef bf bd 06 6e ef bf bd  |......y.....n...|

bash-3.2$ hexdump -C /tmp/1k_via_patched_wrangler | head -n1
00000000  ed ff 79 19 cb 06 6e cf  d6 92 72 d6 89 8b 4f ac  |..y...n...r...O.|

Still works on UTF-8 text:

bash-3.2$ ~/cf-repos/wrangler/target/debug/wrangler kv:key put foo '(╯°□°)╯︵ ┻━┻' --binding KV
✨  Success
bash-3.2$ ~/cf-repos/wrangler/target/debug/wrangler kv:key get foo --binding KV
(╯°□°)╯︵ ┻━┻

@koeninger koeninger requested a review from a team as a code owner April 26, 2021 21:25
@nilslice nilslice merged commit 53f2552 into master Apr 26, 2021
@delete-merged-branch delete-merged-branch bot deleted the koeninger/STOR-1726 branch April 26, 2021 21:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants