Skip to content

Commit

Permalink
issue #2843 - store both versioned and unversioned canonicals in
Browse files Browse the repository at this point in the history 
ParametersMap

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre committed Oct 29, 2021
1 parent 1b3954c commit 8a9c044
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 32 deletions.
78 changes: 64 additions & 14 deletions fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,54 +15,78 @@
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;

import com.ibm.fhir.model.resource.SearchParameter;

/**
* A multi-key map that indexes a set of search parameters by SearchParameter.code and SearchParameter.url
* A multi-key map that indexes a set of search parameters by SearchParameter.code and
* SearchParameter.url / canonical (url + "|" + version)
*/
public class ParametersMap {
private static final Logger log = Logger.getLogger(ParametersMap.class.getName());

private final Map<String, Set<SearchParameter>> codeMap;
private final Map<String, SearchParameter> urlMap;
private final Map<String, SearchParameter> canonicalMap;

/**
* Construct a ParametersMap from a Bundle of SearchParameter
*/
public ParametersMap() {
// LinkedHashMaps to preserve insertion order
codeMap = new LinkedHashMap<>();
urlMap = new LinkedHashMap<>();
canonicalMap = new LinkedHashMap<>();
}

public void insert(String code, SearchParameter parameter) {
Objects.requireNonNull(code, "cannot insert a null code");
Objects.requireNonNull(parameter, "cannot insert a null parameter");

String url = parameter.getUrl().getValue();
String version = parameter.getVersion() == null ? null : parameter.getVersion().getValue();

Set<SearchParameter> previousParams = codeMap.get(code);
if (previousParams != null && previousParams.size() > 0) {
if (log.isLoggable(Level.FINE)) {
log.fine("SearchParameter with code '" + code + "' already exists; adding additional parameter with url '" + url + "'");
log.fine("SearchParameter with code '" + code + "' already exists; adding additional parameter '" + url + "'");
}
}
codeMap.computeIfAbsent(code, k -> new HashSet<>()).add(parameter);

if (urlMap.containsKey(url)) {
SearchParameter previous = urlMap.get(url);
// for versioned search params, we store them in the canonicalMap twice:
// once with their version and once without it
if (canonicalMap.containsKey(url)) {
SearchParameter previous = canonicalMap.get(url);
if (previous.getExpression() == null || previous.getExpression().equals(parameter.getExpression())) {
if (!code.equals(previous.getCode().getValue())) {
log.info("SearchParameter with url '" + url + "' already exists with the same expression; "
log.info("SearchParameter '" + url + "' already exists with the same expression; "
+ "adding additional code '" + code + "'");
}
} else {
log.warning("SearchParameter with url '" + url + "' already exists with a different expression;\n"
+ "replacing [id=" + previous.getId() + ", expression=" + previous.getExpression().getValue()
+ "] with [id=" + parameter.getId() + ", expression=" + parameter.getExpression().getValue() + "]");
log.info("SearchParameter '" + url + "' already exists with a different expression;\n"
+ "replacing [id=" + previous.getId() + ", version=" + previous.getVersion().getValue() + ", expression=" + previous.getExpression().getValue()
+ "] with [id=" + parameter.getId() + ", version=" + parameter.getVersion().getValue() + ", expression=" + parameter.getExpression().getValue() + "]");
}
}
canonicalMap.put(url, parameter);

if (version != null) {
String canonical = url + (version == null ? "" : "|" + version);
if (canonicalMap.containsKey(canonical)) {
SearchParameter previous = canonicalMap.get(canonical);
if (previous.getExpression() == null || previous.getExpression().equals(parameter.getExpression())) {
if (!code.equals(previous.getCode().getValue())) {
log.info("SearchParameter '" + canonical + "' already exists with the same expression; "
+ "adding additional code '" + code + "'");
}
} else {
log.warning("SearchParameter '" + canonical + "' already exists with a different expression;\n"
+ "replacing [id=" + previous.getId() + ", expression=" + previous.getExpression().getValue()
+ "] with [id=" + parameter.getId() + ", expression=" + parameter.getExpression().getValue() + "]");
}
}
canonicalMap.put(canonical, parameter);
}
urlMap.put(url, parameter);
}

public void insertAll(ParametersMap map) {
Expand All @@ -77,12 +101,24 @@ public Set<SearchParameter> lookupByCode(String searchParameterCode) {
return codeMap.get(searchParameterCode);
}

/**
* @deprecated use {@link #lookupByCanonical(String)}, which takes an optional version, instead
*/
@Deprecated
public SearchParameter lookupByUrl(String searchParameterUrl) {
return urlMap.get(searchParameterUrl);
return canonicalMap.get(searchParameterUrl);
}

public SearchParameter lookupByCanonical(String searchParameterCanonical) {
return canonicalMap.get(searchParameterCanonical);
}

public Collection<SearchParameter> values() {
return urlMap.values();
// use List to preserve order
return Collections.unmodifiableList(canonicalMap.entrySet().stream()
.filter(e -> !e.getKey().contains("|"))
.map(e -> e.getValue())
.collect(Collectors.toList()));
}

public boolean isEmpty() {
Expand All @@ -97,7 +133,21 @@ public Set<Entry<String, Set<SearchParameter>>> codeEntries() {
return Collections.unmodifiableSet(codeMap.entrySet());
}

/**
* @deprecated use {@link #canonicalEntries()} instead
*/
@Deprecated
public Set<Entry<String, SearchParameter>> urlEntries() {
return Collections.unmodifiableSet(urlMap.entrySet());
return Collections.unmodifiableSet(canonicalMap.entrySet().stream()
.filter(e -> !e.getKey().contains("|"))
.collect(Collectors.toSet()));
}

/**
* @implSpec Note that versioned search parameters will be listed twice;
* once with their version and once without
*/
public Set<Entry<String, SearchParameter>> canonicalEntries() {
return Collections.unmodifiableSet(canonicalMap.entrySet());
}
}
44 changes: 30 additions & 14 deletions fhir-search/src/main/java/com/ibm/fhir/search/util/SearchUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,25 +297,41 @@ private static Collection<SearchParameter> filterSearchParameters(Map<String, Ma
for (SearchParameter sp : unfilteredSearchParameters) {
String code = sp.getCode().getValue();
String url = sp.getUrl().getValue();
String version = sp.getVersion() == null ? null : sp.getVersion().getValue();
String canonical = url + (version == null ? "" : "|" + version);

if (includedSPs.containsKey(code)) {
String configuredUrl = includedSPs.get(code);
if (configuredUrl != null && configuredUrl.equals(url)) {
results.put(code, sp);
} else if (log.isLoggable(Level.FINE)) {
log.fine("Skipping search parameter with id='" + sp.getId() + "'. "
+ "Tenant configuration for resource='" + resourceType + "' code='" + code + "' url='" + configuredUrl
+ "' does not match url '" + url + "'");
}
String configuredCanonical = includedSPs.get(code);
if (configuredCanonical != null && configuredCanonical.contains("|")) {
if (configuredCanonical.equals(canonical)) {
results.put(code, sp);
} else if (log.isLoggable(Level.FINE)) {
log.fine("Skipping search parameter with id='" + sp.getId() + "'. "
+ "Tenant configuration for resource='" + resourceType + "' code='" + code + "' url='" + configuredCanonical
+ "' does not match url '" + url + "'");
}
} else if (configuredCanonical != null) {
if (configuredCanonical.equals(url)) {
results.put(code, sp);
} else if (log.isLoggable(Level.FINE)) {
log.fine("Skipping search parameter with id='" + sp.getId() + "'. "
+ "Tenant configuration for resource='" + resourceType + "' code='" + code + "' url='" + configuredCanonical
+ "' does not match url '" + url + "'");
}
}
} else if (includeAll) {
// If "*" is contained in the included SP urls, then include the search parameter
// if it doesn't conflict with any of the previously added parameters
if (!results.containsKey(code)) {
results.put(code, sp);
} else {
String configuredUrl = results.get(code).getUrl().getValue();
SearchParameter configuredSP = results.get(code);
String configuredUrl = configuredSP.getUrl().getValue();
String configuredVersion = configuredSP.getVersion() == null ? null : configuredSP.getVersion().getValue();
String configuredCanonical = configuredUrl + (configuredVersion == null ? "" : "|" + configuredVersion);

log.warning("Skipping search parameter with id='" + sp.getId() + "'. "
+ "Found multiple search parameters, '" + configuredUrl + "' and '" + url + "', for code '" + code
+ "Found multiple search parameters, '" + configuredCanonical + "' and '" + canonical + "', for code '" + code
+ "' on resource type '" + resourceType + "'; use search parameter filtering to disambiguate.");
}
}
Expand Down Expand Up @@ -1452,7 +1468,7 @@ private static List<QueryParameterValue> parseQueryParameterValuesString(SearchP
}

/**
* Look up the http://ibm.com/fhir/extension/implicit-system extension in
* Look up the http://ibm.com/fhir/extension/implicit-system extension in
* the given list of Extensions
* @param extensions
* @return the implicit system value, or null if not found
Expand Down Expand Up @@ -2878,17 +2894,17 @@ private static boolean isCanonicalSearchParm(Class<?> resourceType, String expre
// First split by union operator.
for (String subExpression : expression.split("\\|")) {
Class<?> dataType = resourceType;

// Strip any enclosing parens
subExpression = subExpression.trim();
if (subExpression.startsWith("(") && subExpression.endsWith(")")) {
subExpression = subExpression.substring(1, subExpression.length()-1);
}

// Strip out '.where()' functions, handling nested parentheses
subExpression = subExpression
.replaceAll("\\.where(?=\\()(?:(?=.*?\\((?!.*?\\1)(.*\\)(?!.*\\2).*))(?=.*?\\)(?!.*?\\2)(.*)).)+?.*?(?=\\1)[^(]*(?=\\2$)", "");

// Split into components
for (String component : subExpression.split("\\.")) {
component = component.trim();
Expand Down
39 changes: 37 additions & 2 deletions fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,24 +50,55 @@ public class ParametersMapTest {
.code(Code.of("b"))
.expression(string("extension.value as String"))
.build();
SearchParameter sp_b1 = SearchParameter.builder()
.url(Uri.of("http://ibm.com/fhir/test/sp_b"))
.version("1")
.status(PublicationStatus.ACTIVE)
.name(string("b"))
.base(ResourceType.RESOURCE)
.type(SearchParamType.STRING)
.description(Markdown.of("Param with code 'b'"))
.code(Code.of("b"))
.expression(string("extension.value as String"))
.build();
SearchParameter sp_b2 = SearchParameter.builder()
.url(Uri.of("http://ibm.com/fhir/test/sp_b"))
.version("2")
.status(PublicationStatus.ACTIVE)
.name(string("b"))
.base(ResourceType.RESOURCE)
.type(SearchParamType.STRING)
.description(Markdown.of("Param with code 'b'"))
.code(Code.of("b"))
.expression(string("extension.value as String"))
.build();

@Test
public void testParametersMap() {
ParametersMap pm = new ParametersMap();
pm.insert("a", sp_a1);
pm.insert("a", sp_a2);
pm.insert("b", sp_b);
pm.insert("b", sp_b1);
pm.insert("b", sp_b2);

assertTrue(pm.lookupByCode("a").contains(sp_a1));
assertTrue(pm.lookupByCode("a").contains(sp_a2));
assertTrue(pm.lookupByCode("b").contains(sp_b));
assertTrue(pm.lookupByCode("b").contains(sp_b2));

assertEquals(pm.lookupByUrl("http://ibm.com/fhir/test/sp_a1"), sp_a1);
assertEquals(pm.lookupByUrl("http://ibm.com/fhir/test/sp_a2"), sp_a2);
assertEquals(pm.lookupByUrl("http://ibm.com/fhir/test/sp_b"), sp_b);
assertEquals(pm.lookupByUrl("http://ibm.com/fhir/test/sp_b"), sp_b2);

assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_a1"), sp_a1);
assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_a2"), sp_a2);
assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b"), sp_b2);
assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b|1"), sp_b1);
assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b|2"), sp_b2);

assertTrue(pm.codeEntries().size() == 2);
assertTrue(pm.urlEntries().size() == 3);
assertTrue(pm.canonicalEntries().size() == 5); // versionless ones for a1, a2, and b2; versioned ones for b1 and b2
}

/**
Expand All @@ -83,9 +114,11 @@ public void testParametersMap_duplicateUrl() {
assertTrue(pm.lookupByCode("b2").contains(sp_b));

assertEquals(pm.lookupByUrl("http://ibm.com/fhir/test/sp_b"), sp_b);
assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b"), sp_b);

assertTrue(pm.codeEntries().size() == 2);
assertTrue(pm.urlEntries().size() == 1);
assertTrue(pm.canonicalEntries().size() == 1);
}

/**
Expand All @@ -100,8 +133,10 @@ public void testParametersMap_duplicate() {
assertTrue(pm.lookupByCode("b").contains(sp_b));

assertEquals(pm.lookupByUrl("http://ibm.com/fhir/test/sp_b"), sp_b);
assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b"), sp_b);

assertTrue(pm.codeEntries().size() == 1);
assertTrue(pm.urlEntries().size() == 1);
assertTrue(pm.canonicalEntries().size() == 1);
}
}
4 changes: 2 additions & 2 deletions fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersUtilTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2019
* (C) Copyright IBM Corp. 2019, 2021
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -40,10 +40,10 @@
import com.ibm.fhir.search.test.BaseSearchTest;

/**
*
* Tests ParametersUtil
*/
public class ParametersUtilTest extends BaseSearchTest {
public static final boolean DEBUG = true;

@Test
public void testGetBuiltInSearchParameterMap() throws IOException {
Expand Down

0 comments on commit 8a9c044

Please sign in to comment.