dropbox / dropbox-sdk-js

The Official Dropbox API V2 SDK for Javascript
https://www.dropbox.com/developers
MIT License
941 stars 353 forks source link

Uploads should not modify the provided args #969

Open zdrve opened 2 years ago

zdrve commented 2 years ago

Describe the bug

Calling dbx.filesUpload(args) modifies args, in that it deletes args.contents. From the sdk src/dropbox.js:

https://github.com/dropbox/dropbox-sdk-js/blob/6b3404d1af47ab63acd5e7200135c67c92fcc3cf/src/dropbox.js#L133-L146

This makes it harder to handle errors:

   const args = { ... various things ..., contents: someBuffer };
   dbx.filesUpload(args)
    .catch(error => {
      // If it's a 429, wait a bit, then try again
      // ...
      return dbx.filesUpload(args); // BUG: Will result in an empty upload!
    })

To Reproduce

See the second pseudo-code example above:

Expected Behavior

Calling the Dropbox API should not mutate the provided arguments.

Actual Behavior

In at least this case, the Dropbox API mutates the provided argument.

Screenshots

If applicable, add screenshots to help explain your problem.

Versions

I can see the bug still present in the SDK as at 6b3404d1af47ab63acd5e7200135c67c92fcc3cf. See the first code sample, pasted above.

Additional context

I would guess that the fix looks a bit like:

diff --git a/src/dropbox.js b/src/dropbox.js
index fa33f58..6f9c93c 100644
--- a/src/dropbox.js
+++ b/src/dropbox.js
@@ -133,6 +133,7 @@ export default class Dropbox {
   uploadRequest(path, args, auth, host) {
     return this.auth.checkAndRefreshAccessToken()
       .then(() => {
+        args = { ...args };
         const { contents } = args;
         delete args.contents;

but this is not my comfort language so I'm not sure.

greg-db commented 2 years ago

Thanks for writing this up! I'm sharing this with the team and I'll follow up here with any updates.