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

Patch Replaces Can Save Invalid Types #2207

Closed
ssnowski opened this issue Apr 6, 2021 · 2 comments
Closed

Patch Replaces Can Save Invalid Types #2207

ssnowski opened this issue Apr 6, 2021 · 2 comments
Assignees
Labels
bug Something isn't working P1 Priority 1 - Must Have

Comments

@ssnowski
Copy link

ssnowski commented Apr 6, 2021

Describe the bug
When saving with patch, it seems like it can corrupt existing resources, preventing them from being retrieved properly. This seems to be possible by making a request to save an element as a different type than it was previously (EG: save a valueString in the Patient.extension field (type: Extension).

Environment
IBM FHIR Server 4.6.1 via docker image. presumably 4.6.0 as well, but untested

To Reproduce
Steps to reproduce the behavior:

  1. Setup:
POST to fhir-server/api/v4:
{
        "resourceType": "Bundle",
        "type": "transaction",
        "entry": [
        {
                "resource": {
                        "id": "1234",
                        "resourceType": "Patient",
                        "identifier": [{
                                "system": "ibm.com/mrn",
                                "value": "1234"
                        }],
                        "extension": [
                          {
                            "url": "https://ibm.com/ext1",
                            "valueString": "testExtension"
                          }
                        ]
                },
                "request": {
                        "url": "Patient/1234",
                        "method": "PUT"
                }
        }
        ]
}
  1. Corrupt the data:
POST to fhir-server/api/v4:
{
  "resourceType": "Bundle",
  "type": "transaction",
  "entry": [
    {
      "request": {
        "url": "Patient/1234",
        "method": "PATCH"
      },
      "resource": {
        "resourceType": "Parameters",
        "parameter": [
          {
            "name": "operation",
            "part": [
              {
                "name": "type",
                "valueCode": "replace"
              },
              {
                "name": "value",
                "valueString": "this isn't an extension"
              },
              {
                "name": "path",
                "valueString": "Patient.extension[0]"
              }
            ]
          }
        ]
      }
    }
  ]
}
  1. See the error:
GET to fhir-server/api/v4/Patient/1234

Expected behavior
I would have expected step 2 to fail, returning a 4xx error with some text about a type mismatch.

Additional context

@ssnowski ssnowski added the bug Something isn't working label Apr 6, 2021
@lmsurpre lmsurpre added the P1 Priority 1 - Must Have label Apr 6, 2021
@lmsurpre lmsurpre self-assigned this Apr 6, 2021
@prb112 prb112 added this to the Sprint 2021-05 milestone Apr 6, 2021
lmsurpre added a commit that referenced this issue Apr 7, 2021
if a clever user adds objects of the wrong type to the builder (via
reflection), we were never catching this and so the data could become
corrupted

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Apr 7, 2021
if a clever user adds objects of the wrong type to the builder (via
reflection), we were never catching this and so the data could become
corrupted

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Apr 7, 2021
Previously, which exception you got was dependent on what was wrong with
the patch.

And prior to the model changes for this same issue, we actually allowed
invalid patches to be applied (in the case of adding, inserting, and
replacing on a list).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Apr 7, 2021
Previously, which exception you got was dependent on what was wrong with
the patch.

And prior to the model changes for this same issue, we actually allowed
invalid patches to be applied (in the case of adding, inserting, and
replacing on a list).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Apr 7, 2021
Previously, which exception you got was dependent on what was wrong with
the patch.

And prior to the model changes for this same issue, we actually allowed
invalid patches to be applied (in the case of adding, inserting, and
replacing on a list).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Apr 7, 2021
Previously, which exception you got was dependent on what was wrong with
the patch.

And prior to the model changes for this same issue, we actually allowed
invalid patches to be applied (in the case of adding, inserting, and
replacing on a list).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Apr 7, 2021
Previously, which exception you got was dependent on what was wrong with
the patch.

And prior to the model changes for this same issue, we actually allowed
invalid patches to be applied (in the case of adding, inserting, and
replacing on a list).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Apr 7, 2021
Previously, which exception you got was dependent on what was wrong with
the patch.

And prior to the model changes for this same issue, we actually allowed
invalid patches to be applied (in the case of adding, inserting, and
replacing on a list).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Apr 7, 2021
Previously, which exception you got was dependent on what was wrong with
the patch.

And prior to the model changes for this same issue, we actually allowed
invalid patches to be applied (in the case of adding, inserting, and
replacing on a list).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Apr 7, 2021
Previously, which exception you got was dependent on what was wrong with
the patch.

And prior to the model changes for this same issue, we actually allowed
invalid patches to be applied (in the case of adding, inserting, and
replacing on a list).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
JohnTimm added a commit that referenced this issue Apr 7, 2021
issue #2207 - consistently throw FHIRPatchException for bad patches
@tbieste
Copy link
Contributor

tbieste commented Apr 8, 2021

@lmsurpre Attempted the steps to reproduce and it returns with a 500 Internal Server Error with the following response:

{
"resourceType": "OperationOutcome",
"issue": [
{
"severity": "fatal",
"code": "exception",
"details": {
"text": "FHIRPatchException: An error occurred while replacing the value"
}
}
]
}

Instead of getting some type of 4xx error, is this the expected behavior?

lmsurpre added a commit that referenced this issue Apr 8, 2021
previously we threw 500 Sever Error, even in cases where we knew it was
likely a bad patch.  now we favor 400 errors for those, but unexpected
errors should still result in 500 errors

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

tbieste commented Apr 9, 2021

It now returns a 400 Bad Request Error, which matches the expected behavior.

"issue": [
{
"severity": "error",
"code": "invalid",
"details": {
"text": "Invalid patch: An error occurred while replacing the value"
},
"expression": [
"Patient.extension[0]"
]
}
]

Closing.

@tbieste tbieste closed this as completed Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1 Priority 1 - Must Have
Projects
None yet
Development

No branches or pull requests

4 participants