issues #3216 and #2512 - support for alternative reference schemes and bundle ref resolution from FHIRPath ResolveFunction #3335
Conversation
fhir-path/src/main/java/com/ibm/fhir/path/function/ResolveFunction.java
Outdated
Show resolved
Hide resolved
lmsurpre
force-pushed
the
issue-3216
branch
2 times, most recently
from
7873aea to
5903ddc
Compare
In addition, this commit adds support for the `resource:` scheme in our Reference type checking / reference value validation. This scheme is used in the SMART health cards spec and so I think it makes sense to explicitly allow it. I thought that I would need to do additional logic for supporting reference rewriting for this scheme but, based on the test I added to FHIRRestHelperTest, apparently not...we just needed to let it past our parser. Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre
changed the title
resource: scheme and bundle ref resolution from FHIRPathResolve
fhir-path/src/test/java/com/ibm/fhir/path/test/ResolveInternalFragmentReferenceTest.java
Show resolved
Hide resolved
| bundleResources.put(e.getFullUrl().getValue(), e.getResource()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
While this is rare, what happens in a bundle contained in a bundle
Bundle.Entry.Resource.contained.Resource
or
Bundle.Entry.Resource where that is a Bundle
My suspicion is that it could end up processing them as bundled elements.
Is that OK?
There was a problem hiding this comment.
my assumption is that we set the rootResource to the outermost bundle (the one we're evaluating against).
i tried to account for the "bundle-in-a-bundle" case in the algorithm below by continuing to loop until we've reached the top of the tree. I'll add a single test case to double-check it.
There was a problem hiding this comment.
We have some helper functions (in FHIRPathUtil) that get the right resource depending on how it might be nested. These are typically stashed in the EvaluationContext.
There was a problem hiding this comment.
I added the test. I can think of a couple "interesting edge cases" but they seem so edge to me that I don't care so much about what our behavior is there.
-
what to do if we have a Bundle thats contained within some other resource that is not a bundle (and that outer resource is the root/context). With the current impl, we will not do local bundle resolution in that case, which seems fine to me.
-
what to do if we have a relative literal reference (e.g.
Patient/1) in a Bundle.entry where that Bundle is inside another Bundle but the outer Bundle has no fullUrl for this entry. in this scenario, I think the current algorithm will use the fullUrl from the inner bundle's entry to convert the relative URL into an absolute one. I don't know if that behavior is desired or not, but again it seems like such an edge case that the impl seems ok to me
| if (isBundleContext) { | ||
| // we know the root of the tree is a Bundle and the current node is of type Reference, | ||
| // so walk up the tree until we get to highest Bundle.entry.fullUrl in the tree | ||
| // and save the value of its fullUrl |
There was a problem hiding this comment.
While the comment really does help... .
Could you layout a small example? or link to a test example?
If I have to maintain it, it'd help to laser focus on the 'aaah-ha' that's what it does.
There was a problem hiding this comment.
I added some examples to this comment (and also a test case for the Bundle-in-Bundle case)
| // relative reference | ||
| resource = resolveRelativeReference(evaluationContext, node, resourceType, matcher.group(LOGICAL_ID_GROUP), matcher.group(VERSION_ID_GROUP)); | ||
|
|
||
| String bundleReference = FHIRUtil.buildBundleReference(reference, fullUrl); |
There was a problem hiding this comment.
Note for others reviewing... when fuller is null, it'll return the referenceUriString = ref.getReference().getValue();
when this condition is true (ref.getReference() != null && ref.getReference().getValue() != null)
There was a problem hiding this comment.
I assume fuller is a typo and should be fullUrl, but yes that sounds right. FHIRUtil.buildBundleReference wasn't well-documented and so I included some javadoc updates for that method as part of this one
Per discussion at https://chat.fhir.org/#narrow/stream/179166-implementers/topic/Allowed.20uri.20schemes.20for.20Bundle.2Eentry.2EfullUrl.20and.20referenc.2E.2E.2E/near/271636029 We'll still reject literal reference values that begin or end with `:` and anything that looks like a "relative" URL path but doesn't follow the FHIR REST URL Pattern (e.g. a literal value like "123" with no scheme prefix). I also provided some algorithm examples in a comment and added a single local Bundle reference resolution test for the Bundle-in-a-Bundle case. Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre
added
the
removal
lmsurpre
changed the title
resource: scheme and bundle ref resolution from FHIRPathResolve
fhir-model/src/test/java/com/ibm/fhir/model/util/test/ReferenceMappingVisitorTest.java
Show resolved
Hide resolved
I also added some comments to ResolveFunctionTest to address PR feedback Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
This PR adds support for alternative schemes, like
resource:, in ourReference type checking / reference value validation.
Previously, we required reference values to use the
http:,https:, orurn:schemes,but that prevented us from supporting alternative schemes like
resource:which isused in the SMART health cards spec.
I thought that I would need to do additional logic for supporting
reference rewriting for this scheme but, based on the tests I added in
FHIRRestHelperTest and ReferenceMappingVisitorTest, apparently not
...we just needed to let it past our parser.
I also included updates to our FHIRPath ResolveFunction implementation so that we can more easily
resolve local bundle references from FHIRPath.
Signed-off-by: Lee Surprenant lmsurpre@us.ibm.com