Mobile: Fixes #10396: Fix Dropbox sync

[personalizedrefrigerator]

Summary

This pull request applies a workaround to fix a Dropbox sync issue on mobile:

• On iOS: When a GET request to /files/download fails:
  1. List files on Dropbox and find the first non-directory file.
  2. Send a GET request for that file.
  3. Re-request the original file.
• On Android and desktop: Uses a POST request to download files from Dropbox instead of a GET request.
  • The Dropbox API documentation uses POST requests rather than GET requests for /files/download. This forum post is linked as an explanation for why POST won't work for this on iOS.
  • GET requests seem to fail if multiple requests are sent for the same file, but POST requests don't.

Fixes #10396.

[!NOTE]

This pull request targets the release-2.14 branch.

Notes for releasing

Before this change can be released for iOS, we may need to add an iOS privacy manifest for a Joplin update to be accepted by Apple.

Based on the forum post and attempts to reproduce this issue in a local NodeJS REPL, the issue only impacts mobile.

Testing plan

I was able to consistently reproduce the original issue by creating a note, then syncing. As such, it should be verified that:

• It's possible to create and sync a note.
• It's possible to download the note on another device.
• It's possible to download changes to the note on the original device.

I plan to test on the following platforms:

• [x] Android 14
• [x] iOS 17
• [x] Desktop (MacOS)

[laurent22]

Thanks a lot for looking into this so quickly, but I'd like to see if Dropbox is going to answer before merging this.

As I understand in most cases on each request we'll do three extra requests (/list, /get randomfile, /get actualfile), is that right? If so, although it's a fix I feel it would make sync so slow on mobile that it will be unusable.

There's also as you mentioned this issue with the privacy manifest. And finally there's this build issue that will need to be resolved too before we can release another 2.14 version:

[personalizedrefrigerator]

As I understand in most cases on each request we'll do three extra requests (/list, /get randomfile, /get actualfile), is that right? If so, although it's a fix I feel it would make sync so slow on mobile that it will be unusable.

This is only done on failed download requests (which shouldn't be every download request). For me, this issue occurred when requesting info.json — it could be that our sync process requests this file multiple times without first requesting some other file.

[laurent22]

Thanks for clarifying. I need to upgrade macOS before I can build using Xcode 15, I'm going to do that and let's try to release this as quickly as possible.

How about the privacy manifest, do you know how we can add this?

Finally could you try running the test units directly again Dropbox please? As described here: https://joplinapp.org/help/dev/spec/sync#testing It should be fine to limit the tests to just those that contain the "sync" string yarn test --runInBand sync. Also make sure to include the --runInBand parameter

[personalizedrefrigerator]

How about the privacy manifest, do you know how we can add this?

A template is added automatically by newer React Native versions, but I think it still needs to be linked to XCode. Apple documentation on privacy manifests can be found here. The "Next Steps" section of this React Native Community post may also be relevant.

[personalizedrefrigerator]

Most tests pass. A few, however, are failing. (Edit) Re-running the test suite, only one test fails.

``` [ lib ]% yarn tsc && yarn test --runInBand sync RUNS services/synchronizer/Synchronizer.basics.test.js PASS services/synchronizer/Synchronizer.basics.test.js (1363.674 s) PASS services/synchronizer/Synchronizer.resources.test.js (841.363 s) PASS services/synchronizer/Synchronizer.e2ee.test.js (850.989 s) PASS services/synchronizer/Synchronizer.conflicts.test.js (806.697 s) PASS services/synchronizer/synchronizer_MigrationHandler.test.js FAIL services/synchronizer/synchronizer_LockHandler.test.js (419.35 s) ● synchronizer_LockHandler › should not use files that are not locks POST files/upload: Error (409): {"error_summary": "path/disallowed_name/...", "error": {".tag": "path", "reason": {".tag": "disallowed_name"}, "upload_session_id": "pid_upload_session:[[ID HERE]]"}} 176 | // JSON. That way the error message will still show there's a problem but without filling up the log or screen. 177 | const shortResponseText = (`${responseText}`).substr(0, 1024); > 178 | const error = new JoplinError(`${method} ${path}: ${message} (${response.status}): ${shortResponseText}`, code); | ^ 179 | error.httpStatus = response.status; 180 | return error; 181 | }; at newError (DropboxApi.js:178:20) at DropboxApi.newError [as exec] (DropboxApi.js:191:12) at FileApiDriverDropbox.put (file-api-driver-dropbox.js:200:4) ● synchronizer_LockHandler › should auto-refresh a lock POST files/list_folder: Error (409): {"error_summary": "path/not_found/...", "error": {".tag": "path", "path": {".tag": "not_found"}}} 176 | // JSON. That way the error message will still show there's a problem but without filling up the log or screen. 177 | const shortResponseText = (`${responseText}`).substr(0, 1024); > 178 | const error = new JoplinError(`${method} ${path}: ${message} (${response.status}): ${shortResponseText}`, code); | ^ 179 | error.httpStatus = response.status; 180 | return error; 181 | }; at newError (DropboxApi.js:178:20) at DropboxApi.newError [as exec] (DropboxApi.js:191:12) at FileApiDriverDropbox.list (file-api-driver-dropbox.js:101:18) PASS services/synchronizer/Synchronizer.revisions.test.js (456.985 s) PASS services/synchronizer/syncInfoUtils.test.js PASS services/synchronizer/ItemUploader.test.js PASS services/synchronizer/Synchronizer.tags.test.js (219.45 s) PASS services/synchronizer/Synchronizer.ppk.test.js (145.763 s) PASS services/synchronizer/Synchronizer.tools.test.js (178.437 s) PASS services/synchronizer/Synchronizer.sharing.test.js Test Suites: 1 failed, 12 passed, 13 total Tests: 2 failed, 94 passed, 96 total Snapshots: 0 total Time: 5372.384 s Ran all test suites matching /sync/i. ``` **Edit**: Re-running synchronizer_LockHandler causes one of the previously-failing tests to pass: ``` [ lib ]% yarn tsc && yarn test --runInBand synchronizer_LockHandler FAIL services/synchronizer/synchronizer_LockHandler.test.js (411.357 s) ● synchronizer_LockHandler › should not use files that are not locks POST files/upload: Error (409): {"error_summary": "path/disallowed_name/...", "error": {".tag": "path", "reason": {".tag": "disallowed_name"}, "upload_session_id": "pid_upload_session:[[...]]"}} 176 | // JSON. That way the error message will still show there's a problem but without filling up the log or screen. 177 | const shortResponseText = (`${responseText}`).substr(0, 1024); > 178 | const error = new JoplinError(`${method} ${path}: ${message} (${response.status}): ${shortResponseText}`, code); | ^ 179 | error.httpStatus = response.status; 180 | return error; 181 | }; at newError (DropboxApi.js:178:20) at DropboxApi.newError [as exec] (DropboxApi.js:191:12) at FileApiDriverDropbox.put (file-api-driver-dropbox.js:200:4) Test Suites: 1 failed, 1 total Tests: 1 failed, 8 passed, 9 total Snapshots: 0 total Time: 411.403 s, estimated 420 s Ran all test suites matching /synchronizer_LockHandler/i. Jest did not exit one second after the test run has completed. 'This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue. ```

The same test fails when running the tests from the release-2.14 branch.

[laurent22]

POST files/upload: Error (409): {"error_summary": "path/disallowed_name/...", "error": {".tag": "path", "reason": {".tag": "disallowed_name"}, "upload_session_id": "pid_upload_session:[[...]]"}

Is it possible that they've implemented some strange disallow list and they are rejecting the name "garbage.json"? Have you tried naming it differently? How about "desktop.ini"? Maybe it's blocked as well since it could be considered a system file

[personalizedrefrigerator]

How about "desktop.ini"?

If I change desktop.ini to desktop.ini.test, the test passes.

[laurent22]

Thanks for checking, I think we can merge then since all failures seem to be unrelated to your change