Do not 500 and continue IG ingestion when different IGs try to save different ValueSets with colliding FHIR IDs

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/5183-us-core-ig-ingestion-fails-value-set-version-id-conflict.yaml

@@ -0,0 +1,4 @@
+---
+type: fix
+issue: 5183
+title: "The latest US Core IG includes two ValueSets with different contents, but the same FHIR Id and OID via two different included IGs (i.e. `2.16.840.1.113762.1.4.1010.9` via us.cdc.phinvads and us.nlm.vsac).  Ingesting these duplicates in US Core failed with a 500 error. This has been resolved by logging the error and allowing the rest of the ingestion to proceed."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/packages/PackageInstallerSvcImpl.java

@@ -44,6 +44,7 @@
 import ca.uhn.fhir.rest.param.StringParam;
 import ca.uhn.fhir.rest.param.TokenParam;
 import ca.uhn.fhir.rest.param.UriParam;
+import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException;
 import ca.uhn.fhir.util.FhirTerser;
 import ca.uhn.fhir.util.SearchParameterUtil;
 import com.google.common.annotations.VisibleForTesting;
@@ -53,6 +54,7 @@
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.instance.model.api.IPrimitiveType;
 import org.hl7.fhir.r4.model.Identifier;
+import org.hl7.fhir.r4.model.MetadataResource;
 import org.hl7.fhir.utilities.json.model.JsonObject;
 import org.hl7.fhir.utilities.npm.IPackageCacheManager;
 import org.hl7.fhir.utilities.npm.NpmPackage;
@@ -342,7 +344,8 @@ protected void assertFhirVersionsAreCompatible(String fhirVersion, String curren
 	/**
 	 * ============================= Utility methods ===============================
 	 */
-	private void create(
+	@VisibleForTesting
+	void create(
 			IBaseResource theResource,
 			PackageInstallationSpec theInstallationSpec,
 			PackageInstallOutcomeJson theOutcome) {
@@ -365,8 +368,30 @@ private void create(
 						String newIdPart = "npm-" + id.getIdPart();
 						id.setParts(id.getBaseUrl(), id.getResourceType(), newIdPart, id.getVersionIdPart());
 					}
-					updateResource(dao, theResource);
-					ourLog.info("Created resource with existing id");
+
+					try {
+						updateResource(dao, theResource);
+
+						ourLog.info("Created resource with existing id");
+					} catch (ResourceVersionConflictException exception) {
+						final Optional<IBaseResource> optResource = readResourceById(dao, id);
+
+						final String existingResourceUrlOrNull = optResource
+								.filter(MetadataResource.class::isInstance)
+								.map(MetadataResource.class::cast)
+								.map(MetadataResource::getUrl)
+								.orElse(null);
+						final String newResourceUrlOrNull = (theResource instanceof MetadataResource)
+								? ((MetadataResource) theResource).getUrl()
+								: null;
+
+						ourLog.error(
+								"Version conflict error:  This is possibly due to a collision between ValueSets from different IGs that are coincidentally using the same resource ID: [{}] and new resource URL: [{}], with the exisitng resource having URL: [{}].  Ignoring this update and continuing:  The first IG wins.  ",
+								id.getIdPart(),
+								newResourceUrlOrNull,
+								existingResourceUrlOrNull,
+								exception);
+					}
 				}
 			} else {
 				if (theInstallationSpec.isReloadExisting()) {
@@ -394,6 +419,18 @@ private void create(
 		}
 	}
 
+	private Optional<IBaseResource> readResourceById(IFhirResourceDao dao, IIdType id) {
+		try {
+			return Optional.ofNullable(dao.read(id.toUnqualifiedVersionless(), newSystemRequestDetails()));
+
+		} catch (Exception exception) {
+			// ignore because we're running this query to help build the log
+			ourLog.warn("Exception when trying to read resource with ID: {}, message: {}", id, exception.getMessage());
+		}
+
+		return Optional.empty();
+	}
+
 	private IBundleProvider searchResource(IFhirResourceDao theDao, SearchParameterMap theMap) {
 		return theDao.search(theMap, newSystemRequestDetails());
 	}

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/packages/PackageInstallerSvcImplCreateTest.java

@@ -0,0 +1,247 @@
+package ca.uhn.fhir.jpa.packages;
+
+import ca.uhn.fhir.context.FhirContext;
+import ca.uhn.fhir.context.FhirVersionEnum;
+import ca.uhn.fhir.jpa.dao.data.ITermValueSetDao;
+import ca.uhn.fhir.jpa.entity.TermValueSet;
+import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
+import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
+import ca.uhn.fhir.model.primitive.IdDt;
+import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
+import org.hl7.fhir.instance.model.api.IBaseResource;
+import org.hl7.fhir.r4.model.CodeSystem;
+import org.hl7.fhir.r4.model.NamingSystem;
+import org.hl7.fhir.r4.model.ValueSet;
+import org.hl7.fhir.utilities.npm.NpmPackage;
+import org.hl7.fhir.utilities.npm.PackageGenerator;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+
+import javax.annotation.Nonnull;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class PackageInstallerSvcImplCreateTest extends BaseJpaR4Test {
+	private static final String PACKAGE_ID_1 = "package1";
+	private static final String PACKAGE_VERSION = "1.0";
+	private static final String VALUE_SET_OID_FIRST = "2.16.840.1.113762.1.4.1010.9";
+	private static final String VALUE_SET_OID_SECOND = "2.16.840.1.113762.1.4.1010.10";
+	private static final String IG_FIRST = "first.ig.gov";
+	private static final String IG_SECOND = "second.ig.gov";
+	private static final String FIRST_IG_URL_FIRST_OID = String.format("http://%s/fhir/ValueSet/%s", IG_FIRST, VALUE_SET_OID_FIRST);
+	private static final String SECOND_IG_URL_FIRST_OID = String.format("http://%s/fhir/ValueSet/%s", IG_SECOND, VALUE_SET_OID_FIRST);
+	private static final String SECOND_IG_URL_SECOND_OID = String.format("http://%s/fhir/ValueSet/%s", IG_SECOND, VALUE_SET_OID_SECOND);
+	private static final FhirContext ourCtx = FhirContext.forR4Cached();
+	private static final CodeSystem CODE_SYSTEM = createCodeSystem();
+	private static final NpmPackage PACKAGE = createPackage();
+	private static final SystemRequestDetails REQUEST_DETAILS = new SystemRequestDetails();
+
+	@Autowired
+	private ITermValueSetDao myTermValueSetDao;
+
+	@Autowired
+	private PackageInstallerSvcImpl mySvc;
+	@Test
+	void createNamingSystem() throws IOException {
+		final NamingSystem namingSystem = new NamingSystem();
+		namingSystem.getUniqueId().add(new NamingSystem.NamingSystemUniqueIdComponent().setValue("123"));
+
+		create(namingSystem);
+
+		assertEquals(1, myNamingSystemDao.search(SearchParameterMap.newSynchronous(), REQUEST_DETAILS).getAllResources().size());
+	}
+
+	@Test
+	void createWithNoExistingResourcesNoIdOnValueSet() throws IOException {
+		final String version1 = "abc";
+		final String copyright1 = "first";
+
+		createValueSetAndCallCreate(VALUE_SET_OID_FIRST, null, version1, FIRST_IG_URL_FIRST_OID, copyright1);
+
+		final ValueSet actualValueSet1 = getFirstValueSet();
+
+		assertEquals("ValueSet/" + VALUE_SET_OID_FIRST, actualValueSet1.getIdElement().toUnqualifiedVersionless().getValue());
+		assertEquals(FIRST_IG_URL_FIRST_OID, actualValueSet1.getUrl());
+		assertEquals(version1, actualValueSet1.getVersion());
+		assertEquals(copyright1, actualValueSet1.getCopyright());
+	}
+
+	@Test
+	void createWithNoExistingResourcesIdOnValueSet() throws IOException {
+		final String version1 = "abc";
+		final String copyright1 = "first";
+
+		createValueSetAndCallCreate(VALUE_SET_OID_FIRST, null, version1, FIRST_IG_URL_FIRST_OID, copyright1);
+		createValueSetAndCallCreate(VALUE_SET_OID_FIRST, "43", version1, SECOND_IG_URL_FIRST_OID, copyright1);
+
+		final TermValueSet termValueSet = getFirstTermValueSet();
+
+		assertEquals(FIRST_IG_URL_FIRST_OID, termValueSet.getUrl());
+
+		final ValueSet actualValueSet1 = getFirstValueSet();
+
+		assertEquals("ValueSet/" + VALUE_SET_OID_FIRST, actualValueSet1.getIdElement().toUnqualifiedVersionless().getValue());
+		assertEquals(FIRST_IG_URL_FIRST_OID, actualValueSet1.getUrl());
+		assertEquals(version1, actualValueSet1.getVersion());
+		assertEquals(copyright1, actualValueSet1.getCopyright());
+	}
+
+	@Test
+	void createValueSetThenUpdateSameUrl() throws IOException {
+		final String version1 = "abc";
+		final String version2 = "def";
+		final String copyright1 = "first";
+		final String copyright2 = "second";
+
+		createValueSetAndCallCreate(VALUE_SET_OID_FIRST, null, version1, FIRST_IG_URL_FIRST_OID, copyright1);
+		createValueSetAndCallCreate(VALUE_SET_OID_FIRST, "43", version2, FIRST_IG_URL_FIRST_OID, copyright2);
+
+		final ValueSet actualValueSet1 = getFirstValueSet();
+
+		assertEquals("ValueSet/" + VALUE_SET_OID_FIRST, actualValueSet1.getIdElement().toUnqualifiedVersionless().getValue());
+		assertEquals(FIRST_IG_URL_FIRST_OID, actualValueSet1.getUrl());
+		assertEquals(version2, actualValueSet1.getVersion());
+		assertEquals(copyright2, actualValueSet1.getCopyright());
+	}
+
+	@Test
+	void createTwoDifferentValueSets() throws IOException {
+		final String version1 = "abc";
+		final String version2 = "def";
+		final String copyright1 = "first";
+		final String copyright2 = "second";
+
+		createValueSetAndCallCreate(VALUE_SET_OID_FIRST, null, version1, FIRST_IG_URL_FIRST_OID, copyright1);
+		createValueSetAndCallCreate(VALUE_SET_OID_SECOND, "43", version2, SECOND_IG_URL_SECOND_OID, copyright2);
+
+		final List<TermValueSet> all2 = myTermValueSetDao.findAll();
+
+		assertEquals(2, all2.size());
+
+		final TermValueSet termValueSet1 = all2.get(0);
+		final TermValueSet termValueSet2 = all2.get(1);
+
+		assertEquals(FIRST_IG_URL_FIRST_OID, termValueSet1.getUrl());
+		assertEquals(SECOND_IG_URL_SECOND_OID, termValueSet2.getUrl());
+
+		final List<ValueSet> allValueSets = getAllValueSets();
+
+		assertEquals(2, allValueSets.size());
+
+		final ValueSet actualValueSet1 = allValueSets.get(0);
+
+		assertEquals("ValueSet/" + VALUE_SET_OID_FIRST, actualValueSet1.getIdElement().toUnqualifiedVersionless().getValue());
+		assertEquals(FIRST_IG_URL_FIRST_OID, actualValueSet1.getUrl());
+		assertEquals(version1, actualValueSet1.getVersion());
+		assertEquals(copyright1, actualValueSet1.getCopyright());
+
+		final ValueSet actualValueSet2 = allValueSets.get(1);
+
+		assertEquals("ValueSet/" + VALUE_SET_OID_SECOND, actualValueSet2.getIdElement().toUnqualifiedVersionless().getValue());
+		assertEquals(SECOND_IG_URL_SECOND_OID, actualValueSet2.getUrl());
+		assertEquals(version2, actualValueSet2.getVersion());
+		assertEquals(copyright2, actualValueSet2.getCopyright());
+	}
+
+	@Nonnull
+	private List<ValueSet> getAllValueSets() {
+		final List<IBaseResource> allResources = myValueSetDao.search(SearchParameterMap.newSynchronous(), REQUEST_DETAILS).getAllResources();
+
+		assertFalse(allResources.isEmpty());
+		assertTrue(allResources.get(0) instanceof ValueSet);
+
+		return allResources.stream()
+			.map(ValueSet.class::cast)
+			.toList();
+	}
+
+	@Nonnull
+	private ValueSet getFirstValueSet() {
+		final List<IBaseResource> allResources = myValueSetDao.search(SearchParameterMap.newSynchronous(), REQUEST_DETAILS).getAllResources();
+
+		assertEquals(1, allResources.size());
+
+		final IBaseResource resource1 = allResources.get(0);
+		assertTrue(resource1 instanceof ValueSet);
+
+		return (ValueSet) resource1;
+	}
+
+	@Nonnull
+	private TermValueSet getFirstTermValueSet() {
+		final List<TermValueSet> all2 = myTermValueSetDao.findAll();
+
+		assertEquals(1, all2.size());
+
+		return all2.get(0);
+	}
+
+	private void createValueSetAndCallCreate(String theOid, String theResourceVersion, String theValueSetVersion, String theUrl, String theCopyright) throws IOException {
+		create(createValueSet(theOid, theResourceVersion, theValueSetVersion, theUrl, theCopyright));
+	}
+
+	@Nonnull
+	private static ValueSet createValueSet(String theOid, String theResourceVersion, String theValueSetVersion, String theUrl, String theCopyright) {
+		final ValueSet valueSetFromFirstIg = new ValueSet();
+
+		valueSetFromFirstIg.setUrl(theUrl);
+		valueSetFromFirstIg.setId(new IdDt(null, "ValueSet", theOid, theResourceVersion));
+		valueSetFromFirstIg.setVersion(theValueSetVersion);
+		valueSetFromFirstIg.setCopyright(theCopyright);
+
+		return valueSetFromFirstIg;
+	}
+
+	private void create(IBaseResource theResource) throws IOException {
+		mySvc.create(theResource, createInstallationSpec(packageToBytes()), new PackageInstallOutcomeJson());
+	}
+
+	@Nonnull
+	private static CodeSystem createCodeSystem() {
+		final CodeSystem cs = new CodeSystem();
+		cs.setId("CodeSystem/mycs");
+		cs.setUrl("http://my-code-system");
+		cs.setContent(CodeSystem.CodeSystemContentMode.COMPLETE);
+		return cs;
+	}
+
+	@Nonnull
+	private static NpmPackage createPackage() {
+		PackageGenerator manifestGenerator = new PackageGenerator();
+		manifestGenerator.name(PackageInstallerSvcImplCreateTest.PACKAGE_ID_1);
+		manifestGenerator.version(PACKAGE_VERSION);
+		manifestGenerator.description("a package");
+		manifestGenerator.fhirVersions(List.of(FhirVersionEnum.R4.getFhirVersionString()));
+
+		NpmPackage pkg = NpmPackage.empty(manifestGenerator);
+
+		String csString = ourCtx.newJsonParser().encodeResourceToString(CODE_SYSTEM);
+		pkg.addFile("package", "cs.json", csString.getBytes(StandardCharsets.UTF_8), "CodeSystem");
+
+		return pkg;
+	}
+
+	@Nonnull
+	private static PackageInstallationSpec createInstallationSpec(byte[] thePackageContents) {
+		final PackageInstallationSpec spec = new PackageInstallationSpec();
+		spec.setName(PACKAGE_ID_1);
+		spec.setVersion(PACKAGE_VERSION);
+		spec.setInstallMode(PackageInstallationSpec.InstallModeEnum.STORE_AND_INSTALL);
+		spec.setPackageContents(thePackageContents);
+		return spec;
+	}
+
+	@Nonnull
+	private static byte[] packageToBytes() throws IOException {
+		ByteArrayOutputStream stream = new ByteArrayOutputStream();
+		PackageInstallerSvcImplCreateTest.PACKAGE.save(stream);
+		return stream.toByteArray();
+	}
+}