Handle null values in fhir-server-config and throw for exceptional ca…

…se (#2822)

* Handle null values in fhir-server-config and throw for exceptional case

Previously, we'd through an IllegalStateException if we encountered a
null value. This came up because the helm `toJson` function produces all
possibly fields for an object, even when some are null.

If we have an invalid config (such as described above), in
FHIRRestHelper.validateResource we were swallowing the underlying
exception and instread constructing a generic OperationOutcomeIssue with
message "Error retrieving profile configuration". This was being handled
just like any other validation error and so, even though it was a server
config issue, this was not clear to the client.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Update PropertyGroup.getProperties to omit properties with null values

Now that convertJsonValue can return null, this is safer/easier than
updating all the callers to be able to handle null.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Update PropertyGroup.getJsonValue to normalize null and missing props

Previously this method returned null for missing properties, but
JsonValue.NULL for properties that exist in the config with a literal
value of null. We had null checks in all the getTypedProperty methods
that call this one, but none of them handled `JsonValue.NULL` and so
I've updated this one spot.

I also added unit test coverage for both PropertyGroup and
FHIRConfigHelper

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Update fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

Co-authored-by: Michael W Schroeder <66479070+michaelwschroeder@users.noreply.github.com>

Co-authored-by: Michael W Schroeder <66479070+michaelwschroeder@users.noreply.github.com>

fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java

@@ -11,14 +11,14 @@
 import java.util.List;
 import java.util.Map;
 
+import com.ibm.fhir.core.FHIRUtilities;
+
 import jakarta.json.JsonArray;
 import jakarta.json.JsonNumber;
 import jakarta.json.JsonObject;
 import jakarta.json.JsonString;
 import jakarta.json.JsonValue;
 
-import com.ibm.fhir.core.FHIRUtilities;
-
 /**
  * This class represents a collection of properties - a property group. This could be the entire set of properties
  * resulting from loading the configuration, or it could be just a sub-structure within the overall config hierarchy, as
@@ -107,7 +107,7 @@ public String getStringProperty(String propertyName, String defaultValue) throws
     /**
      * This is a convenience function that will retrieve an array property, then convert it
      * to a list of Strings by calling toString() on each array element.
-     * 
+     *
      * @param propertyName the name of the property to retrieve
      * @return a List<String> containing the elements from the JSON array property; possibly null
      * @throws Exception
@@ -237,12 +237,16 @@ public Object[] getArrayProperty(String propertyName) throws Exception {
     /**
      * Returns the properties contained in the PropertyGroup in the form of a list of
      * PropertyEntry instances. If no properties exist, then an empty list will be returned.
+     * Properties with a value of null will be omitted from the list.
      * @throws Exception
      */
     public List<PropertyEntry> getProperties() throws Exception {
         List<PropertyEntry> results = new ArrayList<>();
         for (Map.Entry<String, JsonValue> entry : jsonObj.entrySet()) {
-            results.add(new PropertyEntry(entry.getKey(), convertJsonValue(entry.getValue())));
+            Object jsonValue = convertJsonValue(entry.getValue());
+            if (jsonValue != null) {
+                results.add(new PropertyEntry(entry.getKey(), jsonValue));
+            }
         }
         return results;
     }
@@ -262,7 +266,7 @@ public String toString() {
     /**
      * Converts the specified JsonValue into the appropriate java.lang.* type.
      * @param jsonValue the JsonValue instance to be converted
-     * @return an instance of Boolean, Integer, String, PropertyGroup, or List<Object>
+     * @return either null or an instance of Boolean, Integer, String, PropertyGroup, or List<Object>
      * @throws Exception
      */
     public static Object convertJsonValue(JsonValue jsonValue) throws Exception {
@@ -292,6 +296,8 @@ public static Object convertJsonValue(JsonValue jsonValue) throws Exception {
         case FALSE:
             result = Boolean.FALSE;
             break;
+        case NULL:
+            break;
         default:
             throw new IllegalStateException("Unexpected JSON value type: " + jsonValue.getValueType().name());
         }
@@ -317,6 +323,7 @@ private static Object[] convertJsonArray(JsonArray jsonArray) throws Exception {
      *
      * @param propertyName
      *            the possibly hierarchical property name.
+     * @return the property value as a JsonValue or null if the property is either missing or has a null value
      */
     public JsonValue getJsonValue(String propertyName) {
         String[] pathElements = getPathElements(propertyName);
@@ -325,7 +332,7 @@ public JsonValue getJsonValue(String propertyName) {
         if (subGroup != null) {
             result = subGroup.get(pathElements[pathElements.length - 1]);
         }
-        return result;
+        return result == JsonValue.NULL ? null : result;
     }
 
     /**

fhir-config/src/test/java/com/ibm/fhir/config/test/FHIRConfigHelperTest.java

@@ -1,16 +1,16 @@
 /*
- * (C) Copyright IBM Corp. 2017, 2020
+ * (C) Copyright IBM Corp. 2017, 2021
  *
  * SPDX-License-Identifier: Apache-2.0
  */
 
 package com.ibm.fhir.config.test;
 
+import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotNull;
 import static org.testng.Assert.assertNull;
-import static org.testng.AssertJUnit.assertEquals;
-import static org.testng.AssertJUnit.assertNotNull;
-import static org.testng.AssertJUnit.assertTrue;
+import static org.testng.Assert.assertTrue;
 
 import java.io.PrintWriter;
 import java.util.Arrays;
@@ -34,6 +34,9 @@ public class FHIRConfigHelperTest {
     String[] array2 = { "token3", "token4" };
     List<String> expectedList2 = Arrays.asList(array2);
 
+    String[] arrayWithNull = { "token1", null, "token2" };
+    List<String> expectedListWithNull = Arrays.asList(arrayWithNull);
+
     // String used to write out json config file on the fly.
     String tenant3ConfigTemplate =
             "{ \"fhirServer\": { \"property1\": \"__A__\", \"property2\": \"__B__\" }}";
@@ -62,39 +65,72 @@ public void testDefaultConfig1() throws Exception {
         Double d;
         s = FHIRConfigHelper.getStringProperty("collection/groupA/stringProp1", null);
         assertNotNull(s);
-        assertEquals("defaultValue1", s);
+        assertEquals(s, "defaultValue1");
 
         s = FHIRConfigHelper.getStringProperty("collection/groupA/stringProp2", null);
         assertNotNull(s);
-        assertEquals("defaultValue2", s);
+        assertEquals(s, "defaultValue2");
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/boolProp1", null);
         assertNotNull(b);
-        assertEquals(Boolean.FALSE, b);
+        assertEquals(b, Boolean.FALSE);
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/boolProp2", null);
         assertNotNull(b);
-        assertEquals(Boolean.FALSE, b);
+        assertEquals(b, Boolean.FALSE);
 
         l = FHIRConfigHelper.getStringListProperty("collection/groupB/stringList1");
         assertNotNull(l);
-        assertEquals(expectedList1, l);
+        assertEquals(l, expectedList1);
 
         s = FHIRConfigHelper.getStringProperty("collection/groupB/boolProp1", null);
         assertNotNull(s);
-        assertEquals("false", s);
+        assertEquals(s, "false");
 
         i = FHIRConfigHelper.getIntProperty("collection/groupC/intProp1", null);
         assertNotNull(i);
-        assertEquals(12345, i.intValue());
+        assertEquals(i.intValue(), 12345);
 
         i = FHIRConfigHelper.getIntProperty("collection/groupC/intProp2", null);
         assertNotNull(i);
-        assertEquals(12345, i.intValue());
+        assertEquals(i.intValue(), 12345);
 
         d = FHIRConfigHelper.getDoubleProperty("collection/groupC/doubleProp2", null);
         assertNotNull(d);
-        assertEquals(12345.001, d.doubleValue());
+        assertEquals(d.doubleValue(), 12345.001);
+    }
+
+    @Test
+    public void testNullValues() throws Exception {
+        String tenant = FHIRConfigHelper.getStringProperty("collection/tenant", null);
+        assertNotNull(tenant);
+        assertEquals("default", tenant);
+
+        String s;
+        Boolean b;
+        Integer i;
+        Double d;
+        List<String> l;
+
+        s = FHIRConfigHelper.getStringProperty("collection/groupD/nullProp", "defaultValue1");
+        assertNotNull(s);
+        assertEquals(s, "defaultValue1");
+
+        b = FHIRConfigHelper.getBooleanProperty("collection/groupD/nullProp", false);
+        assertNotNull(b);
+        assertEquals(b, Boolean.FALSE);
+
+        i = FHIRConfigHelper.getIntProperty("collection/groupD/nullProp", 12345);
+        assertNotNull(i);
+        assertEquals(i.intValue(), 12345);
+
+        d = FHIRConfigHelper.getDoubleProperty("collection/groupD/nullProp", 12345.001);
+        assertNotNull(d);
+        assertEquals(d.doubleValue(), 12345.001);
+
+        l = FHIRConfigHelper.getStringListProperty("collection/groupD/stringListWithNullProp");
+        assertNotNull(l);
+        assertEquals(l, expectedListWithNull);
     }
 
     @Test
@@ -110,23 +146,23 @@ public void testTenant1() throws Exception {
         Boolean b;
         s = FHIRConfigHelper.getStringProperty("collection/groupA/stringProp1", null);
         assertNotNull(s);
-        assertEquals("tenant1Value1", s);
+        assertEquals(s, "tenant1Value1");
 
         s = FHIRConfigHelper.getStringProperty("collection/groupA/stringProp2", null);
         assertNotNull(s);
-        assertEquals("defaultValue2", s);
+        assertEquals(s, "defaultValue2");
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/boolProp1", null);
         assertNotNull(b);
-        assertEquals(Boolean.TRUE, b);
+        assertEquals(b, Boolean.TRUE);
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/boolProp2", null);
         assertNotNull(b);
-        assertEquals(Boolean.FALSE, b);
+        assertEquals(b, Boolean.FALSE);
 
         l = FHIRConfigHelper.getStringListProperty("collection/groupB/stringList1");
         assertNotNull(l);
-        assertEquals(expectedList2, l);
+        assertEquals(l, expectedList2);
     }
 
     @Test
@@ -140,25 +176,26 @@ public void testTenant2() throws Exception {
         List<String> l;
         String s;
         Boolean b;
+
         s = FHIRConfigHelper.getStringProperty("collection/groupA/stringProp1", null);
         assertNotNull(s);
-        assertEquals("defaultValue1", s);
+        assertEquals(s, "defaultValue1");
 
         s = FHIRConfigHelper.getStringProperty("collection/groupA/stringProp2", null);
         assertNotNull(s);
-        assertEquals("tenant2Value2", s);
+        assertEquals(s, "tenant2Value2");
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/boolProp1", null);
         assertNotNull(b);
-        assertEquals(Boolean.FALSE, b);
+        assertEquals(b, Boolean.FALSE);
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/boolProp2", null);
         assertNotNull(b);
-        assertEquals(Boolean.TRUE, b);
+        assertEquals(b, Boolean.TRUE);
 
         l = FHIRConfigHelper.getStringListProperty("collection/groupB/stringList1");
         assertNotNull(l);
-        assertEquals(0, l.size());
+        assertEquals(l.size(), 0);
     }
 
     @Test
@@ -178,11 +215,11 @@ public void testTenant3() throws Exception {
         // Load tenant3's config and check the initial property values.
         String s = FHIRConfigHelper.getStringProperty("fhirServer/property1", null);
         assertNotNull(s);
-        assertEquals("property1Value1", s);
+        assertEquals(s, "property1Value1");
 
         s = FHIRConfigHelper.getStringProperty("fhirServer/property2", null);
         assertNotNull(s);
-        assertEquals("property2Value1", s);
+        assertEquals(s, "property2Value1");
 
         // Sleep for just a bit to make sure the updated config file has a newer timestamp.
         Thread.sleep(1000);
@@ -196,11 +233,11 @@ public void testTenant3() throws Exception {
 
         s = FHIRConfigHelper.getStringProperty("fhirServer/property1", null);
         assertNotNull(s);
-        assertEquals("property1Value2", s);
+        assertEquals(s, "property1Value2");
 
         s = FHIRConfigHelper.getStringProperty("fhirServer/property2", null);
         assertNotNull(s);
-        assertEquals("property2Value2", s);
+        assertEquals(s, "property2Value2");
     }
 
     @Test
@@ -218,36 +255,36 @@ public void testTenant4() throws Exception {
         Boolean b;
         s = FHIRConfigHelper.getStringProperty("collection/groupA/stringProp1", null);
         assertNotNull(s);
-        assertEquals("defaultValue1", s);
+        assertEquals(s, "defaultValue1");
 
         s = FHIRConfigHelper.getStringProperty("collection/groupA/stringProp2", null);
         assertNotNull(s);
-        assertEquals("defaultValue2", s);
+        assertEquals(s, "defaultValue2");
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/boolProp1", null);
         assertNotNull(b);
-        assertEquals(Boolean.FALSE, b);
+        assertEquals(b, Boolean.FALSE);
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/boolProp2", null);
         assertNotNull(b);
-        assertEquals(Boolean.FALSE, b);
+        assertEquals(b, Boolean.FALSE);
 
         l = FHIRConfigHelper.getStringListProperty("collection/groupB/stringList1");
         assertNotNull(l);
-        assertEquals(expectedList1, l);
+        assertEquals(l, expectedList1);
 
         Double d = FHIRConfigHelper.getDoubleProperty("collection/groupC/doubleProp1", 1.0);
         assertNotNull(d);
-        assertEquals(12345.001, d);
+        assertEquals(d.doubleValue(), 12345.001);
 
         d = FHIRConfigHelper.getDoubleProperty("collection/groupC/doubleProp2", 1.0);
         assertNotNull(d);
-        assertEquals(12345.001, d);
+        assertEquals(d.doubleValue(), 12345.001);
 
         d = FHIRConfigHelper.getDoubleProperty("collection/groupC/doubleProp3", 1.0);
         assertNotNull(d);
-        assertEquals(1.0, d);
-        
+        assertEquals(d.doubleValue(), 1.0);
+
         assertNotNull(FHIRConfiguration.getInstance().loadConfiguration().toString());
         assertFalse(FHIRConfiguration.getInstance().loadConfiguration().toString().isEmpty());
     }
@@ -268,31 +305,31 @@ public void testTenant5() throws Exception {
         Boolean b;
         s = FHIRConfigHelper.getStringProperty("collection/groupA/newProp1", null);
         assertNotNull(s);
-        assertEquals("newValue1", s);
+        assertEquals(s, "newValue1");
 
         s = FHIRConfigHelper.getStringProperty("collection/groupA/stringProp1", null);
         assertNotNull(s);
-        assertEquals("defaultValue1", s);
+        assertEquals(s, "defaultValue1");
 
         s = FHIRConfigHelper.getStringProperty("collection/groupA/stringProp2", null);
         assertNotNull(s);
-        assertEquals("defaultValue2", s);
+        assertEquals(s, "defaultValue2");
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/boolProp1", null);
         assertNotNull(b);
-        assertEquals(Boolean.FALSE, b);
+        assertEquals(b, Boolean.FALSE);
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/boolProp2", null);
         assertNotNull(b);
-        assertEquals(Boolean.FALSE, b);
+        assertEquals(b, Boolean.FALSE);
 
         b = FHIRConfigHelper.getBooleanProperty("collection/groupB/newBoolProp1", null);
         assertNotNull(b);
-        assertEquals(Boolean.TRUE, b);
+        assertEquals(b, Boolean.TRUE);
 
         l = FHIRConfigHelper.getStringListProperty("collection/groupB/stringList1");
         assertNotNull(l);
-        assertEquals(expectedList1, l);
+        assertEquals(l, expectedList1);
     }
 
     @Test