Skip to content

Commit

Permalink
Merge pull request #3463 from IBM/issue-3446
Browse files Browse the repository at this point in the history 
Issue #3446 reference version missing from parameter hash
  • Loading branch information
@lmsurpre
lmsurpre authored Mar 18, 2022
2 parents 984684e + e733cda commit 9e93d89
Show file tree
Hide file tree
Showing 7 changed files with 292 additions and 18 deletions.
7 changes: 4 additions & 3 deletions fhir-config/src/main/java/com/ibm/fhir/config/FHIRConfigHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,10 @@ private static JsonValue getPropertyFromTenantOrDefault(String propertyName) {
// let's try to find it in the default config.
if (result == null && !FHIRConfiguration.DEFAULT_TENANT_ID.equals(tenantId)) {
try {
if (propertyName.startsWith(FHIRConfiguration.PROPERTY_DATASOURCES)) {
// Issue #639. Prevent datasource lookups from falling back to
// the default datasource which breaks tenant isolation.
if (propertyName.startsWith(FHIRConfiguration.PROPERTY_DATASOURCES)
|| propertyName.startsWith(FHIRConfiguration.PROPERTY_PERSISTENCE_PAYLOAD)) {
// Issue #639 and #3416. Prevent datasource/payload lookups from falling back to
// the default configuration which breaks tenant isolation.
result = null;
} else {
// Non-datasource property, which we allow to fall back to default
Expand Down
34 changes: 32 additions & 2 deletions fhir-config/src/test/java/com/ibm/fhir/config/test/FHIRConfigHelperTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2017, 2021
* (C) Copyright IBM Corp. 2017, 2022
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -388,4 +388,34 @@ public void testDatasourceNoFallback() throws Exception {
PropertyGroup dsPG = FHIRConfigHelper.getPropertyGroup(dsPropertyName);
assertNull(dsPG);
}
}

/**
* Look up the payload persistence configuration for default/default
* and check that the payload type field is found
* @throws Exception
*/
@Test
public void testPayloadLookup() throws Exception {
FHIRRequestContext.set(new FHIRRequestContext("default"));
final String datastoreId = "default";
String dsPropertyName = FHIRConfiguration.PROPERTY_PERSISTENCE_PAYLOAD + "/" + datastoreId;
PropertyGroup dsPG = FHIRConfigHelper.getPropertyGroup(dsPropertyName);
assertNotNull(dsPG);
String type = dsPG.getStringProperty("type");
assertEquals(type, "azure.blob");
}

/**
* Check that we do not fall back to the default tenant config when
* reading payload persistence configuration.
* @throws Exception
*/
@Test
public void testPayloadNoFallback() throws Exception {
FHIRRequestContext.set(new FHIRRequestContext("tenant1"));
final String datastoreId = "default";
String dsPropertyName = FHIRConfiguration.PROPERTY_DATASOURCES + "/" + datastoreId;
PropertyGroup dsPG = FHIRConfigHelper.getPropertyGroup(dsPropertyName);
assertNull(dsPG);
}
}
9 changes: 7 additions & 2 deletions fhir-config/src/test/resources/config/default/fhir-server-config.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,17 @@
}
},
"fhirServer": {
"persistence": {
"persistence": {
"datasources": {
"default": {
"type": "db2"
}
},
"payload": {
"default": {
"type": "azure.blob"
}
}
}
}
}
}
37 changes: 28 additions & 9 deletions ...stence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,11 @@ public MultiResourceResult search(FHIRPersistenceContext context, Class<? extend
List<com.ibm.fhir.persistence.jdbc.dto.Resource> includeDTOList =
newSearchForIncludeResources(searchContext, resourceType, queryBuilder, resourceDao, resourceDTOList);
List<ResourceResult<? extends Resource>> includeResult = this.convertResourceDTOList(resourceDao, includeDTOList, resourceType, null, searchContext.isIncludeResourceData());
// erased version referenced via a versioned reference will be missing the resource
// data field, so we need to filter those out here if resource data is being requested
if (searchContext.isIncludeResourceData()) {
includeResult = includeResult.stream().filter(rr -> rr.getResource() != null).collect(Collectors.toList());
}
resourceResults.addAll(includeResult);
}
}
Expand Down Expand Up @@ -1460,14 +1465,16 @@ protected List<ResourceResult<? extends Resource>> convertResourceDTOList(Resour
}

// Check to make sure we got a Resource if we asked for it and expect there to be one
for (ResourceResult<? extends Resource> resourceResult: resourceResults) {
if (resourceResult.getResource() == null && includeResourceData && !resourceResult.isDeleted()) {
String resourceTypeName = resourceResult.getResourceTypeName();
if (resourceTypeName == null) {
resourceTypeName = resourceType.getSimpleName();
if (includeResourceData) {
for (ResourceResult<? extends Resource> resourceResult: resourceResults) {
if (resourceResult.getResource() == null && !resourceResult.isDeleted()) {
String resourceTypeName = resourceResult.getResourceTypeName();
if (resourceTypeName == null) {
resourceTypeName = resourceType.getSimpleName();
}
throw new FHIRPersistenceException("convertResourceDTO returned no resource for '"
+ resourceTypeName + "/" + resourceResult.getLogicalId() + "'");
}
throw new FHIRPersistenceException("convertResourceDTO returned no resource for '"
+ resourceTypeName + "/" + resourceResult.getLogicalId() + "'");
}
}

Expand Down Expand Up @@ -2269,6 +2276,7 @@ private <T extends Resource> ResourceResult<T> convertResourceDTOToResourceResul
log.entering(CLASSNAME, METHODNAME);
Objects.requireNonNull(resourceDTO, "resourceDTO must be not null");
T resource;
boolean treatErasedAsDeleted = false;

if (includeData) {
// original impl - the resource, if any, was read from the RDBMS
Expand All @@ -2288,11 +2296,14 @@ private <T extends Resource> ResourceResult<T> convertResourceDTOToResourceResul
}
}
} else {
// Queries may return a NULL for the DATA column if the resource has been erased
// or the query was asked not to fetch DATA in the first place
// Queries may return a NULL for the DATA column if the resource has been erased.
resource = null;
treatErasedAsDeleted = true;
}
} else {
// Because we never selected the data column, we don't know if this resource
// version has been erased or not. The only thing we can do is return a null
// resource.
resource = null;
}

Expand All @@ -2313,6 +2324,14 @@ private <T extends Resource> ResourceResult<T> convertResourceDTOToResourceResul
builder.version(resourceDTO.getVersionId());
builder.lastUpdated(resourceDTO.getLastUpdated().toInstant());

// If we encounter a resource version that was erased, its data column will be
// null so we can't parse a resource value. We need to treat it as a deleted
// resource because otherwise the REST layer will expect the resource value to
// be present
if (treatErasedAsDeleted && !resourceDTO.isDeleted()) {
builder.deleted(true);
}

log.exiting(CLASSNAME, METHODNAME);
return builder.build();
}
Expand Down
6 changes: 5 additions & 1 deletion ...rsistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/util/ParameterHashVisitor.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2021
* (C) Copyright IBM Corp. 2021, 2022
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -172,6 +172,7 @@ private void updateDigestWithBigDecimal(BigDecimal value) {
if (value != null) {
ByteBuffer bb = ByteBuffer.allocate(Double.SIZE);
bb.putDouble(value.doubleValue());
bb.flip();
digest.update(bb);
}
digest.update(DELIMITER);
Expand All @@ -185,6 +186,7 @@ private void updateDigestWithInteger(Integer value) {
if (value != null) {
ByteBuffer bb = ByteBuffer.allocate(Integer.SIZE);
bb.putInt(value.intValue());
bb.flip();
digest.update(bb);
}
digest.update(DELIMITER);
Expand All @@ -198,6 +200,7 @@ private void updateDigestWithDouble(Double value) {
if (value != null) {
ByteBuffer bb = ByteBuffer.allocate(Double.SIZE);
bb.putDouble(value.doubleValue());
bb.flip();
digest.update(bb);
}
digest.update(DELIMITER);
Expand All @@ -211,6 +214,7 @@ private void updateDigestWithTimestamp(Timestamp value) {
if (value != null) {
ByteBuffer bb = ByteBuffer.allocate(Long.SIZE);
bb.putLong(value.getTime());
bb.flip();
digest.update(bb);
}
digest.update(DELIMITER);
Expand Down
94 changes: 93 additions & 1 deletion ...istence-jdbc/src/test/java/com/ibm/fhir/persistence/jdbc/test/util/ParameterHashTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2021
* (C) Copyright IBM Corp. 2021, 2022
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -12,6 +12,7 @@
import static org.testng.Assert.assertNotNull;

import java.math.BigDecimal;
import java.sql.Timestamp;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Arrays;
Expand All @@ -32,6 +33,7 @@
import com.ibm.fhir.persistence.jdbc.util.ParameterHashVisitor;
import com.ibm.fhir.persistence.jdbc.util.type.NewNumberParmBehaviorUtil;
import com.ibm.fhir.search.date.DateTimeHandler;
import com.ibm.fhir.search.util.ReferenceUtil;
import com.ibm.fhir.search.util.ReferenceValue;
import com.ibm.fhir.search.util.ReferenceValue.ReferenceType;

Expand Down Expand Up @@ -68,6 +70,96 @@ public void testNoExtractedParameters() throws Exception {
assertEquals(hash1, hash2);
}

@Test
public void testReferenceParams() throws Exception {
ReferenceValue rv1 = ReferenceUtil.createReferenceValueFrom("Patient/patient1/_history/1", "Patient", ReferenceUtil.getBaseUrl(null));
ReferenceParmVal p1 = new ReferenceParmVal();
p1.setName("subject");
p1.setRefValue(rv1);

ParameterHashVisitor visitor1 = new ParameterHashVisitor();
p1.accept(visitor1);

// Make sure that the hash changes when changing the reference version
ReferenceValue rv2 = ReferenceUtil.createReferenceValueFrom("Patient/patient1/_history/2", "Patient", ReferenceUtil.getBaseUrl(null));
p1.setRefValue(rv2);

ParameterHashVisitor visitor2 = new ParameterHashVisitor();
p1.accept(visitor2);
assertNotEquals(visitor1.getBase64Hash(), visitor2.getBase64Hash());
}

@Test
public void testDateParams() throws Exception {
DateParmVal p1 = new DateParmVal();
p1.setName("date");
p1.setValueDateStart(Timestamp.from(Instant.now()));
p1.setValueDateEnd(Timestamp.from(Instant.now()));
ParameterHashVisitor visitor1 = new ParameterHashVisitor();
p1.accept(visitor1);

// Modify the parameter with a different start date and check the hash changes
p1.setValueDateStart(Timestamp.from(Instant.now().plusMillis(1000)));
ParameterHashVisitor visitor2 = new ParameterHashVisitor();
p1.accept(visitor2);
assertNotEquals(visitor1.getBase64Hash(), visitor2.getBase64Hash());

// Now check it changes again when we change the date end value
p1.setValueDateEnd(Timestamp.from(Instant.now().plusMillis(1000)));
ParameterHashVisitor visitor3 = new ParameterHashVisitor();
p1.accept(visitor3);
assertNotEquals(visitor2.getBase64Hash(), visitor3.getBase64Hash());
}

@Test
public void testLocationParams() throws Exception {
LocationParmVal p1 = new LocationParmVal();
p1.setValueLatitude(0.1);
p1.setValueLongitude(1.1);
ParameterHashVisitor visitor1 = new ParameterHashVisitor();
p1.accept(visitor1);

// Modify latitude and check the signature changes
p1.setValueLatitude(0.2);
ParameterHashVisitor visitor2 = new ParameterHashVisitor();
p1.accept(visitor2);
assertNotEquals(visitor1.getBase64Hash(), visitor2.getBase64Hash());

// Modify longitude and check the signature changes
p1.setValueLongitude(1.2);
ParameterHashVisitor visitor3 = new ParameterHashVisitor();
p1.accept(visitor3);
assertNotEquals(visitor2.getBase64Hash(), visitor3.getBase64Hash());
}

@Test
public void testNumberParams() throws Exception {
NumberParmVal p1 = new NumberParmVal();
p1.setValueNumber(BigDecimal.valueOf(10));
p1.setValueNumberLow(BigDecimal.valueOf(5));
p1.setValueNumberHigh(BigDecimal.valueOf(100));
ParameterHashVisitor visitor1 = new ParameterHashVisitor();
p1.accept(visitor1);

// Modify number and check the signature changes
p1.setValueNumber(BigDecimal.valueOf(11));
ParameterHashVisitor visitor2 = new ParameterHashVisitor();
p1.accept(visitor2);
assertNotEquals(visitor1.getBase64Hash(), visitor2.getBase64Hash());

// Modify number low and check the signature changes
p1.setValueNumberLow(BigDecimal.valueOf(6));
ParameterHashVisitor visitor3 = new ParameterHashVisitor();
p1.accept(visitor3);
assertNotEquals(visitor2.getBase64Hash(), visitor3.getBase64Hash());

// Modify number high and check the signature changes
p1.setValueNumberHigh(BigDecimal.valueOf(101));
ParameterHashVisitor visitor4 = new ParameterHashVisitor();
p1.accept(visitor4);
assertNotEquals(visitor3.getBase64Hash(), visitor4.getBase64Hash());
}

@Test
public void testNullValues() throws Exception {
// Create two number param values, one with null low, and one with null high
Expand Down
Loading

0 comments on commit 9e93d89

Please sign in to comment.