Batch option processing cleanup (#4466)

* BulkDataExportOptions null safety

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

@@ -738,7 +738,6 @@ public class ExtendedHSearchClauseBuilder {
 					subMatch = buildNumericClause(value, componentContext);
 					break;
 				case REFERENCE:
-				// wipmb Can we use reference in composite?
 
 				default:
 					break;

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

@@ -84,7 +84,7 @@ public class ExtendedHSearchIndexExtractor {
 
 		retVal.setForcedId(theResource.getIdElement().getIdPart());
 
-		// wipmb mb add a flag ot DaoConfig to suppress this
+		// todo add a flag ot DaoConfig to suppress this
 		extractAutocompleteTokens(theResource, retVal);
 
 		theNewParams.myStringParams.stream()

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

@@ -163,7 +163,6 @@ class HSearchCompositeSearchIndexDataImpl implements CompositeSearchIndexData {
 					assert false: "composite components can't be composite";
 					break;
 
-				// wipmb Can any of these be part of a composite?
 				case REFERENCE:
 					break;
 

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/r4/MemberMatcherR4Helper.java

@@ -143,9 +143,7 @@ public class MemberMatcherR4Helper {
 		updateConsentPatientAndPerformer(theConsent, thePatient);
 		myConsentModifier.accept(theConsent);
 
-		// WIPMB REVIEW QUESTION Which partition should we target?
+		// Trust RequestTenantPartitionInterceptor or PatientIdPartitionInterceptor to assign the partition.
-		// Will RequestTenantPartitionInterceptor or PatientIdPartitionInterceptor do the right thing?
-		// Can we use the userdata field to hint at target partition?
 		myConsentDao.create(theConsent, theRequestDetails);
 	}
 

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java

@@ -376,7 +376,6 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 				CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequest, Pointcut.JPA_PERFTRACE_INDEXSEARCH_QUERY_COMPLETE, params);
 			}
 
-			// todo MB extract this and move to FullText svc
 			// can we skip the database entirely and return the pid list from here?
 			boolean canSkipDatabase =
 				// if we processed an AND clause, and it returned nothing, then nothing can match.
@@ -429,7 +428,7 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 			failIfUsed(Constants.PARAM_CONTENT);
 		}
 
-		// TODO MB someday we'll want a query planner to figure out if we _should_ or _must_ use the ft index, not just if we can.
+		// someday we'll want a query planner to figure out if we _should_ or _must_ use the ft index, not just if we can.
 		return fulltextEnabled && myParams != null &&
 			myParams.getSearchContainedMode() == SearchContainedModeEnum.FALSE &&
 			myFulltextSearchSvc.supportsSomeOf(myParams);

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/search/ExtendedHSearchIndexData.java

@@ -113,7 +113,6 @@ public class ExtendedHSearchIndexData {
 	 * Add if not already present.
 	 */
 	public void addTokenIndexDataIfNotPresent(String theSpName, String theSystem,  String theValue) {
-		// todo MB create a BaseCodingDt that respects equals
 		boolean isPresent = mySearchParamTokens.get(theSpName).stream()
 			.anyMatch(c -> Objects.equals(c.getSystem(), theSystem) && Objects.equals(c.getCode(), theValue));
 		if (!isPresent) {

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/search/HSearchIndexWriter.java

@@ -114,10 +114,8 @@ public class HSearchIndexWriter {
 		}
 
 		DocumentElement tokenIndexNode = getSearchParamIndexNode(theSearchParam, INDEX_TYPE_TOKEN);
-		// TODO mb we can use a token_filter with pattern_capture to generate all three off a single value.  Do this next, after merge.
 		writeTokenFields(tokenIndexNode, theValue);
 		ourLog.debug("Adding Search Param Token: {} -- {}", theSearchParam, theValue);
-		// TODO mb should we write the strings here too?  Or leave it to the old spidx indexing?
 	}
 
 	public void writeTokenFields(DocumentElement theDocumentElement, IBaseCoding theValue) {

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/search/SearchParamTextPropertyBinder.java

@@ -151,11 +151,11 @@ public class SearchParamTextPropertyBinder implements PropertyBinder, PropertyBr
 			IndexSchemaObjectField nestedSpField = indexSchemaElement.objectField(HSearchIndexWriter.NESTED_SEARCH_PARAM_ROOT, ObjectStructure.FLATTENED);
 			nestedSpField.toReference();
 
-			// TODO MB: the lucene/elastic independent api is hurting a bit here.
+			// Note: the lucene/elastic independent api is hurting a bit here.
 			// For lucene, we need a separate field for each analyzer.  So we'll add string (for :exact), and text (for :text).
 			// They aren't marked stored, so there's no space cost beyond the index for each.
-			// But for elastic, I'd rather have a single field defined, with multi-field sub-fields.  The index cost is the same,
+			// But for elastic, we'd rather have a single field defined, with multi-field sub-fields.  The index cost is the same,
-			// but elastic will actually store all fields in the source document.
+			// but elastic will actually store all fields in the source document and consume disk.
 
 			// So triplicate the storage for now. :-(
 			String stringPathGlob = "*.string";

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/BaseSearchParamExtractor.java

@@ -386,8 +386,7 @@ public abstract class BaseSearchParamExtractor implements ISearchParamExtractor
 					// Bug workaround: the component expressions are null in the FhirContextSearchParamRegistry. We can't do anything with them.
 					c.getExpression() == null ||
 
-					// wipmb hack hack alert:
+					// TODO mb Bug workaround: we don't support the %resource variable, but standard SPs on MolecularSequence use it.
-					// Bug workaround: we don't support the %resource variable, but standard SPs on MolecularSequence use it.
 					// Skip them for now.
 					c.getExpression().contains("%resource");
 		}

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/SearchParamExtractorService.java

@@ -265,7 +265,6 @@ public class SearchParamExtractorService {
 		}
 
 		// Composites
-		// wipmb should we have config to skip this cpu work?  Check to see if  HSearch is enabled?
 		// dst2 composites use stuff like value[x] , and we don't support them.
 		if (myContext.getVersion().getVersion().isEqualOrNewerThan(FhirVersionEnum.DSTU3)) {
 			ISearchParamExtractor.SearchParamSet<ResourceIndexedSearchParamComposite> composites = extractSearchParamComposites(theResource);

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/util/JpaParamUtil.java

@@ -150,7 +150,7 @@ public enum JpaParamUtil {
 	public static List<RuntimeSearchParam> resolveComponentParameters(ISearchParamRegistry theSearchParamRegistry, RuntimeSearchParam theParamDef) {
 		List<RuntimeSearchParam> compositeList = resolveCompositeComponentsDeclaredOrder(theSearchParamRegistry, theParamDef);
 
-		// wipmb why is this sorted?  Is the param order flipped too during query-time?
+		// todo mb why is this sorted?  Is the param order flipped too during query-time?
 		compositeList.sort((Comparator.comparing(RuntimeSearchParam::getName)));
 
 		return compositeList;

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

@@ -53,7 +53,7 @@ public class FhirResourceDaoR4SearchFtTest extends BaseJpaR4Test {
 	}
 
 	/**
-	 * TODO mb Extract these tests and run on all: jpa, lucene, es, and mongo. {@link FhirResourceDaoR4SearchWithElasticSearchIT}
+	 * TODO mb Extract these tests and run on all: jpa, lucene, es, etc. {@link FhirResourceDaoR4SearchWithElasticSearchIT}
 	 * {@link FhirResourceDaoR4SearchWithElasticSearchIT#testStringSearch}
 	 */
 	@Test

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

@@ -413,7 +413,7 @@ public class FhirResourceDaoR4StandardQueriesNoFTTest extends BaseJpaTest {
 
 	}
 
-	// wipmb re-enable this.  Some of these fail!
+	// todo mb re-enable this.  Some of these fail!
 	@Disabled
 	@Nested
 	class QuantityAndNormalizedQuantitySearch extends QuantitySearchParameterTestCases {

hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/testutil/BulkExportBatch2TestUtils.java

@@ -5,6 +5,7 @@ import ca.uhn.fhir.jpa.api.model.BulkExportParameters;
 import ca.uhn.fhir.rest.api.server.bulk.BulkDataExportOptions;
 import ca.uhn.fhir.util.Batch2JobDefinitionConstants;
 import ca.uhn.fhir.util.SearchParameterUtil;
+import org.apache.commons.collections4.CollectionUtils;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -33,7 +34,7 @@ public class BulkExportBatch2TestUtils {
 		// if none are provided, the job submitter adds them
 		// but we cannot manually start the job without correct parameters
 		// so we "correct" them here
-		if (theOptions.getResourceTypes() == null || theOptions.getResourceTypes().isEmpty()) {
+		if (CollectionUtils.isEmpty(theOptions.getResourceTypes())) {
 			addAllResourceTypes(parameters, theCtx);
 		}
 		else {

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/bulk/BulkDataExportOptions.java

@@ -22,13 +22,11 @@ package ca.uhn.fhir.rest.api.server.bulk;
 
 import org.hl7.fhir.instance.model.api.IIdType;
 
+import javax.annotation.Nonnull;
 import java.util.Date;
 import java.util.Set;
 
 public class BulkDataExportOptions {
-	public BulkDataExportOptions() {
-
-	}
 
 	public enum ExportStyle {
 		PATIENT,
@@ -73,7 +71,11 @@ public class BulkDataExportOptions {
 		return myOutputFormat;
 	}
 
+	@Nonnull
 	public Set<String> getResourceTypes() {
+		if (myResourceTypes == null) {
+			myResourceTypes = Set.of();
+		}
 		return myResourceTypes;
 	}
 
@@ -81,7 +83,11 @@ public class BulkDataExportOptions {
 		return mySince;
 	}
 
+	@Nonnull
 	public Set<String> getFilters() {
+		if (myFilters == null) {
+			myFilters = Set.of();
+		}
 		return myFilters;
 	}
 

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/interceptor/auth/RuleBulkExportImpl.java

@@ -32,6 +32,8 @@ import java.util.Collection;
 import java.util.Objects;
 import java.util.Set;
 
+import static org.apache.commons.collections4.CollectionUtils.isEmpty;
+import static org.apache.commons.collections4.CollectionUtils.isNotEmpty;
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 public class RuleBulkExportImpl extends BaseRule {
@@ -60,8 +62,8 @@ public class RuleBulkExportImpl extends BaseRule {
 			return null;
 		}
 
-		if (myResourceTypes != null && !myResourceTypes.isEmpty()) {
+		if (isNotEmpty(myResourceTypes)) {
-			if (options.getResourceTypes() == null) {
+			if (isEmpty(options.getResourceTypes())) {
 				return null;
 			}
 			for (String next : options.getResourceTypes()) {

hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/export/BulkExportJobParametersValidator.java

@@ -23,6 +23,7 @@ package ca.uhn.fhir.batch2.jobs.export;
 import ca.uhn.fhir.batch2.api.IJobParametersValidator;
 import ca.uhn.fhir.batch2.jobs.export.models.BulkExportJobParameters;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
+import ca.uhn.fhir.jpa.bulk.export.provider.BulkDataExportProvider;
 import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.server.bulk.BulkDataExportOptions;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -31,11 +32,12 @@ import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Set;
 
 public class BulkExportJobParametersValidator implements IJobParametersValidator<BulkExportJobParameters> {
 
-	public static final String UNSUPPORTED_BINARY_TYPE = "Binary";
+	/** @deprecated use BulkDataExportProvider.UNSUPPORTED_BINARY_TYPE instead */
+	@Deprecated(since="6.3.10")
+	public static final String UNSUPPORTED_BINARY_TYPE = BulkDataExportProvider.UNSUPPORTED_BINARY_TYPE;
 	@Autowired
 	private DaoRegistry myDaoRegistry;
 

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/api/config/DaoConfig.java

@@ -278,7 +278,6 @@ public class DaoConfig {
 	 * queried within Hibernate Search.
 	 *
 	 * @since 5.6.0
-	 * TODO mb test more with this true
 	 */
 	private boolean myAdvancedHSearchIndexing = false;
 	/**

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/bulk/export/provider/BulkDataExportProvider.java

@@ -78,10 +78,11 @@ import java.util.Date;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.NoSuchElementException;
 import java.util.Set;
 import java.util.stream.Collectors;
 
+import static org.apache.commons.collections4.CollectionUtils.isEmpty;
+import static org.apache.commons.collections4.CollectionUtils.isNotEmpty;
 import static org.apache.commons.lang3.StringUtils.isEmpty;
 import static org.slf4j.LoggerFactory.getLogger;
 
@@ -89,6 +90,8 @@ import static org.slf4j.LoggerFactory.getLogger;
 public class BulkDataExportProvider {
 	public static final String FARM_TO_TABLE_TYPE_FILTER_REGEX = "(?:,)(?=[A-Z][a-z]+\\?)";
 	public static final List<String> PATIENT_BULK_EXPORT_FORWARD_REFERENCE_RESOURCE_TYPES = List.of("Practitioner", "Organization");
+	/** Bulk data $export does not include the Binary type */
+	public static final String UNSUPPORTED_BINARY_TYPE = "Binary";
 	private static final Logger ourLog = getLogger(BulkDataExportProvider.class);
 
 	@Autowired
@@ -118,7 +121,7 @@ public class BulkDataExportProvider {
 		@OperationParam(name = JpaConstants.PARAM_EXPORT_SINCE, min = 0, max = 1, typeName = "instant") IPrimitiveType<Date> theSince,
 		@OperationParam(name = JpaConstants.PARAM_EXPORT_TYPE_FILTER, min = 0, max = OperationParam.MAX_UNLIMITED, typeName = "string") List<IPrimitiveType<String>> theTypeFilter,
 		ServletRequestDetails theRequestDetails
-	) throws Exception {
+	) {
 		// JPA export provider
 		validatePreferAsyncHeader(theRequestDetails, JpaConstants.OPERATION_EXPORT);
 
@@ -145,9 +148,9 @@ public class BulkDataExportProvider {
 		parameters.setOriginalRequestUrl(theRequestDetails.getCompleteUrl());
 
 		// If no _type parameter is provided, default to all resource types except Binary
-		if (theOptions.getResourceTypes() == null || theOptions.getResourceTypes().isEmpty()) {
+		if (isEmpty(theOptions.getResourceTypes())) {
 			List<String> resourceTypes = new ArrayList<>(myDaoRegistry.getRegisteredDaoTypes());
-			resourceTypes.remove("Binary");
+			resourceTypes.remove(UNSUPPORTED_BINARY_TYPE);
 			parameters.setResourceTypes(resourceTypes);
 		}
 
@@ -204,18 +207,18 @@ public class BulkDataExportProvider {
 		@OperationParam(name = JpaConstants.PARAM_EXPORT_TYPE_FILTER, min = 0, max = OperationParam.MAX_UNLIMITED, typeName = "string") List<IPrimitiveType<String>> theTypeFilter,
 		@OperationParam(name = JpaConstants.PARAM_EXPORT_MDM, min = 0, max = 1, typeName = "boolean") IPrimitiveType<Boolean> theMdm,
 		ServletRequestDetails theRequestDetails
-	) throws Exception {
+	) {
 		ourLog.debug("Received Group Bulk Export Request for Group {}", theIdParam);
 		ourLog.debug("_type={}", theIdParam);
 		ourLog.debug("_since={}", theSince);
 		ourLog.debug("_typeFilter={}", theTypeFilter);
-		ourLog.debug("_mdm=", theMdm);
+		ourLog.debug("_mdm={}", theMdm);
 
 		validatePreferAsyncHeader(theRequestDetails, JpaConstants.OPERATION_EXPORT);
 
 		BulkDataExportOptions bulkDataExportOptions = buildGroupBulkExportOptions(theOutputFormat, theType, theSince, theTypeFilter, theIdParam, theMdm);
 
-		if (bulkDataExportOptions.getResourceTypes() != null && !bulkDataExportOptions.getResourceTypes().isEmpty()) {
+		if (isNotEmpty(bulkDataExportOptions.getResourceTypes())) {
 			validateResourceTypesAllContainPatientSearchParams(bulkDataExportOptions.getResourceTypes());
 		} else {
 			// all patient resource types
@@ -257,7 +260,7 @@ public class BulkDataExportProvider {
 		@OperationParam(name = JpaConstants.PARAM_EXPORT_TYPE_FILTER, min = 0, max = OperationParam.MAX_UNLIMITED, typeName = "string") List<IPrimitiveType<String>> theTypeFilter,
 		@OperationParam(name = JpaConstants.PARAM_EXPORT_PATIENT, min = 0, max = OperationParam.MAX_UNLIMITED, typeName = "string") List<IPrimitiveType<String>> thePatient,
 		ServletRequestDetails theRequestDetails
-	) throws Exception {
+	) {
 		validatePreferAsyncHeader(theRequestDetails, JpaConstants.OPERATION_EXPORT);
 		BulkDataExportOptions bulkDataExportOptions = buildPatientBulkExportOptions(theOutputFormat, theType, theSince, theTypeFilter, thePatient);
 		validateResourceTypesAllContainPatientSearchParams(bulkDataExportOptions.getResourceTypes());
@@ -276,7 +279,7 @@ public class BulkDataExportProvider {
 		@OperationParam(name = JpaConstants.PARAM_EXPORT_SINCE, min = 0, max = 1, typeName = "instant") IPrimitiveType<Date> theSince,
 		@OperationParam(name = JpaConstants.PARAM_EXPORT_TYPE_FILTER, min = 0, max = OperationParam.MAX_UNLIMITED, typeName = "string") List<IPrimitiveType<String>> theTypeFilter,
 		ServletRequestDetails theRequestDetails
-	) throws Exception {
+	) {
 		validatePreferAsyncHeader(theRequestDetails, JpaConstants.OPERATION_EXPORT);
 		BulkDataExportOptions bulkDataExportOptions = buildPatientBulkExportOptions(theOutputFormat, theType, theSince, theTypeFilter, theIdParam);
 		validateResourceTypesAllContainPatientSearchParams(bulkDataExportOptions.getResourceTypes());

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/util/BulkExportUtils.java

@@ -23,6 +23,7 @@ package ca.uhn.fhir.jpa.util;
 import ca.uhn.fhir.jpa.api.model.BulkExportParameters;
 import ca.uhn.fhir.rest.api.server.bulk.BulkDataExportOptions;
 import ca.uhn.fhir.util.Batch2JobDefinitionConstants;
+import org.apache.commons.collections4.CollectionUtils;
 import org.hl7.fhir.instance.model.api.IIdType;
 
 import java.util.ArrayList;
@@ -40,16 +41,16 @@ public class BulkExportUtils {
 		parameters.setStartDate(theOptions.getSince());
 		parameters.setOutputFormat(theOptions.getOutputFormat());
 		parameters.setExportStyle(theOptions.getExportStyle());
-		if (theOptions.getFilters() != null) {
+		if (CollectionUtils.isNotEmpty(theOptions.getFilters())) {
 			parameters.setFilters(new ArrayList<>(theOptions.getFilters()));
 		}
 		if (theOptions.getGroupId() != null) {
 			parameters.setGroupId(theOptions.getGroupId().getValue());
 		}
-		if (theOptions.getResourceTypes() != null) {
+		if (CollectionUtils.isNotEmpty(theOptions.getResourceTypes())) {
 			parameters.setResourceTypes(new ArrayList<>(theOptions.getResourceTypes()));
 		}
-		if (theOptions.getPatientIds() != null) {
+		if (CollectionUtils.isNotEmpty(theOptions.getPatientIds())) {
 			parameters.setPatientIds(theOptions.getPatientIds().stream().map(IIdType::getValue).collect(Collectors.toList()));
 		}
 		parameters.setExpandMdm(theOptions.isExpandMdm());

hapi-fhir-test-utilities/src/main/java/ca/uhn/fhir/test/utilities/ITestDataBuilder.java

@@ -359,7 +359,6 @@ public interface ITestDataBuilder {
 		return withReference("managingOrganization", theHasMember);
 	}
 
-	// todo mb extract these to something like TestDataBuilderBacking.  Maybe split out create* into child interface since people skip it.
 	/**
 	 * Users of this API must implement this method
 	 */
@@ -393,7 +392,6 @@ public interface ITestDataBuilder {
 		IIdType doUpdateResource(IBaseResource theResource);
 	}
 
-	// todo mb make this the norm.
 	interface WithSupport extends ITestDataBuilder {
 		Support getTestDataBuilderSupport();
 
@@ -416,7 +414,6 @@ public interface ITestDataBuilder {
 
 	/**
 	 * Dummy support to use ITestDataBuilder as just a builder, not a DAO
-	 * todo mb Maybe we should split out the builder into a super-interface and drop this?
 	 */
 	class SupportNoDao implements Support {
 		final FhirContext myFhirContext;