Merge branch '919-prevent-checking-etag-when-missing-remote-file' into 'main' (4dbaae28) · Commits · e / os / eDrive

app/src/main/java/foundation/e/drive/operations/UploadFileOperation.java

+47 −10

Original line number	Diff line number	Diff line
		@@ -23,6 +23,7 @@ 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.ExistenceCheckRemoteOperation;
		import com.owncloud.android.lib.resources.files.FileUtils;
		import com.owncloud.android.lib.resources.files.ReadFileRemoteOperation;
		import com.owncloud.android.lib.resources.files.UploadFileRemoteOperation;
		@@ -44,9 +45,9 @@ import timber.log.Timber;
		* @author Vincent Bourgmayer
		* High level Operation which upload a local file to a remote cloud storage
		*/
		@SuppressWarnings("rawtypes")
		public class UploadFileOperation extends RemoteOperation {
		public final static int FILE_SIZE_FLOOR_FOR_CHUNKED = 3072000; //3MB

		private static final Set<ResultCode> handledFailureCodes = getHandledFailureCodes();

		private final Context context;
		@@ -90,7 +91,8 @@ public class UploadFileOperation extends RemoteOperation {
		Timber.d("Upload %s as chunked file", file.getName());
		uploadResult = uploadChunkedFile(file, client);
		} else {
		uploadResult = uploadFile(file, client);
		final boolean checkEtag = ifMatchETagRequired(client);
		uploadResult = uploadFile(file, client, checkEtag);
		}

		final ResultCode resultCode;
		@@ -169,7 +171,6 @@ public class UploadFileOperation extends RemoteOperation {
		return ResultCode.OK;
		}


		/**
		* Update syncedFileState (etag & last modified) in case of successful upload
		* @param uploadResult The Upload's result instance
		@@ -178,7 +179,7 @@ public class UploadFileOperation extends RemoteOperation {
		*/
		private void updateSyncedFileState(final RemoteOperationResult<String> 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
		//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);
		@@ -259,22 +260,58 @@ public class UploadFileOperation extends RemoteOperation {
		return uploadOperation.execute(client);
		}

		/**
		* Check that the remote file exists
		* @param remotePath path of remote file
		* @param client OwncloudClient instance to perform the request
		* @return true if the remote file exist, false otherwise
		*/
		@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
		public boolean remoteFileExist(@NonNull String remotePath, @NonNull OwnCloudClient client) {
		final ExistenceCheckRemoteOperation operation = new ExistenceCheckRemoteOperation(remotePath, false);
		final RemoteOperationResult result = operation.execute(client);
		return result.isSuccess();
		}

		/**
		* define if upload request should embed a "if-match" header
		* with eTag.
		*
		* - Only check for etag if the file can be downloaded, aka media files
		* (pictures, music, etc.)
		* - Only check for etag, if a previous eTag was already known
		* which means that the file has already been on the cloud
		* - Only check for eTag if the file is still on the cloud or we will get http 412
		*
		* @param client OwnCloudClient instance used to check if remote file exists
		* @return true if the if-match header should be added
		*/
		@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
		public boolean ifMatchETagRequired(@NonNull OwnCloudClient client) {
		return syncedState.isMediaType()
		&& !syncedState.getLastETAG().isEmpty()
		&& remoteFileExist(syncedState.getRemotePath(), client);
		}

		/**
		* Upload a file
		* note: this has been extracted from run(...) for
		* testing purpose
		* @param file File instance representing the file to upload
		* @param client client to run the method. TODO will be replaced by NextcloudClient in future.
		* @param checkEtag indicate if upload request must have a if-match header with eTag value
		* @return RemoteOperationResult the instance must contains etag in resultData if successful.
		*/
		@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
		@NonNull
		public RemoteOperationResult<String> uploadFile(@NonNull final File file, @NonNull final OwnCloudClient client) {
		public RemoteOperationResult<String> uploadFile(@NonNull final File file, @NonNull final OwnCloudClient client, boolean checkEtag) {
		final String timeStamp = ((Long) (file.lastModified() / 1000) ).toString();
		final String eTag = checkEtag ? syncedState.getLastETAG() : null;

		final UploadFileRemoteOperation uploadOperation = new UploadFileRemoteOperation(syncedState.getLocalPath(),
		syncedState.getRemotePath(),
		CommonUtils.getMimeType(file),
		(!this.syncedState.isMediaType() \|\| syncedState.getLastETAG().isEmpty())? null : syncedState.getLastETAG(), //If not null, This can cause error 412; that means remote file has change
		eTag, //If not null, This can cause error 412; that means remote file has change
		timeStamp);
		return uploadOperation.execute(client);
		}

app/src/main/java/foundation/e/drive/services/InitializerService.java

+3 −1

Original line number	Diff line number	Diff line
		@@ -28,6 +28,7 @@ import timber.log.Timber;

		import androidx.annotation.NonNull;
		import androidx.annotation.Nullable;
		import androidx.annotation.VisibleForTesting;
		import androidx.work.WorkManager;

		/**
		@@ -78,7 +79,8 @@ public class InitializerService extends Service {
		* - Account available
		* @return true if condition are met
		*/
		private boolean checkStartConditions(@NonNull final SharedPreferences prefs,
		@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
		public boolean checkStartConditions(@NonNull final SharedPreferences prefs,
		@NonNull final String accountName, @NonNull final String accountType) {
		if (prefs.getBoolean(AppConstants.INITIALIZATION_HAS_BEEN_DONE, false)) {
		Timber.w("Initialization has already been done");

app/src/test/java/foundation/e/drive/TestUtils.java

+3 −10

Original line number	Diff line number	Diff line
		@@ -50,7 +50,6 @@ public abstract class TestUtils {
		TEST_SERVER_URI = properties.getProperty("test.account.url");
		}


		/**
		* Get the valid Account object. Create it if it isn't already instanciated
		* @return Account
		@@ -63,7 +62,6 @@ public abstract class TestUtils {
		return validAccount;
		}


		public static NextcloudClient getNcClient(Context context) {
		final Uri serverUri = Uri.parse(TEST_SERVER_URI);
		return new NextcloudClient(serverUri, TEST_ACCOUNT_NAME, TEST_ACCOUNT_PASSWORD, context);
		@@ -126,9 +124,8 @@ public abstract class TestUtils {
		}
		}


		/**
		* Method adapted from https://github.com/nextcloud/android/blob/master/src/androidTest/java/com/owncloud/android/AbstractIT.java
		* Method adapted from <a href="https://github.com/nextcloud/android/blob/master/src/androidTest/java/com/owncloud/android/AbstractIT.java">...</a>
		* @param filePath path of the file to create
		* @param iteration number of time to write dummy content
		* @return the File instance
		@@ -144,7 +141,6 @@ public abstract class TestUtils {
		file.createNewFile();

		final FileWriter writer = new FileWriter(file);

		for (int i = 0; i < iteration; i++) {
		writer.write("123123123123123123123123123\n");
		}
		@@ -154,16 +150,14 @@ public abstract class TestUtils {
		return file;
		}


		/**
		* Delete a local directory with all its content.
		* Adapted from https://www.geeksforgeeks.org/java-program-to-delete-a-directory/
		* Adapted from <a href="https://www.geeksforgeeks.org/java-program-to-delete-a-directory/">...</a>
		* @param file
		*/
		public static void deleteDirectory(File file)
		{
		for (File subfile : file.listFiles()) {

		if (subfile.isDirectory()) {
		deleteDirectory(subfile);
		}
		@@ -172,7 +166,6 @@ public abstract class TestUtils {
		file.delete();
		}


		public static void initializeWorkmanager(Context context) {
		final Configuration config = new Configuration.Builder()
		.setMinimumLoggingLevel(Log.DEBUG)

app/src/test/java/foundation/e/drive/operations/UploadFileOperationTest.java

+125 −72

Original line number	Diff line number	Diff line
		@@ -21,7 +21,6 @@ import com.owncloud.android.lib.common.UserInfo;
		import com.owncloud.android.lib.common.operations.RemoteOperationResult;

		import org.junit.Before;
		import org.junit.Ignore;
		import org.junit.Test;
		import org.junit.runner.RunWith;
		import org.mockito.Mockito;
		@@ -38,23 +37,18 @@ import foundation.e.drive.database.DbHelper;
		import foundation.e.drive.models.SyncedFileState;
		import foundation.e.drive.models.SyncedFolder;
		import foundation.e.drive.TestUtils;
		import foundation.e.drive.utils.CommonUtils;
		import foundation.e.drive.utils.DavClientProvider;


		@SuppressWarnings("rawtypes")
		@RunWith(RobolectricTestRunner.class)
		@Config(sdk = Build.VERSION_CODES.O, manifest = Config.NONE)
		public class UploadFileOperationTest {

		private List<SyncedFileState> syncedFileStates= new ArrayList<>();
		private final OwnCloudClient ocClient;
		private final NextcloudClient ncClient;
		private final AccountManager accountManager;
		private final Context context;
		private long userFreeQuota;
		private long userTotalQuota;
		private long userUsedQuota;
		private double userRelativeQuota;
		private final long userFreeQuota = 50;
		private final Account account;

		public UploadFileOperationTest() {
		@@ -66,15 +60,11 @@ public class UploadFileOperationTest {
		account = TestUtils.getValidAccount();
		ocClient = DavClientProvider.getInstance().getClientInstance(account, context);
		ncClient = TestUtils.getNcClient(context);
		userFreeQuota = 50;
		userTotalQuota = 100;
		userUsedQuota = 50;
		userRelativeQuota = 50.0;
		}

		@Before
		public void setUp() {
		prepareDB(); //Create DB
		prepareDB();
		assertNotNull("Client is null. unexpected!", ocClient);
		}

		@@ -87,7 +77,6 @@ public class UploadFileOperationTest {
		DbHelper.insertSyncedFolder(createSyncedFolder("medium"), context);
		DbHelper.insertSyncedFolder(createSyncedFolder("large"), context);

		//assertion for debug purpose
		assertEquals("There isn't three folder in DB as expected", 3, DbHelper.getSyncedFolderList(context, true).size());
		}

		@@ -115,7 +104,11 @@ public class UploadFileOperationTest {
		fail(e.getMessage());
		}

		final SyncedFileState sfs = new SyncedFileState(-1, "dummy.txt", TestUtils.TEST_LOCAL_ROOT_FOLDER_PATH+"small/dummy.txt", TestUtils.TEST_REMOTE_ROOT_FOLDER_PATH+"small/dummy.txt", "", 0l, 0, true, 3);
		final SyncedFileState sfs = new SyncedFileState(-1,
		"dummy.txt",
		TestUtils.TEST_LOCAL_ROOT_FOLDER_PATH + "small/dummy.txt",
		TestUtils.TEST_REMOTE_ROOT_FOLDER_PATH + "small/dummy.txt",
		"", 0L, 0, true, 3);
		sfs.setId(DbHelper.manageSyncedFileStateDB(sfs, "INSERT", context));

		syncedFileStates.add(sfs);
		@@ -123,17 +116,15 @@ public class UploadFileOperationTest {
		}

		/**
		* Remove SmallFile locally and remotly if it exist
		* Remove SmallFile locally if it exist
		* This method has just been made to make basic test to work
		* It should be refactored for next state, and test improvement
		* todo: refactor for next state, and test improvement
		* @return true if file deleted or if it doesn't exist
		*/
		private boolean removeSmallFile() {
		if (!syncedFileStates.isEmpty()) {
		final SyncedFileState sfs = syncedFileStates.get(0);
		if (sfs != null) {
		//final RemoveFileOperation removeRemoteFileOp = new RemoveFileOperation(sfs);
		//removeRemoteFileOp.execute(ocClient);
		syncedFileStates.remove(sfs);
		}
		}
		@@ -149,18 +140,17 @@ public class UploadFileOperationTest {
		* test upload of a file and check that it's ok
		*/
		@Test
		//@Ignore("Mocking file uploading doesn't work, and I don't find why; But this isn't related to method under test")
		public void uploadNewSmallFile_shouldwork() {
		//tearDown
		removeSmallFile(); //clean the environnement
		final File file = createSmallFile(); //preparation

		final SyncedFileState sfs_fromDB = DbHelper.loadSyncedFile(context, syncedFileStates.get(0).getLocalPath(), true);
		assertTrue("SyncedFileState loaded from DB must have an empty Etag", sfs_fromDB.getLastETAG().isEmpty());
		boolean checkEtag = false;
		assertTrue("SyncedFileState loaded from DB must have an empty ETag", sfs_fromDB.getLastETAG().isEmpty());
		final UploadFileOperation operation = Mockito.spy(new UploadFileOperation(syncedFileStates.get(0), account, context));
		mockCreateRemoteDir(true, operation, sfs_fromDB.getRemotePath());
		mockUserInfoReading(operation);
		mockSuccessfulFileUpload(operation, file);
		mockFileHeadOperation(true, operation, sfs_fromDB.getRemotePath());
		mockSuccessfulFileUpload(operation, file, false);

		final RemoteOperationResult result = operation.execute(ocClient);
		String errorMsg = "The upload failed:\n http code: " + result.getHttpCode()
		@@ -176,27 +166,6 @@ public class UploadFileOperationTest {
		assertFalse("After upload, the database must store the etag of the syncedFileState. But here it is empty", sfs_fromDBAfterUpload.getLastETAG().isEmpty());
		}




		/**
		* Try to upload a file with a null SyncedFileState instance
		* Must return a "Forbidden" result code
		*/
		@Ignore("Ignored until UploadFileOperation has fixed the NPE")
		@Test
		public void nullSyncedFileState_shouldFail() {

		SyncedFileState syncedFileState = null;
		//Test fails at the moment because of UploadFileOperation's constructor not checking for syncedFileState is null)
		// check https://gitlab.e.foundation/e/apps/eDrive/-/issues/120
		final UploadFileOperation testOperation = new UploadFileOperation(syncedFileState, account, context);

		RemoteOperationResult result = testOperation.execute(ocClient);
		assertEquals("Expected result code was FORBIDDEN but got: "+result.getCode().name(), RemoteOperationResult.ResultCode.FORBIDDEN, result.getCode());
		}


		/**
		* Try to upload local file that has been removed between uploadOperation's creation and
		* ploadOperation execution
		@@ -219,15 +188,13 @@ public class UploadFileOperationTest {
		assertEquals("Expected result code was FORBIDDEN but got: " + result.getCode().name(), RemoteOperationResult.ResultCode.FORBIDDEN, result.getCode());
		}


		/**
		* Assert that uploading a file that is bigger than free quotas isn't allowed
		*/
		@Test
		public void fileSizeBiggerThanFreeQuota_shouldnotBeAllowed() {
		//long freeQuota = getUserRemoteFreeQuota();
		assertFalse("Reading remote free quota fails"+userFreeQuota, -1 == userFreeQuota);
		//We don't care of parameter of UploadFileOperation's constructor
		assertNotEquals("Reading remote free quota fails" + userFreeQuota, -1, userFreeQuota);

		final UploadFileOperation operation = Mockito.spy(new UploadFileOperation(Mockito.mock(SyncedFileState.class), account, context));
		mockUserInfoReading(operation);
		final RemoteOperationResult actualResult = operation.checkAvailableSpace(ncClient, (userFreeQuota+1));
		@@ -239,9 +206,7 @@ public class UploadFileOperationTest {
		*/
		@Test
		public void fileSizeEqualToFreeQuota_shouldBeAllowed() {
		//I don't know why but it always fail for this test.
		//long freeQuota = getUserRemoteFreeQuota();
		assertFalse("Reading remote free quota fails"+userFreeQuota, -1 == userFreeQuota);
		assertNotEquals("Reading remote free quota fails" + userFreeQuota, -1, userFreeQuota);

		final UploadFileOperation operation = Mockito.spy(new UploadFileOperation(Mockito.mock(SyncedFileState.class), account, context));
		mockUserInfoReading(operation);
		@@ -255,15 +220,13 @@ public class UploadFileOperationTest {
		actualResult.getCode());
		}


		/**
		* Assert that uploading a file which size is lower than the amount of free quota is allowed
		*
		*/
		@Test
		public void fileSizeSmallerThanFreeQuota_shouldBeAllowed() {
		//long freeQuota = getUserRemoteFreeQuota();
		assertFalse("Reading remote free quota fails "+userFreeQuota, -1 == userFreeQuota);
		assertNotEquals("Reading remote free quota fails " + userFreeQuota, -1, userFreeQuota);

		final UploadFileOperation operation = Mockito.spy(new UploadFileOperation(Mockito.mock(SyncedFileState.class), account, context));
		mockUserInfoReading(operation);
		@@ -273,19 +236,109 @@ public class UploadFileOperationTest {
		actualResult.getCode());
		}

		@Test
		public void ifMatchEtagRequired_notMediaType_expectedFalse() {
		final SyncedFileState testSyncedState = new SyncedFileState(1,
		"test.jpg",
		"pictures/toto.jpg",
		"pictures/test.jpg",
		"123456",
		11111220,
		1,
		false,
		3);

		assertFalse("SyncedFileState should not have a media type but it has", testSyncedState.isMediaType());

		final UploadFileOperation operationUnderTest = new UploadFileOperation(testSyncedState, account, context);
		final boolean addIfMatchHeader =operationUnderTest.ifMatchETagRequired(ocClient);
		assertFalse("Expected result for ifMatchETagRequired(client) was false but got: true", addIfMatchHeader);
		}

		@Test
		public void ifMatchETagRequired_empyEtag_expectedFalse() {
		final SyncedFileState testSyncedState = new SyncedFileState(1,
		"test.jpg",
		"pictures/toto.jpg",
		"pictures/test.jpg",
		"",
		11111220,
		1,
		true,
		3);

		assertTrue("SyncedFileState's ETag should be empty but isn't", testSyncedState.getLastETAG().isEmpty());
		assertTrue("SyncedFileState is not a media type but it should", testSyncedState.isMediaType());
		final UploadFileOperation operationUnderTest = new UploadFileOperation(testSyncedState, account, context);
		final boolean addIfMatchHeader =operationUnderTest.ifMatchETagRequired(ocClient);
		assertFalse("Expected result for ifMatchETagRequired(client) was false but got: true", addIfMatchHeader);
		}

		@Test
		public void ifMatchETagRequired_missingRemoteFile_expectedFalse() {
		final SyncedFileState testSyncedState = new SyncedFileState(1,
		"test.jpg",
		"pictures/toto.jpg",
		"pictures/test.jpg",
		"123456",
		11111220,
		1,
		true,
		3);

		assertFalse("SyncedFileState's ETag should not be empty but isn't", testSyncedState.getLastETAG().isEmpty());
		assertTrue("SyncedFileState is not a media type but it should", testSyncedState.isMediaType());
		final UploadFileOperation operationUnderTest = Mockito.spy(new UploadFileOperation(testSyncedState, account, context));
		mockFileHeadOperation(false, operationUnderTest, testSyncedState.getRemotePath());
		final boolean addIfMatchHeader =operationUnderTest.ifMatchETagRequired(ocClient);
		assertFalse("Expected result for ifMatchETagRequired(client) is false but got: true", addIfMatchHeader);
		}

		@Test
		public void ifMatchETagRequired_expectedTrue() {
		final SyncedFileState testSyncedState = new SyncedFileState(1,
		"test.jpg",
		"pictures/toto.jpg",
		"pictures/test.jpg",
		"123456",
		11111220,
		1,
		true,
		3);

		assertFalse("SyncedFileState's ETag should not be empty but isn't", testSyncedState.getLastETAG().isEmpty());
		assertTrue("SyncedFileState is not a media type but it should", testSyncedState.isMediaType());
		final UploadFileOperation operationUnderTest = Mockito.spy(new UploadFileOperation(testSyncedState, account, context));
		mockFileHeadOperation(true, operationUnderTest, testSyncedState.getRemotePath());
		final boolean addIfMatchHeader =operationUnderTest.ifMatchETagRequired(ocClient);
		assertTrue("Expected result for ifMatchETagRequired(client) is true but got: false", addIfMatchHeader);
		}

		private void mockUserInfoReading(UploadFileOperation instanceUnderTest) {
		final long userTotalQuota = 100;
		final long userUsedQuota = 50;
		final double userRelativeQuota = 50;
		final Quota quota = new Quota(userFreeQuota, userUsedQuota, userTotalQuota, userRelativeQuota, 100);
		final UserInfo uInfo = new UserInfo("id", true, "toto", "toto@toto.com", "+123456789", "adress", "", "", quota, null);
		final RemoteOperationResult<UserInfo> mockResponse = new RemoteOperationResult<UserInfo>(RemoteOperationResult.ResultCode.OK);
		final RemoteOperationResult<UserInfo> mockResponse = new RemoteOperationResult<>(RemoteOperationResult.ResultCode.OK);
		mockResponse.setResultData(uInfo);
		Mockito.doReturn(mockResponse).when(instanceUnderTest).readUserInfo(ncClient);
		}

		private void mockSuccessfulFileUpload(final UploadFileOperation instanceUnderTest, final File file) {
		@SuppressWarnings("SameParameterValue")
		private void mockSuccessfulFileUpload(final UploadFileOperation instanceUnderTest, final File file, final boolean checkETag) {
		final String eTag = "123456789";
		final RemoteOperationResult<String> mockResponse = new RemoteOperationResult<String>(RemoteOperationResult.ResultCode.OK);
		final RemoteOperationResult<String> mockResponse = new RemoteOperationResult<>(RemoteOperationResult.ResultCode.OK);
		mockResponse.setResultData(eTag);
		Mockito.doReturn(mockResponse).when(instanceUnderTest).uploadFile(file, ocClient);
		Mockito.doReturn(mockResponse).when(instanceUnderTest).uploadFile(file, ocClient, checkETag);
		}

		private void mockFileHeadOperation(boolean expectedResult, final UploadFileOperation instanceUdnerTest, final String remotePath) {
		Mockito.doReturn(expectedResult).when(instanceUdnerTest).remoteFileExist(remotePath, ocClient);
		}

		@SuppressWarnings("SameParameterValue")
		private void mockCreateRemoteDir(boolean successExpected, final UploadFileOperation instanceUnderTest, final String remotePath) {
		Mockito.doReturn(successExpected).when(instanceUnderTest).createRemoteFolder(remotePath, ocClient);
		}
		}
		No newline at end of file

app/src/test/java/foundation/e/drive/services/AbstractServiceIT.java

+1 −1

Original line number	Diff line number	Diff line
		@@ -116,6 +116,6 @@ public abstract class AbstractServiceIT<T extends Service> {
		.putString(AccountManager.KEY_ACCOUNT_TYPE, TEST_ACCOUNT_TYPE)
		.putInt(AppConstants.INITIALFOLDERS_NUMBER, initial_folder_number)
		.putLong(AppConstants.KEY_LAST_SYNC_TIME, last_sync_time)
		.commit();
		.apply();
		}
		}