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

Implement instance-level $validate and enforce mode binding #2964

Closed
lmsurpre opened this issue Nov 9, 2021 · 6 comments
Closed

Implement instance-level $validate and enforce mode binding #2964

lmsurpre opened this issue Nov 9, 2021 · 6 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed P3 Priority 3 - Nice To Have

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Nov 9, 2021

Is your feature request related to a problem? Please describe.
In #2905 we added partial support for the mode input parameter on the $validate operation.
Specifically, when this parameter is set to create or update, we will validate the passed resource by using the corresponding fhir-server-config settings under resources/<resourceType>/profiles.

However, https://www.hl7.org/fhir/operation-resource-validate.html defines mode with a required binding to https://www.hl7.org/fhir/valueset-resource-validation-mode.html whereas our implementation allows any string that would be a valid code.

Specifically, in addition to create and update modes, it defines:

  • a profile mode that it says should only apply to a resource "nominated by id, not provided as a parameter"
  • a delete mode that only makes sense against a previously-ingested resource

But our implementation doesn't even support validation against existing resources in the db.
If you try invoking it at this level, it fails with a 400 Bad Request error that says:

"Input parameter 'resource' is required for the $validate operation"

Describe the solution you'd like

  1. Support (and advertise support for) resource-instance level $validate (at [base]/[resourceType]/[id]/$validate)
  2. Ensure that only valid "mode" codes are accepted by the $validate operation... if an invalid code is sent then return a 400 Bad Request with a reasonable error
  • Ensure delete is only ever invoked at the resource-instance level (although I have no clue when we'd say that a given delete is "invalid"...I guess never?)
  • I don't understand why profile wouldn't be used when validating a passed resource against a particular profile. I think this matches our current behavior if the 'profile' parameter is passed in and that seems fine to me.

Describe alternatives you've considered

Acceptance Criteria

  1. GIVEN [a precondition]
    AND [another precondition]
    WHEN [test step]
    AND [test step]
    THEN [verification step]
    AND [verification step]

Additional context

@prb112 prb112 added the enhancement New feature or request label Nov 9, 2021
@lmsurpre lmsurpre added P3 Priority 3 - Nice To Have help wanted Extra attention is needed good first issue Good for newcomers labels Jan 24, 2022
PrasannaHegde1 added a commit that referenced this issue Aug 3, 2022
…enforce mode binding

Signed-off-by: Prasanna Hegde <prasanna.hegde1@ibm.com>
PrasannaHegde1 added a commit that referenced this issue Aug 4, 2022
Signed-off-by: Prasanna Hegde <prasanna.hegde1@ibm.com>
PrasannaHegde1 added a commit that referenced this issue Aug 4, 2022
Signed-off-by: Prasanna Hegde <prasanna.hegde1@ibm.com>
PrasannaHegde1 added a commit that referenced this issue Aug 4, 2022
…peration-validate module for unit tests

Signed-off-by: Prasanna Hegde <prasanna.hegde1@ibm.com>
PrasannaHegde1 added a commit that referenced this issue Aug 4, 2022
…mock json file for unit testing

Signed-off-by: Prasanna Hegde <prasanna.hegde1@ibm.com>
PrasannaHegde1 added a commit that referenced this issue Aug 5, 2022
Signed-off-by: Prasanna Hegde <prasanna.hegde1@ibm.com>
PrasannaHegde1 added a commit that referenced this issue Aug 5, 2022
Signed-off-by: Prasanna Hegde <prasanna.hegde1@ibm.com>
lmsurpre pushed a commit that referenced this issue Aug 5, 2022
#3841)

* issue #2964 - Support resource-instance level validate operation and enforce mode binding

Signed-off-by: Prasanna Hegde <prasanna.hegde1@ibm.com>

* issue #2964 - fixed review comments on unit tests

Signed-off-by: Prasanna Hegde <prasanna.hegde1@ibm.com>
@punktilious
Copy link
Collaborator

I found a couple of possible issues:

  1. Minor. If a resource is delete, if you try to delete it again you get 200 OK with an OperationOutcome indicating that the resource is already deleted (the operation is idempotent). However, validate mode=delete will give you a 404 for a resource which is currently deleted. So that's a slight discrepancy.
  2. The validate mode create/update/delete appear to ignore the allowed interactions configuration. For example, I think if we disable delete, then validate mode=delete should also say that the resource cannot be deleted.
  3. Validate mode=create indicates no error when the resource currently exists. I'm struggling with what should be the correct behavior for this one because the spec says: "and then checks that the content would be acceptable as a create (e.g. that the content would not violate any uniqueness constraints)"
  4. With updateCreateEnabled set to false, the validate mode=update operation reports NoError even when the resource does not exist
  5. Validate mode=create shows no error if no resource parameter value is provided.
  6. Validate mode=update shows no error if no resource parameter value is provided.

@punktilious
Copy link
Collaborator

In addition, during team discussion we agreed that the validation should always be on the resource provided in the incoming content.

@lmsurpre
Copy link
Member Author

lmsurpre commented Aug 24, 2022

  1. GIVEN fhir-server-config disallows a particular interaction type (e.g. create)
    WHEN $validate is invoked with the corresponding mode (create)
    THEN a 200 OK is returned with an OperationOutcome with issueType ERROR not-supported

  2. GIVEN fhir-server-config with updateCreateEnabled=true (the default)
    AND no resource exists at that endpoint
    WHEN $validate is invoked at the instance level with mode=create
    AND the instance is valid
    THEN 200 OK is returned with no errors in the OperationOutcome

  3. GIVEN fhir-server-config with updateCreateEnabled=false
    AND no resource exists at that endpoint
    WHEN $validate is invoked at the instance level with mode=create
    AND the instance is valid
    THEN 200 OK is returned with no errors in the OperationOutcome

  4. GIVEN fhir-server-config with updateCreateEnabled=true (the default)
    AND a resource already exists at that endpoint
    WHEN $validate is invoked at the instance level with mode=create
    THEN 200 OK is returned with an OperationOutcome with issueType ERROR not-supported

  5. GIVEN fhir-server-config with updateCreateEnabled=false
    AND a resource already exists at that endpoint
    WHEN $validate is invoked at the instance level with mode=create
    THEN 200 OK is returned with an OperationOutcome with issueType ERROR not-supported

  6. GIVEN no resource instance exists at the endpoint
    WHEN $validate is invoked at the instance level with mode=update
    THEN 200 OK is returned with an OperationOutcome with issueType ERROR not-supported

  7. GIVEN the resource instance exists at the endpoint
    WHEN $validate is invoked at the instance level with mode=update
    THEN 200 OK is returned with an OperationOutcome with NoError.

  8. WHEN $validate is invoked without a mode parameter
    AND no resource parameter value is provided
    THEN 400 Bad Request is returned with an OperationOutcome that includes information which states the 'resource' parameter is required when mode is create or update

  9. WHEN $validate is invoked with mode=create
    AND no resource parameter value is provided
    THEN 400 Bad Request is returned with an OperationOutcome that includes information which states the 'resource' parameter is required when mode is create or update

  10. WHEN $validate is invoked with mode=update
    AND no resource parameter value is provided
    THEN 400 Bad Request is returned with an OperationOutcome that includes information which states the 'resource' parameter is required when mode is create or update

  11. WHEN $validate is invoked at the instance level with mode=profile
    AND no resource parameter value is provided
    THEN the resource at this id is read and validated against the nominated profile

  12. WHEN $validate is invoked at the type level with mode=update
    THEN 400 Bad Request is returned with an OperationOutcome that includes information which states the 'resource' parameter is required when mode is create or update

  13. WHEN $validate is invoked at the type level with mode=delete
    THEN 400 Bad Request is returned with an OperationOutcome that includes information which states modes update and delete can only be used when the operation is invoked at the resource instance level.

profile mode is supported at both levels?

@punktilious
Copy link
Collaborator

punktilious commented Aug 31, 2022

@lmsurpre I think the acceptance criteria should be tweaked a little to make it clear whether an existing resource is present. This is unclear for AC3, for example.

AC1: PASS

AC2 FAIL

With fhirServer/persistence/common/updateCreateEnabled equal true and Patient ValAdaDaugherty already existing, the request is failing, contrary to the requirements of AC2:

POST [base]/Patient/ValAdaDaugherty/$validate

HTTP/2 200 
content-type: application/fhir+json
date: Wed, 31 Aug 2022 10:07:03 GMT
content-language: en-GB
content-length: 585

{
  "resourceType": "OperationOutcome",
  "id": "Error",
  "text": {
    "status": "additional",
    "div": "<div xmlns=\"http://www.w3.org/1999/xhtml\"><p>ERROR</p></div>"
  },
  "issue": [
    {
      "severity": "error",
      "code": "invalid",
      "details": {
        "text": "Patient with id 'ValAdaDaugherty' already exists"
      }
    },
    {
      "severity": "warning",
      "code": "not-supported",
      "details": {
        "text": "Profile 'http://hl7.org/fhir/StructureDefinition/Patient' is not supported"
      },
      "expression": [
        "Patient"
      ]
    },
    {
      "severity": "warning",
      "code": "invariant",
      "details": {
        "text": "dom-6: A resource should have narrative for robust management"
      },
      "expression": [
        "Patient"
      ]
    }
  ]
}

AC3: PASS...sort of

With createUpdatedEnabled = false, we get 2 errors.  Also, the text "Resource create, of type 'Patient', is not supported." is misleading. Create for this resource type is supported, but only when the resource currently doesn't exist.

{
  "severity": "error",
  "code": "invalid",
  "details": {
    "text": "Patient with id 'ValAdaDaugherty' already exists"
  }
},
{
  "severity": "error",
  "code": "not-supported",
  "details": {
    "text": "Resource create, of type 'Patient', is not supported."
  }
},

...


@lmsurpre
Copy link
Member Author

lmsurpre commented Aug 31, 2022

I agree AC2 is underspecified.

2. GIVEN fhir-server-config with updateCreateEnabled=true (the default)
WHEN $validate is invoked at the instance level with mode=create
AND the instance is valid
THEN 200 OK is returned with no errors in the OperationOutcome

should probably be split into the two cases.

2a. GIVEN fhir-server-config with updateCreateEnabled=true (the default)
AND a resource exists with id=123
WHEN $validate is invoked at the instance level with id=123 and mode=create
THEN 200 OK is returned with an OperationOutcome with severity ERROR and code invalid

2b. GIVEN fhir-server-config with updateCreateEnabled=true (the default)
WHEN $validate is invoked at the instance level with mode=create
AND the instance is valid
THEN 200 OK is returned with no errors in the OperationOutcome

For AC3, I think the 2 errors is fine.
The first one is saying that it couldn't be created because the resource already exists.
The second one is saying that create-on-update isn't supported. Its for this funny case where you invoke $validate at the instance level but set mode=create. We decided that should return an error if create-on-update is not supported.
So "Create for this resource type is supported, but only when the resource currently doesn't exist." is not right.

@punktilious
Copy link
Collaborator

punktilious commented Aug 31, 2022

I've updated the acceptance criteria to break things out a little more cleanly and will post the results here:

AC1: PASS
AC2: PASS
AC3: PASS
AC4: PASS
AC5: PASS
AC6: PASS
AC7: PASS
AC8: PASS
AC9: PASS
AC10: PASS
AC11: PASS
AC12: PASS
AC13: PASS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed P3 Priority 3 - Nice To Have
Projects
None yet
Development

No branches or pull requests

4 participants