URL Encode Key values

[ironsalsa]

Currently, the path is used for submitting data that may contain special characters. For example, if you try to set up bulk redirects in Workers with KV per https://developers.cloudflare.com/workers/recipes/bulk-redirects/, the key '/pathtoreplace' is stored in KV as 'pathtoreplace'. Currently, the caller has to URL encode the submission to the api.WriteWorkersKV() call to ensure the slashes get through.

I can't think of a situation where we wouldn't want to URL encode a key as soon as the user submits it - adding an encode would do the right thing instead of passively allowing data loss.

[ironsalsa]

Let me know if I ought to go through the full process - I think the change is pretty self-evident and fixes a nasty bug for KV operations.

[ironsalsa]

As an aside, this bug caused me to add 40 KV pairs to a namespace missing the slashes - definitely not fun to troubleshoot.

[jacobbednarz]

@ironsalsa Thank you for the Pull Request! A couple of things that we should address before shipping this one:

• Are you able to update the description to remove any of the Pull Request template that you don't need and fill out the rest? You can delete the standard stuff if you have a way of covering all of the points in it in your own words.
• Can we please add tests for this change? I think this is especially important seeing how you've identified it as a regression and caused undesirable behavior.

Once those are done, we can

[ironsalsa]

@jacobbednarz I've removed the extra cruft, but I'm not sure how to add a test on this one - is there a way to store a KV pair and retrieve it from a test to prove that a key with a "/" stores in the KV and can be retrieved from there?

I'm rather new to writing Golang tests, so any suggestions would be appreciated.

[jacobbednarz]

There are some existing tests in workers_kv_test.go that will get you started. In essence what is happening in most of those tests is that we are creating a local mock webserver on an expected URL (using mux.HandleFunc) outputting the expected payload. In your case, I would copy one of the existing tests for creating keys (TestWorkersKV_WriteWorkersKV looks good) and then update it to the URL and payload you would expect. It's always a good idea to validate your tests fails without your fix and then works once you apply it too.

[ironsalsa]

I'm running into problems - I can't get the original test with the original code to pass if you put a slash in the key, and I'm not 100% sure how to fix it since both the testing API and the code rely on that same key. I think this means the current test is catching the same bug I ran into, but no one tried with a slash before.

@jacobbednarz you can replicate the error in the master branch by changing the line in workers_kv_test.go in func TestWorkersKV_WriteWorkersKV() from:

key := "test_key"

to:

key := "/test_key"

Run the test and it will fail with a 404, since the client is calling a different URL and the API is publishing due to the URL encoding issue.

Here's the error:

--- FAIL: TestWorkersKV_WriteWorkersKV (0.00s)
    /home/marcy/cloudflare-go/workers_kv_test.go:191: 
        	Error Trace:	workers_kv_test.go:191
        	Error:      	Received unexpected error:
        	            	HTTP status 404: content "404 page not found\n"
        	            	github.com/cloudflare/cloudflare-go.(*API).makeRequestWithAuthTypeAndHeaders
        	            		/home/marcy/cloudflare-go/cloudflare.go:260
        	            	github.com/cloudflare/cloudflare-go.(*API).WriteWorkersKV
        	            		/home/marcy/cloudflare-go/workers_kv.go:130
        	            	github.com/cloudflare/cloudflare-go.TestWorkersKV_WriteWorkersKV
        	            		/home/marcy/cloudflare-go/workers_kv_test.go:189
        	            	testing.tRunner
        	            		/usr/bin/go/src/testing/testing.go:827
        	            	runtime.goexit
        	            		/usr/bin/go/src/runtime/asm_amd64.s:1333
        	            	error from makeRequest
        	            	github.com/cloudflare/cloudflare-go.(*API).WriteWorkersKV
        	            		/home/marcy/cloudflare-go/workers_kv.go:134
        	            	github.com/cloudflare/cloudflare-go.TestWorkersKV_WriteWorkersKV
        	            		/home/marcy/cloudflare-go/workers_kv_test.go:189
        	            	testing.tRunner
        	            		/usr/bin/go/src/testing/testing.go:827
        	            	runtime.goexit
        	            		/usr/bin/go/src/runtime/asm_amd64.s:1333
        	Test:       	TestWorkersKV_WriteWorkersKV
FAIL
FAIL	github.com/cloudflare/cloudflare-go	0.004s
Error: Tests failed.

I'm going to be out of touch for over a week, so unless someone else jumps on this it may just sit around.

[jacobbednarz]

@ironsalsa The issue there is that the value is being passed into fmt.Sprintf and there is already a slash at the start of the value so the server mux isn't getting triggered resulting in a 404. Instead the URL you're attempting to hit is /accounts/foo/storage/kv/namespaces/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/values//key. You need to update the simulate the URL that you're going to hit to be the encoded version you expect which here should be /accounts/foo/storage/kv/namespaces/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/values/%2Fkey

[Electroid]

@jacobbednarz @ironsalsa Turns out, even when you add the %2F to the mux, it still doesn't work -- there's an interesting backstory why.

Given the hastle, I would just implement this without the test. It's a simple enough change.

[jacobbednarz]

Seems reasonable given the extra context here 👍 I'm probably a little more cautious than most due to my day to day relying so heavily on git history and test cases but it seems like a suitable case where we don't need too heavy on test coverage. I'd opt for an alternative if/when this is involved in a regression though.

[patryk]

Thanks for contribution, @Electroid !