-
Notifications
You must be signed in to change notification settings - Fork 27
Refactor(service): Separate network calls from Realm transactions #8910
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.
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
base: master
Are you sure you want to change the base?
Conversation
Refactored the `UploadToShelfService` to use a coroutine-based approach for handling data uploads. This change separates network operations from Realm database transactions to prevent long-running transactions and improve performance. - Replaced `executeTransactionAsync` with `kotlinx.coroutines.launch` and `withContext(Dispatchers.IO)`. - Decoupled database reads, network calls, and database writes into distinct steps. - Removed the class-level `mRealm` instance in favor of short-lived, transaction-specific instances. - Updated `uploadUserData`, `uploadSingleUserData`, `uploadToShelf`, `uploadSingleUserToShelf`, `uploadHealth`, and `uploadSingleUserHealth` to follow the new architecture.
❌ 6 blocking issues (6 total)
|
app/src/main/java/org/ole/planet/myplanet/service/UploadToShelfService.kt
Show resolved
Hide resolved
app/src/main/java/org/ole/planet/myplanet/service/UploadToShelfService.kt
Show resolved
Hide resolved
app/src/main/java/org/ole/planet/myplanet/service/UploadToShelfService.kt
Show resolved
Hide resolved
app/src/main/java/org/ole/planet/myplanet/service/UploadToShelfService.kt
Show resolved
Hide resolved
app/src/main/java/org/ole/planet/myplanet/service/UploadToShelfService.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
myplanet/app/src/main/java/org/ole/planet/myplanet/service/UploadToShelfService.kt
Lines 279 to 285 in 8646b05
| val jsonElement = gson.toJsonTree(mutableObj) | |
| val jsonObject = jsonElement.asJsonObject | |
| val updateResponse = apiInterface.putDoc(header, "application/json", "${replacedUrl(model)}/_users/org.couchdb.user:${model.name}", jsonObject).execute() | |
| if (updateResponse.isSuccessful) { | |
| val updatedRev = updateResponse.body()?.get("rev")?.asString |
updateHealthData now calls dbService.realmInstance.executeTransaction { … } but never closes the Realm instance returned by realmInstance. Each call leaks a Realm.getDefaultInstance() and Android will eventually throw IllegalStateException: Realm instance has been leaked once the open instance limit is reached. Use dbService.realmInstance.use { it.executeTransaction { … } } or the provided withRealm helper so the instance is closed after the transaction.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| model.iterations = getString("iterations", fetchDataResponse.body()) | ||
|
|
||
| if (saveKeyIv(apiInterface, model, obj)) { | ||
| updateHealthData(realm, model) | ||
| updateHealthData(model.id, model._id) | ||
| } | ||
| } | ||
| } catch (e: IOException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Persist updated user flags back to Realm
The refactor now works on unmanaged copies of RealmUserModel, but updateExistingUser only mutates the copy (model._rev/model.isUpdated) without writing those changes back to Realm. Because the caller obtained its models via realm.copyFromRealm, the database never clears the isUpdated flag nor stores the new revision, so the same users will be re-sent on every sync and the local _rev stays stale. Wrap these assignments in a Realm transaction so the managed record is updated.
Useful? React with 👍 / 👎.
| val rev = createResponse.body()?.get("rev")?.asString | ||
| model._id = id | ||
| model._rev = rev | ||
| processUserAfterCreation(apiInterface, realm, model, obj) | ||
| processUserAfterCreation(apiInterface, model, obj) | ||
| } | ||
| } catch (e: IOException) { | ||
| e.printStackTrace() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Save new user metadata after single-user sync
When uploadSingleUserData creates a user, uploadNewUser and processUserAfterCreation populate _id, _rev, password keys and health references on the detached RealmUserModel copy but never persist those changes. The previous version modified a managed model inside a Realm transaction, so the database was updated. After this commit, the local realm still lacks the assigned CouchDB id and security fields, which breaks follow-up shelf/health uploads for that user. The helper should write the new values to Realm before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dbService.realmInstance.use { realm -> | ||
| realm.executeTransaction { | ||
| val managedModel = it.where(RealmUserModel::class.java).equalTo("id", model.id).findFirst() | ||
| managedModel?._id = id | ||
| managedModel?._rev = rev | ||
| managedModel?.password_scheme = passwordScheme | ||
| managedModel?.derived_key = derivedKey | ||
| managedModel?.salt = salt | ||
| managedModel?.iterations = iterations | ||
| if (keyIvResult) { | ||
| managedModel?.key = model.key | ||
| managedModel?.iv = model.iv | ||
| } | ||
| updateHealthData(managedModel?.id, managedModel?._id) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid starting a second Realm transaction inside an active one
Within uploadUserData the code updates the newly created user using realm.executeTransaction { … updateHealthData(managedModel?.id, managedModel?._id) } while updateHealthData itself now calls dbService.realmInstance.executeTransaction { … }. This nests a new write transaction (and opens a new Realm instance) while the original transaction is still running on the same thread. Realm forbids nested write transactions and will throw IllegalStateException: Cannot execute transaction while in a transaction, causing the sync to fail for every new user. The previous implementation reused the existing realm instance and avoided this error.
Useful? React with 👍 / 👎.
| val apiInterface = client?.create(ApiInterface::class.java) | ||
| mRealm = dbService.realmInstance | ||
|
|
||
| mRealm.executeTransactionAsync({ realm: Realm -> | ||
| val userModel = realm.where(RealmUserModel::class.java) | ||
| .equalTo("name", userName) | ||
| .findFirst() | ||
|
|
||
| if (userModel != null) { | ||
| try { | ||
| val password = SecurePrefs.getPassword(context, sharedPreferences) ?: "" | ||
| val header = "Basic ${Base64.encodeToString(("${userModel.name}:${password}").toByteArray(), Base64.NO_WRAP)}" | ||
| MainApplication.applicationScope.launch { | ||
| try { | ||
| val userModel = withContext(Dispatchers.IO) { | ||
| dbService.realmInstance.use { realm -> | ||
| val user = realm.where(RealmUserModel::class.java) | ||
| .equalTo("name", userName) | ||
| .findFirst() | ||
| user?.let { realm.copyFromRealm(it) } | ||
| } | ||
| } | ||
|
|
||
| val userExists = checkIfUserExists(apiInterface, header, userModel) | ||
| if (userModel != null) { | ||
| withContext(Dispatchers.IO) { | ||
| val password = SecurePrefs.getPassword(context, sharedPreferences) ?: "" | ||
| val header = "Basic ${Base64.encodeToString(("${userModel.name}:${password}").toByteArray(), Base64.NO_WRAP)}" | ||
| val userExists = checkIfUserExists(apiInterface, header, userModel) | ||
|
|
||
| if (!userExists) { | ||
| uploadNewUser(apiInterface, realm, userModel) | ||
| } else if (userModel.isUpdated) { | ||
| updateExistingUser(apiInterface, header, userModel) | ||
| if (!userExists) { | ||
| uploadNewUser(apiInterface, userModel) | ||
| } else if (userModel.isUpdated) { | ||
| updateExistingUser(apiInterface, header, userModel) | ||
| } | ||
| } | ||
| } catch (e: IOException) { | ||
| e.printStackTrace() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Persist single-user sync updates back to Realm
uploadSingleUserData copies the user from Realm (realm.copyFromRealm) and then calls uploadNewUser/updateExistingUser on that detached copy. Those helpers no longer write the updated _id, _rev, or isUpdated flags back into the database, so the local Realm is never updated after the network request. As a result, the same user will continue to appear as unsynced and shelf uploads will still see a null _id. The previous code operated on managed objects inside a transaction, so state was persisted. This path needs to write the modified user back to Realm after the network call.
Useful? React with 👍 / 👎.
Refactored the `UploadToShelfService` to use a coroutine-based approach for handling data uploads. This change separates network operations from Realm database transactions to prevent long-running transactions and improve performance. - Replaced `executeTransactionAsync` with `kotlinx.coroutines.launch` and `withContext(Dispatchers.IO)`. - Decoupled database reads, network calls, and database writes into distinct steps. - Removed the class-level `mRealm` instance in favor of short-lived, transaction-specific instances. - Updated `uploadUserData`, `uploadSingleUserData`, `uploadToShelf`, `uploadSingleUserToShelf`, `uploadHealth`, and `uploadSingleUserHealth` to follow the new architecture. - Fixed a regression where `uploadSingleUserData` was not persisting changes to the database.
| listener.onSuccess("Error during user data sync: ${e.localizedMessage}") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| managedModel?.iterations = iterations | ||
| if (keyIvResult) { | ||
| managedModel?.key = model.key | ||
| managedModel?.iv = model.iv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| managedModel?._rev = updatedRev | ||
| managedModel?.isUpdated = false | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| e.printStackTrace() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| }) { error -> | ||
| mRealm.close() | ||
| listener?.onSuccess("Error uploading health data for user $userId: ${error.localizedMessage}") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| e.printStackTrace() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored the
UploadToShelfServiceto use a coroutine-based approach for handling data uploads. This change separates network operations from Realm database transactions to prevent long-running transactions and improve performance.executeTransactionAsyncwithkotlinx.coroutines.launchandwithContext(Dispatchers.IO).mRealminstance in favor of short-lived, transaction-specific instances.uploadUserData,uploadSingleUserData,uploadToShelf,uploadSingleUserToShelf,uploadHealth, anduploadSingleUserHealthto follow the new architecture.https://jules.google.com/session/9907850505057940493