Skip to content

Commit a873683

Browse files
dmialesdmiales
committed
Fix resource leaks and improve file upload collision handling
- Fix Cursor resource leaks in UploadsStorageManager and FileDetailSharingFragment by adding proper try-finally blocks to ensure cursors are always closed - Improve file upload collision handling in UploadFileOperation - Add content comparison for non-encrypted files when SKIP policy is used - Report SYNC_CONFLICT when file with same name has different content - Preserve old behavior for encrypted files and edge cases - Change log level from warning to info for informational messages in AutoUploadWorker and UploadErrorNotificationManager
1 parent a2bd815 commit a873683

File tree

5 files changed

+89
-44
lines changed

5 files changed

+89
-44
lines changed

‎app/src/main/java/com/nextcloud/client/jobs/autoUpload/AutoUploadWorker.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -502,7 +502,7 @@ class AutoUploadWorker(
502502
val dateTime = formatter.parse(exifDate, pos)
503503
if (dateTime != null) {
504504
lastModificationTime = dateTime.time
505-
Log_OC.w(TAG, "calculateLastModificationTime calculatedTime is: $lastModificationTime")
505+
Log_OC.i(TAG, "calculateLastModificationTime calculatedTime is: $lastModificationTime")
506506
} else {
507507
Log_OC.w(TAG, "calculateLastModificationTime dateTime is empty")
508508
}

‎app/src/main/java/com/nextcloud/client/jobs/utils/UploadErrorNotificationManager.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ object UploadErrorNotificationManager {
172172
result.code == ResultCode.USER_CANCELLED ||
173173
operation.isMissingPermissionThrown
174174
) {
175-
Log_OC.w(TAG, "operation is successful, cancelled or lack of storage permission")
175+
Log_OC.i(TAG, "operation is successful, cancelled or lack of storage permission")
176176
return false
177177
}
178178

‎app/src/main/java/com/owncloud/android/datamodel/UploadsStorageManager.java‎

Lines changed: 38 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -193,16 +193,21 @@ private void updateUploadStatus(long id, UploadStatus status, UploadResult resul
193193
null
194194
);
195195

196-
if (c != null) {
197-
if (c.getCount() != SINGLE_RESULT) {
198-
Log_OC.e(TAG, c.getCount() + " items for id=" + id
199-
+ " available in UploadDb. Expected 1. Failed to update upload db.");
196+
try {
197+
if (c != null) {
198+
if (c.getCount() != SINGLE_RESULT) {
199+
Log_OC.e(TAG, c.getCount() + " items for id=" + id
200+
+ " available in UploadDb. Expected 1. Failed to update upload db.");
201+
} else {
202+
updateUploadInternal(c, status, result, remotePath, localPath);
203+
}
200204
} else {
201-
updateUploadInternal(c, status, result, remotePath, localPath);
205+
Log_OC.e(TAG, "Cursor is null");
206+
}
207+
} finally {
208+
if (c != null) {
209+
c.close();
202210
}
203-
c.close();
204-
} else {
205-
Log_OC.e(TAG, "Cursor is null");
206211
}
207212

208213
}
@@ -302,9 +307,15 @@ OCUpload getUploadById(long id) {
302307
new String[]{Long.toString(id)},
303308
"_id ASC");
304309

305-
if (cursor != null) {
306-
if (cursor.moveToFirst()) {
307-
result = createOCUploadFromCursor(cursor);
310+
try {
311+
if (cursor != null) {
312+
if (cursor.moveToFirst()) {
313+
result = createOCUploadFromCursor(cursor);
314+
}
315+
}
316+
} finally {
317+
if (cursor != null) {
318+
cursor.close();
308319
}
309320
}
310321
Log_OC.d(TAG, "Retrieve job " + result + " for id " + id);
@@ -414,18 +425,23 @@ private List<OCUpload> getUploadPage(long limit, final long afterId, final boole
414425
pageSelectionArgs,
415426
sortOrder);
416427

417-
if (c != null) {
418-
if (c.moveToFirst()) {
419-
do {
420-
OCUpload upload = createOCUploadFromCursor(c);
421-
if (upload == null) {
422-
Log_OC.e(TAG, "OCUpload could not be created from cursor");
423-
} else {
424-
uploads.add(upload);
425-
}
426-
} while (c.moveToNext() && !c.isAfterLast());
428+
try {
429+
if (c != null) {
430+
if (c.moveToFirst()) {
431+
do {
432+
OCUpload upload = createOCUploadFromCursor(c);
433+
if (upload == null) {
434+
Log_OC.e(TAG, "OCUpload could not be created from cursor");
435+
} else {
436+
uploads.add(upload);
437+
}
438+
} while (c.moveToNext() && !c.isAfterLast());
439+
}
440+
}
441+
} finally {
442+
if (c != null) {
443+
c.close();
427444
}
428-
c.close();
429445
}
430446
return uploads;
431447
}

‎app/src/main/java/com/owncloud/android/operations/UploadFileOperation.java‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ public UploadFileOperation(UploadsStorageManager uploadsStorageManager,
251251
this.user = user;
252252
mUpload = upload;
253253
if (file == null) {
254-
Log_OC.w(TAG, "UploadFileOperation file is null, obtaining from upload");
254+
Log_OC.i(TAG, "UploadFileOperation file is null, obtaining from upload");
255255
mFile = obtainNewOCFileToUpload(
256256
upload.getRemotePath(),
257257
upload.getLocalPath(),
@@ -1232,7 +1232,31 @@ private RemoteOperationResult checkNameCollision(OCFile parentFile,
12321232
switch (mNameCollisionPolicy) {
12331233
case SKIP:
12341234
Log_OC.d(TAG, "user choose to skip upload if same file exists");
1235-
return new RemoteOperationResult<>(ResultCode.OK);
1235+
// For encrypted files, we can't easily compare content, so skip based on name only
1236+
// For non-encrypted files, check if it's actually the same file by content
1237+
if (!encrypted && mContext != null && user != null && mOriginalStoragePath != null) {
1238+
File localFile = new File(mOriginalStoragePath);
1239+
if (localFile.exists()) {
1240+
boolean isSameFile = FileUploadHelper.Companion.instance().isSameFileOnRemote(
1241+
user, localFile, mRemotePath, mContext);
1242+
if (isSameFile) {
1243+
Log_OC.d(TAG, "File is the same on remote, skipping upload");
1244+
return new RemoteOperationResult<>(ResultCode.OK);
1245+
} else {
1246+
Log_OC.d(TAG, "File with same name exists but content is different, reporting conflict");
1247+
// File exists but is different, return conflict so system can handle it
1248+
return new RemoteOperationResult(ResultCode.SYNC_CONFLICT);
1249+
}
1250+
} else {
1251+
Log_OC.w(TAG, "Local file does not exist, cannot compare content");
1252+
// If local file doesn't exist, skip based on name only (old behavior)
1253+
return new RemoteOperationResult<>(ResultCode.OK);
1254+
}
1255+
} else {
1256+
// For encrypted files or when context/user/file path is unavailable,
1257+
// skip based on name only (preserve old behavior)
1258+
return new RemoteOperationResult<>(ResultCode.OK);
1259+
}
12361260
case RENAME:
12371261
mRemotePath = getNewAvailableRemotePath(client, mRemotePath, fileNames, encrypted);
12381262
mWasRenamed = true;

‎app/src/main/java/com/owncloud/android/ui/fragment/FileDetailSharingFragment.java‎

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -624,30 +624,35 @@ private void handleContactResult(@NonNull Uri contactUri) {
624624

625625
Cursor cursor = fileActivity.getContentResolver().query(contactUri, projection, null, null, null);
626626

627-
if (cursor != null) {
628-
if (cursor.moveToFirst()) {
629-
// The contact has only one email address, use it.
630-
int columnIndex = cursor.getColumnIndex(ContactsContract.CommonDataKinds.Email.ADDRESS);
631-
if (columnIndex != -1) {
632-
// Use the email address as needed.
633-
// email variable contains the selected contact's email address.
634-
String email = cursor.getString(columnIndex);
635-
binding.searchView.post(() -> {
636-
binding.searchView.setQuery(email, false);
637-
binding.searchView.requestFocus();
638-
});
627+
try {
628+
if (cursor != null) {
629+
if (cursor.moveToFirst()) {
630+
// The contact has only one email address, use it.
631+
int columnIndex = cursor.getColumnIndex(ContactsContract.CommonDataKinds.Email.ADDRESS);
632+
if (columnIndex != -1) {
633+
// Use the email address as needed.
634+
// email variable contains the selected contact's email address.
635+
String email = cursor.getString(columnIndex);
636+
binding.searchView.post(() -> {
637+
binding.searchView.setQuery(email, false);
638+
binding.searchView.requestFocus();
639+
});
640+
} else {
641+
DisplayUtils.showSnackMessage(binding.getRoot(), R.string.email_pick_failed);
642+
Log_OC.e(FileDetailSharingFragment.class.getSimpleName(), "Failed to pick email address.");
643+
}
639644
} else {
640645
DisplayUtils.showSnackMessage(binding.getRoot(), R.string.email_pick_failed);
641-
Log_OC.e(FileDetailSharingFragment.class.getSimpleName(), "Failed to pick email address.");
646+
Log_OC.e(FileDetailSharingFragment.class.getSimpleName(), "Failed to pick email address as no Email found.");
642647
}
643648
} else {
644649
DisplayUtils.showSnackMessage(binding.getRoot(), R.string.email_pick_failed);
645-
Log_OC.e(FileDetailSharingFragment.class.getSimpleName(), "Failed to pick email address as no Email found.");
650+
Log_OC.e(FileDetailSharingFragment.class.getSimpleName(), "Failed to pick email address as Cursor is null.");
651+
}
652+
} finally {
653+
if (cursor != null) {
654+
cursor.close();
646655
}
647-
cursor.close();
648-
} else {
649-
DisplayUtils.showSnackMessage(binding.getRoot(), R.string.email_pick_failed);
650-
Log_OC.e(FileDetailSharingFragment.class.getSimpleName(), "Failed to pick email address as Cursor is null.");
651656
}
652657
}
653658

0 commit comments

Comments
 (0)