Support conditional create-on-update via If-None-Match #2050

lmsurpre · 2021-03-10T19:49:08Z

Is your feature request related to a problem? Please describe.
In #160 we added support for conditional read via If-None-Match.

However, this header is also useful on the update/create-on-update side. Specifically, it can be used to avoid creating the same resource multiple times.

Describe the solution you'd like
Clients can pass the If-None-Match header to us in their PUT request.
If If-None-Match is set to * and a resource exists at the passed location, then no update performed.

Describe alternatives you've considered
The spec defines a conditional create via a custom If-None-Exists header that is based on FHIR search. We support that today, but it would be extremely hard to get the locking right if we want to guarantee no duplicates (see #2051).

Acceptance Criteria
1.
GIVEN a PUT to [base]/Observation/123
WHEN the If-None-Match header is set to *
AND a resource (any version) already exists at [base]/Observation/123
THEN the response status is HTTP 304 (Not Modified)
AND the response body is set according to the Prefer: return preference (representation=the existing resource, minimal=empty, OperationOutcome=a short description of why the resource wasn't updated)

GIVEN a PUT to [base]/Observation/123
WHEN the If-None-Match header is set to W/"1"
AND a resource with version "1" already exists at [base]/Observation/123
THEN the response status is HTTP 500 (Server Error)
AND the response body is set according to the Prefer: return preference (minimal=empty, OperationOutcome=a description of the error explaining that If-None-Match with a specific version is not supported)

GIVEN a PUT to [base]/Observation/123
WHEN the If-None-Match header is set to *
AND no resource exists at [base]/Observation/123
THEN the response status is HTTP 201 (Created)
AND the response body is set according to the Prefer: return preference (representation=the newly created resource, minimal=empty, OperationOutcome=any warnings or informational messages from validating the new resource version)

GIVEN a PUT to [base]/Observation/123
WHEN the If-None-Match header is not passed
AND a resource exists (any version) at [base]/Observation/123
THEN the response status is HTTP 200 (OK)
AND the response body is set according to the Prefer: return preference (representation=the newly updated resource, minimal=empty, OperationOutcome=any warnings or informational messages from validating the new resource version)

GIVEN a PUT to [base]/Observation/123
WHEN the If-None-Match header is set to *
AND a deleted resource exists at [base]/Observation/123
THEN the response status is HTTP 201 (Created)
AND the response body is set according to the Prefer: return preference (representation=the newly created resource, minimal=empty, OperationOutcome=any warnings or informational messages from validating the new resource version)

Additional context
See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-None-Match

Specifically, the part about If-None-Match: *

The asterisk is a special value representing any resource. They are only useful when uploading a resource, usually with PUT, to check if another resource with the identity has already been uploaded before.

The text was updated successfully, but these errors were encountered:

lmsurpre · 2021-03-10T19:49:24Z

Note from robin:

To support * properly we'll need to tweak our persistence API to do some locking around the id.

However, as noted in the description, this should be much easier than the locking that would be needed to properly support If-None-Exist in a way that will guarantee no duplicates.

lmsurpre · 2021-05-03T15:17:37Z

should be done as a SELECT FOR UPDATE on the logical resource id.

if we want this logic at the REST layer, then we'd need to introduce a PL change so that the rest layer can tell the PL that its doing a read that should get the SELECT FOR UPDATE semantics.

from robin: we need to think this through carefully in the case of bundles because you can easilly get into deadlock cases.

lmsurpre · 2021-05-03T15:25:59Z

lower priority since we believe the workaround is to use #2265

evbaron · 2021-09-15T17:02:14Z

IfNoneMatch should also work for bundles. See #2754

punktilious · 2021-10-23T12:38:45Z

By passing this information down to the persistence layer, we can correctly handle the necessary serialization. The acceptance criteria should be updated to include some deleted resource scenarios.

As we're already updating our persistence API in the next release, it makes sense to implement this now. It probably makes sense to add this to the persistence context to avoid more changes to the signatures. But it's still an API change because the behavior of the interaction will change.

punktilious · 2021-10-28T08:28:21Z

I updated the acceptance criteria to reflect the following design decisions:

only * is supported, not W/"1".
When undeleting a resource, If-None-Match does not have any effect and the response will be 201 as without the If-None-Match header.

Issue #2050 conditional create-on-update using If-None-Match

lmsurpre · 2021-11-09T20:49:44Z

I tested various PUT options things were working as expected.

If-None-Match Header	Current Version on Server	response code	outcome-severity	outcome-text
1		400	fatal	Invalid If-None-Match value
W/"1"		400	fatal	If-None-Match with specific version not supported
*		201	informational	All OK
*	1	304

However, then I noticed the Location header on the 304 response was off:
https://localhost:9443/fhir-server/api/v4/Patient/Patient/Patient1/_history/1

Note the extra /Patient in there. Reopening to get that fixed.

lmsurpre · 2021-11-09T20:57:33Z

After further investigation, I think its an artifact of our refactored update code...not specific to conditional create-on-update with If-None-Match.

UPDATE: I'm not even sure its a regression and so I opened #2965 for it. Moving this one back to QA.

lmsurpre · 2021-11-10T04:12:44Z

I was able to force a 500 Server Error by submitting a transaction bundle that has 2 create-on-update requests, each with "ifNoneMatch": "*" and each with the same id.

[11/10/21, 4:06:46:974 UTC] 0000003b FHIRRestHelpe E   
                                 java.lang.NullPointerException
	at com.ibm.fhir.model.util.FHIRUtil.buildLocationURI(FHIRUtil.java:269)
	at com.ibm.fhir.server.util.FHIRRestHelper.doPatchOrUpdatePersist(FHIRRestHelper.java:772)
	at com.ibm.fhir.server.rest.FHIRRestInteractionVisitorPersist.lambda$doUpdate$5(FHIRRestInteractionVisitorPersist.java:169)
	at com.ibm.fhir.server.rest.FHIRRestInteractionVisitorPersist$$Lambda$548/0x0000000074c4b220.call(Unknown Source)
	at com.ibm.fhir.server.rest.FHIRRestInteractionVisitorPersist.doInteraction(FHIRRestInteractionVisitorPersist.java:273)
	at com.ibm.fhir.server.rest.FHIRRestInteractionVisitorPersist.doUpdate(FHIRRestInteractionVisitorPersist.java:167)
	at com.ibm.fhir.server.rest.FHIRRestInteractionUpdate.accept(FHIRRestInteractionUpdate.java:66)
	at com.ibm.fhir.server.util.FHIRRestHelper.processBundleInteractions(FHIRRestHelper.java:1961)
	at com.ibm.fhir.server.util.FHIRRestHelper.processBundleEntries(FHIRRestHelper.java:1879)
	at com.ibm.fhir.server.util.FHIRRestHelper.doBundle(FHIRRestHelper.java:1445)
	at com.ibm.fhir.server.resources.Batch.bundle(Batch.java:90)

The line number may not match up exactly because I had some local edits, but I'm pretty confident this is an issue.
The problem is that prevResource is still null from earlier when we get into this block:

            // Invoke the 'afterUpdate' interceptor methods.
            if (updateCreate) {
                if (result.getStatus() == InteractionStatus.IF_NONE_MATCH_EXISTED) {
                    // Use the location assigned to the previous resource because we're
                    // not updating anything. Also, we don't fire any 'after' event
                    // for the same reason.
                    ior.setResource(prevResource); // resource wasn't changed
                    ior.setLocationURI(FHIRUtil.buildLocationURI(type, prevResource));
                    ior.setStatus(Response.Status.NOT_MODIFIED);
                }

We might need to issue another read at this point if we need to construct the proper locationURI (e.g. with the proper versionId)...or maybe we can just assume a versionId of 1 since it didn't even exist when we tried to read it just above?

Question: should undeleting a deleted resource be handled like an updateCreate? Without this, we could run into cases where the locationURI could list the deleted version of a resource. Basically, handling the undelete like create-on-update means that we can avoid doing the extra read in the normal IfNoneMatch case. Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

lmsurpre · 2021-11-11T17:21:20Z

We had a healthy team debate about the proper response code for conditional create-on-update when the If-None-Match precondition fails.

The current implementation returns an HTTP 304 Not Modified response for this case.
However, my reading of the HTTP (webdav) spec that defines If-None-Exist, is that it should be an HTTP 412 Precondition failed in the case of a PUT.

The problem with this interpretation is that, if you had a transaction bundle with a set of conditional create-on-update requests (via ifNoneMatch=*), if any one of those fails it will fail the entire bundle.

For this reason, we are now leaning toward making this configurable. The default should be to return the "proper" HTTP 412 Preconditioned failed, but operators will be able to opt in to the less compliant / more usable HTTP 304 behavior via a config property like fhirServer/core/ifNoneMatchReturnsNotModified.

lmsurpre · 2021-11-16T17:27:12Z

Updated results with default config:

If-None-Match Header	Current Version on Server	response code	outcome-severity	outcome-text
1		400	fatal	Invalid If-None-Match value
W/"1"		400	fatal	If-None-Match with specific version not supported
*		201	informational	All OK
*	1	412	fatal	IfNoneMatch precondition failed.

With fhirServer/core/ifNoneMatchReturnsNotModified: true:

If-None-Match Header	Current Version on Server	response code	outcome-severity	outcome-text / location header
*		201	informational	All OK
*	1	304		`[base]/Patient/Patient1/_history/1`
*	2	304		`[base]/Patient/Patient1/_history/2`

And within a transaction bundle where the same resource id is updated twice (each with ifNoneMatch set to "*") it returned 200 OK:

{
  "resourceType": "Bundle",
  "type": "transaction-response",
  "entry": [
    {
      "resource": {...},
      "response": {
        "id": "example",
        "status": "201",
        "location": "Observation/example/_history/1",
        "etag": "W/\"1\"",
        "lastModified": "2021-11-16T17:24:46.874903Z",
        "outcome": {
          "resourceType": "OperationOutcome",
          "issue": [
            {
              "severity": "warning",
              "code": "not-supported",
              "details": {
                "text": "Profile 'http://ibm.com/fhir/pdm/StructureDefinition/pdm-general-observation-profile|1.0.0' is not supported"
              },
              "expression": [
                "Observation"
              ]
            }
          ]
        }
      }
    },
    {
      "response": {
        "status": "304",
        "location": "Observation/example/_history/1",
        "outcome": {
          "resourceType": "OperationOutcome",
          "issue": [
            {
              "severity": "warning",
              "code": "not-supported",
              "details": {
                "text": "Profile 'http://ibm.com/fhir/pdm/StructureDefinition/pdm-general-observation-profile|1.0.0' is not supported"
              },
              "expression": [
                "Observation"
              ]
            }
          ]
        }
      }
    }
  ]
}

Looks good!

lmsurpre · 2021-11-16T17:32:48Z

When ifNoneMatchReturnsNotModified is set to false, the failed precondition fails the transaction bundle with a 400 Bad Request (as expected) and the following response body:

{
  "resourceType": "OperationOutcome",
  "id": "ac-11-a8-8d-ba0249ec-1828-4cd1-846a-acc295c2679f",
  "issue": [
    {
      "severity": "fatal",
      "code": "conflict",
      "details": {
        "text": "IfNoneMatch precondition failed."
      },
      "expression": [
        "<empty>",
        "Bundle.entry[0]"
      ]
    }
  ]
}

The only oddity here is the <empty> expression entry. I'll open an issue for that I guess, but I'm not even sure its worth the effort.

lmsurpre · 2021-11-16T17:34:47Z

the corresponding batch bundle succeeds with 200 OK (again as expected):

{
  "resourceType": "Bundle",
  "type": "batch-response",
  "entry": [
    {
      "resource": {
        "resourceType": "OperationOutcome",
        "id": "ac-11-a8-8d-766453f5-acb8-4e6b-aa64-80720a316026",
        "issue": [
          {
            "severity": "fatal",
            "code": "conflict",
            "details": {
              "text": "IfNoneMatch precondition failed."
            },
            "expression": [
              "<empty>"
            ]
          }
        ]
      },
      "response": {
        "status": "412"
      }
    },
    {
      "resource": {
        "resourceType": "OperationOutcome",
        "id": "ac-11-a8-8d-e38959ec-899d-483f-b34c-556f1fa2d8b6",
        "issue": [
          {
            "severity": "fatal",
            "code": "conflict",
            "details": {
              "text": "IfNoneMatch precondition failed."
            },
            "expression": [
              "<empty>"
            ]
          }
        ]
      },
      "response": {
        "status": "412"
      }
    },
    {
      "resource": {
        "resourceType": "OperationOutcome",
        "id": "ac-11-a8-8d-3dfe0f99-dddc-42bd-8940-9db7dad4479b",
        "issue": [
          {
            "severity": "fatal",
            "code": "conflict",
            "details": {
              "text": "IfNoneMatch precondition failed."
            },
            "expression": [
              "<empty>"
            ]
          }
        ]
      },
      "response": {
        "status": "412"
      }
    },
    {
      "resource": {
        "resourceType": "OperationOutcome",
        "id": "ac-11-a8-8d-970a7e26-60fa-49dc-a039-55d2ad521c88",
        "issue": [
          {
            "severity": "fatal",
            "code": "conflict",
            "details": {
              "text": "IfNoneMatch precondition failed."
            },
            "expression": [
              "<empty>"
            ]
          }
        ]
      },
      "response": {
        "status": "412"
      }
    }
  ]
}

punktilious mentioned this issue Mar 10, 2021

Concurrency limitations with conditional create/update requests #2051

Open

lmsurpre changed the title ~~Support conditional create-on-update vis If-None-Match~~ Support conditional create-on-update via If-None-Match Mar 10, 2021

prb112 added the enhancement New feature or request label Mar 11, 2021

lmsurpre mentioned this issue Apr 21, 2021

Support client opt-in for skipping the update if a resource hasn't changed #2263

Closed

lmsurpre added the P3 Priority 3 - Nice To Have label May 3, 2021

lmsurpre mentioned this issue Sep 10, 2021

Implement conditional update feature (BUNDLE) #2754

Open

lmsurpre mentioned this issue Sep 17, 2021

Reference within a bundle could not be resolved with entry request conditional update #2769

Closed

lmsurpre added this to the Sprint 2021-14 milestone Sep 17, 2021

lmsurpre modified the milestones: Sprint 2021-14, Sprint 2021-15 Oct 21, 2021

lmsurpre added the breaking-change-java Identifies a change in a method signature that an implementer may use. label Oct 24, 2021

punktilious self-assigned this Oct 25, 2021

punktilious added a commit that referenced this issue Oct 29, 2021

Merge pull request #2907 from IBM/issue-2050

d287690

Issue #2050 conditional create-on-update using If-None-Match

lmsurpre closed this as completed Nov 16, 2021

lmsurpre mentioned this issue Nov 16, 2021

Omit expression and detail elements from OperationOutcome issues with no meaningful value #2999

Closed

lmsurpre added the showcase Used to Identify End-of-Sprint Demos label Nov 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support conditional create-on-update via If-None-Match #2050

Support conditional create-on-update via If-None-Match #2050

lmsurpre commented Mar 10, 2021 •

edited by punktilious

Loading

lmsurpre commented Mar 10, 2021 •

edited

Loading

lmsurpre commented May 3, 2021

lmsurpre commented May 3, 2021

evbaron commented Sep 15, 2021

punktilious commented Oct 23, 2021

punktilious commented Oct 28, 2021

lmsurpre commented Nov 9, 2021 •

edited

Loading

lmsurpre commented Nov 9, 2021 •

edited

Loading

lmsurpre commented Nov 10, 2021 •

edited

Loading

lmsurpre commented Nov 11, 2021 •

edited

Loading

lmsurpre commented Nov 16, 2021 •

edited

Loading

lmsurpre commented Nov 16, 2021 •

edited

Loading

lmsurpre commented Nov 16, 2021

Support conditional create-on-update via If-None-Match #2050

Support conditional create-on-update via If-None-Match #2050

Comments

lmsurpre commented Mar 10, 2021 • edited by punktilious Loading

lmsurpre commented Mar 10, 2021 • edited Loading

lmsurpre commented May 3, 2021

lmsurpre commented May 3, 2021

evbaron commented Sep 15, 2021

punktilious commented Oct 23, 2021

punktilious commented Oct 28, 2021

lmsurpre commented Nov 9, 2021 • edited Loading

lmsurpre commented Nov 9, 2021 • edited Loading

lmsurpre commented Nov 10, 2021 • edited Loading

lmsurpre commented Nov 11, 2021 • edited Loading

lmsurpre commented Nov 16, 2021 • edited Loading

lmsurpre commented Nov 16, 2021 • edited Loading

lmsurpre commented Nov 16, 2021

lmsurpre commented Mar 10, 2021 •

edited by punktilious

Loading

lmsurpre commented Mar 10, 2021 •

edited

Loading

lmsurpre commented Nov 9, 2021 •

edited

Loading

lmsurpre commented Nov 9, 2021 •

edited

Loading

lmsurpre commented Nov 10, 2021 •

edited

Loading

lmsurpre commented Nov 11, 2021 •

edited

Loading

lmsurpre commented Nov 16, 2021 •

edited

Loading

lmsurpre commented Nov 16, 2021 •

edited

Loading