From 2d82cc60a221798fddaf7101aa17589a1e96f2e2 Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Fri, 1 Oct 2021 17:40:12 -0400 Subject: [PATCH 1/4] 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 --- .../main/java/com/ibm/fhir/config/PropertyGroup.java | 10 ++++++---- .../registry/ServerRegistryResourceProvider.java | 4 ++-- .../java/com/ibm/fhir/server/util/FHIRRestHelper.java | 3 +-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java b/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java index 57d61636b49..20fca98e6a6 100644 --- a/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java +++ b/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 containing the elements from the JSON array property; possibly null * @throws Exception @@ -262,7 +262,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 + * @return either null or an instance of Boolean, Integer, String, PropertyGroup, or List * @throws Exception */ public static Object convertJsonValue(JsonValue jsonValue) throws Exception { @@ -292,6 +292,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()); } diff --git a/fhir-server/src/main/java/com/ibm/fhir/server/registry/ServerRegistryResourceProvider.java b/fhir-server/src/main/java/com/ibm/fhir/server/registry/ServerRegistryResourceProvider.java index fa42f92ee22..264cc681d4f 100644 --- a/fhir-server/src/main/java/com/ibm/fhir/server/registry/ServerRegistryResourceProvider.java +++ b/fhir-server/src/main/java/com/ibm/fhir/server/registry/ServerRegistryResourceProvider.java @@ -136,13 +136,13 @@ private List computeRegistryResources(Class validateResource(Resource resource) throws FHIRValidationExc } } } catch (Exception e) { - return Collections.singletonList(buildOperationOutcomeIssue(IssueSeverity.ERROR, IssueType.UNKNOWN, - "Error retrieving profile configuration.")); + throw new FHIRValidationException("Error retrieving profile configuration.", e); } // If required profiles were configured, perform validation of asserted profiles against required profiles From 37fb951da1480f6f86135aaf286bff869949f983 Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Mon, 4 Oct 2021 08:07:38 -0400 Subject: [PATCH 2/4] 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 --- .../src/main/java/com/ibm/fhir/config/PropertyGroup.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java b/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java index 20fca98e6a6..5bc37b17133 100644 --- a/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java +++ b/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java @@ -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 getProperties() throws Exception { List results = new ArrayList<>(); for (Map.Entry 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(), convertJsonValue(entry.getValue()))); + } } return results; } From 2938c85bade361d3e992872756a186790b122c4f Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Mon, 4 Oct 2021 09:14:22 -0400 Subject: [PATCH 3/4] 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 --- .../com/ibm/fhir/config/PropertyGroup.java | 3 +- .../config/test/FHIRConfigHelperTest.java | 123 ++++++++++++------ .../fhir/config/test/PropertyGroupTest.java | 56 +++++++- .../config/default/fhir-server-config.json | 4 + 4 files changed, 137 insertions(+), 49 deletions(-) diff --git a/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java b/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java index 5bc37b17133..37bb4fa4170 100644 --- a/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java +++ b/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java @@ -323,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); @@ -331,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; } /** diff --git a/fhir-config/src/test/java/com/ibm/fhir/config/test/FHIRConfigHelperTest.java b/fhir-config/src/test/java/com/ibm/fhir/config/test/FHIRConfigHelperTest.java index 8560c25b080..950f2099f7e 100644 --- a/fhir-config/src/test/java/com/ibm/fhir/config/test/FHIRConfigHelperTest.java +++ b/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 expectedList2 = Arrays.asList(array2); + String[] arrayWithNull = { "token1", null, "token2" }; + List 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 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 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 diff --git a/fhir-config/src/test/java/com/ibm/fhir/config/test/PropertyGroupTest.java b/fhir-config/src/test/java/com/ibm/fhir/config/test/PropertyGroupTest.java index f1899169fe0..4d12077003b 100644 --- a/fhir-config/src/test/java/com/ibm/fhir/config/test/PropertyGroupTest.java +++ b/fhir-config/src/test/java/com/ibm/fhir/config/test/PropertyGroupTest.java @@ -14,16 +14,17 @@ import java.util.List; -import jakarta.json.Json; -import jakarta.json.JsonBuilderFactory; -import jakarta.json.JsonObject; - import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; import com.ibm.fhir.config.PropertyGroup; import com.ibm.fhir.config.PropertyGroup.PropertyEntry; +import jakarta.json.Json; +import jakarta.json.JsonBuilderFactory; +import jakarta.json.JsonObject; +import jakarta.json.JsonValue; + public class PropertyGroupTest { private static final JsonBuilderFactory BUILDER_FACTORY = Json.createBuilderFactory(null); private JsonObject jsonObj1 = null; @@ -73,7 +74,9 @@ public void setup() { .add("int-array", BUILDER_FACTORY.createArrayBuilder() .add(1) .add(2) - .add(3))) + .add(3)) + .add("null-value", JsonValue.NULL) + ) .build(); if (DEBUG) { @@ -183,6 +186,28 @@ public void testObjectArrayProperty() throws Exception { assertEquals("secure", type); } + @Test + public void testNullProperty() throws Exception { + PropertyGroup pg = new PropertyGroup(jsonObj2); + assertNull(pg.getJsonValue("fhir-server/null-value")); + assertNull(pg.getStringProperty("fhir-server/null-value")); + assertNull(pg.getBooleanProperty("fhir-server/null-value")); + assertNull(pg.getIntProperty("fhir-server/null-value")); + assertNull(pg.getDoubleProperty("fhir-server/null-value")); + assertNull(pg.getStringListProperty("fhir-server/null-value")); + } + + @Test + public void testNonExistentProperty() throws Exception { + PropertyGroup pg = new PropertyGroup(jsonObj2); + assertNull(pg.getJsonValue("fhir-server/bogus")); + assertNull(pg.getStringProperty("fhir-server/bogus")); + assertNull(pg.getBooleanProperty("fhir-server/bogus")); + assertNull(pg.getIntProperty("fhir-server/bogus")); + assertNull(pg.getDoubleProperty("fhir-server/bogus")); + assertNull(pg.getStringListProperty("fhir-server/bogus")); + } + @Test(expectedExceptions = { IllegalArgumentException.class }) public void testStringPropertyException() throws Exception { PropertyGroup pg = new PropertyGroup(jsonObj1); @@ -292,4 +317,25 @@ public void testGetProperties3() throws Exception { assertTrue(obj instanceof String); } } + + @Test + public void testGetPropertiesWithNull() throws Exception { + PropertyGroup rootPG = new PropertyGroup(jsonObj2); + PropertyGroup pg = rootPG.getPropertyGroup("fhir-server/notifications/common"); + assertNotNull(pg); + + List properties = pg.getProperties(); + assertNotNull(properties); + assertEquals(1, properties.size()); + + PropertyEntry propEntry = properties.get(0); + assertNotNull(propEntry); + assertEquals("includeResourceTypes", propEntry.getName()); + assertTrue(propEntry.getValue() instanceof List); + assertEquals(2, ((List) propEntry.getValue()).size()); + for (Object obj : (List) propEntry.getValue()) { + assertNotNull(obj); + assertTrue(obj instanceof String); + } + } } diff --git a/fhir-config/src/test/resources/config/default/fhir-server-config.json b/fhir-config/src/test/resources/config/default/fhir-server-config.json index dec30c3cb69..a92bd0b4961 100644 --- a/fhir-config/src/test/resources/config/default/fhir-server-config.json +++ b/fhir-config/src/test/resources/config/default/fhir-server-config.json @@ -16,6 +16,10 @@ "intProp2": "12345", "doubleProp1": 12345.001, "doubleProp2": "12345.001" + }, + "groupD": { + "nullProp": null, + "stringListWithNullProp": ["token1", null, "token2"] } }, "fhirServer": { From 645211e40f2fec27b9ffb7200af2c9d3206958c1 Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Mon, 4 Oct 2021 20:43:36 -0400 Subject: [PATCH 4/4] Update fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java Signed-off-by: Lee Surprenant Co-authored-by: Michael W Schroeder <66479070+michaelwschroeder@users.noreply.github.com> --- .../src/main/java/com/ibm/fhir/config/PropertyGroup.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java b/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java index 37bb4fa4170..9d5c76288c7 100644 --- a/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java +++ b/fhir-config/src/main/java/com/ibm/fhir/config/PropertyGroup.java @@ -245,7 +245,7 @@ public List getProperties() throws Exception { for (Map.Entry entry : jsonObj.entrySet()) { Object jsonValue = convertJsonValue(entry.getValue()); if (jsonValue != null) { - results.add(new PropertyEntry(entry.getKey(), convertJsonValue(entry.getValue()))); + results.add(new PropertyEntry(entry.getKey(), jsonValue)); } } return results;