Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uploads should not modify the provided args #969

Open
zdrve opened this issue Jun 6, 2022 · 1 comment
Open

Uploads should not modify the provided args #969

zdrve opened this issue Jun 6, 2022 · 1 comment
Labels

Comments

@zdrve
Copy link

zdrve commented Jun 6, 2022

Describe the bug

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

uploadRequest(path, args, auth, host) {
return this.auth.checkAndRefreshAccessToken()
.then(() => {
const { contents } = args;
delete args.contents;
const fetchOptions = {
body: contents,
method: 'POST',
headers: {
'Content-Type': 'application/octet-stream',
'Dropbox-API-Arg': httpHeaderSafeJson(args),
},
};

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 6b3404d. 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.

@zdrve zdrve added the bug label Jun 6, 2022
@greg-db
Copy link
Contributor

greg-db commented Jun 6, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants