Add support for multipart downloads for module-format Worker scripts

[sodabrew]

Description

Since #1010 allows uploading a Worker script in module format, calling DownloadWorker on the same script will return a multipart/form-data response.

Asks

1. Update: still need this; for the moment I'm assuming that any multipart worker is in module format. Cloudflare API for download a worker should add a Content-Type header to each multipart/form-data section to specify whether the script in that section is application/javascript or application/javascript+module.
2. Update: took this approach, please review! Guidance on a deep change: makeRequest... et al should return a content-type header not just the body.
3. Update: didn't like this approach: Guidance on a workaround: makeRequest... et al continue to just return the response body, so consumers assume the format, or detect the format with a heuristic. For example, DownloadWorker can check the first two bytes and if they are -- then assume the rest of that line is the multipart boundary string and parse the body accordingly. This is so gross.

Details

This approach digs into the makeRequest... family (makeRequestWithAuthTypeAndHeaders is the full implementation) and adds support to return the first body of a multipart response. makeRequest, et al, does not return the mime-type of the response. Consuming functions mostly assume it is application/json, except DownloadWorker which implicitly assumes it's application/javascript.

While I've solved the initial problem with #1010 that you can upload a module-format worker but if you try to download it, the DownloadWorker method return raw multipart/form-data, I'm stuck on the next problem: the Module true/false status is implied by whether the download script API returns a javascript content-type or a multipart content-type.

There's no further clue because the Cloudflare API for downloading a worker only gives a Content-Disposition header on its response for a module worker:

Content-Type: multipart/form-data; boundary=mimeboundary

--mimeboundary
Content-Disposition: form-data; name="worker.js"

export default { etc etc etc }
--mimeboundary

vs. non-module format:

Content-Type: application/json

addEventListener( etc etc )

This isn't consistent with uploading a module worker, where the Content-Type of the mime-part must be set to application/javascript+module like this:

--mimeboundary
Content-Disposition: form-data; name="worker.js"
Content-Type: application/javascript+module

export default { etc etc etc }
--mimeboundary

Sidenote: why was the method UploadWorkerWithBindings added? The bindings are in a param structure, isn't the point to add new params without creating a new method? And in fact if you call UploadWorker with Module-format worker, it shunts to UploadWorkerWithBindings! The salient difference is whether the upload is in multipart format or a direct script. The code path I can see that isn't in multipart format is a non-module worker without bindings (the original product, so probably also the most common case). Per manual testing, the Cloudflare API will accept a multipart/form-data with a plain script as its body -- that is, UploadWorker is sufficient for all cases and UploadWorkerWithBindings is duplicative.

Has your change been tested?

Works but creates an awkward roundtrip problem in the current iteration:

1. UploadWorker with Module: true because it's a module script.

        scriptFile, err := os.ReadFile("cool-script.js")
        scriptText := string(scriptFile)

        UploadWorker(ctx,
                &cloudflare.WorkerRequestParams{
                        ScriptName: "modtest",
                },
                &cloudflare.WorkerScriptParams{
                        Script: scriptText,
                        Module: true,
                },
        )

4. DownloadWorker with the script name and returns the script body. But what format is it?

        scriptText = DownloadWorker(ctx,
                &cloudflare.WorkerRequestParams{
                        ScriptName: "modtest",
                },
        )

5. UploadWorker ... with Module true or false? There's no indication from DownloadWorker which to use.

        strings.Replace(scriptText, "foo", "bar", -1)

        UploadWorker(ctx,
                &cloudflare.WorkerRequestParams{
                        ScriptName: "modtest",
                },
                &cloudflare.WorkerScriptParams{
                        Script: scriptText,
                        Module: true,   <---- ?????????????????                },
        )

Screenshots (if appropriate):

Types of changes

What sort of change does your code introduce/modify?

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.
• This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs.

[github-actions]

changelog detected ✅

[codecov-commenter]

Codecov Report

Merging #1040 (e1a08e9) into master (e39278e) will increase coverage by 0.09%.
The diff coverage is 72.85%.

@@            Coverage Diff             @@
##           master    #1040      +/-   ##
==========================================
+ Coverage   49.94%   50.04%   +0.09%     
==========================================
  Files         115      116       +1     
  Lines       10991    11047      +56     
==========================================
+ Hits         5490     5528      +38     
- Misses       4338     4350      +12     
- Partials     1163     1169       +6     

Impacted Files	Coverage Δ
teams_accounts.go	53.84% <ø> (ø)
api_shield.go	53.84% <53.84%> (ø)
workers.go	69.38% <72.72%> (-0.04%)	⬇️
cloudflare.go	68.42% <93.75%> (+1.21%)	⬆️
email_routing_settings.go	60.00% <100.00%> (ø)
images.go	50.34% <100.00%> (-0.35%)	⬇️
r2_bucket.go	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[sodabrew]

@jacobbednarz thanks for quickly merging the two test fixits that I pulled out of this PR into #1048 and #1049. I wrote a massive update to the OP with examples and problems of implied content-type. Looking forward to your comments!

[jacobbednarz]

this PR has become quite significantly larger than the intended scope and i'm afraid, i won't be merging it as is with such a large refactor of the core methods. leading into your next question, the experimental client (see https://github.com/cloudflare/cloudflare-go/blob/master/docs/experimental.md) will focus on these larger, sweeping improvements for the next major release. we'll be intentionally shaking away alot of the older library functionality that we have now which would be a better place for type of changes you're proposing here. i wouldn't recommend opening PRs for it at this stage, but, just be aware that it exists.

for now, if you want this functionality, we need to do it within the confines of the library API as it stands todya.

[sodabrew]

Ok, thanks for that feedback. I've adjusted the scope of changes to be a lot narrower, please take another look and let me know.

[jacobbednarz]

i've gone ahead and updated APIResponse to be more http.Response like as that will be the direction for the experimental client when we start porting functionality to it. this looks fine in the tests but do you want to confirm locally that this meets what you're after here before we merge?

[jacobbednarz]

i don't love the name here however, i suspect this will be cleaned up in the experimental client so i'm not too phase but will want to keep it's usage extremely limited until then.

[sodabrew]

Looks great! I've rebased and squashed down to three commits: the core change I arrived at, your edits thereupon, and worker test script whitespace changes. You might need to repush your last commit with your own signing key (I see an "Unverified" note) -- or I could squash your final commit into mine with a Co-authored-by?

[jacobbednarz]

done! thanks for the PR and getting it over the line. i've made some notes internally around the issues here with headers and the request methods not exposing everything which hopefully we can address nicely in the experimental client later on.

[github-actions]

This functionality has been released in v0.49.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!