Add FHIR Type to fromFHIR function signature #46

dehall · 2019-05-24T18:29:45Z

Adds FHIR type to the fromFHIR function signature, as a foundation for future changes to address #35. One solution referenced there uses the approach that the "receiving" function, such as shr.base.Reason.fromFHIR in that example, uses a lookup to figure out what the incoming data is. After some thought that seems potentially error-prone, because the "sending" function should in theory always know what the type of everything actually is.

Where this is most valuable is in cases like Observation which has many different options for value[x]. This proposed approach would result in something like this:

    if (fhir['valueCodeableConcept'] != null) {
      inst.dataValue = FHIRHelper.createInstanceFromFHIR('shr.slicing.DataValue', fhir['valueCodeableConcept'], 'CodeableConcept', shrId, allEntries, mappedResources, referencesOut, false);
    }
    if (fhir['valueQuantity'] != null) {
      inst.dataValue = FHIRHelper.createInstanceFromFHIR('shr.slicing.DataValue', fhir['valueQuantity'], 'Quantity', shrId, allEntries, mappedResources, referencesOut, false);
    }
    if (fhir['valueString'] != null) {
      inst.dataValue = FHIRHelper.createInstanceFromFHIR('shr.slicing.DataValue', fhir['valueString'], 'string', shrId, allEntries, mappedResources, referencesOut, false);
    }
    if (fhir['valueTime'] != null) {
      inst.dataValue = FHIRHelper.createInstanceFromFHIR('shr.slicing.DataValue', fhir['valueTime'], 'time', shrId, allEntries, mappedResources, referencesOut, false);
    }
    if (fhir['valueDateTime'] != null) {
      inst.dataValue = FHIRHelper.createInstanceFromFHIR('shr.slicing.DataValue', fhir['valueDateTime'], 'dateTime', shrId, allEntries, mappedResources, referencesOut, false);
    }

And then DataValue.fromFHIR can reference the given type to know what to do with the given object.

Note that nothing actually uses this yet, that fix will have to come later as I find more time. The intent for now is just that this will make manual fixes easier, so only the function signature being correctly aligned is needed right now.

cmoesel

Haven't fully reviewed/tested, but have a few questions...

cmoesel · 2019-05-24T18:44:12Z

lib/generateClass.js

@@ -1740,7 +1747,8 @@ function generateFromFHIRAssignment(field, fhirElement, fhirElementPath, shrElem
      if (shrType.isPrimitive && !isExtension) {
        rhs = fhirPathString;
      } else {
-        rhs = `FHIRHelper.createInstanceFromFHIR('${shrType.fqn}', ${fhirPathString}, shrId, allEntries, mappedResources, referencesOut, ${isExtension})`;
+        const fhirType = fhirElement.type[0].code;


Is it safe to assume that the type at index 0 is the right type?

Good question. Off the top of my head I'm not sure of all the reasons that a FHIR element might have multiple types, but if this was originally a choice type like value[x] then it's been pre-processed so that it appears to be a single choice with a single type, and that's the use case where this is most relevant.
Not conclusive proof, but running through the current spec there are no instances where there is a second type available to choose here.

OK. Yes, if we've already handled [x] types before this then I think that should be fine.

cmoesel · 2019-05-24T18:48:27Z

lib/generateFactory.js

@@ -41,15 +41,16 @@ function generateFactory(namespaces) {
      cw.blComment(() => {
        cw.ln('Create an instance of a class from its FHIR representation.')
          .ln('@param {Object} fhir - The element data in FHIR format (use `{}` and provide `type` for a blank instance)')
-          .ln(`@param {string} [type] - The (optional) type of the element (e.g., 'http://standardhealthrecord.org/spec/shr/demographics/PersonOfRecord').  This is only used if the type cannot be extracted from the JSON.`)
+          .ln(`@param {string} fhirType - the type of the FHIR object that was passed in, in case not otherwise identifiable from the object itself`)


Is fhirType optional? I noticed you pass in null for it in some of the cases above. So it seems it must be optional? The shrType below explicitly states that it is optional (in the description).

Also note that in jsdoc, you can indicate a parameter is optional by surrounding it in [ ]. See: https://jsdoc.app/tags-param.html#optional-parameters-and-default-values

Actually, I see you did use the [ ] jsdoc syntax in the namespace factory... so I guess you knew. Maybe you missed it here?

Yeah I think I first learned I could do that between here and the namespace factory. For consistency I'll change the namespace factory comment to remove the brackets. Technically whether it's optional or not will depend on the individual class, since most classes won't use it but some will require it.

dehall · 2019-05-28T10:53:11Z

I just thought of a good test case for this, so I will plan to add that today as well. Following along the example in the description above, mock out the DataValue.fromFHIR function and test that it gets passed in the expected fhirType using Observations with varying values

cmoesel · 2019-05-28T12:37:02Z

@dehall -- OK. I was planning to review and merge this today, so let me know when it is ready.

dehall · 2019-05-28T13:12:32Z

@cmoesel ready for review

cmoesel

Code looks good, tests pass, but I'd suggest one change for consistency. See individual comment for details.

cmoesel · 2019-05-28T13:51:38Z

lib/includes/json-helper.js

-  createInstanceFromFHIR: function(key, value, shrId, allEntries=[], mappedResources={}, referencesOut=[], asExtension=false) {
-    if (Array.isArray(value)) {
-      return value.map(v => FHIRHelper.createInstanceFromFHIR(key, v, shrId, allEntries, mappedResources, referencesOut, asExtension));
+  createInstanceFromFHIR: function(shrType, fhir, fhirType, shrId, allEntries=[], mappedResources={}, referencesOut=[], asExtension=false) {


The code has three basic implementations of functions called createInstanceFromFHIR: this one, a high-level one generated on ObjectFactory, and one on each NamespaceFactory.

The latter two have signature:

function(fhir, fhirType, shrType, shrId=uuid(), allEntries=[], mappedResources={}, referencesOut=[], asExtension=false)

But this one has signature:

function(shrType, fhir, fhirType, shrId, allEntries=[], mappedResources={}, referencesOut=[], asExtension=false)

Note that they are almost exactly the same except for the order of the first three arguments. Ideally, there should be consistency between all of them in order to avoid stupid and unintentional mistakes (probably to be made by me).

So... can we either change this signature to match the others or change the others to match this one?

I updated the other locations to match this one, I think it's fewer code changes and in simpler places, plus the ordering feels more consistent with how createInstanceFromFHIR('ns.Class', fhir, ...) turns into Class.fromFHIR(fhir, ....) .
Also I updated the comments for shrType similarly to the above, originally they said "optional" but it can be required depending on other parameters.

cmoesel

Looks good!

dehall added 3 commits May 24, 2019 12:15

initial impl of adding fhirType to fromFHIR signature

0b76561

little more robust

bcd3d87

cleanup and doco

cb179e7

dehall mentioned this pull request May 24, 2019

Class registry concept #47

Merged

cmoesel reviewed May 24, 2019

View reviewed changes

dehall added 2 commits May 28, 2019 09:10

update comment

3b528c6

add test case

71b92ba

cmoesel suggested changes May 28, 2019

View reviewed changes

rearrange parameters

574c7fc

cmoesel approved these changes May 28, 2019

View reviewed changes

cmoesel merged commit 0d46aaf into master May 28, 2019

cmoesel deleted the add_fhirtype branch May 28, 2019 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FHIR Type to fromFHIR function signature #46

Add FHIR Type to fromFHIR function signature #46

dehall commented May 24, 2019

cmoesel left a comment

cmoesel May 24, 2019

dehall May 24, 2019

cmoesel May 24, 2019

cmoesel May 24, 2019

cmoesel May 24, 2019

dehall May 28, 2019

dehall commented May 28, 2019

cmoesel commented May 28, 2019

dehall commented May 28, 2019

cmoesel left a comment

cmoesel May 28, 2019

dehall May 28, 2019

cmoesel left a comment

Add FHIR Type to fromFHIR function signature #46

Add FHIR Type to fromFHIR function signature #46

Conversation

dehall commented May 24, 2019

cmoesel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dehall commented May 28, 2019

cmoesel commented May 28, 2019

dehall commented May 28, 2019

cmoesel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmoesel left a comment

Choose a reason for hiding this comment