Skip to content

refactor: Simplify update upload file #2012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
benjaminVadon wants to merge 3 commits into
base: feature/upload-error-management
Choose a base branch
from
+28 −20
Open

refactor: Simplify update upload file #2012

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4dc35c6
feat: Add method to update db and local instance
benjaminVadon Apr 13, 2026
ba20073
feat: Apply on existing update method
benjaminVadon Apr 13, 2026
f7945f1
chore: Apply feedback to remove disambiguity
benjaminVadon Apr 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 28 additions & 20 deletions app/src/main/java/com/infomaniak/drive/data/models/UploadFile.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,9 @@ open class UploadFile(
fun uploadConflictOption() = if (replaceOnConflict()) ConflictOption.VERSION else ConflictOption.RENAME

fun resetUploadToken() {
getRealmInstance().use { realm ->
uploadFileByUriQuery(realm, uri).findFirst()?.apply {
realm.executeTransaction {
uploadToken = null
uploadHost = null
}
}
updateDbInstance {
it.uploadToken = null
it.uploadHost = null
}
}
Comment on lines 107 to 112
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resetUploadToken() now only updates the Realm row, leaving the in-memory instance’s uploadToken/uploadHost potentially stale. Call sites (e.g., retry/error paths) may continue using this same UploadFile object and re-cancel/retry with an already-reset token. Consider using the new update { ... } helper (or also calling updateCurrentInstance) so both DB and current instance are reset consistently.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@benjaminVadon benjaminVadon Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot it was already the case, which is goal of this PR to remove this bad hidden pattern and explicitly showing we update the DB only. And if we want to update the DB and the local instance (which I think we should), I'll change the code to use update method

Copy link
Copy Markdown
Contributor

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed resetUploadToken() to use update { ... } in commit 90ebd45, so it now updates both the DB and the current in-memory instance consistently.

Copy link
Copy Markdown
Contributor Author

@benjaminVadon benjaminVadon Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I don't want any change I want a human review on it


Expand All @@ -127,24 +123,19 @@ open class UploadFile(
}

fun updateFileSize(newFileSize: Long) {
getRealmInstance().use { realm ->
uploadFileByUriQuery(realm, uri).findFirst()?.let { uploadFile ->
realm.executeTransaction { uploadFile.fileSize = newFileSize }
}
update {
it.fileSize = newFileSize
}
fileSize = newFileSize
}

fun updateUploadToken(newUploadToken: String, uploadHost: String) {
getRealmInstance().use { realm ->
uploadFileByUriQuery(realm, uri).findFirst()?.let { uploadFile ->
realm.executeTransaction {
uploadFile.uploadToken = newUploadToken
uploadFile.uploadHost = uploadHost
}
}
updateDbInstance {
it.uploadToken = newUploadToken
it.uploadHost = uploadHost
}
updateCurrentInstance {
it.uploadToken = newUploadToken
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateUploadToken(newUploadToken, uploadHost) writes both fields to the DB but only updates uploadToken on the current instance. This makes the method’s behavior inconsistent with its parameters and can leave uploadHost null/stale if the caller reads it from the same object after this call. Update uploadHost on the current instance too (or use the update { ... } helper for both fields).

Suggested change
it.uploadToken = newUploadToken
it.uploadToken = newUploadToken
it.uploadHost = uploadHost

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@benjaminVadon benjaminVadon Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, it appears that we don't update the host. I don't know if it's a bad thing or not or just an oversight

}
uploadToken = newUploadToken
}

fun deleteIfExists(keepFile: Boolean = false, customRealm: Realm? = null) {
Expand All @@ -169,6 +160,23 @@ open class UploadFile(
}
}

private fun update(transaction: (UploadFile) -> Unit) {
updateDbInstance(transaction)
updateCurrentInstance(transaction)
}

private fun updateDbInstance(transactionBlock: (UploadFile) -> Unit) {
getRealmInstance().use { realm ->
uploadFileByUriQuery(realm, uri)
.findFirst()
?.let { uploadFile -> realm.executeTransaction { transactionBlock(uploadFile) } }
}
}

private fun updateCurrentInstance(transactionBlock: (UploadFile) -> Unit) {
transactionBlock(this)
}

enum class Type {
SYNC, UPLOAD, SHARED_FILE, SYNC_OFFLINE, CLOUD_STORAGE
}
Expand Down
Loading