Skip to content
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

Added FhirDal for File System IO #376

Merged
merged 13 commits into from
Aug 16, 2022
Merged

Added FhirDal for File System IO #376

merged 13 commits into from
Aug 16, 2022

Conversation

Jinip
Copy link
Contributor

@Jinip Jinip commented Jul 6, 2022

File system IO implementation of FhirPlatform pattern from Clinical_Quality_Language

https://github.com/cqframework/clinical_quality_language/tree/master/Src/java/cqf-fhir/src/main/java/org/cqframework/fhir/api

  • Github Issue:
  • I've read the contribution guidelines
  • Code compiles without errors
  • Tests are created / updated
  • Documentation is created / updated

By creating this PR you acknowledge that your contribution will be licensed under Apache 2.0

@Jinip Jinip requested review from brynrhodes and JPercival July 6, 2022 20:50
@Jinip Jinip closed this Jul 6, 2022
@Jinip Jinip reopened this Jul 8, 2022
@brynrhodes
Copy link
Member

So the implementation looks good, but is assuming the "folder-per-resource-type" strategy, whereas the current implementation supports a more flexible strategy of just "anything in the directory or any sub-directory, including bundled content". Is this something we should support? It definitely adds complexity, especially on the write side, but...

@JPercival
Copy link
Collaborator

It definitely adds complexity, especially on the write side, but..

There would have to be some sort of configuration for the Dal in order to support this in any consistent way. You couldn't know based on just resource type and Id whether to put a resource in the folder, in the Bundle, or in a sub-directory.

whereas the current implementation supports a more flexible strategy of just "anything in the directory or any sub-directory, including bundled content"

Is this true of the IG Publisher and the Refresh operations, or of only the CQL evaluation? These are two slightly different concerns. Actually, for external ValueSets we specifically do not recurse subdirectories when building IGs, right?

This behavior also causes plenty of issues. At least twice in the past week (once on the opioid IG project) a Bundle included in the resource directory caused the "expected Singleton but found List" issue when evaluating CQL due to having multiple Patient resources. Does that extra configuration and flexibility justify the extra development now and debugging time later on IGs?

Given the above, removing the behavior might fix more things than it breaks. 🥲 The sample-content-ig also models the resource-per-folder behavior for new users, right?

@brynrhodes
Copy link
Member

The publisher just has any number of configurable resource directories, it's up to the ig author whether to organize them in to directories. But no, it definitely doesn't recurse, and we rely on that behavior for the current functionality. So...maybe we have a BundleResourceProvider that can use a FileResourceProvider? And the FileResourceProvider is just a directory, and has a configurable recurse option. And then there is a PriorityResourceProvider that can aggregate resource providers?

@brynrhodes
Copy link
Member

Maybe we put that in as a separate story, that's fair, but I'd still prefer to see the strict resource directory behavior, rather than assuming directories per resource type, that's something the publisher doesn't do, and we have been bitten by.

@brynrhodes
Copy link
Member

And it's only the write that is introducing the complexity, so maybe we just document that the "recursive" strategy can establish a file location for updates, but the inserts will always occur at the configured directory, rather than trying to figure out what folder to put new files in.

@brynrhodes
Copy link
Member

Okay, having said all that, we should keep this layer as simple as possible. So maybe this is just the behavior of the FileFhirPlatform, and we introduce other "readers" that can deal with bundles and recursive directories, and those are readonly implementations.

@JPercival
Copy link
Collaborator

Given a file that's read at one directory and written to another, does the tooling delete the first file?

@JPercival
Copy link
Collaborator

Maybe we introduce the notion of ResourceSets based on the configuration of the IG? Each ResourceSet has an associated FHIRDal. Existing Resources are written to wherever they are, new Resources relevant to a specific set are written to the same FHIRDal, and new IG-wide Resources are written to a default ResourceSet that uses the existing convention?

@brynrhodes
Copy link
Member

@JPercival
Copy link
Collaborator

Do we think that "Set" or "Atlas" concept combined with a directory Dal or a Bundle Dal (depending on whatever the structure for a given output needs to be) is sufficient for the refresh use cases?

@JPercival JPercival merged commit 8f97461 into master Aug 16, 2022
@JPercival JPercival deleted the FileFhirPlatform branch August 16, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants