Skip to content

Replaces monarchy.doWithRealm with monarchy.awaitTransaction #5792

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

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented Apr 20, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Replaces monarchy.doWithRealm with monarchy.awaitTransaction

Motivation and context

This fixes a bug that was crashing the whole sync

Screenshots / GIFs

Tests

Test that #5618 still works and no errors are thrown as a result of the sync

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 10

Checklist

@ericdecanini ericdecanini added the PR-Small PR with less than 20 updated lines label Apr 20, 2022
@@ -93,7 +93,7 @@ internal class SyncResponsePostTreatmentAggregatorHandler @Inject constructor(

private fun List<User>.saveLocally() {
val userEntities = map { user -> UserEntityFactory.create(user) }
monarchy.doWithRealm {
monarchy.writeAsync {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be awaitTransaction to force the suspend flow to be synchronous?

I'm wondering if logic further down the flow is expecting these database values to be set

Copy link
Member

Choose a reason for hiding this comment

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

We should have completed this before the next sync yeah

Copy link
Member

Choose a reason for hiding this comment

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

(even if technically we can only have one transaction at a time)

@ericdecanini ericdecanini requested a review from ganfra April 20, 2022 09:11
@ericdecanini ericdecanini changed the title Replaces monarchy.doWithRealm with monarchy.writeAsync Replaces monarchy.doWithRealm with monarchy.awaitTransaction Apr 20, 2022
@github-actions
Copy link

Unit Test Results

114 files  ±0  114 suites  ±0   1m 24s ⏱️ ±0s
202 tests ±0  202 ✔️ ±0  0 💤 ±0  0 ±0 
678 runs  ±0  678 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 8eab24c. ± Comparison against base commit e0556cd.

@ouchadam ouchadam added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Apr 20, 2022
Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

LGTM

@ouchadam ouchadam merged commit 7c7822a into develop Apr 20, 2022
@ouchadam ouchadam deleted the bugfix/eric/realm-write-async branch April 20, 2022 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Small PR with less than 20 updated lines Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants