-
Notifications
You must be signed in to change notification settings - Fork 293
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
Add resource UUID to LocalChangeEntity, fixing changing UUID issue for the same resource #2210
Add resource UUID to LocalChangeEntity, fixing changing UUID issue for the same resource #2210
Conversation
bff2996
to
d251173
Compare
d251173
to
e1c351c
Compare
engine/src/androidTest/java/com/google/android/fhir/db/impl/ResourceDatabaseMigrationTest.kt
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/entities/LocalChangeEntity.kt
Outdated
Show resolved
Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/ResourceDatabaseMigrationTest.kt
Outdated
Show resolved
Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/ResourceDatabaseMigrationTest.kt
Outdated
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.
I don't understand why the alternative solution won't work.
In case of locally deleted resource, we can remove resourceId and still fetch the mapped ResourceEntity via LocalChangeEntity.resourceUuid.
engine/src/main/java/com/google/android/fhir/db/impl/ResourceDatabase.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/entities/LocalChangeEntity.kt
Outdated
Show resolved
Hide resolved
For the DELETE resource event, engine removes its entry from ResourceEntity and adds a corresponding DELETE entry into the LocalChangeEntity. Hence, to sync the change to sever, LocalChangeEntity should have the resourceID so that it can be uploaded / deleted properly on the server. |
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.
LGTM
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.
LGTM!
@anchita-g have you tested the db migration on a real device? thanks! |
Hello, yes I was able to test the db migration on a real device using the demo application. It works seamlessly and I was able to see the new column in the |
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #2209
Description
ResourceDatabaseMigrationTest
to use the migrated database for the particular implemented migration to run assertionsAlternative(s) considered
One alternative in the first scenario was to just keep the resourceUuid and remove resourceId. However, when LocalChanges are uploaded to the server, we would need to look up the ResourceEntity using the resourceUuid in the LocalChangeEntity to obtain the server recognisable identifier of the resource. This would be a problem in case of deleted resources, where in for any LocalChange of a resource that has been deleted, we would not be able to find the corresponding ResourceEntity. Hence, keeping resourceId is also crucial
Type
Choose one: Feature
Screenshots (if applicable)
Checklist
./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the style guide of this project../gradlew check
and./gradlew connectedCheck
to test my changes locally.