Skip to content

Feature Request - Implement toggle to prevent conditional updates from invalidating match criteria. #5290

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

Closed
dmuylwyk opened this issue Sep 7, 2023 · 4 comments · Fixed by #5291
Assignees
Labels

Comments

@dmuylwyk
Copy link
Collaborator

dmuylwyk commented Sep 7, 2023

Describe the Feature Request

Note this was initially and incorrectly categorized as a bug; leaving steps to reproduce and expected behaviour to inform implementation.

Conditional updates are behaving inconsistently with respect to the supplied resource not satisfying the conditional URL. This is expected behaviour since technically it's legal to invalidate match criteria via conditional update. Refer to this comment in the code:

What's legal is not necessarily desirable so we need to introduce a new property that will when enabled prevent conditional updates from invalidating match criteria regardless of the resource version.

The default value for this property should be false to preserve existing behaviour.

Note #5291 already includes a test harness for whichever developer eventually implements this.

Please refer to:

Steps to Reproduce

1. Create a new Patient, for example:

No issues here.

POST https://hapi.fhir.org/baseR4/Patient
{
    "resourceType": "Patient",
    "identifier":
    [
        {
            "system": "http://kookaburra.text/id",
            "value": "kookaburra1"
        }
    ],
    "gender": "male",
    "birthDate": "1980-07-03"
}

Screenshot

busted1

2. Attempt to conditionally update this Patient with an incorrect identifier.value in the conditional URL:

Because no such resource exists in the repository, this conditional update behaves as a create. It rightly fails as expected with an HTTP 400 / HAPI-0929 during "conditional create validation", which is only applied for resource version 1 of a given resource.

PUT https://hapi.fhir.org/baseR4/Patient?identifier=http://kookaburra.text/id|kookaburra2
{
    "resourceType": "Patient",
    "identifier":
    [
        {
            "system": "http://kookaburra.text/id",
            "value": "kookaburra1"
        }
    ],
    "gender": "male",
    "birthDate": "1980-07-03"
}

Screenshot

busted2

3. Attempt to conditionally update this Patient with an incorrect identifier.value in the body:

This succeeds but it I initially thought it shouldn't. Note the conditional URL and resource body no longer match; however, according to the specification it's perfectly legal to invalidate match criteria in this way. More specifically: nothing in the specification explicitly forbids this behaviour.

PUT https://hapi.fhir.org/baseR4/Patient?identifier=http://kookaburra.text/id|kookaburra1
{
    "resourceType": "Patient",
    "identifier":
    [
        {
            "system": "http://kookaburra.text/id",
            "value": "kookaburra2"
        }
    ],
    "gender": "male",
    "birthDate": "1980-07-03"
}

Screenshot

busted3

Expected Behaviour

When the new property to prevent conditional updates from invalidating match criteria is enabled, Step 3 above should also result in an HTTP 400 / HAPI-0929 (or similar error). Essentially, conditional create validation should be applied to resource versions higher than 1.

Environment

GET https://hapi.fhir.org/baseR4/metadata
	"software": {
		"name": "HAPI FHIR Server",
		"version": "6.9.3-SNAPSHOT/7f15e62e20/2023-09-05"
	},

Tested locally with 6.9.4-SNAPSHOT; see:

@tadgh
Copy link
Collaborator

tadgh commented Sep 7, 2023

A couple things:

  1. You are getting the 929 error code as, since there are no matches, you are creating version 1 of a resource, which causes the conditional create validation to occur.
  2. Conditional Updates are permitted to invalidate the conditional URL post-update as done in your Step 3. This is not strictly defined in the spec, and the comment here BaseHapiFhirDao.java indicates that we permit it.

I recommend we do what the comment suggests, and add a toggle to control this behaviour for users who wish to prevent this.

@dmuylwyk
Copy link
Collaborator Author

dmuylwyk commented Sep 7, 2023

Thanks, @tadgh. Will update the ticket to treat as a feature request. Appreciate your help!

@dmuylwyk dmuylwyk added feature and removed bug labels Sep 7, 2023
@dmuylwyk dmuylwyk changed the title Bug - Conditional updates can modify resource body such that conditional URL no longer applied. Feature Request - Implement toggle to prevent conditional updates from invalidating match criteria. Sep 7, 2023
@jmarchionatto
Copy link
Collaborator

@dmuylwyk this could be lateral to the feature main point but could be the subject of a new bug.

in 2. you mentioned: Because no such resource exists in the repository, this is treated as a conditional create. However, according to the spec, a ccreate must include an If-None-Exist: [search parameters] header, which is not included here, so maybe this shouldn't be treated as a ccreate?

@jmarchionatto jmarchionatto self-assigned this Sep 14, 2023
@dmuylwyk
Copy link
Collaborator Author

@dmuylwyk this could be lateral to the feature main point but could be the subject of a new bug.

in 2. you mentioned: Because no such resource exists in the repository, this is treated as a conditional create. However, according to the spec, a ccreate must include an If-None-Exist: [search parameters] header, which is not included here, so maybe this shouldn't be treated as a ccreate?

Ah, let me clarify: this is logically treated as a conditional create. I believe we use the same code but I may be mistaken. The point is that a conditional update is a create when there is no match. Apologies for the confusion. I'll correct the description above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants