google / android-fhir

The Android FHIR SDK is a set of Kotlin libraries for building offline-capable, mobile-first healthcare applications using the HL7® FHIR® standard on Android.
https://google.github.io/android-fhir/
Apache License 2.0
465 stars 246 forks source link

Update the purge API to accept a list of resource IDs #2439

Closed dubdabasoduba closed 2 months ago

dubdabasoduba commented 4 months ago

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

MJ1998 commented 4 months ago

Thanks Benjamin, This seems to be correctly captured. @aditya-07 To take over from here!

jingtang10 commented 4 months ago

@dubdabasoduba @f-odhiambo @ndegwamartin can you please propose someone to work on this? thanks!

ndegwamartin commented 3 months ago

@jingtang10 @aditya-07 in the context of this change, had a few points for clarification.

  1. Since we are in v1.0.0 of the Engine module and this is a breaking change - do we mark the FhirEngine.purge and the Database.purge as deprecated or should we overload the method?
  2. Since the plan is to iterate through the list of ids in the Database.Purge, how should we handle cases where processing of individual ids fail? i.e. should we let the ones that pass the checks purge successfully or should we consider processing the whole list a transaction?
ndegwamartin commented 3 months ago

Based on earlier discussions:

  1. I've overloaded the FhirEngine.purge and modified Database.purge since has access modifier internal which is reused by the FhirEngine overloads.
  2. I've wrapped the whole list in a transaction

PR linked here https://github.com/google/android-fhir/pull/2462 . We can iterate on any feedback

dubdabasoduba commented 3 months ago

Since the plan is to iterate through the list of IDs in the Database. Purge, how should we handle cases where the processing of individual IDs fails? i.e. should we let the ones that pass the checks purge successfully or should we consider processing the whole list as a transaction?

@ndegwamartin I think we should let the resources that pass the checks purge successfully. We can ensure the SDK reports the resources that failed to purge so the implementing apps know how to handle the failures.

ndegwamartin commented 2 months ago

@dubdabasoduba after the conversation today we've agreed to wrap the whole batch list processing logic in a transaction. Thus if any item fails it throws an IllegalStateException. cc @MJ1998