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

Spec-compliant whole-system history #2026

Closed
lmsurpre opened this issue Mar 5, 2021 · 5 comments
Closed

Spec-compliant whole-system history #2026

lmsurpre opened this issue Mar 5, 2021 · 5 comments
Assignees
Labels
break-change-api enhancement New feature or request P2 Priority 2 - Should Have

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Mar 5, 2021

Is your feature request related to a problem? Please describe.
For #1955 we added support for a high-performance changes feed. Originally, Robin was going to implement that feature as a custom operation, but I convinced him that it was so close to the whole-system history interaction that we should just serve it from that endpoint.

From my analysis, the implementation is super close to a compliant whole-system history interaction. However, there are just a couple aspects where we're out of compliance:

  1. We violate
    bdl-8 | Rule | Bundle.entry | fullUrl cannot be a version specific reference | fullUrl.contains('/_history/').not()

http://www.hl7.org/fhir/bundle.html#history says:

Each entry element SHALL contain a request element that describes the change that was made and, if the method is a POST or PUT, a resource that represents the state of the resource at the conclusion of the operation. A response element SHALL also be present so that consumers can access the location header.

I think that using _history in the fullUrl was a conscious decision to avoid duplicating this long string in the response.header, but I think we should revisit that decision (and we need to if we want to be spec-compliant).

  1. We've chosen to ignore this part of the spec:

From https://www.hl7.org/fhir/R4/http.html#history

If the entry.request.method is a PUT or a POST, the entry SHALL contain a resource.

  1. Our history implementation goes "forward" in time, but the spec describes (somewhat poorly) that the history interaction should go "backward" in time:
  • A call like GET [base]/_history might be expected to return the latest set of changes; ours returns the oldest set of changes
  • The next link should point to the next oldest set of changes (going backwards in time), but our next link actually goes to the next newest size of changes

Describe the solution you'd like
To bring our implementation into compliance, while still supporting this high-performance variant for consumers that need it, I propose that we allow clients to opt-in to the behavior that they want.

For number 1 (bdl-8), start including Bundle.entry.response.location with the value that we currently put in fullUrl. In the fullUrl, drop the /_history/:vid suffix.

For number 2 (inclusion of the resource bodies), they can opt-in by passing us a return preference via the HTTP Prefer header.

This builds on the use of "Prefer: return" as documented for create/update/patch/transaction, which lists the following allowed values:

  • Prefer: return=minimal
  • Prefer: return=representation
  • Prefer: return=OperationOutcome

In the case of History, I don't think the OperationOutcome return preference makes much sense, but just allowing minimal vs representation seems like a way to let the client to decide whether they are interested in a summary of the changes or the specific contents of the resources that changed.

My proposal is to make return=representation the default for all history interactions (for both whole-system or resource-instance level), but to use the high-performance implementation (with no return resource) in the case they pass Prefer: return=minimal.

For number 3 (ordering of the results), I'd like to introduce a new _sort parameter.

  • For the current ("changes" feed) behavior, users would pass _sort=forward.
  • For the spec-defined behavior (history operation), users would pass _sort=backward (which is also the default)
  • For an even more optimized version of the changes feed, users would pass _sort=system (which allows the server to sort the resources in whatever order it wants)

UPDATE: FHIR has agreed to adopt the following variants, so we should use these instead:

  • _sort=_lastUpdated
  • _sort=-_lastUpdated (default)
  • _sort=none - data will have no defined sort order

Describe alternatives you've considered
Move our "changes" feed to a custom operation...like Robin had originally planned to do.

Acceptance Criteria
At least one acceptance criteria is included.

GIVEN a _history request
WHEN no Prefer header is passed
THEN the response returns the resource contents in the response entries (spec-compliant)

GIVEN a _history request
WHEN Prefer: return=minimal is passed
THEN the response returns no resource bodies in the response entries

GIVEN a _history request
WHEN Prefer: return=representation is passed
THEN the response returns the resource contents in the response entries

Additional context
We raised the sort ordering stuff at https://chat.fhir.org/#narrow/stream/179166-implementers/topic/Ordering.20for.20the.20whole-system.20history.20interaction

We may want to open a corresponding issue.

Similarly, we may want to share our idea for excluding the resource types with the rest of the FHIR community.

@prb112 prb112 added enhancement New feature or request fhir-operation and removed fhir-operation labels Mar 6, 2021
@lmsurpre lmsurpre added break-change-api P2 Priority 2 - Should Have labels Apr 27, 2021
@punktilious punktilious self-assigned this Dec 19, 2021
punktilious added a commit that referenced this issue Dec 19, 2021
Signed-off-by: Robin Arnold <robin.arnold@ibm.com>
punktilious added a commit that referenced this issue Dec 19, 2021
Signed-off-by: Robin Arnold <robin.arnold@ibm.com>
punktilious added a commit that referenced this issue Dec 20, 2021
Signed-off-by: Robin Arnold <robin.arnold@ibm.com>
@d0roppe
Copy link
Collaborator

d0roppe commented Jan 11, 2022

Reopening this issue, with verification of the header set Prefer: return=representation. on a delete the full resource is included, but there is no way to read that versioned resource. or otherwise see what it used to be. so it should not be included if it is a Delete method.
Also the header Prefer: return=OperationOutcome is the same as Prefer: return=minimal, but it should be like Prefer: return=representation to more match up to the way other query output is handled.

@lmsurpre
Copy link
Member Author

lmsurpre commented Jan 11, 2022

on a delete the full resource is included

this is definitely the bigger issue of the two things mentioned. the spec is fairly clear that the resource content should be omitted for the history of a delete.
from https://hl7.org/fhir/http.html#history

A delete interaction is represented in a history interaction in the following way:

  <entry>
    <!-- no resource included for a delete -->
    <request>
      <method value="DELETE"/>
      <url value="Patient/[id]"/>
    </request>
    <!-- response carries the instant the server processed the delete -->
    <response>
      <lastModified value="2014-08-20T11:05:34.174Z"/>
    </response>
  </entry>

I think its fine to continue including the Entry.response.location for the deleted version though (my 2 cents)

@punktilious
Copy link
Collaborator

Resource will not be added to the bundle entry when the change type == DELETED, and we'll also fetch the resource values now when the return preference is OPERATION_OUTCOME, not just REPRESENTATION.

@punktilious
Copy link
Collaborator

Delete entry now looks like this (test data):

    {
      "id": "1269",
      "fullUrl": "Patient/29eea50f-36e8-4965-8543-1b39016caf1b",
      "request": {
        "method": "DELETE",
        "url": "Patient/29eea50f-36e8-4965-8543-1b39016caf1b"
      },
      "response": {
        "status": "200 OK",
        "location": "Patient/29eea50f-36e8-4965-8543-1b39016caf1b/_history/2",
        "lastModified": "2022-01-07T23:08:45.543816Z"
      }
    },

punktilious added a commit that referenced this issue Jan 12, 2022
Signed-off-by: Robin Arnold <robin.arnold@ibm.com>
punktilious added a commit that referenced this issue Jan 12, 2022
@d0roppe
Copy link
Collaborator

d0roppe commented Jan 12, 2022

The two noted reasons to reopen this issue are now fixed. Closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
break-change-api enhancement New feature or request P2 Priority 2 - Should Have
Projects
None yet
Development

No branches or pull requests

4 participants