Fixes Error prone null checks in Bunsen module #1491

ndegwamartin · 2025-10-06T17:24:59Z

Description of what I changed

Resolves #1474

E2E test

TESTED:

Please replace this with a description of how you tested your PR beyond the
automated e2e/unit tests.

Checklist: I completed these to help reviewers :)

I have read and will follow the
review process.
I am familiar with Google Style Guides for the language I have coded in.

No? Please take some time and review
Java and
Python style guides.
My IDE is configured to follow the Google
code styles.

No? Unsure? ->
configure your IDE.
I have added tests to cover my changes. (If you refactored existing
code that was well tested you do not have to add tests)
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.
If I made any Python code changes, I ran black . and pylint . right
before creating this pull request and added all formatting changes to my
commit.
All new and existing tests passed.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

codecov-commenter · 2025-10-06T17:33:02Z

Codecov Report

❌ Patch coverage is 45.09804% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.40%. Comparing base (c3e8a91) to head (d3200dc).

Files with missing lines	Patch %	Lines
...erner/bunsen/definitions/StructureDefinitions.java	36.36%	2 Missing and 5 partials ⚠️
...ner/bunsen/definitions/HapiCompositeConverter.java	60.00%	0 Missing and 6 partials ⚠️
...ner/bunsen/definitions/HapiContainedConverter.java	0.00%	6 Missing ⚠️
...r/bunsen/avro/tools/GenerateAggregatedSchemas.java	0.00%	3 Missing ⚠️
...a/com/cerner/bunsen/definitions/EnumConverter.java	0.00%	0 Missing and 2 partials ⚠️
...bunsen/definitions/r4/R4FhirConversionSupport.java	0.00%	1 Missing ⚠️
...en/definitions/stu3/Stu3FhirConversionSupport.java	0.00%	1 Missing ⚠️
...java/com/cerner/bunsen/ProfileMappingProvider.java	88.88%	0 Missing and 1 partial ⚠️
...cerner/bunsen/definitions/HapiChoiceConverter.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1491      +/-   ##
============================================
- Coverage     46.52%   46.40%   -0.12%     
- Complexity      677      679       +2     
============================================
  Files            90       90              
  Lines          5892     5913      +21     
  Branches        826      844      +18     
============================================
+ Hits           2741     2744       +3     
- Misses         2843     2848       +5     
- Partials        308      321      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bashir2

Just sharing some preliminary comments as we discussed.

bunsen/pom.xml

bashir2 · 2025-10-15T02:56:39Z

bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/AvroConverter.java

We will get a NPE if getElementType() returns null here; so we should handle it if we allow null in the base class; ditto on line 77 above. Can you please check all usages of getElementType and make sure that null is handled?

All existing usages of getElementType are null safe.

For usages of FhirContext.getResourceDefinition specifically, there's a validation check for blankness which throws an IllegalArgument exception if it is, see https://hapifhir.io/hapi-fhir/apidocs/hapi-fhir-base/src-html/ca/uhn/fhir/context/FhirContext.html#line-594

bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java

bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/tools/GenerateAggregatedSchemas.java

ndegwamartin · 2025-10-30T18:07:58Z

/gcbrun

ndegwamartin · 2025-11-07T13:19:04Z

/gcbrun

bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/tools/GenerateAggregatedSchemas.java

bashir2 · 2025-11-12T17:55:33Z

bunsen/bunsen-avro/src/test/java/com/cerner/bunsen/avro/R4AvroConverterCustomProfileTest.java

        AvroConverter.forResources(fhirContext, patientProfiles, 1);

-    avroBunsenTestProfilePatient =
+    Record testBunsenTestProfilePatientRecord =


nit: What is the benefit of the new aux variable testBunsenTestProfilePatientRecord? If none, then revert it?

We need the variable in order to assert for null before proceeding with the rest of the logic.

The other change is reducing the scope the variable from field to local for the specific test case using it.

bashir2 · 2025-11-12T17:57:51Z

bunsen/bunsen-avro/src/test/java/com/cerner/bunsen/avro/R4AvroConverterUsCoreTest.java

            fhirContext, R4UsCoreProfileData.US_CORE_OBSERVATION_PROFILES, 1);

-    avroObservation = (Record) observationConverter.resourceToAvro(testObservation);
+    Record record = (Record) observationConverter.resourceToAvro(testObservation);


nit: Ditto here and elsewhere that you have added a new aux var; if there is a reason, feel free to keep them, if not please revert them to minimize the changes.

For this case (and most of them), it is the approach we are using to satisfy NullAways nullability check, otherwise it fails, i.e.

Record record = (Record) observationConverter.resourceToAvro(testObservation); Assert.assertNotNull(record); avroObservation = record;

bashir2 · 2025-11-12T18:06:08Z

...sen-core-stu3/src/main/java/com/cerner/bunsen/definitions/stu3/Stu3StructureDefinitions.java

    }

    @Override
+    @Nullable


Shouldn't getFirstTypeProfile also be annotated as @Nullable?

bashir2 · 2025-11-12T18:06:43Z

bunsen/bunsen-core/src/main/java/com/cerner/bunsen/ProfileMapperFhirContexts.java

  private final Map<FhirVersionEnum, FhirContextData> fhirContextMappings;
+
+  @SuppressWarnings(
+      "NullAway.Init") // This is initialized/used via the singleton pattern getInstance method.


nit: please move the comment to a separate line before the annotation (ditto below).

bashir2 · 2025-11-12T20:53:08Z

bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/HapiCompositeConverter.java

      // Meta element.
-      values[valueIndex++] = schemaEntry.result().fromHapi(((IAnyResource) composite).getMeta());
+      Object value = schemaEntry.result().fromHapi(((IAnyResource) composite).getMeta());
+      if (value != null) values[valueIndex++] = value;


Ditto; and I think there is a bigger problem here: When value == null we are skipping valueIndex++ hence creating a mismatch between the values and children list sizes (which I think can cause other issues later in the code, if we break this invariant). In general, I would suggest that null related fixes should either be a no-op or some tests added for code changes to make sure we are not unintentionally introducing new bugs.

bashir2 · 2025-11-12T20:53:37Z

bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/HapiCompositeConverter.java


-          values[valueIndex] = schemaEntry.result().fromHapi(propertyValues);
+          Object value = schemaEntry.result().fromHapi(propertyValues);
+          if (value != null) values[valueIndex] = value;


ditto here and below

bashir2 · 2025-11-12T20:54:18Z

bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/HapiContainedConverter.java

-      values[valueIndex] = schemaEntry.result().fromHapi(composite);
+      if (schemaEntry != null) {
+        Object value = schemaEntry.result().fromHapi(composite);
+        if (value != null) values[valueIndex] = value;


bashir2 · 2025-11-12T20:55:21Z

bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/HapiConverter.java

     * @return the HAPI equivalent.
     */
-    IBase toHapi(Object input);
+    @Nullable IBase toHapi(Object input);


nit: please be consistent with how you place the @Nullable annotation, e.g., always place it in a separate line (like you have done elsewhere).

bashir2 · 2025-11-12T21:10:48Z

bunsen/bunsen-core/src/main/java/com/cerner/bunsen/definitions/StructureDefinitions.java

   * data in siblings.)
   */
+  // TODO perform a deep review of this method's code and the implication of various nullability
+  // check approaches


Leaving this comment here to make sure we resolve this TODO before merge.

Fixes Error prone null checks in Bunsen module

dca2614

ndegwamartin requested a review from bashir2 October 6, 2025 17:24

Marks the areas of code we need deeper investigations with TODO tags

6ccbf76

ndegwamartin marked this pull request as ready for review October 10, 2025 17:22

bashir2 reviewed Oct 15, 2025

View reviewed changes

ndegwamartin added 2 commits October 24, 2025 17:27

Merge remote-tracking branch 'origin/master' into bunsen-nullchecks

1ce573b

Addresses PR Feedback

6ab5f10

ndegwamartin and others added 4 commits November 4, 2025 16:09

Fixes NullAway warnings (Implementation Code)

e0901bb

Fixes nullway warnings | Test code

aea00c9

Merge branch 'master' into bunsen-nullchecks

040b8d8

Merge branch 'master' into bunsen-nullchecks

d3200dc

bashir2 reviewed Nov 12, 2025

View reviewed changes

Fixes Error prone null checks in Bunsen module #1491

Are you sure you want to change the base?

Fixes Error prone null checks in Bunsen module #1491

Uh oh!

Conversation

ndegwamartin commented Oct 6, 2025

Description of what I changed

E2E test

Checklist: I completed these to help reviewers :)

Uh oh!

codecov-commenter commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bashir2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ndegwamartin commented Oct 30, 2025

Uh oh!

ndegwamartin commented Nov 7, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Oct 6, 2025 •

edited

Loading