cds-hooks / docs

CDS Hooks website & specification
http://cds-hooks.org
Apache License 2.0
166 stars 61 forks source link

Context.userId containing ResourceType and ResourceId causes problems #520

Closed mattvarghese closed 3 years ago

mattvarghese commented 4 years ago

Hi Folks,

With the patient-view hook, there are three context variables: userId, patientId, encounterId. Of these patientId and encounterId are just IDs. However, userId is unique - it has the ResourceType and the ID separated by "/" in it. This causes problems.

For example, with patientId I can formulate a query like: patient?_id={{context.patientId}} This is perfectly legitimate, and is sometimes necessary to convert a read into a search to add additional search parameters such as _include etc.

However, if I try the same with userId, then say the query is as below and the ID is 123 practitioner?_id={{context.userId}} This leads to two problems:

  1. The userId may or may not be a practitioner resource. If it is not, and say it's a Patient, there is no way to tell not to try this, and we have failed - or worse, if there happened to be a practitioner resource with the same ID as the patient ID, what does that imply?
  2. When translated, this query becomes practitioner?_id=Practitioner/123 and this just fails because of the repetition of the resource type!

Can we do something to address this? /matt

cfeltner commented 4 years ago

Hi Matt,

I seem to recall that the reason userId was specified this way was to support different user types.
If the other syntax for querying a single resource is used, it works quite well.

GET {baseUrl}/{{context.userId}}

Since we use this syntax we have not had any problems with it.

mattvarghese commented 4 years ago

@cfeltner - that only works if all you want to do is a FHIR READ.

If you want to do a SEARCH, for example because you want to also _include another resource referenced in this user's resource, or pull out some other resource pertinent to this user using a SEARCH on that resource with this user as a search parameter, then you're badly stuck.

Also, wanting to do SEARCH for cases like described above feels like a fairly reasonable / frequent use-case.

cfeltner commented 4 years ago

@mattvarghese For SEARCH by reference, isn't including of the resource type with the logical ID valid (e.g., param=Practitioner/123)? GET [Base]/PractitionerRole?practitioner=Practitioner/123

Perhaps you could give a prefetch example that is causing problems.

mattvarghese commented 4 years ago

Sure - I can give the specific example which led me to write this up. This example is in the light of PR https://github.com/cds-hooks/docs/pull/515 and so I am using PractitionerRole type resource, but the problem itself is generic.

Since Context.UserId can be PractitionerRole after the above PR, I was trying to write a prefetch query that will also include the Practitioner resource referenced in the PractitionerRole resource. For this I could write a query like:

PractitionerRole?_id={{context.userId}}&_include=PractitionerRole:practitioner

if Context.userId was just the ID, this would work. And similar queries using Context.patientId, and Context.encounterId work fine. However, the above query fails because Context.userId contains both type and id.

mattvarghese commented 4 years ago

Another example, in a case where Context.userId is a Patient resource:

Patient?_id={context.userId}&_include=Patient:generalPractitioner

isaacvetter commented 4 years ago

Hey @mattvarghese , @cfeltner,

The CDS wg talked through this issue among related others -- will you guys check out this jira ticket: https://jira.hl7.org/browse/FHIR-25761 ?

We're proposing a potentially breaking change to address this deficiency in pre-fetch.

Isaac

raj-wk commented 4 years ago

@mattvarghese I am trying to understand the use case where there is a need to get to Practitioner from PractitionerRole or is it more about having userId to be in sync with patientId and encounterId.

Also wondering if we can leave the PR https://github.com/cds-hooks/docs/pull/515 as it and introduce new context like below if need be

"context" : { "encounterId": "456", "userInfo": { "resourceType": "RelatedPerson", "id": "123",

cfeltner commented 4 years ago

We, and I imagine many others, are actively using userId in our current CDS Interactions. Therefore, I suggest leaving it's meaning as is and introduce a new ID and type as @isaacvetter has suggested. For where we are currently using it, it still seems like the CDS Service would be interested in the Practitioner ID and not the PractitionerRole ID. The CDS Service is using the Practitioner ID to correlate practitioners between the EHR and the CDS Service. Returning the PractitionerRole ID instead of the Practitioner ID would cause them to perform a separate lookup or return another resource in the prefetch.

By keeping userId as is and adding new context parameters (e.g., FhirUserId, FhirUserType) this would allow for a smoother transition as EHRs and CDS Services deploy updates to support these.

isaacvetter commented 4 years ago

Hey @yashaskram , @cfeltner , @mattvarghese , All -

Overall, CDS Hooks' prefetch functionality is important because it directly enables improved performance of a CDS service. Allowing a service to specify a targeted prefetch queries is quite likely to minimize callback queries and therefore shorten the amount of network traffic and time required to display guidance to a clinician.

Of course, this proposed change would also affect essentially every hook. We definitely don't want to break existing implementations.

Below is one possible, brute-force solution --

patient-view

Metadata Value
specificationVersion 1.0
hookVersion 1.0
Hook maturity 4 - Documented

Workflow

The user has just opened a patient's record.

Context

The patient whose record was opened, including their encounter, if applicable.

Field Optionality Prefetch Token Type Description
userId REQUIRED Yes string The id of the current user.
The resource type MUST be one of Practitioner, PractitionerRole, Patient, or RelatedPerson.
For example, if the user was a Practitioner, this value would be Practitioner/123
userPractitionerId OPTIONAL Yes string The id of the current user, if current user is represented by the FHIR Practitioner resource.
userPractitionerRoleId OPTIONAL Yes string The id of the current user, if current user is represented by the FHIR PractitionerRole resource.
userPatientId OPTIONAL Yes string The id of the current user, if current user is represented by the FHIR Patient resource.
userRelatedPersonId OPTIONAL Yes string The id of the current user, if current user is represented by the FHIR Patient resource.
patientId REQUIRED Yes string The FHIR Patient.id of the current patient in context
encounterId OPTIONAL Yes string The FHIR Encounter.id of the current encounter in context

Examples

"context":{
  "userId" : "PractitionerRole/abc",
  "userPractitionerRoleId" : "abc",
  "userPractitionerId" : "123",
  "patientId" : "1288992"
}
"context":{
  "userId" : "Practitioner/123",
  "patientId" : "1288992",
  "encounterId" : "456"
}

Change Log

Version Description
1.0 Initial Release
1.1 Add PractitionerRole to resources for representing the current user. Add optional user*Id context fields to better enable prefetch.
dennispatterson commented 4 years ago

Catching up here and missed a CDS call where this was discussed, so forgive me if I'm rehashing.

@mattvarghese, do you need the Practitioner id as well for other prefetch queries? I'm trying to understand if the discrete ids were all added because we expect multiple of them to be used simultaneously (e.g. Practitioner and PractitionerRole ids) or if it's just to get to the raw values for each type. Do your other FHIR queries support PractitionerRole id consistently or will they use a mixture of PractitionerRole and Practitioner ids from the context? I’m wondering if for provider-facing use-cases, is it a unique need to have both ids vs just a single id for a consumer-facing scenario?

(Edit: removed invalid prefetch alternative; simplified comment to get more info about the need)

mattvarghese commented 4 years ago

@raj-wk : Apparently, the spec says context may not contain objects. Needs to be a flat dictionary; with top level properties only.

@cfeltner : I'm open to a solution that keeps userId and adds new fields.

@dennispatterson : I am a client. So I don't define prefetch template. I want to be able to support all types of prefetch templates, and I can imagine services which need practitioner role and also practitioner simultaneously in the prefetch. The examples I gave about I think are legitimate prefetch template queries that I should support as a client.

@isaacvetter : all the new IDs are optional, and the original userId which we want to maybe deprecate, is the required one in what you're proposing. I don't know if that is the best way to address this. We should keep userId for backward compatibility, but we should plan to deprecate it in favor of resource type specific IDs? Maybe a "Conditional" requirement (like @jmandel suggested elsewhere) is more appropriate?

yashaskram commented 4 years ago

@mattvarghese I was thinking along of the lines of someObject in the example -> https://cds-hooks.hl7.org/1.0/#examples and https://github.com/cds-hooks/docs/issues/377

isaacvetter commented 4 years ago

Hey @yashaskram , @cfeltner , @mattvarghese , All -

Overall, CDS Hooks' prefetch functionality is important because it directly enables improved performance of a CDS service. Allowing a service to specify a targeted prefetch queries is quite likely to minimize callback queries and therefore shorten the amount of network traffic and time required to display guidance to a clinician.

Of course, this proposed change would also affect essentially every hook. We definitely don't want to break existing implementations.

Below is a possible, more elegant solution that places a bit more work on the CDS Client and doesn't break backward compatibility, keeps our over-the-wire message structure clean and still enables prefetch searches on the user.

This is a modification to the base spec which leaves the patient-view (and every other hook) as is, by essentially creating a few special "prefetch variables" which aren't exactly explicitly provided in context.


Prefetch tokens

A prefetch token is a placeholder in a prefetch template that is replaced by information from the hook's context to construct the FHIR URL used to request the prefetch data.

Prefetch tokens MUST be delimited by {{ and }}, and MUST contain only the qualified path to a hook context field or one of the following user identifiers: userPractitionerId, 'userPractitioneRolerId', userPatientId, or userRelatedPersonId.

Individual hooks specify which of their context fields can be used as prefetch tokens. Only root-level fields with a primitive value within the context object are eligible to be used as prefetch tokens. For example, {{context.medication.id}} is not a valid prefetch token because it attempts to access the id field of the medication field.

*_

Prefetch tokens identifying the user

A prefetch template enables a CDS Service to learn more about the current user through a FHIR read, like so:

{
  "prefetch": {
    "user": "{{context.userId}}"
  }
}

or though a FHIR search:

{
  "prefetch": {
    "user": "PractitionerRole?_id={{userPractitionerId}}&_include=PractitionerRole:practitioner"
  }
}

A prefetch template may include any of the following prefetch tokens:

Token Description
{{userPractitionerId}} FHIR id of the Practitioner resource corresponding to the current user.
{{userPractitionerRoleId}} FHIR id of the Practitioner resource corresponding to the current user.
{{userPatientId}} FHIR id of the Practitioner resource corresponding to the current user.
{{userRelatedPersonId}} FHIR id of the Practitioner resource corresponding to the current user.

No single FHIR resource represents a user, rather Practitioner and PractitionerRole may be jointly used to represent a provider or other, and Patient or Person are used to represent a patient or their proxy. Hook definitions typically define a context.userId field and corresponding prefetch token.

...

brynrhodes commented 4 years ago

What about adding something like a "modifier" that allowed the prefetch token to specify which part of the Id to use?

context.patientId:resourceType context.patientId:resourceId

yashaskram commented 4 years ago

What about adding something like a "modifier" that allowed the prefetch token to specify which part of the Id to use? context.patientId:resourceType context.patientId:resourceId

just wondering with above if below is how prefetch look like?

{
  "prefetch": {
    "patient": "{{context.patientId:resourceType}}/{{context.patientId:resourceId}}",
    "hemoglobin-a1c": "Observation?patient={{context.patientId}}&code=4548-4&_count=1&sort:desc=date",
    "user": "{{context.userId}}"
  }
}

Basically the idea is to avoid second level context references like context.userInfo.resourceType

isaacvetter commented 4 years ago

Hey @yashaskram,

For Bryn's suggestion, yes, you've got the basic idea right. The problem is that there's no value provided by context.patientId:resourceType, cause the resourceType is always Patient. Similarly, context.patientId:resourceId is always === context.patientId. We should limit this change to fixing the problem.

If {{context.userId}} is equal to PractitionerRole/123, the value is like so:

{
  "prefetch": {
    "patient": "Patient/{{context.patientId}}",
    "hemoglobin-a1c": "Observation?patient={{context.patientId}}&code=4548-4&_count=1&sort:desc=date",
    "user": "{{context.userId}}",
    "pracAndRole": "PractitionerRole?_id={{context.userId:resourceId}}&include=PractitionerRole:practitioner",

  }
}

Of course, if {{context.userId}} is equal to Practitioner/abc, a simple CDS client, might not catch that this same prefetch query is invalid (and could even potentially return the wrong results):

    "pracAndRole": "PractitionerRole?_id={{context.userId:resourceId}}&include=PractitionerRole:practitioner",

Both the {{userPractitionerRole}} and the generic :resourceType / :resourceId approaches require a CDS client to perform some validation on the prefetch query. The {{userPractitionerRole}} is more readable and directly targets the problem.

isaacvetter commented 3 years ago

Recurring problem. This prefetch query template doesn't work, because {{context.userId}} contains /, e.g. Practitioner/123; whereas, what we want is only the 123, while also somehow knowing the resourcetype of the id.

PractitionerRole?_id={{context.userId}}&_include=PractitionerRole:practitioner

Possible solutions are well identified, above: 1) Create {{userResourceType}} and {{userResourceId}} 2) Or, enumerate all the ids, with the resource-type implicit in the name of the prefetch token. E.g. {{userPractitionerId}}, {{userPractitionerRoleId}}, {{userRelatedPersonId}}, {{userPatientId}}

For the above query, #1 is appealing simple on the surface, e.g.:

{{context.patientId:resourceType}}/{{context.patientId:resourceId}}

but quickly fails for realistical pre-fetch queries, e.g.:

{{context.patientId:resourceType}}?_id={{context.patientId:resourceId}}&_include=PractitionerRole:practitioner

We should add #2.

cfeltner commented 3 years ago

Seems like specifying the _include would be difficult with trying to keep it generic with {{userResourceType}}. For the 2nd sample pre-fetch query you specified it seems to be implying {{userResourceType}}=PractitionerRole.

If the EHR supports both Practitioner and PractitionerRole, how would EHR know which to put in for {{userResourceType}}? Seems like using the separate {{userPractitionerId}} and {{userPractitionerRoleId}} would be more straight forward. Then 2 pre-fetch queries could be specified:

Practitioner?_id={{userPractitionerId}}
PractitionerRole?_id={{userPractitionerRoleId}}

If the EHR did not support PractitionerRole, it would just not return that pre-fetch data.

isaacvetter commented 3 years ago

Thanks, @cfeltner. Agree with you. @dennispatterson points out that there could be a third option:

  1. Define not a prefetch token property, but rather a prefetch token accessor parameter, e.g.: {{context.userId:PractitionerRole}}. Notably, this {{context.userId:PractitionerRole}} would only and specifically return the FHIR id for the user for the identified resource type.

So, for read we wouldn't be changing anything, it would look like this:

{{context.userId}}

and generate PractitionerRole/abc

For search, it would look like this:

PractitionerRole?_id={{context.userId:PractitionerRole}}&_include=PractitionerRole:practitioner
RelatedPerson?_id={{context.userId:RelatedPerson}}&_include=Patient:patient

or

PractitionerRole/{{context.userId:PractitionerRole}}
Practitioner/{{context.userId:Practitioner}}

...

This pattern does exist in FHIR as a search parameter modifier, perhaps created to better enable chaining.

This approach has the benefit of being more general than the simple enumeration of user resource types, in that it would apply to any contextual prefetch token, e.g. {{context.patient:Patient}}; however, there are currently no other prefetch tokens to which this functionality would apply -- as of right now.

The advantage of .2 is that it actually tightly targets the specific problem of Practitioner and PractitionerRole being tightly related and sometimes necessary at the same time; whereas, a CDS Service won't need both Practitioner and Patient as user as part of the same query.

cfeltner commented 3 years ago

On the use of the accessor, wouldn't it be:

{{context.userId}}=Practitioner/123
{{context.userId:id}}=123

This syntax would seem to provide an easier pattern for the EHR to match against then specifying each resource type (e.g., context.userId:Practitioner, context.userId:PractitionerRole, context.userId:Patient, context.userId:RelatedPerson).

Since only the Practitioner and PractitionerRole are likely to be needed at the same time perhaps another alternative is just to introduce a new context parameter userRoleId. This would seem to minimize the amount of changes since the meaning of userId would remain the same.

dennispatterson commented 3 years ago

On the use of the accessor, wouldn't it be:

{{context.userId}}=Practitioner/123 {{context.userId:id}}=123 This syntax would seem to provide an easier pattern for the EHR to match against then specifying each resource type (e.g., context.userId:Practitioner, context.userId:PractitionerRole, context.userId:Patient, context.userId:RelatedPerson).

I don't completely follow. The ":id" idea looks like Bryn's :resourceId proposal which we've already shown doesn't work when the raw id should only be used in queries where the appropriate type matches. e.g. "PractitionerRole?_id={{context.userId:id}}" but the userId was a Practitioner so there's a mismatch

The advantage of .2 is that it actually tightly targets the specific problem of Practitioner and PractitionerRole being tightly related and sometimes necessary at the same time

Since only the Practitioner and PractitionerRole are likely to be needed at the same time perhaps another alternative is just to introduce a new context parameter userRoleId. This would seem to minimize the amount of changes since the meaning of userId would remain the same.

Both of these quotes hone in on the fact that this isn't just a context field flexing between two distinct types. It's two separate values needed at the same time because PractitionerRole includes the Practitioner type data. When the spec walks through an example of a context with a full Patient resource, it says if there's a need to access a field on that resource, it should be added to the context as a new, separate attribute.

Given that spec example, @cfeltner 's proposal may make sense for userRoleId as a new context parameter for the specific problem of accessing both ids.

"PractitionerRole?_id={{context.userRoleId}}&_include=PractitionerRole:practitioner"
"RelatedPerson?_id={{context.userId:RelatedPerson}}&_include=RelatedPerson:patient"
chandra-bala commented 3 years ago

@dennispatterson, I just want to confirm the proposal here. Are you suggesting that we just add a new context parameter "userRoleId"? This could help fetching both PractitionerRole and Practitioner (using _include) as you proposed. However, this still limits the pre-fetch ability to only get a Practitioner resource along with a PractitionerRole resource but not independently.

While I don't see a use case yet where there is only a need of Practitioner but not PractitionerRole, we should still try to allow for more flexibility. Also, it appears that the pre-fetch should have both the "userId" and the _include query you have above for the CDS Hooks request to generate the "Practitioner" resource if the user doesn't happen to have a userRoleId.

One additional thing I was confused with is the example with "context.userId:RelatedPerson". Are you proposing that we add this accessor capability as well on top of new context parameter "userRoleId"?

dennispatterson commented 3 years ago

@chandra-bala I think it was being recognized that when the user is a practitioner, two ids are needed in context in order to support the optional presence of the role. The userId would hold the practitioner id and the new field would hold the practitioner role id. Both available for independent prefetch queries that need them. The modifier option could be used to distinguish between personas, but since the practitioner persona can use two ids, the separate field assists with that use-case.

isaacvetter commented 3 years ago

Guys,

Since only the Practitioner and PractitionerRole are likely to be needed at the same time perhaps another alternative is just to introduce a new context parameter userRoleId. This would seem to minimize the amount of changes since the meaning of userId would remain the same.

I don't think the above is true.

  1. Introducing context.userRoleId will imply or perhaps require that we update each hook specification and implementers to update their support to begin sending this field. Would this new context field be required? If so, that'd be a breaking change for implementations.
  2. Note that context.userId can be one of four different resource ids -- for which only one would have a role (i.e. when userId is the id of a Patient, RelatedPerson, or PractitionerRole resource, context.userRoleId doesn't make sense).

The userId would hold the practitioner id and the new field would hold the practitioner role id.

  1. Note that changing the meaning of the context.userId field to no longer contain a PractitionerRole would be a breaking change as well, for both clients and services ... which we oughtn't do.

Note that the 80%+ use-case of provider-facing, remote CDS means that there will virtually always be a Practitioner/PractitionerRole id, (regardless of the hook and it's other context fields).

I really think that the proposed PR is the simplest, least invasive and non-breaking addition. Could you please re-review?

cfeltner commented 3 years ago

@isaacvetter I agree that we do not want to introduce breaking changes. I was not proposing a change to the existing userId prefetch token.

The userRoleId would just be an optional prefetch token that would only be needed/used if the CDS Service was wanting to obtain both the Practitioner and PractitionerRole resources in the prefetch. If the CDS Service just needs Practitioner or PractitionerRole in the prefetch they could use the existing userId prefetch token. In regards to updating the hook implementation guides, they would only need to be updated if they wanted to include both the Practitioner and PractitionerRole in the prefetch. With the existing proposal of introducing 4 new prefetch tokens wouldn't the hook implementation guides need to be updated as well?

Doesn't introducing 1 new prefetch token seem simpler than introducing 4 new prefetch tokens?

chandra-bala commented 3 years ago

@cfeltner

If the CDS Service just needs Practitioner or PractitionerRole in the prefetch they could use the existing userId prefetch token.

This is not really accurate as the userId is not a simple ID but rather is of the form Practitioner/<practitionerId> or PractitionerRole/<practitionerRoleId>. The CDS Service currently can only set the pre-fetch query for a user like this:

{{context.userId}}

This will return whatever the resource it was pointing to originally (either Practitioner or PractitionerRole) which is decided by the CDS client. The CDS Service can't control which resource it wants to get with this form of userId.

This is one of the things that Isaac's proposal would address with resource specific pre-fetch tokens. The service could then be setup with the pre-fetch queries like:

  1. Practitioner/{{userPractitionerId}} if it wants Practitioner resource only or
  2. PractitionerRole/{{userPractitionerRoleId}} if it wants PractitionerRole resource only or
  3. PractitionerRole?_id={{userPractitionerRoleId}}&_include=PractitionerRole:practitioner if it wants both

The one thing that I'm not sure about this approach is, what if the user doesn't have a roleId, how would the last query above would be handled? I'm imagining it won't return anything, however, should the CDS Service still have the existing pre-fetch query {{context.userId}} as a fallback in this case to always get some kind of a user? Or should the Service be using both of the first 2 queries as the recommended approach?

cfeltner commented 3 years ago

@chandra-bala Yes, the resource type is included in the userId. We could do the same for the userRoleId. So for a hook focused on provider usage like order-sign, the context would look like:

"context": {
    "userId": "Practitioner/123",
    "userRoleId": "PractitionerRole/1",
    "encounterId": "456"
}

A possible prefetch template could be:

  "prefetch": {
      "practitioner": "{{context.userId}}",
      "role": "{{context.userRoleId}}",
      "encounter": "Encounter/{{context.encounterId}}"
  }
}

Since the context.userId can represent these different types, it would seem that the type of the logged in user would would dictate which would be used or if the hook implementation guide specified the type. If a provider is logged in, then the userId would be Practitioner/123. If a patient is logged in, the userId would be Patient/123.

The userRoleId would be an optional prefetch token and would only be needed if the logged in user is a provider.

brynrhodes commented 3 years ago

Re-reviewing this thread and the proposed solutions, I agree with @isaacvetter 's proposed solution on the grounds that it's really the only solution that has addressed the real issue, which is that the resource type must be statically known in order for pre-fetch templates to be written effectively. In general, you have to know what kind of id you have in order to use it correctly in a pre-fetch template, and so Isaac's proposed solution of "hook-independent pre-fetch tokens" addresses that problem generally for all hooks, doesn't require modification of any existing hook definitions, and allows the solution to be solved without changing any existing behavior.

jmandel commented 3 years ago

In general, you have to know what kind of id you have in order to use it correctly in a pre-fetch template,

Why is Chuck's example not "correct"? Paraphrasing the example:

  "prefetch": {
      "p": "{{context.userId}}",
      "e": "Encounter/{{context.encounterId}}"
  }

If userId can be "Practitioner/123", or "PractitionerRole/456", or even "RelatedPerson/567", this template is still correct, no?

chandra-bala commented 3 years ago

@cfeltner

If a provider is logged in, then the userId would be Practitioner/123. If a patient is logged in, the userId would be Patient/123. The userRoleId would be an optional prefetch token and would only be needed if the logged in user is a provider.

Again, this seems to suggest that we change the meaning of "userId" to only be of type Practitioner/123 if the user is a provider. This would be a breaking change for us as we return PractitionerRole/123 if the user is a provider, otherwise we default to Practitioner/123 (happens for analysts who are testing things).

@jmandel

Why is Chuck's example not "correct"? Paraphrasing the example:

As explained above, Chuck's proposal solves the issue to send both Practitioner and PractitionerRole resources, only if we can definitively make userId a Practitioner/123 whenever the user is provider. However, the existing CDS Hooks spec doesn't specify this and many existing implementations (like ours) could be returning whatever resource they can (we chose PractitionerRole).

Even if we go with this solution and we do make that expectation from now on, this type of ResourceType/Id form of an ID is difficult to make advanced pre-fetch queries like:

PractitionerRole?_id={{userPractitionerRoleId}}&_include=PractitionerRole:practitioner

The above query just can't use the context.userRoleId which is of form PractitionerRole\123 that is being proposed. The CDS client has to do special parsing to make it work, like:

  1. Determine if the usage of {{context.userRoleId}} in the query is for its Id.
  2. If so, replace the token with the Id instead of the ResourceType/Id format.

Based on all these reasons, I'm strongly leaning towards the hook-independent, resource specific pre-fetch tokens for the Ids and completely avoiding any new Ids of the form ResourceType/Id which is just not very flexible (unless we introduce modifiers).

dennispatterson commented 3 years ago

Apologies for misrepresenting @cfeltner's proposal as updated context rather than a prefetch token. I also agree with the statements made by @isaacvetter and @chandra-bala that we can't force existing implementations to stop using PractitionerRole within userId.

My counter-proposal that Isaac shared was a modifier, only applicable for multi-type context fields (of which this is the first), that would allow using the id if only if it's a particular type, kind of like the prefetch templates. Example:

"prefetch": {
      "user": "{{context.userId}}",
      "practitionerAndRole-Modifier": "PractitionerRole?_id={{context.userId:PractitionerRole}}&_include=PractitionerRole:practitioner",
      "practitionerAndRole-Token": "PractitionerRole?_id={{userPractitionerRoleId}}&_include=PractitionerRole:practitioner"
  }

1 and 2 seem interchangeable to me, but 1 defines a pattern for multi-type fields instead of a list of specific tokens.

However, the other goal I'm hoping to clarify is the desire to get the raw Practitioner id when the user is a PractitionerRole. It sounds like we're trying to say when the context.userId is PractitionerRole/123, we also want to support this prefetch:

"prefetch": {
      "practitionerFromRole": "Practitioner/{{userPractitionerId}}"
  }

Is that a must-have? That seems to beg for a separate context parameter, which is what I've been having a hard time with. That ship may have sailed when we made a design choice to add PractitionerRole into userId instead of starting out with allowing for both ids. If so, the token approach seems like the only way here without context redesign.

jmandel commented 3 years ago

@chandra-bala

As explained above, Chuck's proposal solves the issue to send both Practitioner and PractitionerRole resources, only if we can definitively make userId a Practitioner/123 whenever the user is provider. However, the existing CDS Hooks spec doesn't specify this and many existing implementations (like ours) could be returning whatever resource they can (we chose PractitionerRole).

I can't understand what you're saying here. If we say "context.xyz" is a string of the form :resourceType/:id, where resourceType is Patient|RelatedPerson|Pracitioner|PractitionerRole|Person (as in SMART App Launch's fhirUser claim), then a prefetch request can readily ask for

{
  "a": "{{context.xyz}}",
  "b": "PractitionerRole?practitioner={{context.xyz}}",
}

this context would:

... and of course different contexts could be created to meet other needs. The pattern feels pretty flexible.

Does this make sense, or have I already said something confusing?

Edit: I think I'm starting to understand the problem is th other direction, when you're going to practitioner role and you want to look up the associated practitioner. You can't quite do it without string manipulation.

chandra-bala commented 3 years ago

@dennispatterson

However, the other goal I'm hoping to clarify is the desire to get the raw Practitioner id when the user is a PractitionerRole. Is that a must-have?

Yes, we have a business use case where this is exactly what we need. To avoid duplication of data in resources, we implemented the resources such that some details are only available in PractitionerRole (like specialty) and some details are only available in Pratitioner (like NPI). The only way for a CDS Service at this point to get both resources is:

  1. Read the PractitionerRole resource they received in pre-fetch and parse it to get the reference to corresponding Practitioner resource.
  2. Then, run an ad hoc FHIR query for the Practitioner resource.

And this is information that the service wants on every call. So, doing above impacts performance and not to forget any networking setup the service has to do with each CDS client to make such call backs to CDS Client's FHIR server. This is the primary motivation for proposing more flexibility to our pre-fetch functionality.