From c3305093c0a7c1ec950c11345ed154ed314cc098 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Mon, 8 Aug 2022 14:55:07 +0200 Subject: [PATCH 01/24] Update version number in build.gradle --- app/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/build.gradle b/app/build.gradle index bbcd73a2..b7c9e8a5 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -4,7 +4,7 @@ plugins { def versionMajor = 1 -def versionMinor = 1 +def versionMinor = 2 def versionPatch = 0 -- GitLab From 3c6e8af8f54c77ba6af1e923c468c595e858b1e7 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Mon, 8 Aug 2022 15:21:26 +0200 Subject: [PATCH 02/24] Implement usage of chunked file upload Update UploadFileOperation.java: -> Try to upload file as chunked only if size is >= 3 MB -> Put http request execution in method easily mockable for tests -> Remove usage of 'mustRestart' and implement of restart mechanism as it was never called: mustRestart was always set to false. -> Remove 'previousLastModified' class's field as useless --- .../drive/operations/UploadFileOperation.java | 82 +++++++++++++------ 1 file changed, 55 insertions(+), 27 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index 4cfc10b4..8a216af4 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -20,12 +20,17 @@ import com.owncloud.android.lib.common.OwnCloudClient; import com.owncloud.android.lib.common.UserInfo; import com.owncloud.android.lib.common.operations.RemoteOperation; import com.owncloud.android.lib.common.operations.RemoteOperationResult; +import com.owncloud.android.lib.resources.files.ChunkedFileUploadRemoteOperation; import com.owncloud.android.lib.resources.files.CreateFolderRemoteOperation; import com.owncloud.android.lib.resources.files.FileUtils; +import com.owncloud.android.lib.resources.files.ReadFileRemoteOperation; import com.owncloud.android.lib.resources.files.UploadFileRemoteOperation; +import com.owncloud.android.lib.resources.files.model.RemoteFile; import com.owncloud.android.lib.resources.users.GetUserInfoRemoteOperation; import com.owncloud.android.lib.common.operations.RemoteOperationResult.ResultCode; import java.io.File; +import java.util.ArrayList; + import foundation.e.drive.database.DbHelper; import foundation.e.drive.models.SyncedFileState; import foundation.e.drive.utils.CommonUtils; @@ -38,11 +43,9 @@ import foundation.e.drive.utils.DavClientProvider; public class UploadFileOperation extends RemoteOperation { private final static String TAG = UploadFileOperation.class.getSimpleName(); - private int restartCounter =0; - private long previousLastModified; //get to restore real value if all trials fails private final Context context; private final SyncedFileState syncedState; - private final Account account; // /!\ this is temporary because NC library doesn't use NextcloudClient for every operation yet + private final Account account; // /!\ Remove as soon as nextcloud library move all Operation to NextcloudClient instead of OwncloudClient /** * Construct an upload operation with an already known syncedFileState @@ -50,7 +53,6 @@ public class UploadFileOperation extends RemoteOperation { */ public UploadFileOperation (final SyncedFileState syncedFileState, final Account account, final Context context) { this.syncedState = syncedFileState; - this.previousLastModified = syncedState.getLocalLastModified(); this.context = context; this.account = account; } @@ -74,8 +76,6 @@ public class UploadFileOperation extends RemoteOperation { return new RemoteOperationResult(ResultCode.FORBIDDEN); } - final String targetPath = syncedState.getRemotePath(); - //If file already up-to-date & synced if (syncedState.isLastEtagStored() && syncedState.getLocalLastModified() == file.lastModified()) { @@ -89,15 +89,31 @@ public class UploadFileOperation extends RemoteOperation { return new RemoteOperationResult(checkQuotaResult.getCode()); } + final String targetPath = syncedState.getRemotePath(); if (!createRemoteFolder(targetPath, client)) { return new RemoteOperationResult(ResultCode.UNKNOWN_ERROR); } + final int fileSizeFloorForChunked = 3072000; //3MB + final RemoteOperationResult uploadResult; - final ResultCode resultCode; - boolean mustRestart = true; + if (file.length() >= fileSizeFloorForChunked) { - final RemoteOperationResult uploadResult = uploadFile(file, client); + Log.d(TAG, "upload "+file.getName()+" as chunked file"); + uploadResult = uploadChunkedFile(file, client); + if (uploadResult.isSuccess()) { + final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); + ArrayList resultData = result.getData(); + if (result.isSuccess() && resultData != null && !resultData.isEmpty()) { + final String latestETag = ((RemoteFile) resultData.get(0)).getEtag(); + uploadResult.setResultData(latestETag); + } + } + } else { + uploadResult = uploadFile(file, client); + } + + final ResultCode resultCode; if (uploadResult.isSuccess()) { final String etag = uploadResult.getResultData(); if (etag != null) { @@ -105,30 +121,17 @@ public class UploadFileOperation extends RemoteOperation { } syncedState.setLocalLastModified(file.lastModified()); resultCode = uploadResult.getCode(); - mustRestart = false; + } else { - if (uploadResult.getCode() == ResultCode.CONFLICT ) { - resultCode = ResultCode.CONFLICT; - Log.d(TAG, "Catched a conflict result for : "+ file.getName()); - mustRestart = false; - } else if (uploadResult.getCode() == ResultCode.QUOTA_EXCEEDED) { - resultCode = ResultCode.QUOTA_EXCEEDED; - mustRestart = false; + if (uploadResult.getCode() == ResultCode.CONFLICT + || uploadResult.getCode() == ResultCode.QUOTA_EXCEEDED) { + resultCode = uploadResult.getCode(); } else { Log.e(TAG, "UploadFileRemoteOperation for : " + file.getName() + " failed => code: " + uploadResult.getCode()); resultCode = ResultCode.UNKNOWN_ERROR; - mustRestart = false; } } - if (mustRestart) { - if (this.restartCounter < 1) { - this.restartCounter += 1; - return this.run(client); - } else { - syncedState.setLocalLastModified(this.previousLastModified); //Revert syncFileState to its previous state - } - } DbHelper.manageSyncedFileStateDB(syncedState, "UPDATE", context); return new RemoteOperationResult(resultCode); } @@ -163,8 +166,32 @@ public class UploadFileOperation extends RemoteOperation { return GetUserInfoRemoteOperation.execute(client); } + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + public RemoteOperationResult readRemoteFile(final String remotePath, final OwnCloudClient client) { + final ReadFileRemoteOperation readRemoteFile = new ReadFileRemoteOperation(remotePath); + return readRemoteFile.execute(client); + } + /** - * Effectively upload the file + * Upload a chunked file + * Used for file bigger than 3MB + * @param file + * @param client + * @return + */ + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + public RemoteOperationResult uploadChunkedFile(final File file, final OwnCloudClient client) { + final String mimeType = CommonUtils.getMimeType(file); + final ChunkedFileUploadRemoteOperation uploadOperation = new ChunkedFileUploadRemoteOperation(syncedState.getLocalPath(), + syncedState.getRemotePath(), + mimeType, syncedState.getLastETAG(), + syncedState.getLocalLastModified()+"", false); + return uploadOperation.execute(client); + } + + + /** + * Upload a file * note: this has been extracted from run(...) for * testing purpose * @param client client to run the method @@ -187,6 +214,7 @@ public class UploadFileOperation extends RemoteOperation { * @param client still OwnCloudClient at the moment, but will be Nextcloud client in the futur * @return */ + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public boolean createRemoteFolder(String targetPath, OwnCloudClient client) { final String remoteFolderPath = targetPath.substring(0, targetPath.lastIndexOf(FileUtils.PATH_SEPARATOR) + 1); final CreateFolderRemoteOperation createFolderOperation = new CreateFolderRemoteOperation(remoteFolderPath, true); -- GitLab From f414b77f44d8274cad93da419da81d86ecf8120a Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Mon, 8 Aug 2022 15:44:16 +0200 Subject: [PATCH 03/24] check if file exist before to create UploadFileOperation --- .../java/foundation/e/drive/models/SyncWrapper.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java index f6bc9313..50caa073 100644 --- a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java +++ b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java @@ -9,9 +9,12 @@ package foundation.e.drive.models; import android.accounts.Account; import android.content.Context; +import android.util.Log; import com.owncloud.android.lib.common.operations.RemoteOperation; +import java.io.File; + import foundation.e.drive.operations.DownloadFileOperation; import foundation.e.drive.operations.UploadFileOperation; @@ -59,7 +62,14 @@ public class SyncWrapper { switch (request.getOperationType()) { case UPLOAD: final SyncedFileState sfs = request.getSyncedFileState(); - operation = new UploadFileOperation(sfs, account, context); + final File file = new File(sfs.getLocalPath()); + if (!file.exists()) { + operation = null; + Log.w(SyncWrapper.class.getSimpleName(), "createRemoteOperation: local file doesn't exist for upload"); + } else { + operation = new UploadFileOperation(sfs, account, context); + } + break; case DOWNLOAD: final DownloadRequest downloadRequest = (DownloadRequest) request; -- GitLab From d7474f386ac38ece5bee2d2c24389544974dfbd5 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Wed, 10 Aug 2022 16:16:33 +0200 Subject: [PATCH 04/24] move code to check etag after chunkedUpload to be called each time etag is null --- .../drive/operations/UploadFileOperation.java | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index 8a216af4..9115516a 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -98,24 +98,27 @@ public class UploadFileOperation extends RemoteOperation { final RemoteOperationResult uploadResult; if (file.length() >= fileSizeFloorForChunked) { - Log.d(TAG, "upload "+file.getName()+" as chunked file"); uploadResult = uploadChunkedFile(file, client); - if (uploadResult.isSuccess()) { - final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); - ArrayList resultData = result.getData(); - if (result.isSuccess() && resultData != null && !resultData.isEmpty()) { - final String latestETag = ((RemoteFile) resultData.get(0)).getEtag(); - uploadResult.setResultData(latestETag); - } - } } else { uploadResult = uploadFile(file, client); } final ResultCode resultCode; if (uploadResult.isSuccess()) { + //The below if statement should only be called for chunked upload. But + //for some unknown reason, the simple file upload doesn't give the etag in the result + // so, I moved the code here as a security + if (uploadResult.getResultData() == null) { + final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); + final ArrayList resultData = result.getData(); + if (result.isSuccess() && resultData != null && !resultData.isEmpty()) { + final String latestETag = ((RemoteFile) resultData.get(0)).getEtag(); + uploadResult.setResultData(latestETag); + } + } final String etag = uploadResult.getResultData(); + if (etag != null) { syncedState.setLastETAG(etag); } -- GitLab From fabac8d86a82458b7fc3fe99427d1b47c04af7d2 Mon Sep 17 00:00:00 2001 From: Hasib Prince Date: Wed, 10 Aug 2022 15:17:12 +0000 Subject: [PATCH 05/24] Apply 1 suggestion(s) to 1 file(s) --- app/src/main/java/foundation/e/drive/models/SyncWrapper.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java index 50caa073..188cd3ff 100644 --- a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java +++ b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java @@ -66,10 +66,9 @@ public class SyncWrapper { if (!file.exists()) { operation = null; Log.w(SyncWrapper.class.getSimpleName(), "createRemoteOperation: local file doesn't exist for upload"); - } else { - operation = new UploadFileOperation(sfs, account, context); + break; } - + operation = new UploadFileOperation(sfs, account, context); break; case DOWNLOAD: final DownloadRequest downloadRequest = (DownloadRequest) request; -- GitLab From 43271ad3e363c8c997895db77fe95a08dd516ded Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 11 Aug 2022 09:10:27 +0200 Subject: [PATCH 06/24] extract some code from 'run' into a dedicated function for update synced file state instance --- .../drive/operations/UploadFileOperation.java | 45 +++++++++++++------ 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index 9115516a..a9a4eb77 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -109,20 +109,7 @@ public class UploadFileOperation extends RemoteOperation { //The below if statement should only be called for chunked upload. But //for some unknown reason, the simple file upload doesn't give the etag in the result // so, I moved the code here as a security - if (uploadResult.getResultData() == null) { - final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); - final ArrayList resultData = result.getData(); - if (result.isSuccess() && resultData != null && !resultData.isEmpty()) { - final String latestETag = ((RemoteFile) resultData.get(0)).getEtag(); - uploadResult.setResultData(latestETag); - } - } - final String etag = uploadResult.getResultData(); - - if (etag != null) { - syncedState.setLastETAG(etag); - } - syncedState.setLocalLastModified(file.lastModified()); + updateSyncedFileState(uploadResult, file.lastModified(), client); resultCode = uploadResult.getCode(); } else { @@ -139,6 +126,30 @@ public class UploadFileOperation extends RemoteOperation { return new RemoteOperationResult(resultCode); } + + /** + * Update syncedFileState (etag & last modified) in case of successful upload + * @param uploadResult The Upload's result instance + * @param fileLastModified value of local file's last modified + * @param client The client used to check etag if missing + */ + private void updateSyncedFileState(final RemoteOperationResult uploadResult, final long fileLastModified, final OwnCloudClient client) { + if (uploadResult.getResultData() == null) { + final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); + final ArrayList resultData = result.getData(); + if (result.isSuccess() && resultData != null && !resultData.isEmpty()) { + final String latestETag = ((RemoteFile) resultData.get(0)).getEtag(); + uploadResult.setResultData(latestETag); + } + } + final String etag = uploadResult.getResultData(); + + if (etag != null) { + syncedState.setLastETAG(etag); + } + syncedState.setLocalLastModified(fileLastModified); + } + /** * Perform an operation to check available space on server before to upload * @param client OwnCloudClient @@ -169,6 +180,12 @@ public class UploadFileOperation extends RemoteOperation { return GetUserInfoRemoteOperation.execute(client); } + /** + * Read remote file after upload to retrieve eTag + * @param remotePath file's remote path + * @param client + * @return + */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public RemoteOperationResult readRemoteFile(final String remotePath, final OwnCloudClient client) { final ReadFileRemoteOperation readRemoteFile = new ReadFileRemoteOperation(remotePath); -- GitLab From 456fc33d1058220bb44a1d6916ebae5a29f754ac Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 11 Aug 2022 09:17:42 +0200 Subject: [PATCH 07/24] Apply Jonathan's suggestion - Add 'TAG' final static field into SyncWrapper - Make fileSizeFloorForChunked to be a final static field into UploadFileOperation --- .../main/java/foundation/e/drive/models/SyncWrapper.java | 3 ++- .../foundation/e/drive/operations/UploadFileOperation.java | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java index 188cd3ff..a1984972 100644 --- a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java +++ b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java @@ -24,6 +24,7 @@ import foundation.e.drive.operations.UploadFileOperation; * @author Vincent Bourgmayer */ public class SyncWrapper { + private final static String TAG = SyncWrapper.class.getSimpleName(); private final SyncRequest request; private final RemoteOperation remoteOperation; private boolean isRunning; @@ -65,7 +66,7 @@ public class SyncWrapper { final File file = new File(sfs.getLocalPath()); if (!file.exists()) { operation = null; - Log.w(SyncWrapper.class.getSimpleName(), "createRemoteOperation: local file doesn't exist for upload"); + Log.w(TAG, "createRemoteOperation: local file doesn't exist for upload"); break; } operation = new UploadFileOperation(sfs, account, context); diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index a9a4eb77..ad66d8f3 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -46,7 +46,7 @@ public class UploadFileOperation extends RemoteOperation { private final Context context; private final SyncedFileState syncedState; private final Account account; // /!\ Remove as soon as nextcloud library move all Operation to NextcloudClient instead of OwncloudClient - + private final static int FILE_SIZE_FLOOR_FOR_CHUNKED = 3072000; //3MB /** * Construct an upload operation with an already known syncedFileState * @param syncedFileState syncedFileState corresponding to file. @@ -94,10 +94,10 @@ public class UploadFileOperation extends RemoteOperation { return new RemoteOperationResult(ResultCode.UNKNOWN_ERROR); } - final int fileSizeFloorForChunked = 3072000; //3MB + final RemoteOperationResult uploadResult; - if (file.length() >= fileSizeFloorForChunked) { + if (file.length() >= FILE_SIZE_FLOOR_FOR_CHUNKED) { Log.d(TAG, "upload "+file.getName()+" as chunked file"); uploadResult = uploadChunkedFile(file, client); } else { -- GitLab From 26bc0a0261c70aa55ade648cde170d0d6921c57d Mon Sep 17 00:00:00 2001 From: Jonathan Klee Date: Thu, 11 Aug 2022 07:23:04 +0000 Subject: [PATCH 08/24] Apply 1 suggestion(s) to 1 file(s) --- .../java/foundation/e/drive/operations/UploadFileOperation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index ad66d8f3..c9060980 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -98,7 +98,7 @@ public class UploadFileOperation extends RemoteOperation { final RemoteOperationResult uploadResult; if (file.length() >= FILE_SIZE_FLOOR_FOR_CHUNKED) { - Log.d(TAG, "upload "+file.getName()+" as chunked file"); + Log.d(TAG, "upload " + file.getName() + " as chunked file"); uploadResult = uploadChunkedFile(file, client); } else { uploadResult = uploadFile(file, client); -- GitLab From 03626d6242317b6f9ad4da2df7f2aca9f41fc309 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 11 Aug 2022 09:45:20 +0200 Subject: [PATCH 09/24] refactor UploadFileOperation.run() --- .../drive/operations/UploadFileOperation.java | 100 +++++++++++------- 1 file changed, 64 insertions(+), 36 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index c9060980..f971b970 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -42,11 +42,11 @@ import foundation.e.drive.utils.DavClientProvider; */ public class UploadFileOperation extends RemoteOperation { private final static String TAG = UploadFileOperation.class.getSimpleName(); - + private final static int FILE_SIZE_FLOOR_FOR_CHUNKED = 3072000; //3MB private final Context context; private final SyncedFileState syncedState; private final Account account; // /!\ Remove as soon as nextcloud library move all Operation to NextcloudClient instead of OwncloudClient - private final static int FILE_SIZE_FLOOR_FOR_CHUNKED = 3072000; //3MB + /** * Construct an upload operation with an already known syncedFileState * @param syncedFileState syncedFileState corresponding to file. @@ -71,32 +71,13 @@ public class UploadFileOperation extends RemoteOperation { @Override protected RemoteOperationResult run(OwnCloudClient client ) { final File file = new File(syncedState.getLocalPath()); - if (file == null || !file.exists()) { - Log.w(TAG, "Can't get the file. It might have been deleted"); - return new RemoteOperationResult(ResultCode.FORBIDDEN); - } - - //If file already up-to-date & synced - if (syncedState.isLastEtagStored() - && syncedState.getLocalLastModified() == file.lastModified()) { - Log.d(TAG, "syncedState last modified: "+ syncedState.getLocalLastModified()+" <=> file last modified: "+file.lastModified() +": So return sync_conflict"); - return new RemoteOperationResult(ResultCode.SYNC_CONFLICT); - } - final NextcloudClient ncClient = DavClientProvider.getInstance().getNcClientInstance(account, context); - final RemoteOperationResult checkQuotaResult = checkAvailableSpace(ncClient, file.length()); - if (checkQuotaResult.getCode() != ResultCode.OK) { - Log.e(TAG, "Impossible to check quota. Upload of " + syncedState.getLocalPath() + "cancelled"); - return new RemoteOperationResult(checkQuotaResult.getCode()); - } - final String targetPath = syncedState.getRemotePath(); - if (!createRemoteFolder(targetPath, client)) { - return new RemoteOperationResult(ResultCode.UNKNOWN_ERROR); + final ResultCode conditionCheckResult = checkCondition(file, client); + if (conditionCheckResult != ResultCode.OK) { + return new RemoteOperationResult(conditionCheckResult); } - final RemoteOperationResult uploadResult; - if (file.length() >= FILE_SIZE_FLOOR_FOR_CHUNKED) { Log.d(TAG, "upload " + file.getName() + " as chunked file"); uploadResult = uploadChunkedFile(file, client); @@ -106,20 +87,10 @@ public class UploadFileOperation extends RemoteOperation { final ResultCode resultCode; if (uploadResult.isSuccess()) { - //The below if statement should only be called for chunked upload. But - //for some unknown reason, the simple file upload doesn't give the etag in the result - // so, I moved the code here as a security updateSyncedFileState(uploadResult, file.lastModified(), client); resultCode = uploadResult.getCode(); - } else { - if (uploadResult.getCode() == ResultCode.CONFLICT - || uploadResult.getCode() == ResultCode.QUOTA_EXCEEDED) { - resultCode = uploadResult.getCode(); - } else { - Log.e(TAG, "UploadFileRemoteOperation for : " + file.getName() + " failed => code: " + uploadResult.getCode()); - resultCode = ResultCode.UNKNOWN_ERROR; - } + resultCode = onUploadFailure(uploadResult.getCode(), file.getName()); } DbHelper.manageSyncedFileStateDB(syncedState, "UPDATE", context); @@ -127,6 +98,60 @@ public class UploadFileOperation extends RemoteOperation { } + /** + * Handle upload's failure + * @param uploadResult + * @param fileName + * @return + */ + private ResultCode onUploadFailure(final ResultCode uploadResult, final String fileName) { + if (uploadResult != ResultCode.CONFLICT + && uploadResult != ResultCode.QUOTA_EXCEEDED) { + Log.e(TAG, "UploadFileRemoteOperation for : " + fileName + " failed => code: " + uploadResult); + return ResultCode.UNKNOWN_ERROR; + } + return uploadResult; + } + + /** + * Check condition required to upload the file: + * - the local file exist + * - the local file is not already up to date with the remote one + * - there is enough free storage + * - the remote directory exists + * @param file file to upload + * @param client client used to execute some request + * @return ResultCode.OK if everything is alright + */ + private ResultCode checkCondition(final File file, final OwnCloudClient client) { + if (file == null || !file.exists()) { + Log.w(TAG, "Can't get the file. It might have been deleted"); + return ResultCode.FORBIDDEN; + } + + //If file already up-to-date & synced + if (syncedState.isLastEtagStored() + && syncedState.getLocalLastModified() == file.lastModified()) { + Log.d(TAG, "syncedState last modified: "+ syncedState.getLocalLastModified()+" <=> file last modified: "+file.lastModified() +": So return sync_conflict"); + return ResultCode.SYNC_CONFLICT; + } + + final NextcloudClient ncClient = DavClientProvider.getInstance().getNcClientInstance(account, context); + final RemoteOperationResult checkQuotaResult = checkAvailableSpace(ncClient, file.length()); + if (checkQuotaResult.getCode() != ResultCode.OK) { + Log.e(TAG, "Impossible to check quota. Upload of " + syncedState.getLocalPath() + "cancelled"); + return checkQuotaResult.getCode(); + } + + final String targetPath = syncedState.getRemotePath(); + if (!createRemoteFolder(targetPath, client)) { + return ResultCode.UNKNOWN_ERROR; + } + + return ResultCode.OK; + } + + /** * Update syncedFileState (etag & last modified) in case of successful upload * @param uploadResult The Upload's result instance @@ -134,6 +159,9 @@ public class UploadFileOperation extends RemoteOperation { * @param client The client used to check etag if missing */ private void updateSyncedFileState(final RemoteOperationResult uploadResult, final long fileLastModified, final OwnCloudClient client) { + //The below if statement should only be called for chunked upload. But + //for some unknown reason, the simple file upload doesn't give the etag in the result + // so, I moved the code here as a security if (uploadResult.getResultData() == null) { final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); final ArrayList resultData = result.getData(); @@ -211,7 +239,7 @@ public class UploadFileOperation extends RemoteOperation { /** - * Upload a file + * Upload a file * note: this has been extracted from run(...) for * testing purpose * @param client client to run the method -- GitLab From 0134d3648b251995b35370652b901ffec94111e2 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 11 Aug 2022 13:22:55 +0200 Subject: [PATCH 10/24] reduce synchronization thread from 4 to 2 --- .../foundation/e/drive/services/SynchronizationService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java index 020429da..363e8167 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -54,7 +54,7 @@ public class SynchronizationService extends Service implements OnRemoteOperation private ConcurrentHashMap startedSync; //Integer is thread index (1 to workerAmount) private Account account; - private final int workerAmount = 4; + private final int workerAmount = 2; private Thread[] threadPool; @Deprecated private OwnCloudClient ocClient; -- GitLab From e6aef86d3ed1d46d9ffacb65c080e75b0854f511 Mon Sep 17 00:00:00 2001 From: Jonathan Klee Date: Thu, 11 Aug 2022 12:57:02 +0000 Subject: [PATCH 11/24] Apply 1 suggestion(s) to 1 file(s) --- .../java/foundation/e/drive/operations/UploadFileOperation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index f971b970..5c0f872f 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -45,7 +45,7 @@ public class UploadFileOperation extends RemoteOperation { private final static int FILE_SIZE_FLOOR_FOR_CHUNKED = 3072000; //3MB private final Context context; private final SyncedFileState syncedState; - private final Account account; // /!\ Remove as soon as nextcloud library move all Operation to NextcloudClient instead of OwncloudClient + private final Account account; // TODO Remove as soon as nextcloud library move all Operation to NextcloudClient instead of OwncloudClient /** * Construct an upload operation with an already known syncedFileState -- GitLab From e6ebe46059fb7abbdb2acc572ffa9184a597bfd4 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 11 Aug 2022 15:03:20 +0200 Subject: [PATCH 12/24] update documentation of method --- .../drive/operations/UploadFileOperation.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index f971b970..f520c9ef 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -211,8 +211,8 @@ public class UploadFileOperation extends RemoteOperation { /** * Read remote file after upload to retrieve eTag * @param remotePath file's remote path - * @param client - * @return + * @param client Owncloudclient instance. @TODO will be replaced by NextcloudClient in future. + * @return RemoteOperationResult instance containing failure details or RemoteFile instance */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public RemoteOperationResult readRemoteFile(final String remotePath, final OwnCloudClient client) { @@ -223,9 +223,9 @@ public class UploadFileOperation extends RemoteOperation { /** * Upload a chunked file * Used for file bigger than 3MB - * @param file - * @param client - * @return + * @param file File to upload + * @param client OwncloudClient to perform the upload. @TODO will be replaced by NextcloudClient in future. + * @return RemoteOperationResult instance containing success or failure status with details */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public RemoteOperationResult uploadChunkedFile(final File file, final OwnCloudClient client) { @@ -242,8 +242,8 @@ public class UploadFileOperation extends RemoteOperation { * Upload a file * note: this has been extracted from run(...) for * testing purpose - * @param client client to run the method - * @return RemoteOperationResult + * @param client client to run the method. @TODO will be replaced by NextcloudClient in future. + * @return RemoteOperationResult the instance must contains etag in resultData if successful. */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public RemoteOperationResult uploadFile(final File file, final OwnCloudClient client) { @@ -258,9 +258,9 @@ public class UploadFileOperation extends RemoteOperation { /** * Create remote parent folder of the file if missing - * @param targetPath - * @param client still OwnCloudClient at the moment, but will be Nextcloud client in the futur - * @return + * @param targetPath Path of remote directory to create or check for existence + * @param client Client to perform the request. @TODO will be replaced by NextcloudClient in future. + * @return true if the parent directory has been created, false either */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public boolean createRemoteFolder(String targetPath, OwnCloudClient client) { -- GitLab From b097a63d8c1091103b3298cff58b8b4a985e7d58 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Mon, 8 Aug 2022 14:55:07 +0200 Subject: [PATCH 13/24] Update version number in build.gradle --- app/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/build.gradle b/app/build.gradle index bbcd73a2..b7c9e8a5 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -4,7 +4,7 @@ plugins { def versionMajor = 1 -def versionMinor = 1 +def versionMinor = 2 def versionPatch = 0 -- GitLab From edd505dae73d60b67c679b3544c1735e0f15d29c Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Mon, 8 Aug 2022 15:21:26 +0200 Subject: [PATCH 14/24] Implement usage of chunked file upload Update UploadFileOperation.java: -> Try to upload file as chunked only if size is >= 3 MB -> Put http request execution in method easily mockable for tests -> Remove usage of 'mustRestart' and implement of restart mechanism as it was never called: mustRestart was always set to false. -> Remove 'previousLastModified' class's field as useless --- .../drive/operations/UploadFileOperation.java | 82 +++++++++++++------ 1 file changed, 55 insertions(+), 27 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index 4cfc10b4..8a216af4 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -20,12 +20,17 @@ import com.owncloud.android.lib.common.OwnCloudClient; import com.owncloud.android.lib.common.UserInfo; import com.owncloud.android.lib.common.operations.RemoteOperation; import com.owncloud.android.lib.common.operations.RemoteOperationResult; +import com.owncloud.android.lib.resources.files.ChunkedFileUploadRemoteOperation; import com.owncloud.android.lib.resources.files.CreateFolderRemoteOperation; import com.owncloud.android.lib.resources.files.FileUtils; +import com.owncloud.android.lib.resources.files.ReadFileRemoteOperation; import com.owncloud.android.lib.resources.files.UploadFileRemoteOperation; +import com.owncloud.android.lib.resources.files.model.RemoteFile; import com.owncloud.android.lib.resources.users.GetUserInfoRemoteOperation; import com.owncloud.android.lib.common.operations.RemoteOperationResult.ResultCode; import java.io.File; +import java.util.ArrayList; + import foundation.e.drive.database.DbHelper; import foundation.e.drive.models.SyncedFileState; import foundation.e.drive.utils.CommonUtils; @@ -38,11 +43,9 @@ import foundation.e.drive.utils.DavClientProvider; public class UploadFileOperation extends RemoteOperation { private final static String TAG = UploadFileOperation.class.getSimpleName(); - private int restartCounter =0; - private long previousLastModified; //get to restore real value if all trials fails private final Context context; private final SyncedFileState syncedState; - private final Account account; // /!\ this is temporary because NC library doesn't use NextcloudClient for every operation yet + private final Account account; // /!\ Remove as soon as nextcloud library move all Operation to NextcloudClient instead of OwncloudClient /** * Construct an upload operation with an already known syncedFileState @@ -50,7 +53,6 @@ public class UploadFileOperation extends RemoteOperation { */ public UploadFileOperation (final SyncedFileState syncedFileState, final Account account, final Context context) { this.syncedState = syncedFileState; - this.previousLastModified = syncedState.getLocalLastModified(); this.context = context; this.account = account; } @@ -74,8 +76,6 @@ public class UploadFileOperation extends RemoteOperation { return new RemoteOperationResult(ResultCode.FORBIDDEN); } - final String targetPath = syncedState.getRemotePath(); - //If file already up-to-date & synced if (syncedState.isLastEtagStored() && syncedState.getLocalLastModified() == file.lastModified()) { @@ -89,15 +89,31 @@ public class UploadFileOperation extends RemoteOperation { return new RemoteOperationResult(checkQuotaResult.getCode()); } + final String targetPath = syncedState.getRemotePath(); if (!createRemoteFolder(targetPath, client)) { return new RemoteOperationResult(ResultCode.UNKNOWN_ERROR); } + final int fileSizeFloorForChunked = 3072000; //3MB + final RemoteOperationResult uploadResult; - final ResultCode resultCode; - boolean mustRestart = true; + if (file.length() >= fileSizeFloorForChunked) { - final RemoteOperationResult uploadResult = uploadFile(file, client); + Log.d(TAG, "upload "+file.getName()+" as chunked file"); + uploadResult = uploadChunkedFile(file, client); + if (uploadResult.isSuccess()) { + final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); + ArrayList resultData = result.getData(); + if (result.isSuccess() && resultData != null && !resultData.isEmpty()) { + final String latestETag = ((RemoteFile) resultData.get(0)).getEtag(); + uploadResult.setResultData(latestETag); + } + } + } else { + uploadResult = uploadFile(file, client); + } + + final ResultCode resultCode; if (uploadResult.isSuccess()) { final String etag = uploadResult.getResultData(); if (etag != null) { @@ -105,30 +121,17 @@ public class UploadFileOperation extends RemoteOperation { } syncedState.setLocalLastModified(file.lastModified()); resultCode = uploadResult.getCode(); - mustRestart = false; + } else { - if (uploadResult.getCode() == ResultCode.CONFLICT ) { - resultCode = ResultCode.CONFLICT; - Log.d(TAG, "Catched a conflict result for : "+ file.getName()); - mustRestart = false; - } else if (uploadResult.getCode() == ResultCode.QUOTA_EXCEEDED) { - resultCode = ResultCode.QUOTA_EXCEEDED; - mustRestart = false; + if (uploadResult.getCode() == ResultCode.CONFLICT + || uploadResult.getCode() == ResultCode.QUOTA_EXCEEDED) { + resultCode = uploadResult.getCode(); } else { Log.e(TAG, "UploadFileRemoteOperation for : " + file.getName() + " failed => code: " + uploadResult.getCode()); resultCode = ResultCode.UNKNOWN_ERROR; - mustRestart = false; } } - if (mustRestart) { - if (this.restartCounter < 1) { - this.restartCounter += 1; - return this.run(client); - } else { - syncedState.setLocalLastModified(this.previousLastModified); //Revert syncFileState to its previous state - } - } DbHelper.manageSyncedFileStateDB(syncedState, "UPDATE", context); return new RemoteOperationResult(resultCode); } @@ -163,8 +166,32 @@ public class UploadFileOperation extends RemoteOperation { return GetUserInfoRemoteOperation.execute(client); } + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + public RemoteOperationResult readRemoteFile(final String remotePath, final OwnCloudClient client) { + final ReadFileRemoteOperation readRemoteFile = new ReadFileRemoteOperation(remotePath); + return readRemoteFile.execute(client); + } + /** - * Effectively upload the file + * Upload a chunked file + * Used for file bigger than 3MB + * @param file + * @param client + * @return + */ + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + public RemoteOperationResult uploadChunkedFile(final File file, final OwnCloudClient client) { + final String mimeType = CommonUtils.getMimeType(file); + final ChunkedFileUploadRemoteOperation uploadOperation = new ChunkedFileUploadRemoteOperation(syncedState.getLocalPath(), + syncedState.getRemotePath(), + mimeType, syncedState.getLastETAG(), + syncedState.getLocalLastModified()+"", false); + return uploadOperation.execute(client); + } + + + /** + * Upload a file * note: this has been extracted from run(...) for * testing purpose * @param client client to run the method @@ -187,6 +214,7 @@ public class UploadFileOperation extends RemoteOperation { * @param client still OwnCloudClient at the moment, but will be Nextcloud client in the futur * @return */ + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public boolean createRemoteFolder(String targetPath, OwnCloudClient client) { final String remoteFolderPath = targetPath.substring(0, targetPath.lastIndexOf(FileUtils.PATH_SEPARATOR) + 1); final CreateFolderRemoteOperation createFolderOperation = new CreateFolderRemoteOperation(remoteFolderPath, true); -- GitLab From 64759b4376cdac1739c363f9367b97962d7fb6f7 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Mon, 8 Aug 2022 15:44:16 +0200 Subject: [PATCH 15/24] check if file exist before to create UploadFileOperation --- .../java/foundation/e/drive/models/SyncWrapper.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java index f6bc9313..50caa073 100644 --- a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java +++ b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java @@ -9,9 +9,12 @@ package foundation.e.drive.models; import android.accounts.Account; import android.content.Context; +import android.util.Log; import com.owncloud.android.lib.common.operations.RemoteOperation; +import java.io.File; + import foundation.e.drive.operations.DownloadFileOperation; import foundation.e.drive.operations.UploadFileOperation; @@ -59,7 +62,14 @@ public class SyncWrapper { switch (request.getOperationType()) { case UPLOAD: final SyncedFileState sfs = request.getSyncedFileState(); - operation = new UploadFileOperation(sfs, account, context); + final File file = new File(sfs.getLocalPath()); + if (!file.exists()) { + operation = null; + Log.w(SyncWrapper.class.getSimpleName(), "createRemoteOperation: local file doesn't exist for upload"); + } else { + operation = new UploadFileOperation(sfs, account, context); + } + break; case DOWNLOAD: final DownloadRequest downloadRequest = (DownloadRequest) request; -- GitLab From 9b715ab72e92a8aaee422ae7935b787f8967c649 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Wed, 10 Aug 2022 16:16:33 +0200 Subject: [PATCH 16/24] move code to check etag after chunkedUpload to be called each time etag is null --- .../drive/operations/UploadFileOperation.java | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index 8a216af4..9115516a 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -98,24 +98,27 @@ public class UploadFileOperation extends RemoteOperation { final RemoteOperationResult uploadResult; if (file.length() >= fileSizeFloorForChunked) { - Log.d(TAG, "upload "+file.getName()+" as chunked file"); uploadResult = uploadChunkedFile(file, client); - if (uploadResult.isSuccess()) { - final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); - ArrayList resultData = result.getData(); - if (result.isSuccess() && resultData != null && !resultData.isEmpty()) { - final String latestETag = ((RemoteFile) resultData.get(0)).getEtag(); - uploadResult.setResultData(latestETag); - } - } } else { uploadResult = uploadFile(file, client); } final ResultCode resultCode; if (uploadResult.isSuccess()) { + //The below if statement should only be called for chunked upload. But + //for some unknown reason, the simple file upload doesn't give the etag in the result + // so, I moved the code here as a security + if (uploadResult.getResultData() == null) { + final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); + final ArrayList resultData = result.getData(); + if (result.isSuccess() && resultData != null && !resultData.isEmpty()) { + final String latestETag = ((RemoteFile) resultData.get(0)).getEtag(); + uploadResult.setResultData(latestETag); + } + } final String etag = uploadResult.getResultData(); + if (etag != null) { syncedState.setLastETAG(etag); } -- GitLab From e46bbd082f2e603d9908a2012939f708c8b80779 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 11 Aug 2022 09:10:27 +0200 Subject: [PATCH 17/24] extract some code from 'run' into a dedicated function for update synced file state instance --- .../drive/operations/UploadFileOperation.java | 45 +++++++++++++------ 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index 9115516a..a9a4eb77 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -109,20 +109,7 @@ public class UploadFileOperation extends RemoteOperation { //The below if statement should only be called for chunked upload. But //for some unknown reason, the simple file upload doesn't give the etag in the result // so, I moved the code here as a security - if (uploadResult.getResultData() == null) { - final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); - final ArrayList resultData = result.getData(); - if (result.isSuccess() && resultData != null && !resultData.isEmpty()) { - final String latestETag = ((RemoteFile) resultData.get(0)).getEtag(); - uploadResult.setResultData(latestETag); - } - } - final String etag = uploadResult.getResultData(); - - if (etag != null) { - syncedState.setLastETAG(etag); - } - syncedState.setLocalLastModified(file.lastModified()); + updateSyncedFileState(uploadResult, file.lastModified(), client); resultCode = uploadResult.getCode(); } else { @@ -139,6 +126,30 @@ public class UploadFileOperation extends RemoteOperation { return new RemoteOperationResult(resultCode); } + + /** + * Update syncedFileState (etag & last modified) in case of successful upload + * @param uploadResult The Upload's result instance + * @param fileLastModified value of local file's last modified + * @param client The client used to check etag if missing + */ + private void updateSyncedFileState(final RemoteOperationResult uploadResult, final long fileLastModified, final OwnCloudClient client) { + if (uploadResult.getResultData() == null) { + final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); + final ArrayList resultData = result.getData(); + if (result.isSuccess() && resultData != null && !resultData.isEmpty()) { + final String latestETag = ((RemoteFile) resultData.get(0)).getEtag(); + uploadResult.setResultData(latestETag); + } + } + final String etag = uploadResult.getResultData(); + + if (etag != null) { + syncedState.setLastETAG(etag); + } + syncedState.setLocalLastModified(fileLastModified); + } + /** * Perform an operation to check available space on server before to upload * @param client OwnCloudClient @@ -169,6 +180,12 @@ public class UploadFileOperation extends RemoteOperation { return GetUserInfoRemoteOperation.execute(client); } + /** + * Read remote file after upload to retrieve eTag + * @param remotePath file's remote path + * @param client + * @return + */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public RemoteOperationResult readRemoteFile(final String remotePath, final OwnCloudClient client) { final ReadFileRemoteOperation readRemoteFile = new ReadFileRemoteOperation(remotePath); -- GitLab From 8bc97a349b482921bd74089bb8fa9f9b2ab37f4e Mon Sep 17 00:00:00 2001 From: Hasib Prince Date: Wed, 10 Aug 2022 15:17:12 +0000 Subject: [PATCH 18/24] Apply 1 suggestion(s) to 1 file(s) --- app/src/main/java/foundation/e/drive/models/SyncWrapper.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java index 50caa073..188cd3ff 100644 --- a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java +++ b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java @@ -66,10 +66,9 @@ public class SyncWrapper { if (!file.exists()) { operation = null; Log.w(SyncWrapper.class.getSimpleName(), "createRemoteOperation: local file doesn't exist for upload"); - } else { - operation = new UploadFileOperation(sfs, account, context); + break; } - + operation = new UploadFileOperation(sfs, account, context); break; case DOWNLOAD: final DownloadRequest downloadRequest = (DownloadRequest) request; -- GitLab From 39da53efd1f3a5140ce271a598713f9cce62f2d9 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 11 Aug 2022 09:17:42 +0200 Subject: [PATCH 19/24] Apply Jonathan's suggestion - Add 'TAG' final static field into SyncWrapper - Make fileSizeFloorForChunked to be a final static field into UploadFileOperation --- .../main/java/foundation/e/drive/models/SyncWrapper.java | 3 ++- .../foundation/e/drive/operations/UploadFileOperation.java | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java index 188cd3ff..a1984972 100644 --- a/app/src/main/java/foundation/e/drive/models/SyncWrapper.java +++ b/app/src/main/java/foundation/e/drive/models/SyncWrapper.java @@ -24,6 +24,7 @@ import foundation.e.drive.operations.UploadFileOperation; * @author Vincent Bourgmayer */ public class SyncWrapper { + private final static String TAG = SyncWrapper.class.getSimpleName(); private final SyncRequest request; private final RemoteOperation remoteOperation; private boolean isRunning; @@ -65,7 +66,7 @@ public class SyncWrapper { final File file = new File(sfs.getLocalPath()); if (!file.exists()) { operation = null; - Log.w(SyncWrapper.class.getSimpleName(), "createRemoteOperation: local file doesn't exist for upload"); + Log.w(TAG, "createRemoteOperation: local file doesn't exist for upload"); break; } operation = new UploadFileOperation(sfs, account, context); diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index a9a4eb77..ad66d8f3 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -46,7 +46,7 @@ public class UploadFileOperation extends RemoteOperation { private final Context context; private final SyncedFileState syncedState; private final Account account; // /!\ Remove as soon as nextcloud library move all Operation to NextcloudClient instead of OwncloudClient - + private final static int FILE_SIZE_FLOOR_FOR_CHUNKED = 3072000; //3MB /** * Construct an upload operation with an already known syncedFileState * @param syncedFileState syncedFileState corresponding to file. @@ -94,10 +94,10 @@ public class UploadFileOperation extends RemoteOperation { return new RemoteOperationResult(ResultCode.UNKNOWN_ERROR); } - final int fileSizeFloorForChunked = 3072000; //3MB + final RemoteOperationResult uploadResult; - if (file.length() >= fileSizeFloorForChunked) { + if (file.length() >= FILE_SIZE_FLOOR_FOR_CHUNKED) { Log.d(TAG, "upload "+file.getName()+" as chunked file"); uploadResult = uploadChunkedFile(file, client); } else { -- GitLab From 6a10e9111e19d0e3170d60096fb3ab3796d9fc20 Mon Sep 17 00:00:00 2001 From: Jonathan Klee Date: Thu, 11 Aug 2022 07:23:04 +0000 Subject: [PATCH 20/24] Apply 1 suggestion(s) to 1 file(s) --- .../java/foundation/e/drive/operations/UploadFileOperation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index ad66d8f3..c9060980 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -98,7 +98,7 @@ public class UploadFileOperation extends RemoteOperation { final RemoteOperationResult uploadResult; if (file.length() >= FILE_SIZE_FLOOR_FOR_CHUNKED) { - Log.d(TAG, "upload "+file.getName()+" as chunked file"); + Log.d(TAG, "upload " + file.getName() + " as chunked file"); uploadResult = uploadChunkedFile(file, client); } else { uploadResult = uploadFile(file, client); -- GitLab From 8a4fb7c45cd41773dbeee4d1ba70bfa3c22a5d73 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 11 Aug 2022 09:45:20 +0200 Subject: [PATCH 21/24] refactor UploadFileOperation.run() --- .../drive/operations/UploadFileOperation.java | 100 +++++++++++------- 1 file changed, 64 insertions(+), 36 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index c9060980..f971b970 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -42,11 +42,11 @@ import foundation.e.drive.utils.DavClientProvider; */ public class UploadFileOperation extends RemoteOperation { private final static String TAG = UploadFileOperation.class.getSimpleName(); - + private final static int FILE_SIZE_FLOOR_FOR_CHUNKED = 3072000; //3MB private final Context context; private final SyncedFileState syncedState; private final Account account; // /!\ Remove as soon as nextcloud library move all Operation to NextcloudClient instead of OwncloudClient - private final static int FILE_SIZE_FLOOR_FOR_CHUNKED = 3072000; //3MB + /** * Construct an upload operation with an already known syncedFileState * @param syncedFileState syncedFileState corresponding to file. @@ -71,32 +71,13 @@ public class UploadFileOperation extends RemoteOperation { @Override protected RemoteOperationResult run(OwnCloudClient client ) { final File file = new File(syncedState.getLocalPath()); - if (file == null || !file.exists()) { - Log.w(TAG, "Can't get the file. It might have been deleted"); - return new RemoteOperationResult(ResultCode.FORBIDDEN); - } - - //If file already up-to-date & synced - if (syncedState.isLastEtagStored() - && syncedState.getLocalLastModified() == file.lastModified()) { - Log.d(TAG, "syncedState last modified: "+ syncedState.getLocalLastModified()+" <=> file last modified: "+file.lastModified() +": So return sync_conflict"); - return new RemoteOperationResult(ResultCode.SYNC_CONFLICT); - } - final NextcloudClient ncClient = DavClientProvider.getInstance().getNcClientInstance(account, context); - final RemoteOperationResult checkQuotaResult = checkAvailableSpace(ncClient, file.length()); - if (checkQuotaResult.getCode() != ResultCode.OK) { - Log.e(TAG, "Impossible to check quota. Upload of " + syncedState.getLocalPath() + "cancelled"); - return new RemoteOperationResult(checkQuotaResult.getCode()); - } - final String targetPath = syncedState.getRemotePath(); - if (!createRemoteFolder(targetPath, client)) { - return new RemoteOperationResult(ResultCode.UNKNOWN_ERROR); + final ResultCode conditionCheckResult = checkCondition(file, client); + if (conditionCheckResult != ResultCode.OK) { + return new RemoteOperationResult(conditionCheckResult); } - final RemoteOperationResult uploadResult; - if (file.length() >= FILE_SIZE_FLOOR_FOR_CHUNKED) { Log.d(TAG, "upload " + file.getName() + " as chunked file"); uploadResult = uploadChunkedFile(file, client); @@ -106,20 +87,10 @@ public class UploadFileOperation extends RemoteOperation { final ResultCode resultCode; if (uploadResult.isSuccess()) { - //The below if statement should only be called for chunked upload. But - //for some unknown reason, the simple file upload doesn't give the etag in the result - // so, I moved the code here as a security updateSyncedFileState(uploadResult, file.lastModified(), client); resultCode = uploadResult.getCode(); - } else { - if (uploadResult.getCode() == ResultCode.CONFLICT - || uploadResult.getCode() == ResultCode.QUOTA_EXCEEDED) { - resultCode = uploadResult.getCode(); - } else { - Log.e(TAG, "UploadFileRemoteOperation for : " + file.getName() + " failed => code: " + uploadResult.getCode()); - resultCode = ResultCode.UNKNOWN_ERROR; - } + resultCode = onUploadFailure(uploadResult.getCode(), file.getName()); } DbHelper.manageSyncedFileStateDB(syncedState, "UPDATE", context); @@ -127,6 +98,60 @@ public class UploadFileOperation extends RemoteOperation { } + /** + * Handle upload's failure + * @param uploadResult + * @param fileName + * @return + */ + private ResultCode onUploadFailure(final ResultCode uploadResult, final String fileName) { + if (uploadResult != ResultCode.CONFLICT + && uploadResult != ResultCode.QUOTA_EXCEEDED) { + Log.e(TAG, "UploadFileRemoteOperation for : " + fileName + " failed => code: " + uploadResult); + return ResultCode.UNKNOWN_ERROR; + } + return uploadResult; + } + + /** + * Check condition required to upload the file: + * - the local file exist + * - the local file is not already up to date with the remote one + * - there is enough free storage + * - the remote directory exists + * @param file file to upload + * @param client client used to execute some request + * @return ResultCode.OK if everything is alright + */ + private ResultCode checkCondition(final File file, final OwnCloudClient client) { + if (file == null || !file.exists()) { + Log.w(TAG, "Can't get the file. It might have been deleted"); + return ResultCode.FORBIDDEN; + } + + //If file already up-to-date & synced + if (syncedState.isLastEtagStored() + && syncedState.getLocalLastModified() == file.lastModified()) { + Log.d(TAG, "syncedState last modified: "+ syncedState.getLocalLastModified()+" <=> file last modified: "+file.lastModified() +": So return sync_conflict"); + return ResultCode.SYNC_CONFLICT; + } + + final NextcloudClient ncClient = DavClientProvider.getInstance().getNcClientInstance(account, context); + final RemoteOperationResult checkQuotaResult = checkAvailableSpace(ncClient, file.length()); + if (checkQuotaResult.getCode() != ResultCode.OK) { + Log.e(TAG, "Impossible to check quota. Upload of " + syncedState.getLocalPath() + "cancelled"); + return checkQuotaResult.getCode(); + } + + final String targetPath = syncedState.getRemotePath(); + if (!createRemoteFolder(targetPath, client)) { + return ResultCode.UNKNOWN_ERROR; + } + + return ResultCode.OK; + } + + /** * Update syncedFileState (etag & last modified) in case of successful upload * @param uploadResult The Upload's result instance @@ -134,6 +159,9 @@ public class UploadFileOperation extends RemoteOperation { * @param client The client used to check etag if missing */ private void updateSyncedFileState(final RemoteOperationResult uploadResult, final long fileLastModified, final OwnCloudClient client) { + //The below if statement should only be called for chunked upload. But + //for some unknown reason, the simple file upload doesn't give the etag in the result + // so, I moved the code here as a security if (uploadResult.getResultData() == null) { final RemoteOperationResult result = readRemoteFile(syncedState.getRemotePath(), client); final ArrayList resultData = result.getData(); @@ -211,7 +239,7 @@ public class UploadFileOperation extends RemoteOperation { /** - * Upload a file + * Upload a file * note: this has been extracted from run(...) for * testing purpose * @param client client to run the method -- GitLab From 91871c082cb8dc5d6dd27322db787857fee6d233 Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 11 Aug 2022 13:22:55 +0200 Subject: [PATCH 22/24] reduce synchronization thread from 4 to 2 --- .../foundation/e/drive/services/SynchronizationService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java index 020429da..363e8167 100644 --- a/app/src/main/java/foundation/e/drive/services/SynchronizationService.java +++ b/app/src/main/java/foundation/e/drive/services/SynchronizationService.java @@ -54,7 +54,7 @@ public class SynchronizationService extends Service implements OnRemoteOperation private ConcurrentHashMap startedSync; //Integer is thread index (1 to workerAmount) private Account account; - private final int workerAmount = 4; + private final int workerAmount = 2; private Thread[] threadPool; @Deprecated private OwnCloudClient ocClient; -- GitLab From df44bcedad207291e19ee4fe0eacb8ff1391eaaf Mon Sep 17 00:00:00 2001 From: vincent Bourgmayer Date: Thu, 11 Aug 2022 15:03:20 +0200 Subject: [PATCH 23/24] update documentation of method --- .../drive/operations/UploadFileOperation.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index f971b970..f520c9ef 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -211,8 +211,8 @@ public class UploadFileOperation extends RemoteOperation { /** * Read remote file after upload to retrieve eTag * @param remotePath file's remote path - * @param client - * @return + * @param client Owncloudclient instance. @TODO will be replaced by NextcloudClient in future. + * @return RemoteOperationResult instance containing failure details or RemoteFile instance */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public RemoteOperationResult readRemoteFile(final String remotePath, final OwnCloudClient client) { @@ -223,9 +223,9 @@ public class UploadFileOperation extends RemoteOperation { /** * Upload a chunked file * Used for file bigger than 3MB - * @param file - * @param client - * @return + * @param file File to upload + * @param client OwncloudClient to perform the upload. @TODO will be replaced by NextcloudClient in future. + * @return RemoteOperationResult instance containing success or failure status with details */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public RemoteOperationResult uploadChunkedFile(final File file, final OwnCloudClient client) { @@ -242,8 +242,8 @@ public class UploadFileOperation extends RemoteOperation { * Upload a file * note: this has been extracted from run(...) for * testing purpose - * @param client client to run the method - * @return RemoteOperationResult + * @param client client to run the method. @TODO will be replaced by NextcloudClient in future. + * @return RemoteOperationResult the instance must contains etag in resultData if successful. */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public RemoteOperationResult uploadFile(final File file, final OwnCloudClient client) { @@ -258,9 +258,9 @@ public class UploadFileOperation extends RemoteOperation { /** * Create remote parent folder of the file if missing - * @param targetPath - * @param client still OwnCloudClient at the moment, but will be Nextcloud client in the futur - * @return + * @param targetPath Path of remote directory to create or check for existence + * @param client Client to perform the request. @TODO will be replaced by NextcloudClient in future. + * @return true if the parent directory has been created, false either */ @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) public boolean createRemoteFolder(String targetPath, OwnCloudClient client) { -- GitLab From a73ad381b511dc6a31af423a769b31c98b19d27a Mon Sep 17 00:00:00 2001 From: Jonathan Klee Date: Thu, 11 Aug 2022 12:57:02 +0000 Subject: [PATCH 24/24] Apply 1 suggestion(s) to 1 file(s) --- .../java/foundation/e/drive/operations/UploadFileOperation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java index f520c9ef..2c6bdb09 100644 --- a/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java +++ b/app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java @@ -45,7 +45,7 @@ public class UploadFileOperation extends RemoteOperation { private final static int FILE_SIZE_FLOOR_FOR_CHUNKED = 3072000; //3MB private final Context context; private final SyncedFileState syncedState; - private final Account account; // /!\ Remove as soon as nextcloud library move all Operation to NextcloudClient instead of OwncloudClient + private final Account account; // TODO Remove as soon as nextcloud library move all Operation to NextcloudClient instead of OwncloudClient /** * Construct an upload operation with an already known syncedFileState -- GitLab