DocumentReference 30_40 converter produces NPE on valid DocumentReference.status element

[tadgh]

1. Create a document reference in R4.
2. Set theDocumentReference.docStatus = null
3. Set theDocumentReference.getDocStatusElement().addExtension(someExtension)
4. Call VersionConvertorFactory_30_40.convertResource(theDocumentReference);
5. Notice the NPE:

java.lang.NullPointerException: Cannot invoke "org.hl7.fhir.r4.model.DocumentReference$ReferredDocumentStatus.ordinal()" because the return value of "org.hl7.fhir.r4.model.Enumeration.getValue()" is null

	at org.hl7.fhir.convertors.conv30_40.resources30_40.DocumentReference30_40.convertReferredDocumentStatus(DocumentReference30_40.java:275)
	at org.hl7.fhir.convertors.conv30_40.resources30_40.DocumentReference30_40.convertDocumentReference(DocumentReference30_40.java:33)
	at org.hl7.fhir.convertors.conv30_40.resources30_40.Resource30_40.convertResource(Resource30_40.java:276)
	at org.hl7.fhir.convertors.conv30_40.resources30_40.Resource30_40.convertResource(Resource30_40.java:213)
	at org.hl7.fhir.convertors.conv30_40.VersionConvertor_30_40.convertResource(VersionConvertor_30_40.java:111)
	at org.hl7.fhir.convertors.conv30_40.resources30_40.Bundle30_40.convertBundleEntryComponent(Bundle30_40.java:63)
	at org.hl7.fhir.convertors.conv30_40.resources30_40.Bundle30_40.convertBundle(Bundle30_40.java:48)
	at org.hl7.fhir.convertors.conv30_40.resources30_40.Resource30_40.convertResource(Resource30_40.java:242)
	at org.hl7.fhir.convertors.conv30_40.resources30_40.Resource30_40.convertResource(Resource30_40.java:213)
	at org.hl7.fhir.convertors.conv30_40.VersionConvertor_30_40.convertResource(VersionConvertor_30_40.java:111)
	at org.hl7.fhir.convertors.factory.VersionConvertorFactory_30_40.convertResource(VersionConvertorFactory_30_40.java:25)
	at org.hl7.fhir.convertors.factory.VersionConvertorFactory_30_40.convertResource(VersionConvertorFactory_30_40.java:20)
	```
	
Note that this example uses doc status, but I imagine this may exist for any enum'ed field? Worth looking into. 

[dmuylwyk]

Note that this example uses doc status, but I imagine this may exist for any enum'ed field? Worth looking into.

This may be limited to DocumentReference.status but I suspect it isn't.

The general case appears to be converting an unpopulated element of type code—which is generally backed by an enum—that includes one or more extensions. The most common extension would likely be a data-absent-reason extension but it's perfectly valid to add any extension to an unpopulated element in this manner.

[grahamegrieve]

@dotasek I think we've discussed this before - it does need a sweeping fix for this, to check if the enum .getValue() is null before doing the switch. Is my memory correct?

[dotasek]

@grahamegrieve I think what was done before was this: https://github.com/hapifhir/org.hl7.fhir.core/pull/1697/files, which is also involved null handling, but not what this fix is asking for.

I'll look into it.

[dotasek]

@grahamegrieve a structural search of this shows that these specific switches ( switch(enumeration.getValue() ) show up 2,369 times in the convertor code.

Some are covered with this kind of null check, which I believe is the correct approach:

if (src.getValue() == null) {
  tgt.setValue(org.hl7.fhir.dstu2.model.ElementDefinition.AggregationMode.NULL);
} else {
  switch (src.getValue()) {
    //...
  }
}

I will not be manually sorting through each of the 2,369 instances so I'm going to try to refine those results. On my first look, most of the missing null checks are in the resource conversions (e.g. org.hl7.fhir.convertors.conv10_30.resources10_30), but again, I can't be sure.

[dotasek]

For clarity, I believe the correct pattern for the fix would be the following, demonstrated on DocumentReference30_40.convertReferredDocumentStatus:

 if (src.getValue() == null) {
      tgt.setValue(org.hl7.fhir.dstu3.model.DocumentReference.ReferredDocumentStatus.NULL);
    } else {
      switch (src.getValue()) {
        //..
      }
    }

If that's correct, I can work on applying it, once I sort out how to find all the unchecked switches.

[grahamegrieve]

yes we're sure not manually reviewing >2000 references. Whatever we do is going to be done by code

but setting it to .NULL feels like a problem to me. It's not .NULL in the source, it's null, and we should set it to that value. It's supposed to be the same thing, but .NULL is not consistently handled - I've fixed bugs to do with that before, and I'm not even sure I should have defined them now

[dotasek]

Hm. So what do we do with all the existing conversions that convert to .NULL? Should they be fixed to null as well?

[dotasek]

@tadgh @jamesagnew: Grahame's response regarding the correct conversion behaviour came in Zulip:

I think you should ask the smileCDR team / James if they have an opinion

Intuitively I think the places where it does something like this are incorrect:

if (src.getValue() == null) {
  tgt.setValue(some.hl7.model.Enumeration.NULL);
} else {
  switch (src.getValue()) {
    //..
  }
}

I think the correct way to be performing this is the use an actual null, like so:

if (src.getValue() == null) {
  tgt.setValue(null);
} else {
  switch (src.getValue()) {
    //..
  }
}

This change will touch a lot of code, but I'm not sure if the impact will just be a few gently breaking tests that will just need an expected field removed or something worse.