Backup: Lots of fixes, refactoring and documentation #887
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For unknown error, there is now response body. So JSON.parse crash. Updated TS types of patch-package.
…arge Possible with last update of cozy-stack https://github.com/cozy/cozy-stack/pull/4069/files
Not totally agnostic. Linked to cozy-stack. Will help next features link "share with".
When killing app during a backup, backup was stuck as running even if it was not running. Now, if we start the app and backup is set as running, we set it as to do because it is not possible. It is only when app start, you we can still move between webview while a backup is running
We try to stay as close as operating system : - folder structure on Android - album on iOS
On iOS, media are set with a remote path to '/' because there is no folder in backup folder. But when restoring, remote path was set to empty string so algorithm was considering these photos as not backuped.
Backup folder is a magic folder. But in some part of the code, we need to use the path of the backup folder from local backup config. So now before uploading files, we check if the backup folder has been renamed and we update local backup config if needed.
Crash--
reviewed
Jul 30, 2023
Crash--
reviewed
Jul 30, 2023
| name: newPhotosMagicFolder.name, | ||
| path: newPhotosMagicFolder.path | ||
| } | ||
| } |
There was a problem hiding this comment.
Pourquoi ne pas retourner tout newPhotosMagicFolder? 🤔
Crash--
reviewed
Jul 30, 2023
|
|
||
| throw e | ||
| } | ||
| } |
There was a problem hiding this comment.
La méthode de cozy-client ne suffisait pas ?
Crash--
approved these changes
Jul 31, 2023
This reverts commit 2a1d096.
Crash--
force-pushed
the
fix/backup-crash-rnbu
branch
3 times, most recently
from
0b56b06
to
33bc61b
Compare
Like that, cozy-stack is able to cut the connexion before the end if there is no space left on the device.
This is how I use it : `./gradlew allDeps > allDeps.txt`
We have an http2 issue when uploading a file to a cozy when we reach the quota limit. In that case, the stack closes the connexion but it can appears after having already send a few bytes. In that case, okhttp was not returning the right headers and we ended up with a: `stream closed: cancel`. With this version, okhttp is returning the right header! see the okhttp changelog: https://github.com/square/okhttp/blob/master/CHANGELOG.md#version-500-alpha1 specially: `Fix: Attempt to read the response body even if the server canceled the request. This will cause some calls to return nice error codes like HTTP/1.1 429 Too Many Requests instead of transport errors like SocketException: Connection reset and StreamResetException: stream was reset: CANCEL. `
Crash--
force-pushed
the
fix/backup-crash-rnbu
branch
from
33bc61b
to
d0cfea6
Compare
Ldoppea
approved these changes
Aug 3, 2023
| export const isQuotaExceededError = (error: UploadError): boolean => { | ||
| return ( | ||
| error.statusCode === 413 && | ||
| error.errors[0].detail === 'The file is too big and exceeds the disk quota' |
There was a problem hiding this comment.
error.errors content should be checked first here to prevent undefined access error if error[0] does not exist
| export const setBackupAsToDo = async (client: CozyClient): Promise<void> => { | ||
| const localBackupConfig = await getLocalBackupConfig(client) | ||
|
|
||
| localBackupConfig.currentBackup.status = 'to_do' |
There was a problem hiding this comment.
nit: Maybe to_do should be extracted as a global var?
| } | ||
|
|
||
| void checkAndFixIfBackupStuckInRunning() | ||
| }, [client]) |
There was a problem hiding this comment.
How do we ensure this is called only on startup if there is a dependency on client? (however if the client changes I suppose it is expected to clear the backup status)
| } catch (e) { | ||
| const error = e as UploadError | ||
|
|
||
| if (error.statusCode === 409) { |
There was a problem hiding this comment.
nit: Would be great to have an explicitly named method like for other methods (isQuotaExceededError, isFatalError etc) so we easily understand that this catch is about name conflicts
Previous implementation would throw if the received error do not contain any `errors` array. this can happen if the error is not thrown by cozy-client Also this commit improve type check to ensure the receive error is the one we expect. Casting objects using `as` should be avoided as this will never ensure type compatibility
Ldoppea
force-pushed
the
fix/backup-crash-rnbu
branch
from
0dd6468
to
f7cf0f8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.