dropbox-sdk-js icon indicating copy to clipboard operation
dropbox-sdk-js copied to clipboard

Uploads should not modify the provided args

Open ghost opened this issue 3 years ago • 1 comments

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:

  • construct args for a filesUpload request
  • make request
  • observe that args.contents is gone, thus setting a trap for retry logic

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

  • What version of the SDK are you using? 10.23.0
  • What version of the language are you using? node v18.0.0, typescript 4.5.4
  • Are you using Javascript or Typescript? - typescript
  • What platform are you using? (if applicable) - macos

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.

ghost avatar Jun 06 '22 12:06 ghost

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

greg-db avatar Jun 06 '22 15:06 greg-db