-
Notifications
You must be signed in to change notification settings - Fork 296
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
Resource consolidation for resources created with POST #2217
Merged
omarismail94
merged 28 commits into
google:master
from
anchita-g:resource_consolidation
Oct 26, 2023
Merged
Resource consolidation for resources created with POST #2217
omarismail94
merged 28 commits into
google:master
from
anchita-g:resource_consolidation
Oct 26, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Clean up SyncJobStatus and add FhirSynchronizer test * remove changes in MAVM * refactor more
anchita-g
changed the title
Resource consolidation
POST PerResource URL Request Resource consolidation
Sep 29, 2023
jingtang10
requested changes
Oct 4, 2023
...rc/main/java/com/google/android/fhir/sync/upload/consolidator/DefaultResourceConsolidator.kt
Outdated
Show resolved
Hide resolved
...va/com/google/android/fhir/sync/upload/consolidator/PostPerResourceUrlRequestConsolidator.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/sync/upload/consolidator/ResourceConsolidator.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt
Outdated
Show resolved
Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt
Show resolved
Hide resolved
anchita-g
changed the title
POST PerResource URL Request Resource consolidation
POST URL Request Resource consolidation
Oct 6, 2023
anchita-g
changed the title
POST URL Request Resource consolidation
Resource consolidation for resources created with POST
Oct 9, 2023
omarismail94
reviewed
Oct 9, 2023
engine/src/main/java/com/google/android/fhir/db/ResourceNotFoundException.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/JsonUtils.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/dao/LocalChangeDao.kt
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/dao/LocalChangeDao.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/dao/LocalChangeDao.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/dao/LocalChangeDao.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/JsonUtils.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/JsonUtils.kt
Outdated
Show resolved
Hide resolved
jingtang10
previously requested changes
Oct 11, 2023
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/LocalChangeDao.kt
Show resolved
Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt
Outdated
Show resolved
Hide resolved
engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt
Outdated
Show resolved
Hide resolved
...rc/main/java/com/google/android/fhir/sync/upload/consolidator/DefaultResourceConsolidator.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.
Just two minor comments!
engine/src/main/java/com/google/android/fhir/db/impl/dao/LocalChangeDao.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/dao/LocalChangeDao.kt
Outdated
Show resolved
Hide resolved
@anchita-g can you merge the master branch and resolve any conflicts? can approve once that is done and the comments are addressed |
omarismail94
approved these changes
Oct 26, 2023
omarismail94
dismissed
jingtang10’s stale review
October 26, 2023 14:44
Anchita has resolved all comments
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
One of the PRs to fix #760
Description
POST
. We would need to consolidate the IDs of the newly created resources in anyLocalChange
referring to these created resources as well in anyResource
referring to these created resources.LocalChangeResourceReferenceEntity
which maintains a mapping between aLocalChange
and any references to a resource the contents of theLocalChange
contains. This would help us in easily looking up all theLocalChange
s that need to be updated when a referred resource is assigned a new ID by the server. Similarly, we can use the mappedLocalChange
to get al the resources that refer to the said resource. These resources as well as their respective references would need to be updated as well.LocalChange
to the "references" present in the payload of theLocalChange
along with a test case to test the migration.Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?
Resource
or theLocalChange
of the resource referring to the created resource was to use the FHIR path that is mentioned in theLocalChangeResourceReferenceEntity
. However, in cases where there are lists of references, FHIR path does not indicate the index in the list at which the reference exists. In such cases we would have to resolve to updating the reference value by either replacing string value by regex match or use JSON update strategy. Similar case holds forLocalChange
where in for theUPDATE
typeLocalChange
, updating the reference using FHIR path would not be possible.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.