Skip to content

Commit

Permalink
issue #2313 - refactor FingerprintVisitor to avoid startsWith checks
Browse files Browse the repository at this point in the history 
Previously, we constructed the excludePaths on each leaf node, then
checked whether the current path started with any of those values.

This was faulty because the exclude path Resource.id accidentally
resulted in us ignoring fields like Resource.identifier as well.

Now we construct the excludePaths as a set during the initial
doVisitStart and we don't need "startsWith" because we the visitor
supports returning a boolean to control whether child paths should be
visited or not.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre committed May 4, 2021
1 parent fd824a9 commit fffe4b0
Show file tree
Hide file tree
Showing 2 changed files with 219 additions and 51 deletions.
95 changes: 44 additions & 51 deletions fhir-model/src/main/java/com/ibm/fhir/model/visitor/ResourceFingerprintVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
import java.time.Year;
import java.time.YearMonth;
import java.time.ZonedDateTime;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;

import com.ibm.fhir.model.resource.Resource;
import com.ibm.fhir.model.util.SaltHash;
Expand All @@ -31,17 +34,14 @@ public class ResourceFingerprintVisitor extends PathAwareVisitor {
private static final int BYTES_FOR_256_BITS = 256 / 8;
private static final SecureRandom RANDOM = new SecureRandom();

// the salt we use for computing the hash
// The salt we use for computing the hash
private final byte[] salt;

// The name of the resource we first encounter
private String currentResourceName;
// Paths to exclude from the fingerprint
private Set<String> excludePaths;

private final MessageDigest digest;

// for tracking array elements
int index;

/**
* Public constructor. Uses the given salt
* @param salt
Expand Down Expand Up @@ -93,84 +93,82 @@ public SaltHash getSaltAndHash() {

@Override
protected void doVisitStart(String elementName, int elementIndex, Resource resource) {
if (this.currentResourceName == null) {
this.currentResourceName = resource.getClass().getSimpleName();
if (excludePaths == null) {
String currentResourceName = resource.getClass().getSimpleName();
excludePaths = new HashSet<>(Arrays.asList(
currentResourceName + ".id",
currentResourceName + ".meta.versionId",
currentResourceName + ".meta.lastUpdated")
);
}
}

@Override
public boolean visit(java.lang.String elementName, int index, com.ibm.fhir.model.type.String value) {
// Exclude meta.versionId from the fingerprint because it gets injected by the FHIR server.
return includePath();
}

@Override
public boolean visit(java.lang.String elementName, int index, com.ibm.fhir.model.type.Instant value) {
// Exclude meta.lastUpdated from the fingerprint because it gets injected by the FHIR server.
return includePath();
}

@Override
public void visit(java.lang.String elementName, byte[] value) {
if (includePath()) {
digest.update(getPath().getBytes(StandardCharsets.UTF_8));
digest.update(value);
}
digest.update(getPath().getBytes(StandardCharsets.UTF_8));
digest.update(value);
}

@Override
public void visit(java.lang.String elementName, BigDecimal value) {
if (includePath()) {
updateDigest(getPath(), value.toString());
}
updateDigest(getPath(), value.toString());
}

@Override
public void visit(java.lang.String elementName, java.lang.Boolean value) {
if (includePath()) {
updateDigest(getPath(), value.toString());
}
updateDigest(getPath(), value.toString());
}

@Override
public void visit(java.lang.String elementName, java.lang.Integer value) {
if (includePath()) {
ByteBuffer bb = ByteBuffer.allocate(4);
bb.putInt(value);
digest.update(bb);
}
ByteBuffer bb = ByteBuffer.allocate(4);
bb.putInt(value);
digest.update(bb);
}

@Override
public void visit(java.lang.String elementName, LocalDate value) {
if (includePath()) {
updateDigest(getPath(), value.toString());
}
updateDigest(getPath(), value.toString());
}

@Override
public void visit(java.lang.String elementName, LocalTime value) {
if (includePath()) {
updateDigest(getPath(), value.toString());
}
updateDigest(getPath(), value.toString());
}

@Override
public void doVisit(java.lang.String elementName, java.lang.String value) {
// exclude the id and meta.versionId values from the fingerprint
// because they are injected by FHIR. NOTE: startsWith is important
// because we need to ignore any extension fields which may be
// present
// exclude the Resource.id from the fingerprint because it is injected by the FHIR server
if (includePath()) {
updateDigest(getPath(), value);
}
}

@Override
public void visit(java.lang.String elementName, Year value) {
if (includePath()) {
updateDigest(getPath(), value.toString());
}
updateDigest(getPath(), value.toString());
}

@Override
public void visit(java.lang.String elementName, YearMonth value) {
if (includePath()) {
updateDigest(getPath(), value.toString());
}
updateDigest(getPath(), value.toString());
}

@Override
public void visit(java.lang.String elementName, ZonedDateTime value) {
// Exclude the lastUpdated value from the fingerprint because this value
// is injected by the FHIR persistence layer
if (includePath()) {
updateDigest(getPath(), value.toString());
}

updateDigest(getPath(), value.toString());
}

/**
Expand All @@ -188,11 +186,6 @@ protected void updateDigest(String name, String value) {
* @return
*/
protected boolean includePath() {
String idName = currentResourceName + ".id";
String versionIdName = currentResourceName + ".meta.versionId";
String lastUpdatedName = currentResourceName + ".meta.lastUpdated";
String path = getPath();
return !path.startsWith(idName) && !path.startsWith(versionIdName) && !path.startsWith(lastUpdatedName);

return !excludePaths.contains(getPath());
}
}
175 changes: 175 additions & 0 deletions fhir-model/src/test/java/com/ibm/fhir/model/util/test/ResourceFingerprintVisitorTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
/*
* (C) Copyright IBM Corp. 2021
*
* SPDX-License-Identifier: Apache-2.0
*/
package com.ibm.fhir.model.util.test;

import static com.ibm.fhir.model.type.String.string;
import static org.junit.Assert.assertNotEquals;
import static org.testng.Assert.assertEquals;

import java.util.stream.Collectors;

import org.testng.annotations.Test;

import com.ibm.fhir.model.resource.Patient;
import com.ibm.fhir.model.test.TestUtil;
import com.ibm.fhir.model.type.Extension;
import com.ibm.fhir.model.type.Id;
import com.ibm.fhir.model.type.Identifier;
import com.ibm.fhir.model.type.Instant;
import com.ibm.fhir.model.type.code.IdentifierUse;
import com.ibm.fhir.model.type.code.ResourceType;
import com.ibm.fhir.model.util.SaltHash;
import com.ibm.fhir.model.visitor.ResourceFingerprintVisitor;

/**
*
*/
public class ResourceFingerprintVisitorTest {
@Test
public void testEqualResources() throws Exception {
Patient patient = TestUtil.getMinimalResource(ResourceType.PATIENT);

ResourceFingerprintVisitor resourceFingerprintVisitor = new ResourceFingerprintVisitor();
patient.accept(resourceFingerprintVisitor);
SaltHash baseline = resourceFingerprintVisitor.getSaltAndHash();

// same resource should match
resourceFingerprintVisitor = new ResourceFingerprintVisitor(baseline);
patient.accept(resourceFingerprintVisitor);
assertEquals(resourceFingerprintVisitor.getSaltAndHash(), baseline);

// shallowly rebuilt resource should match
resourceFingerprintVisitor = new ResourceFingerprintVisitor(baseline);
patient.toBuilder().build().accept(resourceFingerprintVisitor);
assertEquals(resourceFingerprintVisitor.getSaltAndHash(), baseline);

// deeply rebuilt resource should match
patient = patient.toBuilder()
.meta(patient.getMeta().toBuilder()
.tag(patient.getMeta().getTag().stream()
.map(t -> t.toBuilder()
.code(t.getCode().toBuilder().build())
.build())
.collect(Collectors.toList()))
.build())
.build();
resourceFingerprintVisitor = new ResourceFingerprintVisitor(baseline);
patient.accept(resourceFingerprintVisitor);
assertEquals(resourceFingerprintVisitor.getSaltAndHash(), baseline);
}

@Test
public void testUnequalResources_add() throws Exception {
Patient patient = TestUtil.getMinimalResource(ResourceType.PATIENT);

ResourceFingerprintVisitor resourceFingerprintVisitor = new ResourceFingerprintVisitor();
patient.accept(resourceFingerprintVisitor);
SaltHash baseline = resourceFingerprintVisitor.getSaltAndHash();

patient = patient.toBuilder()
.identifier(Identifier.builder()
.use(IdentifierUse.USUAL)
.build())
.build();
resourceFingerprintVisitor = new ResourceFingerprintVisitor(baseline);
patient.accept(resourceFingerprintVisitor);
assertNotEquals(resourceFingerprintVisitor.getSaltAndHash(), baseline);
}

@Test
public void testUnequalResources_remove() throws Exception {
Patient patient = TestUtil.getMinimalResource(ResourceType.PATIENT);

ResourceFingerprintVisitor resourceFingerprintVisitor = new ResourceFingerprintVisitor();
patient.accept(resourceFingerprintVisitor);
SaltHash baseline = resourceFingerprintVisitor.getSaltAndHash();

patient = patient.toBuilder()
.meta(null)
.build();
resourceFingerprintVisitor = new ResourceFingerprintVisitor(baseline);
patient.accept(resourceFingerprintVisitor);
assertNotEquals(resourceFingerprintVisitor.getSaltAndHash(), baseline);
}

@Test
public void testUnequalResources_reorder() throws Exception {
Patient patient = TestUtil.getMinimalResource(ResourceType.PATIENT);
patient = patient.toBuilder()
.identifier(Identifier.builder()
.use(IdentifierUse.USUAL)
.build())
.identifier(Identifier.builder()
.use(IdentifierUse.OFFICIAL)
.build())
.build();

ResourceFingerprintVisitor resourceFingerprintVisitor = new ResourceFingerprintVisitor();
patient.accept(resourceFingerprintVisitor);
SaltHash baseline = resourceFingerprintVisitor.getSaltAndHash();

patient = patient.toBuilder()
.identifier(Identifier.builder()
.use(IdentifierUse.OFFICIAL)
.build())
.identifier(Identifier.builder()
.use(IdentifierUse.USUAL)
.build())
.build();
resourceFingerprintVisitor = new ResourceFingerprintVisitor(baseline);
patient.accept(resourceFingerprintVisitor);

assertNotEquals(resourceFingerprintVisitor.getSaltAndHash(), baseline);
}

@Test
public void testIgnoredPaths() throws Exception {
Patient patient = TestUtil.getMinimalResource(ResourceType.PATIENT);

ResourceFingerprintVisitor resourceFingerprintVisitor = new ResourceFingerprintVisitor();
patient.accept(resourceFingerprintVisitor);
SaltHash baseline = resourceFingerprintVisitor.getSaltAndHash();
System.out.println(patient);

patient = patient.toBuilder()
.id("test")
.meta(patient.getMeta().toBuilder()
.versionId(Id.of("ignoreMe"))
.lastUpdated(Instant.now())
.build())
.build();
resourceFingerprintVisitor = new ResourceFingerprintVisitor(baseline);
patient.accept(resourceFingerprintVisitor);

System.out.println(patient);
assertEquals(resourceFingerprintVisitor.getSaltAndHash(), baseline);
}

@Test
public void testUnequalResources_extension() throws Exception {
Patient patient = TestUtil.getMinimalResource(ResourceType.PATIENT);

ResourceFingerprintVisitor resourceFingerprintVisitor = new ResourceFingerprintVisitor();
patient.accept(resourceFingerprintVisitor);
SaltHash baseline = resourceFingerprintVisitor.getSaltAndHash();

patient = patient.toBuilder()
.meta(patient.getMeta().toBuilder()
.tag(patient.getMeta().getTag().get(0).toBuilder()
.code(patient.getMeta().getTag().get(0).getCode().toBuilder()
.extension(Extension.builder()
.url("http://example.com")
.value(string("primitive extension"))
.build())
.build())
.build())
.build())
.build();
resourceFingerprintVisitor = new ResourceFingerprintVisitor(baseline);
patient.accept(resourceFingerprintVisitor);
assertNotEquals(resourceFingerprintVisitor.getSaltAndHash(), baseline);
}
}

0 comments on commit fffe4b0

Please sign in to comment.