Stream: cds hooks/github
Topic: docs / Issue #133 Extension specification issues
Github Notifications (Jan 02 2018 at 17:00):
JustinStauffer opened Issue #133
This is in regard to the concept of extensions discussed in issue 76 (https://github.com/cds-hooks/docs/issues/76). Since that issue is closed, I created a new one to track this.
Currently, the CDS Hooks extension model uses a dynamically named element (https://github.com/cds-hooks/docs/commit/b34534ee30c61e99489f86df7662efde65559663).
An example is:
"extension": { "example-client-conformance": "http://my.hooks.org/fhir/102/Conformance/patientview" }This dynamically named element will require customization to off-the-shelf JSON libraries because it uses a dynamically named element (e.g. "example-client-conformance") whose name is not determined at design time. For example, if you have a C# .NET class that represents this extension element, it would look like this:
public class extension { public string example-client-conformance { get; set; } }
(This example would not work in .NET because the property name has dashes but let's forget that detail for now.)Now if someone wants to use a different extension name (such as "myextensionproperty"), application developers will need to add another property to their class called "myextensionproperty";
public class extension { public string example-client-conformance { get; set; } public string myextensionproperty { get; set; } }That is obviously not sustainable.
Yes, there are ways one can program around this and everyone will need to do so to handle elements like prefetch. However, this is another case that requires customization and since this piece of the specification is new, we should fix it before it is finalized so we do not require yet more customization to off-the-shelf APIs which makes it more difficult for client and service developers to support this part of the specification.
Instead of the current approach, can we model this more like a FHIR Extension (http://hl7.org/fhir/extensibility.html)? I don't think we have to get _quite as complicated_ as FHIR's extensibility model but using a key/value pair would be much more preferable than using dynamically named JSON properties.
For example, this is readable/writable using a standard JSON formatter/writer and does not require customizing out-of-the-box / off-the-shelf JSON libraries:
"services": [ { "extension": [{ "url": "http://domain/example-client-conformance", "value": "http://my.hooks.org/fhir/102/Conformance/patientview" }, { "url": "http://domain/example-client-another-extension", "value": "another-extension-value" }] }Notice that I simplified this a little from the FHIR Extension type in that I am just using one "value" property instead of "valueInt", "valueDecimal", "valueString", etc.
Github Notifications (Jan 08 2018 at 21:59):
kpshek commented on Issue #133
Hi @JustinStauffer. Is the core of your issue here the same as #89? I realize that #89 was in regards to prefetch but this reads to me as being technically the same argument -- dynamic keys pose a challenge in your use case.
In these two issues are technically the same (but just different domains -- prefetch vs extensions), can you elaborate on what caused #89 to ultimately get closed? Namely, rather than trying to model the extensions as first class attributes for JSON serialization libraries, can you simply get/set the extension via the JSON map/hash that all libraries I've worked with offer?
For instance, using Json.NET, here is documentation from that project which shows how this is done.
Github Notifications (Jan 08 2018 at 22:08):
JustinStauffer commented on Issue #133
Yes, they are stemming from the same root cause.
Basically, if you take a .NET class (or presumably Java) and simply wish to deserialize it into a JSON representation, you cannot do that when the element name (which is represented via a property on that class) has a variable name that is not pre-defined at design time.
To get around that, you have to do something like making a new class (like one called "DynamicallyNamedElement") that has a property representing the dynamic element name and its value (instead of using a simple string, decimal, etc.). Then you need to customize your deserialization code to detect the presence of that "DynamicallyNamedElement" class and leave it out of your resulting JSON and make the appropriate name adjustments for the dynamic element name. Essentially, it calls for customization to your JSON formatting code wherever these dynamic element names came up.
I was able to work around this issue for prefetch using an approach like I mentioned above. It seemed to me at the time that nobody was willing to change the specification then because multiple people had implemented prefetch the way it was designed and did not want to change their implementation. However, since this is a new part of the specification, we will not have that justification this time and should do our best to make the spec work with off-the-shelf tools and approaches that do not require making custom JSON formatting/deserialization/serialization code to handle quirks in our message model.
Github Notifications (Jan 09 2018 at 11:31):
kpshek commented on Issue #133
@JustinStauffer - What .NET JSON library are you using?
Github Notifications (Jan 09 2018 at 14:52):
JustinStauffer commented on Issue #133
Newtonsoft.
I wasn't aware of the functionality you linked above. Seems helpful. However, it still requires customization that would not be necessary if we modeled this as a key/value pair just like HL7 has done with FHIR. Why are we proposing to do it differently when there is already an existing solution to this in a very similar specification that does not require such customization?
Github Notifications (Jan 09 2018 at 15:50):
kpshek commented on Issue #133
@JustinStauffer - Cool! I guessed right with the example in my previous comment. :smile:
I'd prefer to design the API without looking at each and every JSON library across the various languages and platforms. Generally speaking, this is a sound approach as all of the JSON libraries I've used in the past are more than capable of handling de/serialization of a variety of structures. In this case, we have a very clear & clean path for how you can accomplish this with the Newtonsoft Json.NET library.
The design of how extensions are handled in CDS Hooks also is consistent with our
prefetchstructure and I think this is a lot of value in structural consistency. Additionally, the community overwhelming agreed that our approach there was not concerning (which resulted in #89 being closed).
Github Notifications (Jan 16 2018 at 16:21):
JustinStauffer commented on Issue #133
What is the justification for doing this differently than an existing, tested standard called FHIR?
Github Notifications (Jan 16 2018 at 21:12):
kpshek commented on Issue #133
@JustinStauffer - The community didn't agree to this with prefetch because existing implementation had already implemented it one way. The problem described there (same issue here) did not receive support from the community which is why we didn't change the design.
CDS Hooks leverages FHIR data but isn't based upon FHIR API design constructs. Your question of of why we didn't design it according to a particular standard isn't the right question to ask as we could all point to competing examples from other APIs, standards, etc.
Instead, the question and discussion should focus on why your proposal is a better design. As discussed earlier, you have a method to de/serialize the JSON with Json.NET (in the WGM discussion around #89, other .NET developers in the room did not share your concerns). Additionally, your proposed design runs counter to our design for
prefetchand your comment today did not address why it would be acceptable to have conflicting designs for prefetch and extensions.Barring other issues or a more compelling argument, I don't see a need to change our extension design. Others following this thread are encouraged to join in the discussion.
Github Notifications (Jan 16 2018 at 21:47):
brynrhodes commented on Issue #133
I agree with @kpshek here, consistency with the prefetch approach, combined with ease of use for the most common consumers provide strong justification for the approach we're proposing.
Note also that the specification is not prescriptive on this point. The specification is only providing an example that we think is in line with the rest of the specification.
Github Notifications (Jan 16 2018 at 21:53):
JustinStauffer commented on Issue #133
I wasn't involved in the community when prefetch was first being discussed and if I was, I would have raised more of a stink about it then. How do you have a "standard" message structure when the _element names_ are not defined? That seems ludicrous to me and I don't like that in prefetch but I am dealing with it because I know that ship has sailed.
Also, here's the issue with the "solution" that's available in Newtonsoft. Our approach takes a simple object model that a customer can create -- not a developer, a customer analyst for example. That simple model is then translated into a C# object and that object is used to serialize/deserialize to/from JSON. With the currently proposed solution, we'd have to make adjustments to this translation since it is no longer a simple one-to-one mapping. Of course there are ways to work around this but that's not the main problem... Despite all the details of JSON library implementations, the proposed solution simply doesn't work at design time for the same reason that prefetch doesn't. Compare this with any other schema-based model representing a message structure. HL7, IHE, etc. No other schema-based models use elements with dynamic element names because you cannot represent them in a schema. Does the "community" not care about this? Do we not care about having schema-based validation, code generation tools, etc.? I suspect most in the community probably don't understand the consequences of that or don't care because they're taking less (or not at all) design-time driven approaches.
Yes I know CDS Hooks is not FHIR but it seems to me we should indeed be borrowing ideas from other existing, more proven standards that have larger communities contributing to them. Why do we need to reinvent the wheel just because CDS Hooks is a different use case? FHIR has solved this and CDS Hooks is _barely_ different from FHIR and this is a divergence from FHIR which will make it less simple for folks familiar with and currently supporting FHIR to implement this specification because they keep having exceptions in one place or the other to how things are done in every other message model in existence.
Also, prefetch is not the only comparable in CDS Hooks. Look at "Action" in the Response message. You have Action/type and Action/resource where type = create, update, or delete (so it's a key-value pair exactly like I want for Extension). Why was that not designed as Action/create/resource, Action/update/resource, Action/delete/resource, or Action/somefuturething/resource? Therefore, the proposed Extension model is not consistent with how CDS Hooks has defined the Action element.
Finally, you said we should focus on why the "alternative" more-FHIR-like approach is better without talking about details of APIs but then you use the JSON.NET API as argument against my proposed change... I think the points I raise above about the Action element model, design-time tooling and validation, as well as the fact that FHIR has already solved this issue are pretty compelling reasons to use their approach (or at least something more similar).
Github Notifications (Jan 16 2018 at 22:00):
JustinStauffer commented on Issue #133
Also, the original proposal for Extensions made by robs16 used the FHIR-based approach:
https://github.com/cds-hooks/docs/issues/76I don't see any discussion in that thread about using the currently proposed approach in the following commit by @brynrhodes: https://github.com/cds-hooks/docs/commit/b34534ee30c61e99489f86df7662efde65559663
Where is the discussion history available describing why the proposal was changed and implemented the way it was?
Github Notifications (Jan 16 2018 at 22:02):
brynrhodes commented on Issue #133
The discussion is on issue #76. We proposed the approach we have implemented and asked for comment, we didn't receive any for over a month so we committed the change as described in that discussion (namely an extension element whose contents were not prescribed by the spec).
Github Notifications (Jan 16 2018 at 22:03):
JustinStauffer commented on Issue #133
@brynrhodes sorry, I missed that original proposal. I found out about this from a co-worker. Consider my comments on this thread as feedback on that proposal 1 month ago.
Github Notifications (Jan 17 2018 at 00:00):
brynrhodes commented on Issue #133
No worries @JustinStauffer. I think the core of the issue is that these are extensions, they're by definition not known at design time, and messaging schemas that encode extensions just change where they have to be dealt with. To put it another way, it seems like imposing a constraint of strongly-typed systems on an API that's expressed natively in a technology that doesn't have that constraint, and if we do that, the cost is that systems that _can_ deal with dynamically-named elements lose that capability.
Github Notifications (Jan 17 2018 at 16:30):
JustinStauffer commented on Issue #133
That's incorrect that all extensions are not known at design time. Extensions are sometimes used as temporary placeholders for new features that are intended to be brought into the standard but need short-lived homes for early adopters.
I also don't quite get what you're saying "the cost is that systems that can deal with dynamically-named elements lose that capability". What capability? What benefit is there to a dynamically named element?
Github Notifications (Jan 28 2018 at 03:31):
brettmarquard closed Issue #133
Github Notifications (Jan 28 2018 at 03:31):
brettmarquard commented on Issue #133
Discussed offline with @JustinStauffer, agreed to close.
Last updated: Apr 12 2022 at 19:14 UTC
