Fixes a bug with tags. (#4813)

* Test, fix

* Drop constraint, add migration

* Add changelog

* Fix userSelected null vs false

* Fix merge

* Fix up checkstyle whining

* One more failure

* Fix test

* wip

* changelog clarity

Co-authored-by: James Agnew <jamesagnew@gmail.com>

* change index

---------

Co-authored-by: Michael Buckley <michaelabuckley@gmail.com>
Co-authored-by: James Agnew <jamesagnew@gmail.com>

hapi-fhir-base/src/main/java/ca/uhn/fhir/model/api/Tag.java

@@ -19,13 +19,14 @@
  */
 package ca.uhn.fhir.model.api;
 
-import java.net.URI;
-
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.builder.ToStringBuilder;
 import org.apache.commons.lang3.builder.ToStringStyle;
 import org.hl7.fhir.instance.model.api.IBaseCoding;
 
+import java.net.URI;
+import java.util.Objects;
+
 /**
  * A single tag
  * <p>
@@ -58,7 +59,7 @@ public class Tag extends BaseElement implements IElement, IBaseCoding {
 	private String myScheme;
 	private String myTerm;
 	private String myVersion;
-	private boolean myUserSelected;
+	private Boolean myUserSelected;
 
 	public Tag() {
 	}
@@ -114,37 +115,22 @@ public class Tag extends BaseElement implements IElement, IBaseCoding {
 		if (getClass() != obj.getClass())
 			return false;
 		Tag other = (Tag) obj;
-		if (myScheme == null) {
-			if (other.myScheme != null)
-				return false;
-		} else if (!myScheme.equals(other.myScheme))
-			return false;
-		if (myTerm == null) {
-			if (other.myTerm != null)
-				return false;
-		} else if (!myTerm.equals(other.myTerm))
-			return false;
 
-		if (myVersion == null) {
+		return
-			if (other.getVersion() != null)
+			Objects.equals(myScheme, other.myScheme) &&
-				return false;
+			Objects.equals(myTerm, other.myTerm) &&
-		} else if (!myVersion.equals(other.getVersion()))
+			Objects.equals(myVersion, other.myVersion) &&
-			return false;
+			Objects.equals(myUserSelected, other.myUserSelected);
-
-		if (myUserSelected != other.getUserSelected())
-			return false;
-
-		return true;
 	}
 
 	@Override
 	public int hashCode() {
 		final int prime = 31;
 		int result = 1;
-		result = prime * result + ((myScheme == null) ? 0 : myScheme.hashCode());
+		result = prime * result + Objects.hashCode(myScheme);
-		result = prime * result + ((myTerm == null) ? 0 : myTerm.hashCode());
+		result = prime * result + Objects.hashCode(myTerm);
-		result = prime * result + ((myVersion == null) ? 0 : myVersion.hashCode());
+		result = prime * result + Objects.hashCode(myVersion);
-		result = prime * result + Boolean.hashCode(myUserSelected);
+		result = prime * result + Objects.hashCode(myUserSelected);
 		return result;
 	}
 
@@ -234,7 +220,9 @@ public class Tag extends BaseElement implements IElement, IBaseCoding {
 	}
 
 	@Override
-	public boolean getUserSelected() { return myUserSelected; }
+	public boolean getUserSelected() { return myUserSelected != null && myUserSelected; }
+
+	public Boolean getUserSelectedBoolean() { return myUserSelected; }
 
 	@Override
 	public IBaseCoding setUserSelected(boolean theUserSelected) {
@@ -242,4 +230,8 @@ public class Tag extends BaseElement implements IElement, IBaseCoding {
 		return this;
 	}
 
+	public void setUserSelectedBoolean(Boolean theUserSelected) {
+		myUserSelected = theUserSelected;
+	}
+
 }

hapi-fhir-base/src/main/java/ca/uhn/fhir/parser/JsonParser.java

@@ -724,6 +724,15 @@ public class JsonParser extends BaseParser implements IJsonLikeParser {
 		}
 
 		if (theResource instanceof IResource) {
+			parseMetaForDSTU2(theResDef, theResource, theEventWriter, theContainedResource, theEncodeContext, resDef);
+		}
+
+		encodeCompositeElementToStreamWriter(theResDef, theResource, theResource, theEventWriter, theContainedResource, new CompositeChildElement(resDef, theEncodeContext), theEncodeContext);
+
+		theEventWriter.endObject();
+	}
+
+	private void parseMetaForDSTU2(RuntimeResourceDefinition theResDef, IBaseResource theResource, BaseJsonLikeWriter theEventWriter, boolean theContainedResource, EncodeContext theEncodeContext, RuntimeResourceDefinition resDef) throws IOException {
 		IResource resource = (IResource) theResource;
 		// Object securityLabelRawObj =
 
@@ -782,6 +791,7 @@ public class JsonParser extends BaseParser implements IJsonLikeParser {
 					writeOptionalTagWithTextNode(theEventWriter, "system", tag.getScheme());
 					writeOptionalTagWithTextNode(theEventWriter, "code", tag.getTerm());
 					writeOptionalTagWithTextNode(theEventWriter, "display", tag.getLabel());
+					// wipmb should we be writing the new properties here?  There must be another path.
 					theEventWriter.endObject();
 				}
 				theEventWriter.endArray();
@@ -793,11 +803,6 @@ public class JsonParser extends BaseParser implements IJsonLikeParser {
 		}
 	}
 
-		encodeCompositeElementToStreamWriter(theResDef, theResource, theResource, theEventWriter, theContainedResource, new CompositeChildElement(resDef, theEncodeContext), theEncodeContext);
-
-		theEventWriter.endObject();
-	}
-
 
 	private void addExtensionMetadata(RuntimeResourceDefinition theResDef, IBaseResource theResource,
 												 boolean theContainedResource,

hapi-fhir-base/src/test/java/ca/uhn/fhir/model/api/TagTest.java

@@ -27,7 +27,7 @@ public class TagTest {
 	@Test
 	public void testHashCode() {
 		Tag tag1 = new Tag().setScheme("scheme").setTerm("term").setLabel("label");
-		assertEquals(-1029266947, tag1.hashCode());
+		assertEquals(-1029268184, tag1.hashCode());
 	}
 
 	@Test

hapi-fhir-converter/src/main/java/ca/uhn/hapi/converters/canonical/VersionCanonicalizer.java

@@ -364,7 +364,9 @@ public class VersionCanonicalizer {
 			retVal.setSystem(coding.getSystem());
 			retVal.setDisplay(coding.getDisplay());
 			retVal.setVersion(coding.getVersion());
-			retVal.setUserSelected(!coding.getUserSelectedElement().isEmpty() && coding.getUserSelected());
+			if (!coding.getUserSelectedElement().isEmpty()) {
+				retVal.setUserSelected( coding.getUserSelected() );
+			}
 
 			return retVal;
 		}

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4819-tag-userSelected-null.yaml

@@ -0,0 +1,4 @@
+---
+type: fix
+issue: 4819
+title: "Tags no longer provide a default `false` value for `userSelected`."

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/4813-fix-tag-nonunique.yaml

@@ -0,0 +1,4 @@
+---
+type: fix
+issue: 4813
+title: "Under heavy concurrency, a bug resulted in identical tag definitions being rejected with a `NonUniqueResultException` some of the time.  This has been corrected."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java

@@ -247,6 +247,8 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 	@Autowired
 	private PlatformTransactionManager myTransactionManager;
 
+	protected final CodingSpy myCodingSpy = new CodingSpy();
+
 	@VisibleForTesting
 	public void setExternallyStoredResourceServiceRegistryForUnitTest(ExternallyStoredResourceServiceRegistry theExternallyStoredResourceServiceRegistry) {
 		myExternallyStoredResourceServiceRegistry = theExternallyStoredResourceServiceRegistry;
@@ -283,7 +285,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 		if (tagList != null) {
 			for (Tag next : tagList) {
 				TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.TAG, next.getScheme(), next.getTerm(),
-					next.getLabel(), next.getVersion(), next.getUserSelected());
+					next.getLabel(), next.getVersion(), myCodingSpy.getBooleanObject(next));
 				if (def != null) {
 					ResourceTag tag = theEntity.addTag(def);
 					allDefs.add(tag);
@@ -323,7 +325,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 		if (tagList != null) {
 			for (IBaseCoding next : tagList) {
 				TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.TAG, next.getSystem(), next.getCode(),
-					next.getDisplay(), next.getVersion(), next.getUserSelected());
+					next.getDisplay(), next.getVersion(), myCodingSpy.getBooleanObject(next));
 				if (def != null) {
 					ResourceTag tag = theEntity.addTag(def);
 					theAllTags.add(tag);
@@ -335,7 +337,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 		List<? extends IBaseCoding> securityLabels = theResource.getMeta().getSecurity();
 		if (securityLabels != null) {
 			for (IBaseCoding next : securityLabels) {
-				TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.SECURITY_LABEL, next.getSystem(), next.getCode(), next.getDisplay(), null, null);
+				TagDefinition def = getTagOrNull(theTransactionDetails, TagTypeEnum.SECURITY_LABEL, next.getSystem(), next.getCode(), next.getDisplay(), next.getVersion(),  myCodingSpy.getBooleanObject(next));
 				if (def != null) {
 					ResourceTag tag = theEntity.addTag(def);
 					theAllTags.add(tag);
@@ -394,7 +396,6 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 		MemoryCacheService.TagDefinitionCacheKey key = toTagDefinitionMemoryCacheKey(theTagType, theScheme, theTerm, theVersion, theUserSelected);
 
 		TagDefinition retVal = myMemoryCacheService.getIfPresent(MemoryCacheService.CacheEnum.TAG_DEFINITION, key);
-
 		if (retVal == null) {
 			HashMap<MemoryCacheService.TagDefinitionCacheKey, TagDefinition> resolvedTagDefinitions = theTransactionDetails
 				.getOrCreateUserData(HapiTransactionService.XACT_USERDATA_KEY_RESOLVED_TAG_DEFINITIONS, HashMap::new);
@@ -425,6 +426,7 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 													 String theVersion, Boolean theUserSelected) {
 
 		TypedQuery<TagDefinition> q = buildTagQuery(theTagType, theScheme, theTerm, theVersion, theUserSelected);
+		q.setMaxResults(1);
 
 		TransactionTemplate template = new TransactionTemplate(myTransactionManager);
 		template.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW);
@@ -525,9 +527,9 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 			? builder.isNull(from.get("myVersion"))
 			: builder.equal(from.get("myVersion"), theVersion));
 
-		predicates.add( isNull(theUserSelected) || isFalse(theUserSelected)
+		predicates.add( isNull(theUserSelected)
-			? builder.isFalse(from.get("myUserSelected"))
+			? builder.isNull(from.get("myUserSelected"))
-			: builder.isTrue(from.get("myUserSelected")));
+			: builder.equal(from.get("myUserSelected"), theUserSelected));
 
 		cq.where(predicates.toArray(new Predicate[0]));
 		return myEntityManager.createQuery(cq);
@@ -817,12 +819,14 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> extends BaseStora
 
 		// Update the resource to contain the old tags
 		allTagsOld.forEach(tag -> {
-			theResource.getMeta()
+			IBaseCoding iBaseCoding = theResource.getMeta()
 				.addTag()
 				.setCode(tag.getTag().getCode())
 				.setSystem(tag.getTag().getSystem())
-				.setVersion(tag.getTag().getVersion())
+				.setVersion(tag.getTag().getVersion());
-				.setUserSelected(tag.getTag().getUserSelected());
+				if (tag.getTag().getUserSelected() != null) {
+					iBaseCoding.setUserSelected(tag.getTag().getUserSelected());
+				}
 		});
 
 		theEntity.setHasTags(!allTagsNew.isEmpty());

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java

@@ -834,6 +834,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 				}
 			}
 
+
 			if (!hasTag) {
 				theEntity.setHasTags(true);
 

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/CodingSpy.java

@@ -0,0 +1,61 @@
+package ca.uhn.fhir.jpa.dao;
+
+import ca.uhn.fhir.i18n.Msg;
+import ca.uhn.fhir.model.primitive.BooleanDt;
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.hl7.fhir.instance.model.api.IBaseBooleanDatatype;
+import org.hl7.fhir.instance.model.api.IBaseCoding;
+
+import java.lang.reflect.Field;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * We are trying to preserve null behaviour despite IBaseCoding using primitive boolean for userSelected.
+ */
+public class CodingSpy {
+	final Map<Class, Field> mySpies = new HashMap<>();
+
+	/**
+	 * Reach into the Coding and pull out the Boolean instead of the boolean.
+	 * @param theValue the Coding, or CodingDt
+	 * @return the Boolean
+	 */
+	public Boolean getBooleanObject(IBaseCoding theValue) {
+		Field spy = getSpy(theValue);
+		try {
+			Object o = spy.get(theValue);
+			if (o == null) {
+				return null;
+			}
+			if (o instanceof BooleanDt) {
+				BooleanDt booleanDt = (BooleanDt) o;
+				return booleanDt.getValue();
+			}
+			if (o instanceof IBaseBooleanDatatype) {
+				IBaseBooleanDatatype booleanValue = (IBaseBooleanDatatype) o;
+				return booleanValue.getValue();
+			}
+			if (o instanceof Boolean) {
+				return (Boolean) o;
+			}
+			throw new RuntimeException(Msg.code(2342) + "unsupported type :" + theValue.getClass().getName());
+		} catch (IllegalAccessException theException) {
+			// should never happen - all Coding models have this field.
+			throw new RuntimeException(Msg.code(2343) + "illegal access during reflection", theException);
+		}
+	}
+
+	private Field getSpy(IBaseCoding theValue) {
+		return mySpies.computeIfAbsent(theValue.getClass(), k -> getFieldHandle(k));
+	}
+
+	private static Field getFieldHandle(Class k) {
+		Field result = FieldUtils.getField(k, "userSelected", true);
+		if (result == null) {
+			result = FieldUtils.getField(k, "myUserSelected", true);
+		}
+		return result;
+	}
+
+}

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java

@@ -39,6 +39,7 @@ import ca.uhn.fhir.jpa.model.entity.ResourceEncodingEnum;
 import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
 import ca.uhn.fhir.jpa.model.entity.ResourceTable;
 import ca.uhn.fhir.jpa.model.entity.ResourceTag;
+import ca.uhn.fhir.jpa.model.entity.TagDefinition;
 import ca.uhn.fhir.jpa.model.entity.TagTypeEnum;
 import ca.uhn.fhir.jpa.partition.IPartitionLookupSvc;
 import ca.uhn.fhir.model.api.IResource;
@@ -72,7 +73,7 @@ import java.util.Date;
 import java.util.List;
 
 import static ca.uhn.fhir.jpa.dao.BaseHapiFhirDao.decodeResource;
-import static org.apache.commons.lang3.StringUtils.defaultString;
+import static java.util.Objects.nonNull;
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 public class JpaStorageResourceParser implements IJpaStorageResourceParser {
@@ -350,19 +351,26 @@ public class JpaStorageResourceParser implements IJpaStorageResourceParser {
 				List<IBaseCoding> securityLabels = new ArrayList<>();
 				List<IdDt> profiles = new ArrayList<>();
 				for (BaseTag next : theTagList) {
-					switch (next.getTag().getTagType()) {
+					TagDefinition nextTag = next.getTag();
+					switch (nextTag.getTagType()) {
 						case PROFILE:
-							profiles.add(new IdDt(next.getTag().getCode()));
+							profiles.add(new IdDt(nextTag.getCode()));
 							break;
 						case SECURITY_LABEL:
 							IBaseCoding secLabel = (IBaseCoding) myFhirContext.getVersion().newCodingDt();
-							secLabel.setSystem(next.getTag().getSystem());
+							secLabel.setSystem(nextTag.getSystem());
-							secLabel.setCode(next.getTag().getCode());
+							secLabel.setCode(nextTag.getCode());
-							secLabel.setDisplay(next.getTag().getDisplay());
+							secLabel.setDisplay(nextTag.getDisplay());
+							// wipmb these technically support userSelected and version
 							securityLabels.add(secLabel);
 							break;
 						case TAG:
-							tagList.add(new Tag(next.getTag().getSystem(), next.getTag().getCode(), next.getTag().getDisplay()));
+							// wipmb check xml, etc.
+							Tag e = new Tag(nextTag.getSystem(), nextTag.getCode(), nextTag.getDisplay());
+							e.setVersion(nextTag.getVersion());
+							// careful! These are Boolean, not boolean.
+							e.setUserSelectedBoolean(nextTag.getUserSelected());
+							tagList.add(e);
 							break;
 					}
 				}
@@ -434,7 +442,12 @@ public class JpaStorageResourceParser implements IJpaStorageResourceParser {
 						tag.setCode(next.getTag().getCode());
 						tag.setDisplay(next.getTag().getDisplay());
 						tag.setVersion(next.getTag().getVersion());
-						tag.setUserSelected(next.getTag().getUserSelected());
+						Boolean userSelected = next.getTag().getUserSelected();
+						// the tag is created with a null userSelected, but the api is primitive boolean.
+						// Only update if we are non-null.
+						if (nonNull(userSelected)) {
+							tag.setUserSelected(userSelected);
+						}
 						break;
 				}
 			}

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java

@@ -108,6 +108,19 @@ public class HapiFhirJpaMigrationTasks extends BaseMigrationTasks<VersionEnum> {
       .unique(false)
       .online(true)
       .withColumns("RES_ID", "RES_UPDATED", "PARTITION_ID");
+    
+		Builder.BuilderWithTableName tagDefTable = version.onTable("HFJ_TAG_DEF");
+		tagDefTable.dropIndex("20230505.1", "IDX_TAGDEF_TYPESYSCODEVERUS");
+
+		tagDefTable.dropIndex("20230505.2", "IDX_TAG_DEF_TP_CD_SYS");
+		tagDefTable
+			.addIndex("20230505.3", "IDX_TAG_DEF_TP_CD_SYS")
+			.unique(false)
+			.online(false)
+			.withColumns("TAG_TYPE", "TAG_CODE", "TAG_SYSTEM", "TAG_ID", "TAG_VERSION", "TAG_USER_SELECTED");
+
+
+
 	}
 
 	protected void init660() {
@@ -362,7 +375,13 @@ public class HapiFhirJpaMigrationTasks extends BaseMigrationTasks<VersionEnum> {
 			.online(true)
 			.withColumns("SEARCH_PID")
 			.onlyAppliesToPlatforms(NON_AUTOMATIC_FK_INDEX_PLATFORMS);
+
+		{  //We added this constraint when userSelected and Version were added. It is no longer necessary.
+			Builder.BuilderWithTableName tagDefTable = version.onTable("HFJ_TAG_DEF");
+			tagDefTable.dropIndex("20230503.1", "IDX_TAGDEF_TYPESYSCODEVERUS");
 		}
+	}
+
 
 	private void init620() {
 		Builder version = forVersion(VersionEnum.V6_2_0);

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/CodingSpyTest.java

@@ -0,0 +1,83 @@
+package ca.uhn.fhir.jpa.dao;
+
+import com.google.common.collect.Lists;
+import org.hl7.fhir.instance.model.api.IBaseCoding;
+import org.hl7.fhir.r4.model.BooleanType;
+import org.hl7.fhir.r4.model.Coding;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import javax.annotation.Nonnull;
+import java.util.List;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.blankOrNullString;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+class CodingSpyTest {
+
+	/**
+	 * Ensure we can read the default null value of userSelected on Coding
+	 */
+
+	@ParameterizedTest
+	@MethodSource("getCases")
+	void canReadValueUserSelected(IBaseCoding theObject, Boolean theValue)  {
+		IBaseCoding value = theObject.setSystem("http://example.com").setCode("value");
+		if (theValue != null) {
+			theObject.setUserSelected(theValue);
+		}
+
+		Boolean result = new CodingSpy().getBooleanObject(theObject);
+
+		assertEquals(theValue, result);
+	}
+
+	@Test
+	void canReadNullUserSelected()  {
+		Coding value = new Coding().setSystem("http://example.com").setCode("value");
+
+		Boolean result = new CodingSpy().getBooleanObject(value);
+
+		assertNull(result);
+	}
+
+	static List<Arguments> getCases() {
+		var classes = List.of(
+			org.hl7.fhir.r4.model.Coding.class,
+			org.hl7.fhir.r5.model.Coding.class,
+			ca.uhn.fhir.model.api.Tag.class
+		);
+		var values = Lists.newArrayList(true, false, null);
+		return classes.stream()
+			.flatMap(k-> values.stream()
+				.map(v-> Arguments.of(getNewInstance(k), v)))
+			.toList();
+	}
+
+	@Nonnull
+	private static Object getNewInstance(Class<? extends IBaseCoding> k) {
+		try {
+			return k.getDeclaredConstructor().newInstance();
+		} catch (Exception theE) {
+			throw new RuntimeException(theE);
+		}
+	}
+
+
+	@Test
+	void booleanNulls() {
+	    // given
+		BooleanType b = new BooleanType();
+
+	    // when
+		var s = b.asStringValue();
+
+		assertThat(s, blankOrNullString());
+	}
+
+
+}

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/TagDefinition.java

@@ -46,11 +46,7 @@ import java.util.Collection;
 @Table(
 	name = "HFJ_TAG_DEF",
 	indexes = {
-		@Index(name = "IDX_TAG_DEF_TP_CD_SYS", columnList = "TAG_TYPE, TAG_CODE, TAG_SYSTEM, TAG_ID"),
+		@Index(name = "IDX_TAG_DEF_TP_CD_SYS", columnList = "TAG_TYPE, TAG_CODE, TAG_SYSTEM, TAG_ID, TAG_VERSION, TAG_USER_SELECTED"),
-	},
-	uniqueConstraints = {
-		@UniqueConstraint(name = "IDX_TAGDEF_TYPESYSCODEVERUS",
-			columnNames = {"TAG_TYPE", "TAG_SYSTEM", "TAG_CODE", "TAG_VERSION", "TAG_USER_SELECTED"})
 	}
 )
 public class TagDefinition implements Serializable {
@@ -161,14 +157,16 @@ public class TagDefinition implements Serializable {
 		}
 	}
 
+	/**
+	 * Warning - this is nullable, while IBaseCoding getUserSelected isn't.
+	 * wipmb maybe rename?
+	 */
 	public Boolean getUserSelected() {
-		// TODO: LD: this is not ideal as we are implicitly assuming null is false.
+		return myUserSelected;
-		//  Ideally we should fix IBaseCoding to return wrapper Boolean but that will involve another core/hapi release
-		return myUserSelected != null ? myUserSelected : false;
 	}
 
 	public void setUserSelected(Boolean theUserSelected) {
-		myUserSelected = theUserSelected != null && theUserSelected;
+		myUserSelected = theUserSelected;
 	}
 
 

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4MetaTest.java

@@ -9,6 +9,7 @@ import ca.uhn.fhir.rest.api.server.IBundleProvider;
 import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 import ca.uhn.fhir.rest.param.TokenParam;
 import ca.uhn.fhir.system.HapiTestSystemProperties;
+import ch.qos.logback.classic.Level;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.Bundle;
 import org.hl7.fhir.r4.model.Coding;
@@ -41,6 +42,8 @@ import static org.hamcrest.Matchers.empty;
 import static org.junit.jupiter.api.Assertions.assertAll;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -153,6 +156,30 @@ public class FhirResourceDaoR4MetaTest extends BaseJpaR4Test {
 		assertEquals("bar", patient2.getMeta().getSecurityFirstRep().getCode());
 	}
 
+	/**
+	 * Make sure round-trips with tags don't add a userSelected property.
+	 * wipmb tag userSelected test
+	 * Verify https://github.com/hapifhir/hapi-fhir/issues/4819
+	 */
+	@Test
+	void testAddTag_userSelectedStaysNull() {
+	    // given
+		Patient patient1 = new Patient();
+		patient1.getMeta().addTag().setSystem("http://foo").setCode("bar");
+		patient1.setActive(true);
+		IIdType pid1 = myPatientDao.create(patient1).getId();
+
+		// when
+		var patientReadback = myPatientDao.read(pid1);
+		ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(patientReadback));
+
+		// then
+		var tag = patientReadback.getMeta().getTag().get(0);
+		assertNotNull(tag);
+		assertNull(tag.getUserSelectedElement().asStringValue());
+	}
+
+
 	@Nested
 	public class TestTagWithVersionAndUserSelected {
 
@@ -204,6 +231,7 @@ public class FhirResourceDaoR4MetaTest extends BaseJpaR4Test {
 				.setSystem(expectedSystem1)
 				.setCode(expectedCode1)
 				.setDisplay(expectedDisplay1);
+			assertNull(newTag.getUserSelectedElement().asStringValue());
 			assertFalse(newTag.getUserSelected());
 			newTag.setVersion(expectedVersion1)
 				.setUserSelected(expectedUserSelected1);

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java

@@ -5,6 +5,8 @@ import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
 import ca.uhn.fhir.jpa.model.dao.JpaPid;
 import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
 import ca.uhn.fhir.jpa.model.entity.ResourceTable;
+import ca.uhn.fhir.jpa.model.entity.TagDefinition;
+import ca.uhn.fhir.jpa.model.entity.TagTypeEnum;
 import ca.uhn.fhir.jpa.model.util.JpaConstants;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
@@ -96,6 +98,33 @@ public class FhirResourceDaoR4UpdateTest extends BaseJpaR4Test {
 	}
 
 
+	@Test
+	public void testTagCollision() {
+		TagDefinition def = new TagDefinition();
+		def.setTagType(TagTypeEnum.TAG);
+		def.setSystem("system");
+		def.setCode("coding");
+		def.setDisplay("display");
+		def.setUserSelected(null);
+
+
+		Patient p = new Patient();
+		p.getMeta().addTag("system", "coding", "display");
+
+		myMemoryCacheService.invalidateAllCaches();
+		myPatientDao.create(p, new SystemRequestDetails());
+		//inject conflicting.
+		myTagDefinitionDao.saveAndFlush(def);
+		myMemoryCacheService.invalidateAllCaches();
+
+		myPatientDao.create(p, new SystemRequestDetails());
+		myMemoryCacheService.invalidateAllCaches();
+
+		myPatientDao.create(p, new SystemRequestDetails());
+
+	}
+
+
 	@Test
 	public void testCreateAndUpdateWithoutRequest() {
 		String methodName = "testUpdateByUrl";