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

Bundle transaction processing should replace all local references, not just ones that start with "urn:" #2512

Closed
lmsurpre opened this issue Jun 15, 2021 · 9 comments
Assignees
Labels
enhancement New feature or request P2 Priority 2 - Should Have waiting-for-spec-clarification

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Jun 15, 2021

Is your feature request related to a problem? Please describe.
From https://www.hl7.org/fhir/http.html#trules

A transaction may include references from one resource to another in the bundle, including circular references where resources refer to each other. When the server assigns a new id to any resource in the bundle which has a POST method as part of the processing rules above, it SHALL also update any references to that resource in the same bundle as they are processed (see about Ids in a bundle). References to resources that are not part of the bundle are left untouched. Version-specific references should remain as version-specific references after the references have been updated. Note that when building a transaction, a client can use arbitrarily chosen version references since they will all be re-assigned anyway. Servers SHALL replace all matching links in the bundle, whether they are found in the resource ids, resource references, elements of type uri, url, oid, uuid, and <a href=“” & <img src=“” in the narrative. Elements of type canonical are not replaced.

Today, the IBM FHIR Server only updates literal references when they start with the "urn:" prefix.

Describe the solution you'd like
Update all literal references in the bundle when they match a fullUrl in the bundle that is associated with a resource being POSTed.

To determine whether a literal reference needs updating or not, we should follow the guidance at
https://www.hl7.org/fhir/bundle.html#references and I believe that we already have code for this.

Describe alternatives you've considered

Acceptance Criteria

  1. GIVEN a transaction bundle with resources A and B
    AND resource A is a POST
    AND resource B is a POST or PUT with a literal relative reference to the fullUrl for resource A
    WHEN the bundle is posted to the server
    THEN the literal reference in resource B is updated to the assigned resource id of resource A

  2. GIVEN a transaction bundle with resources A and B
    AND resource A is a POST
    AND resource B is a POST or PUT with a literal absolute reference to the fullUrl for resource A
    WHEN the bundle is posted to the server
    THEN the literal reference in resource B is updated to the assigned resource id of resource A

Additional context

@prb112 prb112 added the enhancement New feature or request label Jun 15, 2021
@lmsurpre lmsurpre added the P2 Priority 2 - Should Have label Sep 7, 2021
@punktilious
Copy link
Collaborator

This should be taken care of in #1869. Need to double-check the implementation.

@lmsurpre
Copy link
Member Author

lmsurpre commented Nov 8, 2021

#1869 was closed, but I don't think we got this one in there. Sound right, @punktilious ?

@michaelwschroeder michaelwschroeder self-assigned this Nov 15, 2021
@michaelwschroeder
Copy link
Contributor

The scope of this issue is to replace all local references (not just those starting with the urn: prefix) in elements of type Reference only. Issue #2993 has been opened to address replacing local references in the following element types, per the FHIR spec:

  • resource ids
  • elements of type uri, url, oid, uuid
  • <a href=“” and <img src=“” tags in the narrative

michaelwschroeder added a commit that referenced this issue Nov 17, 2021
Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>
michaelwschroeder added a commit that referenced this issue Nov 29, 2021
Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>
michaelwschroeder added a commit that referenced this issue Dec 3, 2021
Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>
lmsurpre pushed a commit that referenced this issue Dec 6, 2021
* Issue #2512 - Bundle processing replace all local references

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>

* Issue #2512 - address review comments

Signed-off-by: Mike Schroeder <mschroed@us.ibm.com>
@lmsurpre lmsurpre added this to the Sprint 2021-16 milestone Dec 6, 2021
@lmsurpre
Copy link
Member Author

lmsurpre commented Jan 14, 2022

I posted the following transaction bundle (adapted from the a smart health card example):

{
  "resourceType": "Bundle",
  "type": "transaction",
  "entry": [
    {
      "fullUrl": "resource:0",
			"request": {
        "method": "POST",
        "url": "Patient"
      },
      "resource": {
        "resourceType": "Patient",
        "name": [
          {
            "family": "Anyperson",
            "given": [
              "John",
              "B."
            ]
          }
        ],
        "birthDate": "1951-01-20"
      }
    },
    {
      "fullUrl": "resource:1",
			"request": {
				"method": "POST",
				"url": "Immunization"
			},
      "resource": {
        "resourceType": "Immunization",
        "status": "completed",
        "vaccineCode": {
          "coding": [
            {
              "system": "http://hl7.org/fhir/sid/cvx",
              "code": "207"
            }
          ]
        },
        "patient": {
          "reference": "resource:0"
        },
        "occurrenceDateTime": "2021-01-01",
        "performer": [
          {
            "actor": {
              "display": "ABC General Hospital"
            }
          }
        ],
        "lotNumber": "0000001"
      }
    },
    {
      "fullUrl": "resource:2",
			"request": {
				"method": "POST",
				"url": "Immunication"
			},
      "resource": {
        "resourceType": "Immunization",
        "status": "completed",
        "vaccineCode": {
          "coding": [
            {
              "system": "http://hl7.org/fhir/sid/cvx",
              "code": "207"
            }
          ]
        },
        "patient": {
          "reference": "resource:0"
        },
        "occurrenceDateTime": "2021-01-29",
        "performer": [
          {
            "actor": {
              "display": "ABC General Hospital"
            }
          }
        ],
        "lotNumber": "0000007"
      }
    }
  ]
}

I expected the transaction to be process and for the patient references in resource:1 and resource:2 to be replaced with the server-assigned id for resource:0.

Instead, I got a 400 Bad Request:

{
	"resourceType": "OperationOutcome",
	"issue": [
		{
			"severity": "fatal",
			"code": "invalid",
			"details": {
				"text": "FHIRProvider: Invalid reference value or resource type not found in reference value: &#39;resource:0&#39; for element: &#39;patient&#39; [Bundle.entry[1]]"
			},
			"expression": [
				"Bundle.entry[1]"
			]
		}
	]
}

I think what is happening is we're doing our reference type validation before rewriting the references.
I think that reference type checking is actually performed during our parse validation, which makes this a lot trickier.
We can verify this by configuring the server to skip reference type checking, but I don't think suggesting users to turn that off is a suitable solution.
We might need to make a change on this one.

@lmsurpre lmsurpre self-assigned this Jan 14, 2022
@lmsurpre lmsurpre removed this from the Sprint 2021-16 milestone Jan 14, 2022
@lmsurpre
Copy link
Member Author

If we agree that we should support transaction bundles like this one, here's the simplest solution I could think of:

  1. do the bundle parse without parse validation
  2. do the reference rewriting and ensure parse validation is ON when we reconstruct the objects with the new reference values

@lmsurpre
Copy link
Member Author

lmsurpre commented Jan 14, 2022

Robin brought up a different option which would be to make our parse validation smarter about what is a valid bundle...probably by making it conditional on the type of bundle (e.g. special reference type checking for bundles of type batch or transaction)

@lmsurpre
Copy link
Member Author

lmsurpre commented Feb 10, 2022

neither of those options are real simple/clean.

Bundle parse validation actually happens in the FHIRProvider.readFrom (a JAX-RS provider for the application/fhir media types). To skip parse validation there would require some special-case logic where we introspect the uriInfo and take special action on a post the base url. Bleh.

Making our parse validation smarter isn't a real simple win either because the ValidationSupport.checkReferenceType calls are in the individual resource types and backbone elements that have Reference data types and those methods are clueless about whether they happen to be getting parsed/validated as part of a larger batch/transaction bundle or not.

To be honest, the option I'm leaning most towards now is moving reference type checking from parse validaton to secondary validation (FHIRValidator)...then it should be easier to control the behavior using either of the options presented above.
@JohnTimm feel free to weigh in on this one if you're interested :-)

@lmsurpre
Copy link
Member Author

Before going much further with this, I decided to re-read the latest proposed wording on bundle reference resolution: https://jira.hl7.org/browse/FHIR-29271?focusedCommentId=183020&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-183020

To me, this wording makes it more clear that if you have a fullUrl that doesn't begin with http:, https:, or urn:, then there should be no expectation of reference replacement.

I decided to ask for further clarification at
https://chat.fhir.org/#narrow/stream/179166-implementers/topic/Allowed.20uri.20schemes.20for.20Bundle.2Eentry.2EfullUrl/near/271621146

lmsurpre added a commit that referenced this issue Feb 11, 2022
This commit also adds support for the `resource:` scheme in our Bundle
validation. This scheme is used in the SMART health cards spec and so I
think it makes sense to explicitly allow it.

This PR does *not* add support for reference rewriting for this
`resource:` scheme at present, but I think that is a logical follow-on
for issue #2512.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Feb 11, 2022
This commit also adds support for the `resource:` scheme in our Bundle
validation. This scheme is used in the SMART health cards spec and so I
think it makes sense to explicitly allow it.

This PR does *not* add support for reference rewriting for this
`resource:` scheme at present, but I think that is a logical follow-on
for issue #2512.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Feb 11, 2022
This commit also adds support for the `resource:` scheme in our Bundle
validation. This scheme is used in the SMART health cards spec and so I
think it makes sense to explicitly allow it.

This PR does *not* add support for reference rewriting for this
`resource:` scheme at present, but I think that is a logical follow-on
for issue #2512.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Feb 11, 2022
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 added a commit that referenced this issue Feb 14, 2022
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 a commit that referenced this issue Feb 15, 2022
issues #3216 and #2512 - support for alternative reference schemes and bundle ref resolution from FHIRPath ResolveFunction
@tbieste
Copy link
Contributor

tbieste commented Feb 21, 2022

Closing after successful QA testing with the following transaction bundle:

{
"resourceType": "Bundle",
"type": "transaction",
"entry": [
{
"fullUrl": "resource:0",
"request": {
"method": "POST",
"url": "Patient"
},
"resource": {
"resourceType": "Patient",
"name": [
{
"family": "Anyperson",
"given": [
"John",
"B."
]
}
],
"birthDate": "1951-01-20"
}
},
{
"fullUrl": "resource:1",
"request": {
"method": "POST",
"url": "Immunization"
},
"resource": {
"resourceType": "Immunization",
"status": "completed",
"vaccineCode": {
"coding": [
{
"system": "http://hl7.org/fhir/sid/cvx",
"code": "207"
}
]
},
"patient": {
"reference": "resource:0"
},
"occurrenceDateTime": "2021-01-01",
"performer": [
{
"actor": {
"display": "ABC General Hospital"
}
}
],
"lotNumber": "0000001"
}
},
{
"fullUrl": "resource:2",
"request": {
"method": "POST",
"url": "Immunization"
},
"resource": {
"resourceType": "Immunization",
"status": "completed",
"vaccineCode": {
"coding": [
{
"system": "http://hl7.org/fhir/sid/cvx",
"code": "207"
}
]
},
"patient": {
"reference": "resource:0"
},
"occurrenceDateTime": "2021-01-29",
"performer": [
{
"actor": {
"display": "ABC General Hospital"
}
}
],
"lotNumber": "0000007"
}
}
]
}

@tbieste tbieste closed this as completed Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P2 Priority 2 - Should Have waiting-for-spec-clarification
Projects
None yet
Development

No branches or pull requests

5 participants