Implement StructureMap-based extraction #566

ekigamba · 2021-06-21T16:20:19Z

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #543

Description

Refactor ResourceMapper#extract function to support both definition-based and StructureMap-based
Add interface to allow external implementation of fetching StructureMap
Add tests for StructureMap-based extraction
Add ca.uhn.hapi.fhir:org.hl7.fhir.validation:5.3.0 dependency for running tests
Change ResourceMapper#extract signature to return Bundle
Add overloaded ResourceMapper#extract method with extra StructureMapProvider param
Update ResourceMapperTest methods descriptions

Alternative(s) considered
No, this is a FHIR SDC extraction specification

Type
Feature

Screenshots (if applicable)
NA

Checklist

I have read and acknowledged the Code of conduct
I have read How to Contribute
I have read the Developer's guide
I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA
I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach
I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project
I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally
I have built and run the reference app(s) to verify my change fixes the issue and/or does not break the reference app(s)

jingtang10 · 2021-06-23T12:05:02Z

buildSrc/src/main/kotlin/Dependencies.kt

@@ -126,6 +128,7 @@ object Dependencies {
    const val material = "1.3.0"
    const val retrofit = "2.7.2"
    const val truth = "1.0.1"
+    const val hapiFhirValidations = "5.3.0"


alphabetical order please

also this can probably just be merged with the structures version number anyway?

Yea, I'll change that

jingtang10 · 2021-06-23T12:07:53Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+   * Extract a FHIR resource from the [questionnaire] and [questionnaireResponse].
   *
-   * This method assumes there is only one FHIR resource to be extracted from the given
-   * `questionnaire` and `questionnaireResponse`.
+   * This method only supports definition-based extraction. StructureMap-based extraction will be
+   * invoked if the [Questionnaire] declares a targetStructureMap extension. However, the returned
+   * [Bundle] will be empty since the [StructureMapProvider] is not provided
+   *
+   * @return [Bundle] containing the extracted [Resource]s or empty Bundle if the extraction fails.
+   * An exception might also be thrown in a few cases
+   */
+  fun extract(questionnaire: Questionnaire, questionnaireResponse: QuestionnaireResponse): Bundle {
+    return extract(questionnaire, questionnaireResponse, null)
+  }
+
+  /**
+   * Extract a FHIR resource from the [questionnaire] and [questionnaireResponse].
+   *
+   * This method supports both Definition-based and StructureMap-based extraction.
+   * StructureMap-based extraction will fail and an empty [Bundle] will be returned if the
+   * [structureMapProvider] is not passed.
   */
-  fun extract(questionnaire: Questionnaire, questionnaireResponse: QuestionnaireResponse): Base {
+  fun extract(
+    questionnaire: Questionnaire,
+    questionnaireResponse: QuestionnaireResponse,
+    structureMapProvider: StructureMapProvider?
+  ): Bundle {
+    return if (questionnaire.targetStructureMap == null)
+      extractByDefinitionBased(questionnaire, questionnaireResponse)
+    else extractByStructureMapBased(questionnaire, questionnaireResponse, structureMapProvider)
+  }


can you make the structure map provider an optional param in this API and merge these two functions?

Sorry, forgot about the Kotlin default param

jingtang10 · 2021-06-23T12:11:22Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+    structureMapProvider: StructureMapProvider?
+  ): Bundle {
+    if (structureMapProvider == null) return Bundle()
+    val pcm = FilesystemPackageCacheManager(true, ToolsVersion.TOOLS_VERSION)


Suggested change

val pcm = FilesystemPackageCacheManager(true, ToolsVersion.TOOLS_VERSION)

val packageCacheManager = FilesystemPackageCacheManager(true, ToolsVersion.TOOLS_VERSION)

jingtang10 · 2021-06-23T12:11:32Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+    val structureMapUrl = questionnaire.targetStructureMap ?: return Bundle()
+    val structureMap = structureMapProvider.getStructureMap(structureMapUrl)
+
+    val scu = StructureMapUtilities(contextR4)


Suggested change

val scu = StructureMapUtilities(contextR4)

val structureMapUtilities = StructureMapUtilities(contextR4)

jingtang10 · 2021-06-23T12:22:25Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+        isCanRunWithoutTerminology = true
+      }
+
+    val structureMapUrl = questionnaire.targetStructureMap ?: return Bundle()


this has already been checked in the wrapper function

jingtang10 · 2021-06-23T12:30:17Z

datacapture/src/test/java/com/google/android/fhir/datacapture/mapping/ResourceMapperTest.kt

@@ -461,7 +470,7 @@ class ResourceMapperTest {
  }

  @Test
-  fun `extract() should allow extracting with unanswered questions`() {
+  fun `extract() should allow extracting correct Patient resource with unanswered questions using definition-based extraction`() {


Suggested change

fun `extract() should allow extracting correct Patient resource with unanswered questions using definition-based extraction`() {

fun `extract() should perform definition-based extraction with unanswered questions `() {

jingtang10 · 2021-06-23T12:30:35Z

datacapture/src/test/java/com/google/android/fhir/datacapture/mapping/ResourceMapperTest.kt

-  private fun String.toDateFromFormatYyyyMmDd(): Date? {
-    return SimpleDateFormat("yyyy-MM-dd").parse(this)
+  @Test
+  fun `extract() should allow StructureMap-based extraction and return correct Patient resource`() {


Suggested change

fun `extract() should allow StructureMap-based extraction and return correct Patient resource`() {

fun `extract() should perform StructureMap-based extraction`() {

jingtang10 · 2021-06-23T12:31:45Z

datacapture/src/test/java/com/google/android/fhir/datacapture/mapping/ResourceMapperTest.kt

+        }
+      """.trimIndent()
+
+    val pcm = FilesystemPackageCacheManager(true, ToolsVersion.TOOLS_VERSION)


as before - avoid using abbreviations.

jingtang10 · 2021-06-23T13:58:47Z

datacapture/src/test/java/com/google/android/fhir/datacapture/mapping/ResourceMapperTest.kt

+    // Package name manually checked from
+    // https://simplifier.net/packages?Text=hl7.fhir.core&fhirVersion=All+FHIR+Versions


jingtang10 · 2021-06-23T14:00:16Z

datacapture/src/test/java/com/google/android/fhir/datacapture/mapping/ResourceMapperTest.kt

+    val contextR4 = SimpleWorkerContext.fromPackage(pcm.loadPackage("hl7.fhir.r4.core", "4.0.1"))
+    contextR4.isCanRunWithoutTerminology = true
+    val scu = StructureMapUtilities(contextR4)
+    val map: StructureMap =


did this map come from somewhere? or did you author this?

if it's an example from somewhere can you add a link? thanks!

I authored this

- Refactor ResourceMapper#extract function to support both definition-based and StructureMap-based - Add interface to allow external implementation of fetching StructureMap - Add tests for StructureMap-based extraction - Add ca.uhn.hapi.fhir:org.hl7.fhir.validation:5.3.0 dependency for running tests - Change ResourceMapper#extract signature to return Bundle - Add overloaded ResourceMapper#extract method with extra StructureMapProvider param - Update ResourceMapperTest methods descriptions Fixes #543

- Use local version of hl7.fhir.r4.core provided with the library aar - Use private app storage as the local cache of the NPM package - Add zipping utility used to decompress package into private app storage - Fix test names in ResourceMapper

jingtang10 · 2021-06-30T13:07:11Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+/**
+ * The map from the `name`s to `expression`s in the
+ * [item extraction context extension](http://build.fhir.org/ig/HL7/sdc/StructureDefinition-sdc-questionnaire-itemExtractionContext.html)
+ * s.
+ */
+val Questionnaire.targetStructureMap: String?
+  get() {
+    val extensionValue =
+      this.extension.singleOrNull { it.url == TARGET_STRUCTURE_MAP }?.value ?: return null
+    return if (extensionValue is CanonicalType) extensionValue.valueAsString else null
+  }
+


can you create a new file called MoreQuestionnaires.kt in the datacapture package?

jingtang10 · 2021-06-30T13:07:58Z

datacapture/src/test/java/com/google/android/fhir/datacapture/mapping/ResourceMapperTest.kt

 class ResourceMapperTest {
  @Test
-  fun extract() {
+  fun `extract() should allow perform definition-based extraction`() {


Suggested change

fun `extract() should allow perform definition-based extraction`() {

fun `extract() should perform definition-based extraction`() {

jingtang10 · 2021-06-30T13:12:35Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+  ): Bundle {
+    if (structureMapProvider == null || context == null) return Bundle()
+
+    // Load the npm package


this comment doesn't add much -- partly because the function has got a good name.

if you want to explain why this is needed, or how this is done, etc, please update this comment. otherwise, remove this comment if it's simply repeating the function name.

jingtang10 · 2021-06-30T13:27:53Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+  private fun extractByStructureMapBased(
+    questionnaire: Questionnaire,
+    questionnaireResponse: QuestionnaireResponse,
+    structureMapProvider: StructureMapProvider?,


Suggested change

structureMapProvider: StructureMapProvider?,

structureMapProvider: (String) -> StructureMap,

jingtang10 · 2021-06-30T13:28:02Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+interface StructureMapProvider {
+
+  fun getStructureMap(fullUrl: String): StructureMap?
+}


datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

- The SimpleWorkerContext will only be loaded once

- Move Questionnaire extension to MoreQuestionnaire.kt file - Add tests for MoreQuestionnaire.kt and NpmPackageProvider.kt

…tion

jingtang10 · 2021-07-07T15:29:59Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+    return if (questionnaire.targetStructureMap == null)
+      extractByDefinitionBased(questionnaire, questionnaireResponse)
+    else
+      extractByStructureMapBased(


Suggested change

extractByStructureMapBased(

extractByStructureMap(

Sounds better. Done

jingtang10 · 2021-07-07T15:30:59Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+    context: Context? = null
+  ): Bundle {
+    return if (questionnaire.targetStructureMap == null)
+      extractByDefinitionBased(questionnaire, questionnaireResponse)


Suggested change

extractByDefinitionBased(questionnaire, questionnaireResponse)

extractByDefinitions(questionnaire, questionnaireResponse)

Sounds better. Done

jingtang10 · 2021-07-07T15:32:44Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+    val structureMapUrl = questionnaire.targetStructureMap!!
+    val structureMap = structureMapProvider(structureMapUrl)


Suggested change

val structureMapUrl = questionnaire.targetStructureMap!!

val structureMap = structureMapProvider(structureMapUrl)

val structureMap = structureMapProvider(questionnaire.targetStructureMap!!)

jingtang10 · 2021-07-07T15:35:28Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+import org.hl7.fhir.r4.context.SimpleWorkerContext
+import org.hl7.fhir.utilities.npm.NpmPackage
+
+object NpmPackageProvider {


Can we leave this as public?

The whole initial setup process takes around 3 minutes on a good device and I plan to have this happening after installation and application start so that extraction is fast having done this previously.

The target application would have to setup the module after installation and load the modules at application start

3 minutes! that's mad... can you add a documentation here to say this process is very time consuming

I see this is is already documented here.

I'll add extra documentation on the specific methods and some guiding documentation on the class itself

general comment: the file IO functions should be suspend functions

jingtang10 · 2021-07-07T15:36:01Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+  lateinit var npmPackage: NpmPackage
+  lateinit var contextR4: SimpleWorkerContext
+
+  fun loadSimpleWorkerContextWithPackage(context: Context): SimpleWorkerContext {


Suggested change

fun loadSimpleWorkerContextWithPackage(context: Context): SimpleWorkerContext {

fun loadSimpleWorkerContext(context: Context): SimpleWorkerContext {

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

jingtang10 · 2021-07-07T15:38:43Z

datacapture/src/test/java/com/google/android/fhir/datacapture/MoreQuestionnairesKtTest.kt

+import org.hl7.fhir.r4.model.Questionnaire
+import org.junit.Test
+
+class MoreQuestionnairesKtTest {


Suggested change

class MoreQuestionnairesKtTest {

class MoreQuestionnairesTest {

Done, 🤦 my mistake

jingtang10 · 2021-07-07T15:39:27Z

...apture/src/test/java/com/google/android/fhir/datacapture/utilities/NpmPackageProviderTest.kt

+    Assert.assertThrows(UninitializedPropertyAccessException::class.java) {
+      NpmPackageProvider::contextR4.get()
+    }
+
+    val npmPackage = NpmPackageProvider.loadNpmPackage(ApplicationProvider.getApplicationContext())
+
+    NpmPackageProvider.loadSimpleWorkerContext(npmPackage)
+
+    Truth.assertThat(NpmPackageProvider.contextR4).isNotNull()


these are two test cases.

Kinda but logically it's one. The aim of the test is make sure that the method loadSimpleWorkerContext can initialize contextR4 when not previously initialized.

We need to confirm that contextR4 is not initialized before starting the test. Is this what you meant?

i think you can have a test case for the exception if you don't call the load function. and another test case for if you call the function.

these are fundamentally 2 different cases -- i think for unit tests we try to keep each test case really really simple and follows the 3 steps -> arrange act and assert.

here's some more background: https://livebook.manning.com/book/effective-unit-testing/chapter-4/112

also import assertThat

Cool, I'll remove the exception asserts in all the test cases and import assertThat

jingtang10 · 2021-07-07T15:39:39Z

...apture/src/test/java/com/google/android/fhir/datacapture/utilities/NpmPackageProviderTest.kt

+    Assert.assertThrows(UninitializedPropertyAccessException::class.java) {
+      NpmPackageProvider::npmPackage.get()
+    }
+
+    NpmPackageProvider.loadNpmPackage(ApplicationProvider.getApplicationContext())
+
+    Truth.assertThat(NpmPackageProvider.npmPackage).isNotNull()


again two test cases -- same below.

Kinda but logically it's one. The aim of the test is make sure that the method loadSimpleWorkerContext can initialize npmPackage when not previously initialized.

We need to confirm that npmPackage is not initialized before starting the test. Is this what you meant?

The same applies for the test below in loadSimpleWorkerContext() should cache SimpleWorkerContext

…tion

- The default package for the StructureMap is a Bundle. This is no longer inferred from the StructureMap. This helps support declaring multiple targets eg. Bundle, Patient, RelatedPerson

…tion

jingtang10

still prefer to split the tests -- see comments in file

…tion

jingtang10 · 2021-07-13T07:19:47Z

...e/src/main/java/com/google/android/fhir/datacapture/mapping/NpmPackageInitializationError.kt

+package com.google.android.fhir.datacapture.mapping
+
+/** Thrown to indicate that the FHIR core package could not be initialized successfully */
+class NpmPackageInitializationError : RuntimeException {


I feel this should be a checked exception rather than unchecked. What's the reason to choose RuntimeException instead of Exception?

...apture/src/test/java/com/google/android/fhir/datacapture/utilities/NpmPackageProviderTest.kt

jingtang10 · 2021-07-13T07:31:09Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+        packageDirectory.delete()
+      }
+
+      Log.e(ResourceMapper::class.java.name, e.stackTraceToString())


not sure we need this if throwing exception - also strange to use ResourceMapper class name.

jingtang10 · 2021-07-13T07:31:47Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/TarGzipUtility.kt

+ * Provides utility for decompressing tar.gz or tgz files using the commons-io and commons-compress
+ * libraries


Suggested change

* Provides utility for decompressing tar.gz or tgz files using the commons-io and commons-compress

* libraries

* Utilities for decompressing tar.gz or tgz files using the commons-io and commons-compress

* libraries.

jingtang10 · 2021-07-13T07:38:50Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/TarGzipUtility.kt

+
+  @Throws(IOException::class)
+  fun decompress(compressedFilePath: String?, out: File?) {
+    TarArchiveInputStream(GzipCompressorInputStream(FileInputStream(compressedFilePath))).use { fin


what does fin stand for? maybe use a more readable name?

Suggested change

TarArchiveInputStream(GzipCompressorInputStream(FileInputStream(compressedFilePath))).use { fin

TarArchiveInputStream(GzipCompressorInputStream(FileInputStream(compressedFilePath))).use { stream

jingtang10 · 2021-07-13T07:55:40Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+   * The whole process can take 1-2 minutes.
+   */
+  private fun setupNpmPackage(context: Context) {
+    val filename = "packages.fhir.org-hl7.fhir.r4.core-4.0.1.tgz"


make it a static const.

jingtang10 · 2021-07-13T07:56:27Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+      IOUtils.closeQuietly(inputStream)
+      IOUtils.closeQuietly(outputStream)


should be in finally { ... } block?

jingtang10 · 2021-07-13T07:59:44Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+    val structureMap = structureMapProvider(questionnaire.targetStructureMap!!)
+
+    val structureMapUtilities = StructureMapUtilities(contextR4)
+    val targetResource = Bundle()
+    structureMapUtilities.transform(contextR4, questionnaireResponse, structureMap, targetResource)
+    return targetResource


Suggested change

val structureMap = structureMapProvider(questionnaire.targetStructureMap!!)

val structureMapUtilities = StructureMapUtilities(contextR4)

val targetResource = Bundle()

structureMapUtilities.transform(contextR4, questionnaireResponse, structureMap, targetResource)

return targetResource

return Bundle().apply {

StructureMapUtilities(contextR4).transform(contextR4, questionnaireResponse, structureMapProvider(questionnaire.targetStructureMap!!), this)

}

jingtang10 · 2021-07-13T10:33:41Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+import org.hl7.fhir.r4.context.SimpleWorkerContext
+import org.hl7.fhir.utilities.npm.NpmPackage
+
+object NpmPackageProvider {


general comment: the file IO functions should be suspend functions

jingtang10 · 2021-07-13T10:36:31Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+  lateinit var npmPackage: NpmPackage
+  lateinit var contextR4: SimpleWorkerContext
+
+  fun loadSimpleWorkerContext(context: Context): SimpleWorkerContext {


if you call this function, you'll end up calling setupNpmPackage without checking if it's been loaded before?

Good question, setupNpmPackage does the check itself and only proceeds if not previously loaded

ekigamba · 2021-07-13T17:52:03Z

#566 (comment)

@jingtang10

general comment: the file IO functions should be suspend functions

Correct, we will generally want to change the ResourceMapper extraction functions to suspend functions

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

jingtang10 · 2021-07-15T10:18:59Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+      // Delete the folders
+      val packageDirectory = File(outDir)
+      if (packageDirectory.exists()) {
+        packageDirectory.delete()


can you handle it and add a logging statement?

jingtang10 · 2021-07-15T10:23:20Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+    val outDir =
+      Environment.getDataDirectory().getAbsolutePath() +
+        "/data/${context.applicationContext.packageName}/npm_packages/"
+    return outDir


Suggested change

val outDir =

Environment.getDataDirectory().getAbsolutePath() +

"/data/${context.applicationContext.packageName}/npm_packages/"

return outDir

return

Environment.getDataDirectory().getAbsolutePath() +

"/data/${context.applicationContext.packageName}/npm_packages/"

jingtang10 · 2021-07-15T10:23:42Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+    )
+  }
+
+  /** Generate the path to the local npm packages directory */


Suggested change

/** Generate the path to the local npm packages directory */

/** Generates the path to the local npm packages directory. */

same with other function documentation

jingtang10 · 2021-07-15T10:41:15Z

buildSrc/src/main/kotlin/Dependencies.kt

+  const val commonsCompress = "org.apache.commons:commons-compress:${Versions.commonsCompress}"
+  const val commonsIo = "commons-io:commons-io:${Versions.commonsIo}"


Suggested change

const val commonsCompress = "org.apache.commons:commons-compress:${Versions.commonsCompress}"

const val commonsIo = "commons-io:commons-io:${Versions.commonsIo}"

const val apacheCommonsCompress = "org.apache.commons:commons-compress:${Versions.apacheCommonsCompress}"

const val apacheCommonsIo = "commons-io:commons-io:${Versions.apacheCommonsIo}"

jingtang10 · 2021-07-15T10:51:46Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+    context: Context?
+  ): Bundle {
+    if (structureMapProvider == null || context == null) return Bundle()
+    val contextR4 = NpmPackageProvider.loadSimpleWorkerContext(context)


Suggested change

val contextR4 = NpmPackageProvider.loadSimpleWorkerContext(context)

val simpleWorkerContext = NpmPackageProvider.loadSimpleWorkerContext(context)

jingtang10 · 2021-07-15T10:54:13Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

+          contextR4,
+          questionnaireResponse,
+          structureMapProvider(questionnaire.targetStructureMap!!),
+          this


make it easier to read by annotating the params

Suggested change

contextR4,

questionnaireResponse,

structureMapProvider(questionnaire.targetStructureMap!!),

this

appInfo = simpleWorkerContext,

source = questionnaireResponse,

map = ...,

target = this

I just tried this and I get the error Named arguments are not allowed for non-Kotlin functions

in that case add comments:

/* appInfo= */ simpleWorkerContext, /* source= */ questionnaireResponse, ...

jingtang10 · 2021-07-15T10:55:37Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+   * loaded.
+   */
+  lateinit var npmPackage: NpmPackage
+  lateinit var contextR4: SimpleWorkerContext


Suggested change

lateinit var contextR4: SimpleWorkerContext

lateinit var simpleWorkerContext: SimpleWorkerContext

jingtang10 · 2021-07-15T10:58:24Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/TarGzipUtility.kt

+        if (entry.isDirectory) {
+          continue
+        }


hold on - is this going to create an infinite loop if the entry is directory?

Suggested change

if (entry.isDirectory) {

continue

}

if (entry.isDirectory) {

entry = stream.nextTarEntry

continue

}

Yea, nice catch

jingtang10 · 2021-07-15T11:03:22Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+import org.hl7.fhir.r4.context.SimpleWorkerContext
+import org.hl7.fhir.utilities.npm.NpmPackage
+
+/**


looking at this file i'm still slightly confused by the apis.

it still feels like it's doing too much. can we maybe have 2 classes

NpmPackageProvider

SimpleWorkerContextProvider

so in the former you have getNpmPackage(context: Context) and in the latter you have getSimpleWorkerContext(context: Context). The latter depends on the former. And both caches what needs to be cached within themselves.

I have split the functionalities into separate files

…ovider

jingtang10 · 2021-07-20T08:48:27Z

...e/src/main/java/com/google/android/fhir/datacapture/utilities/SimpleWorkerContextProvider.kt

+   *
+   * The operations takes 20 seconds to 1 minute.
+   */
+  fun loadSimpleWorkerContext(npmPackage: NpmPackage): SimpleWorkerContext {


can you just get rid of this function? (i.e. inline it)?

really feels like one of these two apis can go.

jingtang10 · 2021-07-20T08:52:17Z

...e/src/main/java/com/google/android/fhir/datacapture/utilities/SimpleWorkerContextProvider.kt

+   * The whole process can take 1-3 minutes on a clean installation, otherwise, it should take 20
+   * seconds to 1 minute .
+   */
+  fun loadSimpleWorkerContext(context: Context): SimpleWorkerContext {


these need to be suspend functions. do you plan to do this in a follow up PR?

jingtang10 · 2021-07-20T08:57:30Z

...e/src/main/java/com/google/android/fhir/datacapture/utilities/SimpleWorkerContextProvider.kt

+ */
+object SimpleWorkerContextProvider {
+
+  lateinit var simpleWorkerContext: SimpleWorkerContext


make this private -- the cache shouldn't be exposed.

jingtang10 · 2021-07-20T08:57:42Z

...c/test/java/com/google/android/fhir/datacapture/utilities/SimpleWorkerContextProviderTest.kt

+      )
+
+    assertThat(generatedSimpleWorkerContext)
+      .isEqualTo(SimpleWorkerContextProvider.simpleWorkerContext)


Suggested change

.isEqualTo(SimpleWorkerContextProvider.simpleWorkerContext)

.isEqualTo(SimpleWorkerContextProvider.loadSimpleWorkerContext())

jingtang10 · 2021-07-20T09:15:03Z

...apture/src/test/java/com/google/android/fhir/datacapture/utilities/NpmPackageProviderTest.kt

+import org.robolectric.annotation.Config
+import org.robolectric.util.ReflectionHelpers
+
+@RunWith(RobolectricTestRunner::class)


what is this class testing? is it not just testing your shadow?

Yea, at this point that's correct. I'm going to remove this and in case there any extra changes then you can point them out when you review again

jingtang10

Great work! 👌

jingtang10 · 2021-07-21T08:57:31Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/NpmPackageProvider.kt

+   * Call [loadNpmPackage] to load it. The method handles skips the operation if it's already
+   * loaded.
+   */
+  lateinit var npmPackage: NpmPackage


this needs to be private also

…tion

ekigamba · 2021-07-22T07:05:20Z

Great work!

Thanks 🙏

ekigamba force-pushed the ek/sdc-structure-map-extraction-implementation branch 2 times, most recently from 6e00788 to 323ffaf Compare June 21, 2021 17:21

ekigamba marked this pull request as ready for review June 22, 2021 08:01

ekigamba requested review from aditya-07, deepankarb, jingtang10, joiskash, kevinmost, pankajkatoch and yigit as code owners June 22, 2021 08:01

ekigamba force-pushed the ek/sdc-structure-map-extraction-implementation branch from 976ead4 to 73748c5 Compare June 22, 2021 13:51

ekigamba mentioned this pull request Jun 23, 2021

Integrate StructureMap-based support opensrp/fhircore#307

Closed

jingtang10 requested changes Jun 23, 2021

View reviewed changes

ekigamba requested a review from stevenckngaa as a code owner June 29, 2021 22:16

ekigamba added 4 commits June 30, 2021 01:18

Code cleanup

06fe548

Code cleanup

4a94690

ekigamba force-pushed the ek/sdc-structure-map-extraction-implementation branch from 7eee7b5 to 890f9aa Compare June 29, 2021 22:26

Fix spotless issues

afd1a26

jingtang10 requested changes Jun 30, 2021

View reviewed changes

ekigamba and others added 4 commits June 30, 2021 21:44

Improve StructureMap-based performance by caching SimpleWorkerContext

aa746bf

- The SimpleWorkerContext will only be loaded once

Move npm package utility to NpmPackageProvider

3e8609e

- Move Questionnaire extension to MoreQuestionnaire.kt file - Add tests for MoreQuestionnaire.kt and NpmPackageProvider.kt

Merge branch 'master' into ek/sdc-structure-map-extraction-implementa…

d6fc9ab

…tion

Merge branch 'master' into ek/sdc-structure-map-extraction-implementa…

712aa4b

…tion

ekigamba requested a review from jingtang10 July 5, 2021 20:13

Merge branch 'master' into ek/sdc-structure-map-extraction-implementa…

39201b3

…tion

jingtang10 requested changes Jul 7, 2021

View reviewed changes

ekigamba added 2 commits July 9, 2021 10:57

Merge branch 'master' into ek/sdc-structure-map-extraction-implementa…

214e34c

…tion

Rename ResourceMapper and NpmPackageProvider methods, and a test

d535675

ekigamba force-pushed the ek/sdc-structure-map-extraction-implementation branch from 980799a to d535675 Compare July 9, 2021 09:34

ekigamba requested a review from jingtang10 July 9, 2021 13:27

ekigamba and others added 2 commits July 9, 2021 17:04

Add support for multiple targets in StructureMap extraction

134e7ae

- The default package for the StructureMap is a Bundle. This is no longer inferred from the StructureMap. This helps support declaring multiple targets eg. Bundle, Patient, RelatedPerson

Merge branch 'master' into ek/sdc-structure-map-extraction-implementa…

449cb92

…tion

jingtang10 requested changes Jul 9, 2021

View reviewed changes

ekigamba added 2 commits July 12, 2021 14:03

Add NpmPackageProvider docs and update the test

30feb3f

Merge branch 'master' into ek/sdc-structure-map-extraction-implementa…

bccfb2c

…tion

ekigamba requested a review from jingtang10 July 12, 2021 14:03

jingtang10 requested changes Jul 13, 2021

View reviewed changes

ekigamba mentioned this pull request Jul 13, 2021

Add Related persons to patient registration opensrp/fhircore#390

Merged

5 tasks

ekigamba force-pushed the ek/sdc-structure-map-extraction-implementation branch from 6c55329 to e3f77f1 Compare July 13, 2021 19:23

Code cleanup

cc9f44e

ekigamba force-pushed the ek/sdc-structure-map-extraction-implementation branch from e3f77f1 to cc9f44e Compare July 13, 2021 20:44

jingtang10 requested changes Jul 15, 2021

View reviewed changes

ekigamba added 2 commits July 15, 2021 19:40

Code cleanup

e9015c8

Move some NpmPackageProvider functionality into SimpleWorkerContextPr…

d8b9cb3

…ovider

ekigamba force-pushed the ek/sdc-structure-map-extraction-implementation branch from d8066ef to d8b9cb3 Compare July 16, 2021 15:43

ekigamba mentioned this pull request Jul 19, 2021

Add related person fields to registration questionnaire opensrp/fhircore#184

Closed

1 task

Create SimpleWorkerContextProvider test

8c1354c

jingtang10 reviewed Jul 20, 2021

View reviewed changes

Remove redundant functions and tests

87812f0

jingtang10 approved these changes Jul 21, 2021

View reviewed changes

ekigamba and others added 2 commits July 21, 2021 12:16

Code cleanup

283ac5f

Merge branch 'master' into ek/sdc-structure-map-extraction-implementa…

b4bc795

…tion

ekigamba merged commit bac35db into master Jul 21, 2021

ekigamba deleted the ek/sdc-structure-map-extraction-implementation branch July 21, 2021 11:14

jingtang10 assigned ekigamba Jul 22, 2021

stevenckngaa mentioned this pull request Dec 9, 2021

Validate the size of FHIR library doesn't exceed a predefined threshold #958

Open

	val pcm = FilesystemPackageCacheManager(true, ToolsVersion.TOOLS_VERSION)
	val packageCacheManager = FilesystemPackageCacheManager(true, ToolsVersion.TOOLS_VERSION)

	val scu = StructureMapUtilities(contextR4)
	val structureMapUtilities = StructureMapUtilities(contextR4)

	fun `extract() should allow extracting correct Patient resource with unanswered questions using definition-based extraction`() {
	fun `extract() should perform definition-based extraction with unanswered questions `() {

	fun `extract() should allow StructureMap-based extraction and return correct Patient resource`() {
	fun `extract() should perform StructureMap-based extraction`() {

		// Package name manually checked from
		// https://simplifier.net/packages?Text=hl7.fhir.core&fhirVersion=All+FHIR+Versions

	fun `extract() should allow perform definition-based extraction`() {
	fun `extract() should perform definition-based extraction`() {

	structureMapProvider: StructureMapProvider?,
	structureMapProvider: (String) -> StructureMap,

	extractByDefinitionBased(questionnaire, questionnaireResponse)
	extractByDefinitions(questionnaire, questionnaireResponse)

		val structureMapUrl = questionnaire.targetStructureMap!!
		val structureMap = structureMapProvider(structureMapUrl)

	fun loadSimpleWorkerContextWithPackage(context: Context): SimpleWorkerContext {
	fun loadSimpleWorkerContext(context: Context): SimpleWorkerContext {

	class MoreQuestionnairesKtTest {
	class MoreQuestionnairesTest {

		* Provides utility for decompressing tar.gz or tgz files using the commons-io and commons-compress
		* libraries

	TarArchiveInputStream(GzipCompressorInputStream(FileInputStream(compressedFilePath))).use { fin
	TarArchiveInputStream(GzipCompressorInputStream(FileInputStream(compressedFilePath))).use { stream

		IOUtils.closeQuietly(inputStream)
		IOUtils.closeQuietly(outputStream)

	/** Generate the path to the local npm packages directory */
	/** Generates the path to the local npm packages directory. */

		const val commonsCompress = "org.apache.commons:commons-compress:${Versions.commonsCompress}"
		const val commonsIo = "commons-io:commons-io:${Versions.commonsIo}"

	val contextR4 = NpmPackageProvider.loadSimpleWorkerContext(context)
	val simpleWorkerContext = NpmPackageProvider.loadSimpleWorkerContext(context)

-          contextR4,
-          questionnaireResponse,
-          structureMapProvider(questionnaire.targetStructureMap!!),
-          this
+          appInfo = simpleWorkerContext,
+          source = questionnaireResponse,
+          map = ...,
+          target = this

	lateinit var contextR4: SimpleWorkerContext
	lateinit var simpleWorkerContext: SimpleWorkerContext

	.isEqualTo(SimpleWorkerContextProvider.simpleWorkerContext)
	.isEqualTo(SimpleWorkerContextProvider.loadSimpleWorkerContext())

Implement StructureMap-based extraction #566

Implement StructureMap-based extraction #566

Conversation

ekigamba commented Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekigamba Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekigamba Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekigamba Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingtang10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekigamba commented Jun 21, 2021 •

edited

Loading

ekigamba Jul 9, 2021 •

edited

Loading

ekigamba Jul 12, 2021 •

edited

Loading

ekigamba Jul 9, 2021 •

edited

Loading