do not throw exceptions on valueset expansion (hapifhir#5997)

* fixing throwing of expcetion for unsupported filter * updating comment * spotless --------- Co-authored-by: leif stawnyczy <[email protected]>
opensrp · Jun 10, 2024 · b39e2f8 · b39e2f8
1 parent 488bf6f
commit b39e2f8
Show file tree

Hide file tree

Showing 3 changed files with 87 additions and 4 deletions.
diff --git a/...hir/changelog/7_4_0/5996-do-not-throw-exceptions-if-encountering-unsupported-filters.yaml b/...hir/changelog/7_4_0/5996-do-not-throw-exceptions-if-encountering-unsupported-filters.yaml
@@ -0,0 +1,8 @@
+---
+type: fix
+issue: 5006
+title: "Expanding a ValueSet on a property using a filter operator that is
+        not supported (ISA, NOTISA, etc) would throw an exception and fail
+        expansion.
+        This has been fixed.
+"
diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermReadSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermReadSvcImpl.java
@@ -1452,7 +1452,6 @@ private void handleFilterPropertyDefault(
 		} else {
 			Term term = new Term(CONCEPT_PROPERTY_PREFIX_NAME + theFilter.getProperty(), value);
 			switch (theFilter.getOp()) {
-				case ISA:
 				case EQUAL:
 					theB.must(theF.match().field(term.field()).matching(term.text()));
 					break;
@@ -1468,13 +1467,21 @@ private void handleFilterPropertyDefault(
 						theB.filter(theF.terms().field(term.field()).matchingAny(valueSet));
 					}
 					break;
+				case ISA:
+				case ISNOTA:
+				case DESCENDENTOF:
+				case GENERALIZES:
 				default:
 					/*
 					 * We do not need to handle REGEX, because that's handled in parent
-					 * We also don't handle EXISTS because that's a separate area (with different term)
+					 * We also don't handle EXISTS because that's a separate area (with different term).
+					 * We add a match-none filter because otherwise it matches everything (not desired).
 					 */
-					throw new InvalidRequestException(Msg.code(2526) + "Unsupported property filter "
-							+ theFilter.getOp().getDisplay());
+					ourLog.error(
+							"Unsupported property filter {}. This may affect expansion, but will not cause errors.",
+							theFilter.getOp().getDisplay());
+					theB.must(theF.matchNone());
+					break;
 			}
 		}
 	}

diff --git a/...hir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/term/IValueSetExpansionIT.java b/...hir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/term/IValueSetExpansionIT.java
@@ -11,13 +11,19 @@
 import ca.uhn.fhir.jpa.term.api.ITermReadSvc;
 import ca.uhn.fhir.parser.IParser;
 import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.read.ListAppender;
 import org.hl7.fhir.r4.model.CodeSystem;
 import org.hl7.fhir.r4.model.DateTimeType;
 import org.hl7.fhir.r4.model.DecimalType;
 import org.hl7.fhir.r4.model.IntegerType;
 import org.hl7.fhir.r4.model.ValueSet;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.EnumSource;
+import org.mockito.ArgumentCaptor;
+import org.slf4j.LoggerFactory;
 
 import java.time.Instant;
 import java.time.temporal.ChronoUnit;
@@ -28,6 +34,8 @@
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 
 public interface IValueSetExpansionIT {
 	static final String CODE_SYSTEM_CODE = "PRODUCT-MULTI-SOURCE";
@@ -205,6 +213,66 @@ default void expandByIdentifier_withBooleanFilteredValuesInInclude_addsMatchingV
 		}
 	}
 
+	/**
+	 * We exclude filters we do support (tested in this class),
+	 * as well as NULL (which isn't a "real" filter),
+	 * and REGEX (tested elsewhere).
+	 */
+	@ParameterizedTest
+	@EnumSource(
+			value = ValueSet.FilterOperator.class,
+			mode = EnumSource.Mode.EXCLUDE,
+			names = {"EQUAL", "EXISTS", "IN", "NOTIN", "REGEX", "NULL"})
+	default void expandValueSet_withUnsupportedFilters_doesNotThrow(ValueSet.FilterOperator theOperator) {
+		// setup
+		IParser parser = getFhirContext().newJsonParser();
+		Logger logger = (Logger) LoggerFactory.getLogger(TermReadSvcImpl.class);
+		ListAppender<ILoggingEvent> listAppender = mock(ListAppender.class);
+
+		// setup CodeSystem
+		// one really isn't necessary for this test, but we'll include it for completeness
+		CodeSystem codeSystem = parser.parseResource(CodeSystem.class, CODE_SYSTEM_STR_BASE);
+
+		// setup valueset
+		ValueSet valueSet = parser.parseResource(ValueSet.class, VALUE_SET_STR_BASE);
+		ValueSet.ConceptSetComponent conceptSetComponent =
+				valueSet.getCompose().getInclude().get(0);
+		ValueSet.ConceptSetFilterComponent filterComponent = new ValueSet.ConceptSetFilterComponent();
+		filterComponent.setProperty(PROPERTY_NAME);
+		filterComponent.setOp(theOperator);
+		filterComponent.setValue("anything");
+		conceptSetComponent.setFilter(List.of(filterComponent));
+
+		// test
+		boolean preExpand = getJpaStorageSettings().isPreExpandValueSets();
+		Level logLevel = logger.getLevel();
+
+		getJpaStorageSettings().setPreExpandValueSets(true);
+		logger.setLevel(Level.ERROR);
+		logger.addAppender(listAppender);
+		try {
+			ValueSet expanded = createCodeSystemAndValueSetAndReturnExpandedValueSet(codeSystem, valueSet);
+
+			assertNotNull(expanded);
+			assertNotNull(expanded.getExpansion());
+			assertTrue(expanded.getExpansion().getContains().isEmpty());
+
+			ArgumentCaptor<ILoggingEvent> loggingEventArgumentCaptor = ArgumentCaptor.forClass(ILoggingEvent.class);
+			verify(listAppender).doAppend(loggingEventArgumentCaptor.capture());
+			List<ILoggingEvent> errors = loggingEventArgumentCaptor.getAllValues().stream()
+					.filter(e -> e.getLevel() == Level.ERROR)
+					.toList();
+			assertFalse(errors.isEmpty());
+			ILoggingEvent first = errors.get(0);
+			assertTrue(first.getFormattedMessage().contains("Unsupported property filter"));
+			assertTrue(first.getFormattedMessage().contains(theOperator.getDisplay()));
+		} finally {
+			getJpaStorageSettings().setPreExpandValueSets(preExpand);
+			logger.setLevel(logLevel);
+			logger.detachAppender(listAppender);
+		}
+	}
+
 	@ParameterizedTest
 	@EnumSource(
 			value = ValueSet.FilterOperator.class,