do not throw exceptions on valueset expansion (#5997)
* fixing throwing of expcetion for unsupported filter * updating comment * spotless --------- Co-authored-by: leif stawnyczy <leifstawnyczy@leifs-mbp.home>
This commit is contained in:
parent
488bf6f18a
commit
b39e2f8909
|
@ -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.
|
||||||
|
"
|
|
@ -1452,7 +1452,6 @@ public class TermReadSvcImpl implements ITermReadSvc, IHasScheduledJobs {
|
||||||
} else {
|
} else {
|
||||||
Term term = new Term(CONCEPT_PROPERTY_PREFIX_NAME + theFilter.getProperty(), value);
|
Term term = new Term(CONCEPT_PROPERTY_PREFIX_NAME + theFilter.getProperty(), value);
|
||||||
switch (theFilter.getOp()) {
|
switch (theFilter.getOp()) {
|
||||||
case ISA:
|
|
||||||
case EQUAL:
|
case EQUAL:
|
||||||
theB.must(theF.match().field(term.field()).matching(term.text()));
|
theB.must(theF.match().field(term.field()).matching(term.text()));
|
||||||
break;
|
break;
|
||||||
|
@ -1468,13 +1467,21 @@ public class TermReadSvcImpl implements ITermReadSvc, IHasScheduledJobs {
|
||||||
theB.filter(theF.terms().field(term.field()).matchingAny(valueSet));
|
theB.filter(theF.terms().field(term.field()).matchingAny(valueSet));
|
||||||
}
|
}
|
||||||
break;
|
break;
|
||||||
|
case ISA:
|
||||||
|
case ISNOTA:
|
||||||
|
case DESCENDENTOF:
|
||||||
|
case GENERALIZES:
|
||||||
default:
|
default:
|
||||||
/*
|
/*
|
||||||
* We do not need to handle REGEX, because that's handled in parent
|
* 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 "
|
ourLog.error(
|
||||||
+ theFilter.getOp().getDisplay());
|
"Unsupported property filter {}. This may affect expansion, but will not cause errors.",
|
||||||
|
theFilter.getOp().getDisplay());
|
||||||
|
theB.must(theF.matchNone());
|
||||||
|
break;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
@ -11,6 +11,10 @@ import ca.uhn.fhir.jpa.term.api.ITermDeferredStorageSvc;
|
||||||
import ca.uhn.fhir.jpa.term.api.ITermReadSvc;
|
import ca.uhn.fhir.jpa.term.api.ITermReadSvc;
|
||||||
import ca.uhn.fhir.parser.IParser;
|
import ca.uhn.fhir.parser.IParser;
|
||||||
import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
|
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.CodeSystem;
|
||||||
import org.hl7.fhir.r4.model.DateTimeType;
|
import org.hl7.fhir.r4.model.DateTimeType;
|
||||||
import org.hl7.fhir.r4.model.DecimalType;
|
import org.hl7.fhir.r4.model.DecimalType;
|
||||||
|
@ -18,6 +22,8 @@ import org.hl7.fhir.r4.model.IntegerType;
|
||||||
import org.hl7.fhir.r4.model.ValueSet;
|
import org.hl7.fhir.r4.model.ValueSet;
|
||||||
import org.junit.jupiter.params.ParameterizedTest;
|
import org.junit.jupiter.params.ParameterizedTest;
|
||||||
import org.junit.jupiter.params.provider.EnumSource;
|
import org.junit.jupiter.params.provider.EnumSource;
|
||||||
|
import org.mockito.ArgumentCaptor;
|
||||||
|
import org.slf4j.LoggerFactory;
|
||||||
|
|
||||||
import java.time.Instant;
|
import java.time.Instant;
|
||||||
import java.time.temporal.ChronoUnit;
|
import java.time.temporal.ChronoUnit;
|
||||||
|
@ -28,6 +34,8 @@ import static org.junit.jupiter.api.Assertions.assertFalse;
|
||||||
import static org.junit.jupiter.api.Assertions.assertNotNull;
|
import static org.junit.jupiter.api.Assertions.assertNotNull;
|
||||||
import static org.junit.jupiter.api.Assertions.assertTrue;
|
import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||||
import static org.junit.jupiter.api.Assertions.fail;
|
import static org.junit.jupiter.api.Assertions.fail;
|
||||||
|
import static org.mockito.Mockito.mock;
|
||||||
|
import static org.mockito.Mockito.verify;
|
||||||
|
|
||||||
public interface IValueSetExpansionIT {
|
public interface IValueSetExpansionIT {
|
||||||
static final String CODE_SYSTEM_CODE = "PRODUCT-MULTI-SOURCE";
|
static final String CODE_SYSTEM_CODE = "PRODUCT-MULTI-SOURCE";
|
||||||
|
@ -205,6 +213,66 @@ public interface IValueSetExpansionIT {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* 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
|
@ParameterizedTest
|
||||||
@EnumSource(
|
@EnumSource(
|
||||||
value = ValueSet.FilterOperator.class,
|
value = ValueSet.FilterOperator.class,
|
||||||
|
|
Loading…
Reference in New Issue