More guidance on syntax for deletes

lmckenzi commented 6 years ago

What does a "delete" action look like? There are no examples in the spec. "resource" is noted as being of type object, so it's not clear if I should say this: "resource" : "someId" or this "resource":{"id": "someId"}

(Isaac indicated that his expectation was "resource": {"resourceType": "MedicationRequest", "id": "someId" } - and given that ids are only unique within a resource type, that's probably wise)

It would be good to at least include an example showing expected syntax.

dennispatterson commented 4 years ago

Note that the example was added, but does not follow the above format and rather only lists the FHIR reference - https://github.com/cds-hooks/docs/commit/052825b5f8b63385300eb8f2a9cc4792ad7082bc . From reading the spec history, I think the committed example reflects the intent of the authors.

While this representation of deletes is convenient, I do think there's a remaining type problem that the existing spec doesn't address. An action.resource is defined as a JSON object which inherently means it's surrounded by curly braces. What the resource description and example showcase is that the type actually switches to JSON string in the case of deletes.

One update that could be made to clarify the existing state is to explicitly note that the type is object/string and clarify the description to call out that when it's a delete, then it's a string.

An alternate approach (that I haven't thought through fully) would be to call out deleteAction as a separate definition with the same fields, but with the explicit resource type change to string, then have suggestion.actions say it includes multiple action models. Essentially it remains spec compatible but is more explicit by showing that delete follows a different model.

mattvarghese commented 3 years ago

If a FHIR resource with only a FHIR ID and a resourceType is still legitimate, I would prefer that over having divergent typing for the resource property. Divergent types are a pain in most languages other than JavaScript.

Another option without actually separating the deleteAction as Dennis suggests above would be to add a 'resourceLocation' property the action. Create and update use 'resource', delete uses 'resourceLocation' - but if it's okay to have a FHIR resource with only just type and id, my vote would be for that..

mattvarghese commented 3 years ago

I've created https://github.com/cds-hooks/docs/pull/554 for this

dennispatterson commented 3 years ago

To elaborate on my thoughts above, one non-breaking way to approach this is to change Action into more of a parent object, with sub objects defining their own schema.

suggestion.actions - contains an Array of Actions

Action (Type object)

type - string - The type of action being performed.
description - string - Human-readable description of the suggested action MAY be presented to the end-user.

CreateAction (Type Action)

type - string - 'create'
resource - object - a new FHIR resource to be created

UpdateAction (Type Action)

type - string - 'update'
resource - object - the updated FHIR resource in its entirety and not just the changed fields

DeleteAction (type Action)

type - string - 'delete'
resource - string - the FHIR relative reference of the resource to remove (e.g. ServiceRequest/123)

This modeling is non-breaking to the 1.0 spec, but allows capturing the difference in models when a delete action is suggested by flexing on Action.type.

mattvarghese commented 3 years ago

@dennispatterson - this will however be rather burdensome for both services and clients to implement. Most languages don't have a conditional datamodel capability.

Also, the spec itself has a discrepancy - so that whatever we do, some part of it is breaking. So I'd break the part which is best broken to make life easier for both services and clients?

dennispatterson commented 3 years ago

implementers already do this kind of flexing today with FHIR's resourceType. You look at the resourceType to know which model to parse. Here you look at the action type

mattvarghese commented 3 years ago

I guess that is true. But each instance where we have to add such flexibility is additional development cost?

*Update 01/16/2020: Actually, @dennispatterson, what you say is not exactly true. In an object oriented language, resource is a FHIR Object, which usually is a base class with various FHIR resources as derived classes. The above really requires one to now add "string" as a derived class of FHIR Object, which is really not viable nor meaningful.

dennispatterson commented 3 years ago

At least for this case, the flexibility is already built-in, but definitely confusing as-is. If we want to make a breaking change to have deletes be an object, we'll need community buy-in. I posted on https://chat.fhir.org/#narrow/stream/179159-cds-hooks/topic/Format.20for.20a.20delete to see if we can have more input on the options.

dennispatterson commented 3 years ago

As a third option, I wanted to bring up another idea from @mattvarghese about having a resourceLocation (or similar field) for deletes.

mattvarghese commented 3 years ago

So apparently, DaVinci has again gone ahead and added their own wording in their IG: http://hl7.org/fhir/us/davinci-crd/2019May/hooks.html instead of cross-referencing to the CDS Hooks spec.

At the same time, a MedicationRequest for example MUST contain a status, an intent, a medicationCodeableConcept / medicationReference, and a subject. So using a resource with only resourceType and id is not technically congruent with the FHIR specification / will fail any resource level validators..

IMO the best option might be to have resourceLocation field for delete. But then we should also make sure DaVinci updates their IG?

lmckenzi commented 3 years ago

Not quite fair to say we didn't talk to CDS Hooks. We raised this issue a year and a half ago (when this issue got started). We proceeded with a recommendation from Isaac which was the best we had to go on at the time. Unfortunately, the IG is now published. However, if the CDS Hook community is able to land this 'soon', we could possibly put out a technical correction before too many payers implement based on the wrong thing.

mattvarghese commented 3 years ago

@lmckenzi - I realized after I wrote my comment that you represent DaVinci. And so I already updated my wording. Apologies..

However, DaVinci specs should cross-reference / point to CDS Hooks rather than replicating wording on how to delete etc, which would avoid such problems in the future perhaps?

Agree, this is one we should try to correct soon.

lmckenzi commented 3 years ago

The issue is that the wording in the Hooks spec on 'Delete' wasn't terribly clear - so we felt that additional guidance was necessary. Obviously if we get to the point where the Hooks spec is clear, a direct reference may be feasible.

dennispatterson commented 3 years ago

Original PR that introduced the existing text - https://github.com/cds-hooks/docs/pull/60

Proposed resolution with new reference field - https://github.com/cds-hooks/docs/pull/554#issuecomment-763776619

cds-hooks / docs