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.

Sign up for GitHub

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

Jump to bottom

Make skippable updates the default #3614

Closed
lmsurpre opened this issue May 3, 2022 · 1 comment
Closed

Make skippable updates the default #3614

lmsurpre opened this issue May 3, 2022 · 1 comment
Assignees
@lmsurpre
Labels
enhancement New feature or request

Comments

@lmsurpre
Copy link
Member

lmsurpre commented May 3, 2022 •
edited
Loading

Is your feature request related to a problem? Please describe.
In #2263 we added opt-in support for skipping updates when the the resource content matches the existing resource content.
This seems to be working well, but most clients don't know to opt in for this.

Describe the solution you'd like
There seems to already be broad support for this idea in the community (e.g. https://chat.fhir.org/#narrow/stream/179166-implementers/topic/Handling.20of.20Updates/near/270802907 ) and so I think we could remove the client opt-in and just make this the normal behavior for 5.0.

I'm thinking that we retire the custom X-FHIR-UPDATE-IF-MODIFIED header (the name was confusing anyway) and always skip the update in this case, but I'd also be fine with renaming the header to something like X-FHIR-FORCE-UPDATE (with a default of false).

Describe alternatives you've considered
Make it configurable via a server config property instead.

Acceptance Criteria

  1. GIVEN a patch or update
    AND the updated resource matches the existing one (sans Resource.id, meta.versionId, and meta.lastUpdated)
    WHEN HTTP header X-FHIR-FORCE-UPDATE is absent or set to "false"
    THEN the update is skipped

  2. GIVEN a patch or update
    AND the updated resource matches the existing one (sans Resource.id, meta.versionId, and meta.lastUpdated)
    WHEN HTTP header X-FHIR-FORCE-UPDATE is set to "true"
    THEN the update is performed

Additional context
Add any other context or screenshots about the feature request here.
@lmsurpre lmsurpre added the enhancement New feature or request label May 3, 2022
@lmsurpre lmsurpre self-assigned this May 3, 2022
lmsurpre added a commit that referenced this issue May 3, 2022
@lmsurpre
issue #3614 - replace "updateIfModified" with "forceUpdate"
a35d772 
Replaced the old "X-FHIR-UPDATE-IF-MODIFIED" header with a new
"X-FHIR-FORCE-UPDATE" one. The default is still false, but now that
means that no-op updates are now skipped by default.

To force the server to perform a no-op update, users must now explicitly
set this X-FHIR-FORCE-UPDATE header to true.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue May 3, 2022
@lmsurpre
issue #3614 - IfNoneMatch short-circuit behavior
f2d57c2 
Previously we just passed the value to FHIRPersistence and let it handle
this.  We still need FHIRPersistence to double-check it (in case of a
race), but we should be able to handle the simple case up-front from the
REST layer.

The real motivator for this change is that we compute "skippable
updates" in the REST layer *prior* to passing it to FHIRPersistence and
I wanted to check the precondition before we perform that optimization.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue May 3, 2022
@lmsurpre
issue #3614 - replace "updateIfModified" with "forceUpdate"
629c9b6 
Replaced the old "X-FHIR-UPDATE-IF-MODIFIED" header with a new
"X-FHIR-FORCE-UPDATE" one. The default is still false, but now that
means that no-op updates are now skipped by default.

To force the server to perform a no-op update, users must now explicitly
set this X-FHIR-FORCE-UPDATE header to true.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue May 3, 2022
@lmsurpre
issue #3614 - IfNoneMatch short-circuit behavior
3e4548d 
Previously we just passed the value to FHIRPersistence and let it handle
this.  We still need FHIRPersistence to double-check it (in case of a
race), but we should be able to handle the simple case up-front from the
REST layer.

The real motivator for this change is that we compute "skippable
updates" in the REST layer *prior* to passing it to FHIRPersistence and
I wanted to check the precondition before we perform that optimization.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue May 4, 2022
@lmsurpre
issue #3614 - IfNoneMatch short-circuit behavior
f408aae 
Previously we just passed the value to FHIRPersistence and let it handle
this.  We still need FHIRPersistence to double-check it (in case of a
race), but we should be able to handle the simple case up-front from the
REST layer.

The real motivator for this change is that we compute "skippable
updates" in the REST layer *prior* to passing it to FHIRPersistence and
I wanted to check the precondition before we perform that optimization.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@d0roppe
Copy link
Collaborator

d0roppe commented May 10, 2022

Verified this is now the new default, closing issue.

@d0roppe d0roppe closed this as completed May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lmsurpre lmsurpre

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@lmsurpre @d0roppe