From 02303d01c4cfff629a51b234b3969382f98f77e9 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Wed, 9 Apr 2025 12:26:07 -0700 Subject: [PATCH 01/39] Issue 52098: Switch lookupByAlternateKey to enum with three choices --- .../api/dataiterator/DataIteratorContext.java | 50 +++++++++++++- .../api/dataiterator/SimpleTranslator.java | 66 ++++++++++++++----- .../labkey/api/exp/list/ListDefinition.java | 5 +- .../api/query/AbstractQueryImportAction.java | 27 ++++---- .../api/query/QueryImportPipelineJob.java | 14 +++- .../labkey/experiment/api/LineageTest.java | 1 + .../controllers/exp/ExperimentController.java | 2 +- .../samples/AbstractExpFolderImporter.java | 2 +- .../list/controllers/ListController.java | 2 +- .../labkey/list/model/ListDefinitionImpl.java | 11 ++-- .../org/labkey/list/model/ListImporter.java | 2 +- .../list/model/ListQueryUpdateService.java | 4 +- .../labkey/query/QueryServiceImplTestCase.jsp | 3 +- .../study/assay/StudyPublishManager.java | 5 +- .../study/controllers/StudyController.java | 2 +- .../study/model/DatasetImportTestCase.jsp | 2 +- .../org/labkey/study/model/StudyManager.java | 4 +- 17 files changed, 151 insertions(+), 51 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java index 3a3a71e9d14..664f5486b9e 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java @@ -37,6 +37,39 @@ */ public class DataIteratorContext { + public enum LookupResolutionType + { + primaryKey(1, -1), // known that the use will always supply the pk value + alternateKey(-1, 1), // known that the use will never supply the pk value + alternateThenPrimaryKey(2, 1), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first + // TODO do we need to support this? If not, we can change the properties here to just have a "supportsAlternateKey" since alternate will always be checked first. + primaryThenAlternateKey(1, 2); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (added for compatibility; prefer the other options) + + final int _primaryResolutionOrder; + final int _alternateResolutionOrder; + + LookupResolutionType(int primaryResolutionOrder, int alternateResolutionOrder) + { + _primaryResolutionOrder = primaryResolutionOrder; + _alternateResolutionOrder = alternateResolutionOrder; + } + + public int getPrimaryResolutionOrder() + { + return _primaryResolutionOrder; + } + + public int getAlternateResolutionOrder() + { + return _alternateResolutionOrder; + } + + public boolean usesAlternateKey() + { + return _alternateResolutionOrder != -1; + } + } + /* NOTE: DIC is not really meant to be a set up parameter block targetOption and selectIds should probably be moved out in a future @@ -50,7 +83,7 @@ public class DataIteratorContext boolean _failFast = true; boolean _verbose = false; boolean _supportAutoIncrementKey = false; - boolean _allowImportLookupByAlternateKey = false; + LookupResolutionType _lookupResolutionType = LookupResolutionType.primaryKey; QueryImportPipelineJob _backgroundJob = null; boolean _crossTypeImport = false; boolean _crossFolderImport = false; @@ -162,15 +195,26 @@ public void setSupportAutoIncrementKey(boolean supportAutoIncrementKey) _supportAutoIncrementKey = supportAutoIncrementKey; } + @NotNull + public LookupResolutionType getLookupResolutionType() + { + return _lookupResolutionType == null ? LookupResolutionType.primaryKey : _lookupResolutionType; + } + + public void setLookupResolutionType(LookupResolutionType lookupResolutionType) + { + _lookupResolutionType = lookupResolutionType; + } + public boolean isAllowImportLookupByAlternateKey() { - return _allowImportLookupByAlternateKey; + return getLookupResolutionType() == LookupResolutionType.alternateThenPrimaryKey; } /** When true, allow importing lookup columns by the lookup table's alternate key instead of by primary key. */ public void setAllowImportLookupByAlternateKey(boolean allowImportLookupByAlternateKey) { - _allowImportLookupByAlternateKey = allowImportLookupByAlternateKey; + _lookupResolutionType = allowImportLookupByAlternateKey ? LookupResolutionType.alternateThenPrimaryKey : LookupResolutionType.primaryKey; } public boolean isCrossTypeImport() diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 249eef58228..cc44b5e3048 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -949,10 +949,11 @@ protected class RemapPostConvertColumn extends SimpleConvertColumn final ColumnInfo _toCol; final RemapMissingBehavior _missing; final boolean _includeTitleColumn; + final DataIteratorContext.LookupResolutionType _lookupResolutionType; final private RemapPostConvert _remapper; - public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, final int fromIndex, final @NotNull ColumnInfo toCol, RemapMissingBehavior missing, boolean includeTitleColumn) + public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, final int fromIndex, final @NotNull ColumnInfo toCol, RemapMissingBehavior missing, boolean includeTitleColumn, @NotNull DataIteratorContext.LookupResolutionType lookupResolutionType) { super(convertCol.fieldName, convertCol.index, convertCol.type); _convertCol = convertCol; @@ -960,32 +961,64 @@ public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, fin _missing = missing; _includeTitleColumn = includeTitleColumn; _remapper = new RemapPostConvert(_toCol.getFkTableInfo(), _includeTitleColumn, _missing, false, true); + _lookupResolutionType = lookupResolutionType; } - @Override - protected Object convert(Object o) + protected Object convertWithPrimaryColumn(Object o, int orderNum) { - try + if (_lookupResolutionType.getPrimaryResolutionOrder() >= orderNum) { - Object value = _convertCol.convert(o); - ForeignKey fk = _toCol.getFk(); - // issue 40909 : allow String columns to resolve lookups by alternate key if the raw lookup fails to resolve - if (fk != null && Objects.equals(o, value) && _toCol.getJdbcType().isText()) + try { - if (_remapper.getPkColumn().getJdbcType().isText()) + // _convertCol here will be the column for the primary key type (unless type inference has gone wrong?) + Object value = _convertCol.convert(o); + ForeignKey fk = _toCol.getFk(); + // issue 40909 : allow String columns to resolve lookups by alternate key if the raw lookup fails to resolve + if (fk != null && Objects.equals(o, value) && _toCol.getJdbcType().isText()) { - Object remappedValue = _remapper.mappedValue(o); - value = remappedValue != null ? remappedValue : value; + if (_remapper.getPkColumn().getJdbcType().isText()) + { + Object remappedValue = _remapper.mappedValue(o); + value = remappedValue != null ? remappedValue : value; + } } + return value; + } + catch (ConversionException ex) + { + return null; } - return value; } - catch (ConversionException ex) + return null; + } + + protected Object convertWithRemapper(Object o, int orderNum) + { + if (_lookupResolutionType.getAlternateResolutionOrder() >= orderNum) { // don't want to attempt to resolve by target table PK because we already know there is a type mismatch _remapper.setIncludePkLookup(false); return _remapper.mappedValue(o); } + return null; + } + + @Override + protected Object convert(Object o) + { + if (o == null) + return null; + + int convertNum = 1; + Object value = convertWithPrimaryColumn(o, convertNum); + if (value != null) + return value; + convertNum++; + value = convertWithRemapper(o, convertNum); + if (value != null) + return value; + convertNum++; + return convertWithPrimaryColumn(o, convertNum); } } @@ -1334,7 +1367,8 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro c = new PropertyConvertColumn(name, fromIndex, mvIndex, mv, pt, type); ForeignKey fk = col.getFk(); - if (fk != null && _context.isAllowImportLookupByAlternateKey() && fk.allowImportByAlternateKey()) + DataIteratorContext.LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); + if (fk != null && lookupResolutionType.usesAlternateKey() && fk.allowImportByAlternateKey()) { // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); @@ -1342,7 +1376,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro RemapMissingBehavior missing = remapMissingBehavior; if (missing == null) missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; - c = new RemapPostConvertColumn(c, fromIndex, col, missing, true); + c = new RemapPostConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); } boolean multiValue = fk instanceof MultiValuedForeignKey; @@ -2204,7 +2238,7 @@ public void convertRemapTest() throws Exception // don't throw error if remap can't be resolved { DataIteratorContext context = new DataIteratorContext(); - context.setAllowImportLookupByAlternateKey(true); + context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue); diff --git a/api/src/org/labkey/api/exp/list/ListDefinition.java b/api/src/org/labkey/api/exp/list/ListDefinition.java index 40e5a70b432..0185bcf2e6a 100644 --- a/api/src/org/labkey/api/exp/list/ListDefinition.java +++ b/api/src/org/labkey/api/exp/list/ListDefinition.java @@ -22,6 +22,7 @@ import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.TableInfo; +import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.exp.DomainNotFoundException; import org.labkey.api.exp.PropertyType; import org.labkey.api.exp.property.Domain; @@ -299,8 +300,8 @@ public static BodySetting getForValue(int value) ListItem getListItemForEntityId(String entityId, User user); int insertListItems(User user, Container container, List listItems) throws IOException; - int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, boolean importByAlternateKey) throws IOException; - int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, boolean importByAlternateKey, QueryUpdateService.InsertOption insertOption) throws IOException; + int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType) throws IOException; + int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType, QueryUpdateService.InsertOption insertOption) throws IOException; @Nullable TableInfo getTable(User user); @Nullable TableInfo getTable(User user, Container c); diff --git a/api/src/org/labkey/api/query/AbstractQueryImportAction.java b/api/src/org/labkey/api/query/AbstractQueryImportAction.java index b4b122aadb8..66892055a0f 100644 --- a/api/src/org/labkey/api/query/AbstractQueryImportAction.java +++ b/api/src/org/labkey/api/query/AbstractQueryImportAction.java @@ -304,13 +304,13 @@ public enum Params saveToPipeline, useAsync, importIdentity, - importLookupByAlternateKey, format, insertOption, crossTypeImport, allowCreateStorage, crossFolderImport, - useTransactionAuditCache + useTransactionAuditCache, + lookupResolutionType, } @Nullable @@ -325,7 +325,6 @@ protected Map getOptionParamsMap() { _optionParamsMap = new HashMap<>(); _optionParamsMap.put(Params.importIdentity, Boolean.valueOf(getParam(Params.importIdentity))); - _optionParamsMap.put(Params.importLookupByAlternateKey, Boolean.valueOf(getParam(Params.importLookupByAlternateKey))); _optionParamsMap.put(Params.crossTypeImport, Boolean.valueOf(getParam(Params.crossTypeImport))); _optionParamsMap.put(Params.allowCreateStorage, Boolean.valueOf(getParam(Params.allowCreateStorage))); _optionParamsMap.put(Params.crossFolderImport, Boolean.valueOf(getParam(Params.crossFolderImport))); @@ -339,6 +338,11 @@ protected boolean getOptionParamValue(Params p) return getOptionParamsMap().getOrDefault(p, false); } + protected DataIteratorContext.LookupResolutionType getLookupResolutionType() + { + return DataIteratorContext.LookupResolutionType.valueOf(getParam(Params.lookupResolutionType)); + } + protected boolean skipInsertOptionValidation() { return false; @@ -575,6 +579,7 @@ else if (!dataFileDir.exists()) .setAuditBehaviorType(behaviorType) .setAuditUserComment(_auditUserComment) .setOptionParamsMap(getOptionParamsMap()) + .setLookupResolutionType(getLookupResolutionType()) .setAllowLineageColumns(allowLineageColumns()) .setJobDescription(getQueryImportDescription()) .setJobNotificationProvider(getQueryImportJobNotificationProviderName()); @@ -789,17 +794,16 @@ protected ActionURL getSuccessURL(FORM form) /* TODO change prototype to take DataIteratorBuilder, and DataIteratorContext */ protected int importData(DataLoader dl, FileStream file, String originalName, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, TransactionAuditProvider.@Nullable TransactionAuditEvent auditEvent, @Nullable String auditUserComment) throws IOException { - return importData(dl, _target, _updateService, _insertOption, getOptionParamsMap(), errors, auditBehaviorType, auditEvent, auditUserComment, getUser(), getContainer(), null); + return importData(dl, _target, _updateService, _insertOption, getOptionParamsMap(), getLookupResolutionType(), errors, auditBehaviorType, auditEvent, auditUserComment, getUser(), getContainer(), null); } - public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container) + public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, DataIteratorContext.LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container) { - return createDataIteratorContext(insertOption, optionParamsMap, auditBehaviorType, auditUserComment, errors, logger, container, null); + return createDataIteratorContext(insertOption, optionParamsMap, lookupResolutionType, auditBehaviorType, auditUserComment, errors, logger, container, null); } - public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container, QueryImportPipelineJob importJob) + public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, DataIteratorContext.LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container, QueryImportPipelineJob importJob) { - boolean importLookupByAlternateKey = optionParamsMap.getOrDefault(AbstractQueryImportAction.Params.importLookupByAlternateKey, false); boolean importIdentity = optionParamsMap.getOrDefault(AbstractQueryImportAction.Params.importIdentity, false); boolean crossTypeImport = optionParamsMap.getOrDefault(AbstractQueryImportAction.Params.crossTypeImport, false); boolean allowCreateStorage = optionParamsMap.getOrDefault(AbstractQueryImportAction.Params.allowCreateStorage, false); @@ -809,7 +813,8 @@ public static DataIteratorContext createDataIteratorContext(QueryUpdateService.I DataIteratorContext context = new DataIteratorContext(errors); context.setBackgroundJob(importJob); context.setInsertOption(insertOption); - context.setAllowImportLookupByAlternateKey(importLookupByAlternateKey); + if (lookupResolutionType != null) + context.setLookupResolutionType(lookupResolutionType); if (auditBehaviorType != null) { context.putConfigParameter(DetailedAuditLogDataIterator.AuditConfigs.AuditBehavior, auditBehaviorType); @@ -831,9 +836,9 @@ public static DataIteratorContext createDataIteratorContext(QueryUpdateService.I return context; } - public static int importData(DataLoader dl, TableInfo target, QueryUpdateService updateService, QueryUpdateService.InsertOption insertOption, Map optionParamsMap, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, TransactionAuditProvider.@Nullable TransactionAuditEvent auditEvent, @Nullable String auditUserComment, User user, Container container, @Nullable Logger logger) throws IOException + public static int importData(DataLoader dl, TableInfo target, QueryUpdateService updateService, QueryUpdateService.InsertOption insertOption, Map optionParamsMap, DataIteratorContext.LookupResolutionType lookupResolutionType, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, TransactionAuditProvider.@Nullable TransactionAuditEvent auditEvent, @Nullable String auditUserComment, User user, Container container, @Nullable Logger logger) throws IOException { - return importData(dl, target, updateService, createDataIteratorContext(insertOption, optionParamsMap, auditBehaviorType, auditUserComment, errors, logger, container), auditEvent, user, container); + return importData(dl, target, updateService, createDataIteratorContext(insertOption, optionParamsMap, lookupResolutionType, auditBehaviorType, auditUserComment, errors, logger, container), auditEvent, user, container); } public static int importData(DataLoader dl, TableInfo target, QueryUpdateService updateService, @NotNull DataIteratorContext context, TransactionAuditProvider.@Nullable TransactionAuditEvent auditEvent, User user, Container container) throws IOException diff --git a/api/src/org/labkey/api/query/QueryImportPipelineJob.java b/api/src/org/labkey/api/query/QueryImportPipelineJob.java index ae26ecb1ef3..6cc2194f516 100644 --- a/api/src/org/labkey/api/query/QueryImportPipelineJob.java +++ b/api/src/org/labkey/api/query/QueryImportPipelineJob.java @@ -60,6 +60,7 @@ public static class QueryImportAsyncContextBuilder String _auditUserComment = null; boolean _allowLineageColumns = false; Map _optionParamsMap = new HashMap<>(); + DataIteratorContext.LookupResolutionType _lookupResolutionType = null; String _jobDescription; @@ -208,6 +209,17 @@ public QueryImportAsyncContextBuilder setOptionParamsMap(Map getLineageImportAliases() throws IOException protected void initContext(DataLoader dl, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment) throws IOException { - _context = createDataIteratorContext(_insertOption, getOptionParamsMap(), auditBehaviorType, auditUserComment, errors, null, getContainer()); + _context = createDataIteratorContext(_insertOption, getOptionParamsMap(), getLookupResolutionType(), auditBehaviorType, auditUserComment, errors, null, getContainer()); if (_context.isCrossFolderImport() && !getContainer().hasProductFolders()) _context.setCrossFolderImport(false); diff --git a/experiment/src/org/labkey/experiment/samples/AbstractExpFolderImporter.java b/experiment/src/org/labkey/experiment/samples/AbstractExpFolderImporter.java index ba810338ff7..5d6d6d31e15 100644 --- a/experiment/src/org/labkey/experiment/samples/AbstractExpFolderImporter.java +++ b/experiment/src/org/labkey/experiment/samples/AbstractExpFolderImporter.java @@ -293,7 +293,7 @@ protected void importTsvData(FolderImportContext ctx, XarContext xarContext, Str DataIteratorContext context = new DataIteratorContext(errors); context.setInsertOption(QueryUpdateService.InsertOption.MERGE); context.putConfigParameter(QueryUpdateService.ConfigParameters.SkipInsertOptionValidation, Boolean.TRUE); // allow merge during folder import, needed for eval data loading - context.setAllowImportLookupByAlternateKey(true); + context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.alternateThenPrimaryKey); ((AbstractQueryUpdateService)qus).setAttachmentDirectory(dir.getDir(tableName)); Map options = new HashMap<>(); try diff --git a/list/src/org/labkey/list/controllers/ListController.java b/list/src/org/labkey/list/controllers/ListController.java index c65bee814fd..1f1df90c440 100644 --- a/list/src/org/labkey/list/controllers/ListController.java +++ b/list/src/org/labkey/list/controllers/ListController.java @@ -738,7 +738,7 @@ public ModelAndView getView(ListDefinitionForm form, BindException errors) throw @Override protected int importData(DataLoader dl, FileStream file, String originalName, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, @Nullable TransactionAuditProvider.TransactionAuditEvent auditEvent, @Nullable String auditUserComment) throws IOException { - return _list.importListItems(getUser(), getContainer(), dl, errors, null, null, false, getOptionParamValue(Params.importLookupByAlternateKey), _insertOption); + return _list.importListItems(getUser(), getContainer(), dl, errors, null, null, false, getLookupResolutionType(), _insertOption); } @Override diff --git a/list/src/org/labkey/list/model/ListDefinitionImpl.java b/list/src/org/labkey/list/model/ListDefinitionImpl.java index 29398ba717a..0b1c02adda2 100644 --- a/list/src/org/labkey/list/model/ListDefinitionImpl.java +++ b/list/src/org/labkey/list/model/ListDefinitionImpl.java @@ -29,6 +29,7 @@ import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; +import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.exp.DomainNotFoundException; import org.labkey.api.exp.Lsid; import org.labkey.api.exp.ObjectProperty; @@ -586,18 +587,18 @@ public int insertListItems(User user, Container container, List listIt MapLoader loader = new MapLoader(rows); // TODO: Find out the attachment directory? - return insertListItems(user, container, loader, ve, null, null, false, false); + return insertListItems(user, container, loader, ve, null, null, false, DataIteratorContext.LookupResolutionType.primaryKey); } @Override - public int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, boolean importByAlternateKey) + public int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType) { - return importListItems(user, container, loader, errors, attachmentDir, progress, supportAutoIncrementKey, importByAlternateKey, QueryUpdateService.InsertOption.INSERT); + return importListItems(user, container, loader, errors, attachmentDir, progress, supportAutoIncrementKey, lookupResolutionType, QueryUpdateService.InsertOption.INSERT); } @Override - public int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, boolean importByAlternateKey, QueryUpdateService.InsertOption insertOption) + public int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType, QueryUpdateService.InsertOption insertOption) { ListQuerySchema schema = new ListQuerySchema(user, container); TableInfo table = schema.getTable(_def.getName()); @@ -605,7 +606,7 @@ public int importListItems(User user, Container container, DataLoader loader, @N { ListQueryUpdateService lqus = (ListQueryUpdateService) table.getUpdateService(); if (null != lqus) - return lqus.insertUsingDataIterator(loader, user, container, errors, attachmentDir, progress, supportAutoIncrementKey, importByAlternateKey, insertOption); + return lqus.insertUsingDataIterator(loader, user, container, errors, attachmentDir, progress, supportAutoIncrementKey, insertOption, lookupResolutionType); } return 0; } diff --git a/list/src/org/labkey/list/model/ListImporter.java b/list/src/org/labkey/list/model/ListImporter.java index e68bd4d2d67..7766cd165f6 100644 --- a/list/src/org/labkey/list/model/ListImporter.java +++ b/list/src/org/labkey/list/model/ListImporter.java @@ -258,7 +258,7 @@ private boolean processSingle(VirtualFile sourceDir, ListDefinition def, String } } - def.importListItems(user, c, loader, batchErrors, sourceDir.getDir(FileUtil.makeLegalName(def.getName())), null, supportAI, false, _importContext.useMerge() ? QueryUpdateService.InsertOption.MERGE : QueryUpdateService.InsertOption.IMPORT); + def.importListItems(user, c, loader, batchErrors, sourceDir.getDir(FileUtil.makeLegalName(def.getName())), null, supportAI, null, _importContext.useMerge() ? QueryUpdateService.InsertOption.MERGE : QueryUpdateService.InsertOption.IMPORT); } for (ValidationException v : batchErrors.getRowErrors()) diff --git a/list/src/org/labkey/list/model/ListQueryUpdateService.java b/list/src/org/labkey/list/model/ListQueryUpdateService.java index d32f1f7a9a2..0bd1890acd0 100644 --- a/list/src/org/labkey/list/model/ListQueryUpdateService.java +++ b/list/src/org/labkey/list/model/ListQueryUpdateService.java @@ -205,7 +205,7 @@ private User getListUser(User user, Container container) } public int insertUsingDataIterator(DataLoader loader, User user, Container container, BatchValidationException errors, @Nullable VirtualFile attachmentDir, - @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, boolean importLookupsByAlternateKey, InsertOption insertOption) + @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, InsertOption insertOption, DataIteratorContext.LookupResolutionType lookupResolutionType) { if (!_list.isVisible(user)) throw new UnauthorizedException("You do not have permission to insert data into this table."); @@ -215,7 +215,7 @@ public int insertUsingDataIterator(DataLoader loader, User user, Container conta context.setFailFast(false); context.setInsertOption(insertOption); // this method is used by ListImporter and BackgroundListImporter context.setSupportAutoIncrementKey(supportAutoIncrementKey); - context.setAllowImportLookupByAlternateKey(importLookupsByAlternateKey); + context.setLookupResolutionType(lookupResolutionType); setAttachmentDirectory(attachmentDir); TableInfo ti = _list.getTable(updatedUser); diff --git a/query/src/org/labkey/query/QueryServiceImplTestCase.jsp b/query/src/org/labkey/query/QueryServiceImplTestCase.jsp index 54a728e7834..651639daa1e 100644 --- a/query/src/org/labkey/query/QueryServiceImplTestCase.jsp +++ b/query/src/org/labkey/query/QueryServiceImplTestCase.jsp @@ -29,6 +29,7 @@ <%@ page import="java.sql.SQLException" %> <%@ page import="java.util.Arrays" %> <%@ page import="java.util.List" %> +<%@ page import="org.labkey.api.dataiterator.DataIteratorContext" %> <%@ page extends="org.labkey.api.jsp.JspTest.DRT" %> <%! final static String tableName = "testGetSelectSqlSort"; @@ -43,7 +44,7 @@ ListDefinition list = s.getList(c, tableName); var data = new ReaderInputStream(new StringReader("A,B,C\n6,4,3\n1,8,6\n7,1,9\n2,5,1\n8,9,4\n3,2,7\n9,6,10\n4,10,2\n10,3,5\n5,7,8\n")); - list.insertListItems(user, c, new TabLoader.CsvFactory().createLoader(data,true), new BatchValidationException(), null, null, false, false); + list.insertListItems(user, c, new TabLoader.CsvFactory().createLoader(data,true), new BatchValidationException(), null, null, false, DataIteratorContext.LookupResolutionType.primaryKey); // wrap this list so we can mangle the sort properties UserSchema schema = (UserSchema)DefaultSchema.get(user,c,"lists"); diff --git a/study/src/org/labkey/study/assay/StudyPublishManager.java b/study/src/org/labkey/study/assay/StudyPublishManager.java index 9cd723bec63..b784f7a2ecd 100644 --- a/study/src/org/labkey/study/assay/StudyPublishManager.java +++ b/study/src/org/labkey/study/assay/StudyPublishManager.java @@ -58,6 +58,7 @@ import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; +import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.exp.ChangePropertyDescriptorException; import org.labkey.api.exp.ExperimentException; import org.labkey.api.exp.PropertyDescriptor; @@ -984,7 +985,7 @@ public UploadLog saveUploadData(User user, Dataset dsd, FileStream tsv, String f * Return an array of LSIDs from the newly created dataset entries, * along with the upload log. */ - public Pair, UploadLog> importDatasetTSV(User user, StudyImpl study, DatasetDefinition dsd, DataLoader dl, boolean importLookupByAlternateKey, FileStream fileIn, String originalFileName, Map columnMap, BatchValidationException errors, QueryUpdateService.InsertOption insertOption, @Nullable AuditBehaviorType auditBehaviorType) + public Pair, UploadLog> importDatasetTSV(User user, StudyImpl study, DatasetDefinition dsd, DataLoader dl, DataIteratorContext.LookupResolutionType lookupResolutionType, FileStream fileIn, String originalFileName, Map columnMap, BatchValidationException errors, QueryUpdateService.InsertOption insertOption, @Nullable AuditBehaviorType auditBehaviorType) { DbScope scope = StudySchema.getInstance().getScope(); @@ -1003,7 +1004,7 @@ public Pair, UploadLog> importDatasetTSV(User user, StudyImpl study if (defaultQCStateId != null) defaultQCState = DataStateManager.getInstance().getStateForRowId(study.getContainer(), defaultQCStateId.intValue()); lsids = StudyManager.getInstance().importDatasetData(user, dsd, dl, columnMap, errors, DatasetDefinition.CheckForDuplicates.sourceOnly, - defaultQCState, insertOption, null, importLookupByAlternateKey, auditBehaviorType); + defaultQCState, insertOption, null, lookupResolutionType, auditBehaviorType); if (!errors.hasErrors()) { diff --git a/study/src/org/labkey/study/controllers/StudyController.java b/study/src/org/labkey/study/controllers/StudyController.java index 640a8c5c3c2..fba1514e835 100644 --- a/study/src/org/labkey/study/controllers/StudyController.java +++ b/study/src/org/labkey/study/controllers/StudyController.java @@ -2687,7 +2687,7 @@ protected int importData(DataLoader dl, FileStream file, String originalName, Ba columnMap.put(_form.getSequenceNum(), column); } - Pair, UploadLog> result = StudyPublishManager.getInstance().importDatasetTSV(getUser(), _study, _def, dl, getOptionParamValue(Params.importLookupByAlternateKey), file, originalName, columnMap, errors, _form.getInsertOption(), auditBehaviorType); + Pair, UploadLog> result = StudyPublishManager.getInstance().importDatasetTSV(getUser(), _study, _def, dl, getLookupResolutionType(), file, originalName, columnMap, errors, _form.getInsertOption(), auditBehaviorType); if (!result.getKey().isEmpty()) { diff --git a/study/src/org/labkey/study/model/DatasetImportTestCase.jsp b/study/src/org/labkey/study/model/DatasetImportTestCase.jsp index 68d1b78438e..6801849ec80 100644 --- a/study/src/org/labkey/study/model/DatasetImportTestCase.jsp +++ b/study/src/org/labkey/study/model/DatasetImportTestCase.jsp @@ -683,7 +683,7 @@ private void importRows(Dataset def, List> rows, @Nullable L StudyManager.getInstance().importDatasetData( _context.getUser(), (DatasetDefinition) def, dl, columnMap, - errors, DatasetDefinition.CheckForDuplicates.sourceAndDestination, null, QueryUpdateService.InsertOption.IMPORT, logger, false, null); + errors, DatasetDefinition.CheckForDuplicates.sourceAndDestination, null, QueryUpdateService.InsertOption.IMPORT, logger, null, null); if (expectedErrors == null) { diff --git a/study/src/org/labkey/study/model/StudyManager.java b/study/src/org/labkey/study/model/StudyManager.java index 2f695c956cf..e6eb71644ee 100644 --- a/study/src/org/labkey/study/model/StudyManager.java +++ b/study/src/org/labkey/study/model/StudyManager.java @@ -3449,14 +3449,14 @@ public List importDatasetData(User user, DatasetDefinition def, @Nullable DataState defaultQCState, QueryUpdateService.InsertOption insertOption, Logger logger, - boolean importLookupByAlternateKey, + DataIteratorContext.LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType) throws IOException { DataIteratorContext context = new DataIteratorContext(errors); context.setInsertOption(insertOption); - context.setAllowImportLookupByAlternateKey(importLookupByAlternateKey); + context.setLookupResolutionType(lookupResolutionType); Map options = new HashMap<>(); options.put(DetailedAuditLogDataIterator.AuditConfigs.AuditBehavior, auditBehaviorType); From 8e172a1f9861e9e36df94794a1fa06d7a718ab35 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Wed, 9 Apr 2025 12:26:07 -0700 Subject: [PATCH 02/39] Issue 52098: Switch lookupByAlternateKey to enum with three choices --- .../api/dataiterator/DataIteratorContext.java | 50 +++++++++++++- .../api/dataiterator/SimpleTranslator.java | 66 ++++++++++++++----- .../labkey/api/exp/list/ListDefinition.java | 5 +- .../api/query/AbstractQueryImportAction.java | 27 ++++---- .../api/query/QueryImportPipelineJob.java | 14 +++- .../labkey/experiment/api/LineageTest.java | 1 + .../controllers/exp/ExperimentController.java | 2 +- .../samples/AbstractExpFolderImporter.java | 2 +- .../list/controllers/ListController.java | 2 +- .../labkey/list/model/ListDefinitionImpl.java | 11 ++-- .../org/labkey/list/model/ListImporter.java | 2 +- .../list/model/ListQueryUpdateService.java | 4 +- .../labkey/query/QueryServiceImplTestCase.jsp | 3 +- .../study/assay/StudyPublishManager.java | 5 +- .../study/controllers/StudyController.java | 2 +- .../study/model/DatasetImportTestCase.jsp | 2 +- .../org/labkey/study/model/StudyManager.java | 4 +- 17 files changed, 151 insertions(+), 51 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java index 3a3a71e9d14..664f5486b9e 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java @@ -37,6 +37,39 @@ */ public class DataIteratorContext { + public enum LookupResolutionType + { + primaryKey(1, -1), // known that the use will always supply the pk value + alternateKey(-1, 1), // known that the use will never supply the pk value + alternateThenPrimaryKey(2, 1), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first + // TODO do we need to support this? If not, we can change the properties here to just have a "supportsAlternateKey" since alternate will always be checked first. + primaryThenAlternateKey(1, 2); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (added for compatibility; prefer the other options) + + final int _primaryResolutionOrder; + final int _alternateResolutionOrder; + + LookupResolutionType(int primaryResolutionOrder, int alternateResolutionOrder) + { + _primaryResolutionOrder = primaryResolutionOrder; + _alternateResolutionOrder = alternateResolutionOrder; + } + + public int getPrimaryResolutionOrder() + { + return _primaryResolutionOrder; + } + + public int getAlternateResolutionOrder() + { + return _alternateResolutionOrder; + } + + public boolean usesAlternateKey() + { + return _alternateResolutionOrder != -1; + } + } + /* NOTE: DIC is not really meant to be a set up parameter block targetOption and selectIds should probably be moved out in a future @@ -50,7 +83,7 @@ public class DataIteratorContext boolean _failFast = true; boolean _verbose = false; boolean _supportAutoIncrementKey = false; - boolean _allowImportLookupByAlternateKey = false; + LookupResolutionType _lookupResolutionType = LookupResolutionType.primaryKey; QueryImportPipelineJob _backgroundJob = null; boolean _crossTypeImport = false; boolean _crossFolderImport = false; @@ -162,15 +195,26 @@ public void setSupportAutoIncrementKey(boolean supportAutoIncrementKey) _supportAutoIncrementKey = supportAutoIncrementKey; } + @NotNull + public LookupResolutionType getLookupResolutionType() + { + return _lookupResolutionType == null ? LookupResolutionType.primaryKey : _lookupResolutionType; + } + + public void setLookupResolutionType(LookupResolutionType lookupResolutionType) + { + _lookupResolutionType = lookupResolutionType; + } + public boolean isAllowImportLookupByAlternateKey() { - return _allowImportLookupByAlternateKey; + return getLookupResolutionType() == LookupResolutionType.alternateThenPrimaryKey; } /** When true, allow importing lookup columns by the lookup table's alternate key instead of by primary key. */ public void setAllowImportLookupByAlternateKey(boolean allowImportLookupByAlternateKey) { - _allowImportLookupByAlternateKey = allowImportLookupByAlternateKey; + _lookupResolutionType = allowImportLookupByAlternateKey ? LookupResolutionType.alternateThenPrimaryKey : LookupResolutionType.primaryKey; } public boolean isCrossTypeImport() diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 249eef58228..cc44b5e3048 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -949,10 +949,11 @@ protected class RemapPostConvertColumn extends SimpleConvertColumn final ColumnInfo _toCol; final RemapMissingBehavior _missing; final boolean _includeTitleColumn; + final DataIteratorContext.LookupResolutionType _lookupResolutionType; final private RemapPostConvert _remapper; - public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, final int fromIndex, final @NotNull ColumnInfo toCol, RemapMissingBehavior missing, boolean includeTitleColumn) + public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, final int fromIndex, final @NotNull ColumnInfo toCol, RemapMissingBehavior missing, boolean includeTitleColumn, @NotNull DataIteratorContext.LookupResolutionType lookupResolutionType) { super(convertCol.fieldName, convertCol.index, convertCol.type); _convertCol = convertCol; @@ -960,32 +961,64 @@ public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, fin _missing = missing; _includeTitleColumn = includeTitleColumn; _remapper = new RemapPostConvert(_toCol.getFkTableInfo(), _includeTitleColumn, _missing, false, true); + _lookupResolutionType = lookupResolutionType; } - @Override - protected Object convert(Object o) + protected Object convertWithPrimaryColumn(Object o, int orderNum) { - try + if (_lookupResolutionType.getPrimaryResolutionOrder() >= orderNum) { - Object value = _convertCol.convert(o); - ForeignKey fk = _toCol.getFk(); - // issue 40909 : allow String columns to resolve lookups by alternate key if the raw lookup fails to resolve - if (fk != null && Objects.equals(o, value) && _toCol.getJdbcType().isText()) + try { - if (_remapper.getPkColumn().getJdbcType().isText()) + // _convertCol here will be the column for the primary key type (unless type inference has gone wrong?) + Object value = _convertCol.convert(o); + ForeignKey fk = _toCol.getFk(); + // issue 40909 : allow String columns to resolve lookups by alternate key if the raw lookup fails to resolve + if (fk != null && Objects.equals(o, value) && _toCol.getJdbcType().isText()) { - Object remappedValue = _remapper.mappedValue(o); - value = remappedValue != null ? remappedValue : value; + if (_remapper.getPkColumn().getJdbcType().isText()) + { + Object remappedValue = _remapper.mappedValue(o); + value = remappedValue != null ? remappedValue : value; + } } + return value; + } + catch (ConversionException ex) + { + return null; } - return value; } - catch (ConversionException ex) + return null; + } + + protected Object convertWithRemapper(Object o, int orderNum) + { + if (_lookupResolutionType.getAlternateResolutionOrder() >= orderNum) { // don't want to attempt to resolve by target table PK because we already know there is a type mismatch _remapper.setIncludePkLookup(false); return _remapper.mappedValue(o); } + return null; + } + + @Override + protected Object convert(Object o) + { + if (o == null) + return null; + + int convertNum = 1; + Object value = convertWithPrimaryColumn(o, convertNum); + if (value != null) + return value; + convertNum++; + value = convertWithRemapper(o, convertNum); + if (value != null) + return value; + convertNum++; + return convertWithPrimaryColumn(o, convertNum); } } @@ -1334,7 +1367,8 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro c = new PropertyConvertColumn(name, fromIndex, mvIndex, mv, pt, type); ForeignKey fk = col.getFk(); - if (fk != null && _context.isAllowImportLookupByAlternateKey() && fk.allowImportByAlternateKey()) + DataIteratorContext.LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); + if (fk != null && lookupResolutionType.usesAlternateKey() && fk.allowImportByAlternateKey()) { // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); @@ -1342,7 +1376,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro RemapMissingBehavior missing = remapMissingBehavior; if (missing == null) missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; - c = new RemapPostConvertColumn(c, fromIndex, col, missing, true); + c = new RemapPostConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); } boolean multiValue = fk instanceof MultiValuedForeignKey; @@ -2204,7 +2238,7 @@ public void convertRemapTest() throws Exception // don't throw error if remap can't be resolved { DataIteratorContext context = new DataIteratorContext(); - context.setAllowImportLookupByAlternateKey(true); + context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue); diff --git a/api/src/org/labkey/api/exp/list/ListDefinition.java b/api/src/org/labkey/api/exp/list/ListDefinition.java index 40e5a70b432..0185bcf2e6a 100644 --- a/api/src/org/labkey/api/exp/list/ListDefinition.java +++ b/api/src/org/labkey/api/exp/list/ListDefinition.java @@ -22,6 +22,7 @@ import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.TableInfo; +import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.exp.DomainNotFoundException; import org.labkey.api.exp.PropertyType; import org.labkey.api.exp.property.Domain; @@ -299,8 +300,8 @@ public static BodySetting getForValue(int value) ListItem getListItemForEntityId(String entityId, User user); int insertListItems(User user, Container container, List listItems) throws IOException; - int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, boolean importByAlternateKey) throws IOException; - int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, boolean importByAlternateKey, QueryUpdateService.InsertOption insertOption) throws IOException; + int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType) throws IOException; + int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType, QueryUpdateService.InsertOption insertOption) throws IOException; @Nullable TableInfo getTable(User user); @Nullable TableInfo getTable(User user, Container c); diff --git a/api/src/org/labkey/api/query/AbstractQueryImportAction.java b/api/src/org/labkey/api/query/AbstractQueryImportAction.java index b4b122aadb8..66892055a0f 100644 --- a/api/src/org/labkey/api/query/AbstractQueryImportAction.java +++ b/api/src/org/labkey/api/query/AbstractQueryImportAction.java @@ -304,13 +304,13 @@ public enum Params saveToPipeline, useAsync, importIdentity, - importLookupByAlternateKey, format, insertOption, crossTypeImport, allowCreateStorage, crossFolderImport, - useTransactionAuditCache + useTransactionAuditCache, + lookupResolutionType, } @Nullable @@ -325,7 +325,6 @@ protected Map getOptionParamsMap() { _optionParamsMap = new HashMap<>(); _optionParamsMap.put(Params.importIdentity, Boolean.valueOf(getParam(Params.importIdentity))); - _optionParamsMap.put(Params.importLookupByAlternateKey, Boolean.valueOf(getParam(Params.importLookupByAlternateKey))); _optionParamsMap.put(Params.crossTypeImport, Boolean.valueOf(getParam(Params.crossTypeImport))); _optionParamsMap.put(Params.allowCreateStorage, Boolean.valueOf(getParam(Params.allowCreateStorage))); _optionParamsMap.put(Params.crossFolderImport, Boolean.valueOf(getParam(Params.crossFolderImport))); @@ -339,6 +338,11 @@ protected boolean getOptionParamValue(Params p) return getOptionParamsMap().getOrDefault(p, false); } + protected DataIteratorContext.LookupResolutionType getLookupResolutionType() + { + return DataIteratorContext.LookupResolutionType.valueOf(getParam(Params.lookupResolutionType)); + } + protected boolean skipInsertOptionValidation() { return false; @@ -575,6 +579,7 @@ else if (!dataFileDir.exists()) .setAuditBehaviorType(behaviorType) .setAuditUserComment(_auditUserComment) .setOptionParamsMap(getOptionParamsMap()) + .setLookupResolutionType(getLookupResolutionType()) .setAllowLineageColumns(allowLineageColumns()) .setJobDescription(getQueryImportDescription()) .setJobNotificationProvider(getQueryImportJobNotificationProviderName()); @@ -789,17 +794,16 @@ protected ActionURL getSuccessURL(FORM form) /* TODO change prototype to take DataIteratorBuilder, and DataIteratorContext */ protected int importData(DataLoader dl, FileStream file, String originalName, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, TransactionAuditProvider.@Nullable TransactionAuditEvent auditEvent, @Nullable String auditUserComment) throws IOException { - return importData(dl, _target, _updateService, _insertOption, getOptionParamsMap(), errors, auditBehaviorType, auditEvent, auditUserComment, getUser(), getContainer(), null); + return importData(dl, _target, _updateService, _insertOption, getOptionParamsMap(), getLookupResolutionType(), errors, auditBehaviorType, auditEvent, auditUserComment, getUser(), getContainer(), null); } - public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container) + public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, DataIteratorContext.LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container) { - return createDataIteratorContext(insertOption, optionParamsMap, auditBehaviorType, auditUserComment, errors, logger, container, null); + return createDataIteratorContext(insertOption, optionParamsMap, lookupResolutionType, auditBehaviorType, auditUserComment, errors, logger, container, null); } - public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container, QueryImportPipelineJob importJob) + public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, DataIteratorContext.LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container, QueryImportPipelineJob importJob) { - boolean importLookupByAlternateKey = optionParamsMap.getOrDefault(AbstractQueryImportAction.Params.importLookupByAlternateKey, false); boolean importIdentity = optionParamsMap.getOrDefault(AbstractQueryImportAction.Params.importIdentity, false); boolean crossTypeImport = optionParamsMap.getOrDefault(AbstractQueryImportAction.Params.crossTypeImport, false); boolean allowCreateStorage = optionParamsMap.getOrDefault(AbstractQueryImportAction.Params.allowCreateStorage, false); @@ -809,7 +813,8 @@ public static DataIteratorContext createDataIteratorContext(QueryUpdateService.I DataIteratorContext context = new DataIteratorContext(errors); context.setBackgroundJob(importJob); context.setInsertOption(insertOption); - context.setAllowImportLookupByAlternateKey(importLookupByAlternateKey); + if (lookupResolutionType != null) + context.setLookupResolutionType(lookupResolutionType); if (auditBehaviorType != null) { context.putConfigParameter(DetailedAuditLogDataIterator.AuditConfigs.AuditBehavior, auditBehaviorType); @@ -831,9 +836,9 @@ public static DataIteratorContext createDataIteratorContext(QueryUpdateService.I return context; } - public static int importData(DataLoader dl, TableInfo target, QueryUpdateService updateService, QueryUpdateService.InsertOption insertOption, Map optionParamsMap, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, TransactionAuditProvider.@Nullable TransactionAuditEvent auditEvent, @Nullable String auditUserComment, User user, Container container, @Nullable Logger logger) throws IOException + public static int importData(DataLoader dl, TableInfo target, QueryUpdateService updateService, QueryUpdateService.InsertOption insertOption, Map optionParamsMap, DataIteratorContext.LookupResolutionType lookupResolutionType, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, TransactionAuditProvider.@Nullable TransactionAuditEvent auditEvent, @Nullable String auditUserComment, User user, Container container, @Nullable Logger logger) throws IOException { - return importData(dl, target, updateService, createDataIteratorContext(insertOption, optionParamsMap, auditBehaviorType, auditUserComment, errors, logger, container), auditEvent, user, container); + return importData(dl, target, updateService, createDataIteratorContext(insertOption, optionParamsMap, lookupResolutionType, auditBehaviorType, auditUserComment, errors, logger, container), auditEvent, user, container); } public static int importData(DataLoader dl, TableInfo target, QueryUpdateService updateService, @NotNull DataIteratorContext context, TransactionAuditProvider.@Nullable TransactionAuditEvent auditEvent, User user, Container container) throws IOException diff --git a/api/src/org/labkey/api/query/QueryImportPipelineJob.java b/api/src/org/labkey/api/query/QueryImportPipelineJob.java index ae26ecb1ef3..6cc2194f516 100644 --- a/api/src/org/labkey/api/query/QueryImportPipelineJob.java +++ b/api/src/org/labkey/api/query/QueryImportPipelineJob.java @@ -60,6 +60,7 @@ public static class QueryImportAsyncContextBuilder String _auditUserComment = null; boolean _allowLineageColumns = false; Map _optionParamsMap = new HashMap<>(); + DataIteratorContext.LookupResolutionType _lookupResolutionType = null; String _jobDescription; @@ -208,6 +209,17 @@ public QueryImportAsyncContextBuilder setOptionParamsMap(Map getLineageImportAliases() throws IOException protected void initContext(DataLoader dl, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment) throws IOException { - _context = createDataIteratorContext(_insertOption, getOptionParamsMap(), auditBehaviorType, auditUserComment, errors, null, getContainer()); + _context = createDataIteratorContext(_insertOption, getOptionParamsMap(), getLookupResolutionType(), auditBehaviorType, auditUserComment, errors, null, getContainer()); if (_context.isCrossFolderImport() && !getContainer().hasProductFolders()) _context.setCrossFolderImport(false); diff --git a/experiment/src/org/labkey/experiment/samples/AbstractExpFolderImporter.java b/experiment/src/org/labkey/experiment/samples/AbstractExpFolderImporter.java index ba810338ff7..5d6d6d31e15 100644 --- a/experiment/src/org/labkey/experiment/samples/AbstractExpFolderImporter.java +++ b/experiment/src/org/labkey/experiment/samples/AbstractExpFolderImporter.java @@ -293,7 +293,7 @@ protected void importTsvData(FolderImportContext ctx, XarContext xarContext, Str DataIteratorContext context = new DataIteratorContext(errors); context.setInsertOption(QueryUpdateService.InsertOption.MERGE); context.putConfigParameter(QueryUpdateService.ConfigParameters.SkipInsertOptionValidation, Boolean.TRUE); // allow merge during folder import, needed for eval data loading - context.setAllowImportLookupByAlternateKey(true); + context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.alternateThenPrimaryKey); ((AbstractQueryUpdateService)qus).setAttachmentDirectory(dir.getDir(tableName)); Map options = new HashMap<>(); try diff --git a/list/src/org/labkey/list/controllers/ListController.java b/list/src/org/labkey/list/controllers/ListController.java index c65bee814fd..1f1df90c440 100644 --- a/list/src/org/labkey/list/controllers/ListController.java +++ b/list/src/org/labkey/list/controllers/ListController.java @@ -738,7 +738,7 @@ public ModelAndView getView(ListDefinitionForm form, BindException errors) throw @Override protected int importData(DataLoader dl, FileStream file, String originalName, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, @Nullable TransactionAuditProvider.TransactionAuditEvent auditEvent, @Nullable String auditUserComment) throws IOException { - return _list.importListItems(getUser(), getContainer(), dl, errors, null, null, false, getOptionParamValue(Params.importLookupByAlternateKey), _insertOption); + return _list.importListItems(getUser(), getContainer(), dl, errors, null, null, false, getLookupResolutionType(), _insertOption); } @Override diff --git a/list/src/org/labkey/list/model/ListDefinitionImpl.java b/list/src/org/labkey/list/model/ListDefinitionImpl.java index 29398ba717a..0b1c02adda2 100644 --- a/list/src/org/labkey/list/model/ListDefinitionImpl.java +++ b/list/src/org/labkey/list/model/ListDefinitionImpl.java @@ -29,6 +29,7 @@ import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; +import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.exp.DomainNotFoundException; import org.labkey.api.exp.Lsid; import org.labkey.api.exp.ObjectProperty; @@ -586,18 +587,18 @@ public int insertListItems(User user, Container container, List listIt MapLoader loader = new MapLoader(rows); // TODO: Find out the attachment directory? - return insertListItems(user, container, loader, ve, null, null, false, false); + return insertListItems(user, container, loader, ve, null, null, false, DataIteratorContext.LookupResolutionType.primaryKey); } @Override - public int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, boolean importByAlternateKey) + public int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType) { - return importListItems(user, container, loader, errors, attachmentDir, progress, supportAutoIncrementKey, importByAlternateKey, QueryUpdateService.InsertOption.INSERT); + return importListItems(user, container, loader, errors, attachmentDir, progress, supportAutoIncrementKey, lookupResolutionType, QueryUpdateService.InsertOption.INSERT); } @Override - public int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, boolean importByAlternateKey, QueryUpdateService.InsertOption insertOption) + public int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType, QueryUpdateService.InsertOption insertOption) { ListQuerySchema schema = new ListQuerySchema(user, container); TableInfo table = schema.getTable(_def.getName()); @@ -605,7 +606,7 @@ public int importListItems(User user, Container container, DataLoader loader, @N { ListQueryUpdateService lqus = (ListQueryUpdateService) table.getUpdateService(); if (null != lqus) - return lqus.insertUsingDataIterator(loader, user, container, errors, attachmentDir, progress, supportAutoIncrementKey, importByAlternateKey, insertOption); + return lqus.insertUsingDataIterator(loader, user, container, errors, attachmentDir, progress, supportAutoIncrementKey, insertOption, lookupResolutionType); } return 0; } diff --git a/list/src/org/labkey/list/model/ListImporter.java b/list/src/org/labkey/list/model/ListImporter.java index e68bd4d2d67..7766cd165f6 100644 --- a/list/src/org/labkey/list/model/ListImporter.java +++ b/list/src/org/labkey/list/model/ListImporter.java @@ -258,7 +258,7 @@ private boolean processSingle(VirtualFile sourceDir, ListDefinition def, String } } - def.importListItems(user, c, loader, batchErrors, sourceDir.getDir(FileUtil.makeLegalName(def.getName())), null, supportAI, false, _importContext.useMerge() ? QueryUpdateService.InsertOption.MERGE : QueryUpdateService.InsertOption.IMPORT); + def.importListItems(user, c, loader, batchErrors, sourceDir.getDir(FileUtil.makeLegalName(def.getName())), null, supportAI, null, _importContext.useMerge() ? QueryUpdateService.InsertOption.MERGE : QueryUpdateService.InsertOption.IMPORT); } for (ValidationException v : batchErrors.getRowErrors()) diff --git a/list/src/org/labkey/list/model/ListQueryUpdateService.java b/list/src/org/labkey/list/model/ListQueryUpdateService.java index d32f1f7a9a2..0bd1890acd0 100644 --- a/list/src/org/labkey/list/model/ListQueryUpdateService.java +++ b/list/src/org/labkey/list/model/ListQueryUpdateService.java @@ -205,7 +205,7 @@ private User getListUser(User user, Container container) } public int insertUsingDataIterator(DataLoader loader, User user, Container container, BatchValidationException errors, @Nullable VirtualFile attachmentDir, - @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, boolean importLookupsByAlternateKey, InsertOption insertOption) + @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, InsertOption insertOption, DataIteratorContext.LookupResolutionType lookupResolutionType) { if (!_list.isVisible(user)) throw new UnauthorizedException("You do not have permission to insert data into this table."); @@ -215,7 +215,7 @@ public int insertUsingDataIterator(DataLoader loader, User user, Container conta context.setFailFast(false); context.setInsertOption(insertOption); // this method is used by ListImporter and BackgroundListImporter context.setSupportAutoIncrementKey(supportAutoIncrementKey); - context.setAllowImportLookupByAlternateKey(importLookupsByAlternateKey); + context.setLookupResolutionType(lookupResolutionType); setAttachmentDirectory(attachmentDir); TableInfo ti = _list.getTable(updatedUser); diff --git a/query/src/org/labkey/query/QueryServiceImplTestCase.jsp b/query/src/org/labkey/query/QueryServiceImplTestCase.jsp index 54a728e7834..651639daa1e 100644 --- a/query/src/org/labkey/query/QueryServiceImplTestCase.jsp +++ b/query/src/org/labkey/query/QueryServiceImplTestCase.jsp @@ -29,6 +29,7 @@ <%@ page import="java.sql.SQLException" %> <%@ page import="java.util.Arrays" %> <%@ page import="java.util.List" %> +<%@ page import="org.labkey.api.dataiterator.DataIteratorContext" %> <%@ page extends="org.labkey.api.jsp.JspTest.DRT" %> <%! final static String tableName = "testGetSelectSqlSort"; @@ -43,7 +44,7 @@ ListDefinition list = s.getList(c, tableName); var data = new ReaderInputStream(new StringReader("A,B,C\n6,4,3\n1,8,6\n7,1,9\n2,5,1\n8,9,4\n3,2,7\n9,6,10\n4,10,2\n10,3,5\n5,7,8\n")); - list.insertListItems(user, c, new TabLoader.CsvFactory().createLoader(data,true), new BatchValidationException(), null, null, false, false); + list.insertListItems(user, c, new TabLoader.CsvFactory().createLoader(data,true), new BatchValidationException(), null, null, false, DataIteratorContext.LookupResolutionType.primaryKey); // wrap this list so we can mangle the sort properties UserSchema schema = (UserSchema)DefaultSchema.get(user,c,"lists"); diff --git a/study/src/org/labkey/study/assay/StudyPublishManager.java b/study/src/org/labkey/study/assay/StudyPublishManager.java index 9cd723bec63..b784f7a2ecd 100644 --- a/study/src/org/labkey/study/assay/StudyPublishManager.java +++ b/study/src/org/labkey/study/assay/StudyPublishManager.java @@ -58,6 +58,7 @@ import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; +import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.exp.ChangePropertyDescriptorException; import org.labkey.api.exp.ExperimentException; import org.labkey.api.exp.PropertyDescriptor; @@ -984,7 +985,7 @@ public UploadLog saveUploadData(User user, Dataset dsd, FileStream tsv, String f * Return an array of LSIDs from the newly created dataset entries, * along with the upload log. */ - public Pair, UploadLog> importDatasetTSV(User user, StudyImpl study, DatasetDefinition dsd, DataLoader dl, boolean importLookupByAlternateKey, FileStream fileIn, String originalFileName, Map columnMap, BatchValidationException errors, QueryUpdateService.InsertOption insertOption, @Nullable AuditBehaviorType auditBehaviorType) + public Pair, UploadLog> importDatasetTSV(User user, StudyImpl study, DatasetDefinition dsd, DataLoader dl, DataIteratorContext.LookupResolutionType lookupResolutionType, FileStream fileIn, String originalFileName, Map columnMap, BatchValidationException errors, QueryUpdateService.InsertOption insertOption, @Nullable AuditBehaviorType auditBehaviorType) { DbScope scope = StudySchema.getInstance().getScope(); @@ -1003,7 +1004,7 @@ public Pair, UploadLog> importDatasetTSV(User user, StudyImpl study if (defaultQCStateId != null) defaultQCState = DataStateManager.getInstance().getStateForRowId(study.getContainer(), defaultQCStateId.intValue()); lsids = StudyManager.getInstance().importDatasetData(user, dsd, dl, columnMap, errors, DatasetDefinition.CheckForDuplicates.sourceOnly, - defaultQCState, insertOption, null, importLookupByAlternateKey, auditBehaviorType); + defaultQCState, insertOption, null, lookupResolutionType, auditBehaviorType); if (!errors.hasErrors()) { diff --git a/study/src/org/labkey/study/controllers/StudyController.java b/study/src/org/labkey/study/controllers/StudyController.java index 242d561b5a3..3e98f1b4c2d 100644 --- a/study/src/org/labkey/study/controllers/StudyController.java +++ b/study/src/org/labkey/study/controllers/StudyController.java @@ -2692,7 +2692,7 @@ protected int importData(DataLoader dl, FileStream file, String originalName, Ba columnMap.put(_form.getSequenceNum(), column); } - Pair, UploadLog> result = StudyPublishManager.getInstance().importDatasetTSV(getUser(), _study, _def, dl, getOptionParamValue(Params.importLookupByAlternateKey), file, originalName, columnMap, errors, _form.getInsertOption(), auditBehaviorType); + Pair, UploadLog> result = StudyPublishManager.getInstance().importDatasetTSV(getUser(), _study, _def, dl, getLookupResolutionType(), file, originalName, columnMap, errors, _form.getInsertOption(), auditBehaviorType); if (!result.getKey().isEmpty()) { diff --git a/study/src/org/labkey/study/model/DatasetImportTestCase.jsp b/study/src/org/labkey/study/model/DatasetImportTestCase.jsp index 68d1b78438e..6801849ec80 100644 --- a/study/src/org/labkey/study/model/DatasetImportTestCase.jsp +++ b/study/src/org/labkey/study/model/DatasetImportTestCase.jsp @@ -683,7 +683,7 @@ private void importRows(Dataset def, List> rows, @Nullable L StudyManager.getInstance().importDatasetData( _context.getUser(), (DatasetDefinition) def, dl, columnMap, - errors, DatasetDefinition.CheckForDuplicates.sourceAndDestination, null, QueryUpdateService.InsertOption.IMPORT, logger, false, null); + errors, DatasetDefinition.CheckForDuplicates.sourceAndDestination, null, QueryUpdateService.InsertOption.IMPORT, logger, null, null); if (expectedErrors == null) { diff --git a/study/src/org/labkey/study/model/StudyManager.java b/study/src/org/labkey/study/model/StudyManager.java index 91ee6bb01ae..69b0350d832 100644 --- a/study/src/org/labkey/study/model/StudyManager.java +++ b/study/src/org/labkey/study/model/StudyManager.java @@ -3443,14 +3443,14 @@ public List importDatasetData(User user, DatasetDefinition def, @Nullable DataState defaultQCState, QueryUpdateService.InsertOption insertOption, Logger logger, - boolean importLookupByAlternateKey, + DataIteratorContext.LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType) throws IOException { DataIteratorContext context = new DataIteratorContext(errors); context.setInsertOption(insertOption); - context.setAllowImportLookupByAlternateKey(importLookupByAlternateKey); + context.setLookupResolutionType(lookupResolutionType); Map options = new HashMap<>(); options.put(DetailedAuditLogDataIterator.AuditConfigs.AuditBehavior, auditBehaviorType); From 3f315e233eab62cbaef4a1488a8d1476dd388053 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 14 Apr 2025 06:56:30 -0700 Subject: [PATCH 03/39] Clear maps if we are to include the PK lookup so we'll reload to include the pk map --- .../api/dataiterator/SimpleTranslator.java | 62 +++++++++++++------ 1 file changed, 43 insertions(+), 19 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index cc44b5e3048..4cb716c1c4c 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -221,6 +221,8 @@ public RemapPostConvert(@NotNull TableInfo targetTable, boolean includeTitleColu public void setIncludePkLookup(boolean includePkLookup) { _includePkLookup = includePkLookup; + if (includePkLookup) + _maps = null; } public ColumnInfo getPkColumn() @@ -331,6 +333,7 @@ private Object fetch(Triple> triple, final ColumnInfo pkCol = triple.getLeft(); final ColumnInfo altKeyCol = triple.getMiddle(); final MultiValuedMap map = triple.getRight(); + boolean typeMismatch = false; // check if we've already fetched the key Collection vs; @@ -350,11 +353,20 @@ private Object fetch(Triple> triple, } // ArrayListValuedHashMap returns an empty collection if 'k' is not in the map. - if (bulkLoaded == null || bulkLoaded.isEmpty()) + if (bulkLoaded == null || bulkLoaded.isEmpty() ) { - TableSelector ts = createSelector(pkCol, altKeyCol, k); - ts.fillMultiValuedMap(map); - vs = map.get(k); + if (altKeyCol.getJdbcType().getJavaClass().isAssignableFrom(k.getClass())) + { + TableSelector ts = createSelector(pkCol, altKeyCol, k); + ts.fillMultiValuedMap(map); + vs = map.get(k); + } + else + { + typeMismatch = true; + vs = Collections.emptyList(); + } + } else { @@ -964,9 +976,9 @@ public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, fin _lookupResolutionType = lookupResolutionType; } - protected Object convertWithPrimaryColumn(Object o, int orderNum) + protected Object convertWithPrimaryColumn(Object o, boolean primaryTried) { - if (_lookupResolutionType.getPrimaryResolutionOrder() >= orderNum) + if (_lookupResolutionType.usePrimaryKey() && _lookupResolutionType.usePrimaryFirst() != primaryTried) { try { @@ -978,6 +990,7 @@ protected Object convertWithPrimaryColumn(Object o, int orderNum) { if (_remapper.getPkColumn().getJdbcType().isText()) { + _remapper.setIncludePkLookup(true); Object remappedValue = _remapper.mappedValue(o); value = remappedValue != null ? remappedValue : value; } @@ -992,13 +1005,19 @@ protected Object convertWithPrimaryColumn(Object o, int orderNum) return null; } - protected Object convertWithRemapper(Object o, int orderNum) + protected Object convertWithRemapper(Object o) { - if (_lookupResolutionType.getAlternateResolutionOrder() >= orderNum) + if (_lookupResolutionType.useAlternateKey()) { - // don't want to attempt to resolve by target table PK because we already know there is a type mismatch - _remapper.setIncludePkLookup(false); - return _remapper.mappedValue(o); + try + { + _remapper.setIncludePkLookup(false); + return _remapper.mappedValue(o); + } + catch (ConversionException ex) + { + return null; + } } return null; } @@ -1009,16 +1028,17 @@ protected Object convert(Object o) if (o == null) return null; - int convertNum = 1; - Object value = convertWithPrimaryColumn(o, convertNum); + boolean triedPrimary = false; + Object value = o; + value = convertWithPrimaryColumn(o, triedPrimary); + triedPrimary = true; if (value != null) return value; - convertNum++; - value = convertWithRemapper(o, convertNum); + value = convertWithRemapper(o); if (value != null) return value; - convertNum++; - return convertWithPrimaryColumn(o, convertNum); + value = convertWithPrimaryColumn(o, triedPrimary); + return value; } } @@ -1368,14 +1388,18 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro ForeignKey fk = col.getFk(); DataIteratorContext.LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); - if (fk != null && lookupResolutionType.usesAlternateKey() && fk.allowImportByAlternateKey()) + if (fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) { // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); RemapMissingBehavior missing = remapMissingBehavior; if (missing == null) - missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; + missing = col.isRequired() || hasValidator || !lookupResolutionType.usePrimaryKey() ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; + // For enum tables, we should not be using number names for the values, so we will look up by primary key first (resolving rowIds) then alternate. + // This assures the type is an Integer when a rowId is given (e.g., from a row read from the database), not a string. + if (fk.getLookupTableInfo() instanceof EnumTableInfo) + lookupResolutionType = DataIteratorContext.LookupResolutionType.primaryThenAlternateKey; c = new RemapPostConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); } From f7d2cf6baa581301b9705c7865e0dc70546abd64 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 14 Apr 2025 06:56:48 -0700 Subject: [PATCH 04/39] Different parameterization of LookupResolutionType --- .../api/dataiterator/DataIteratorContext.java | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java index 664f5486b9e..b306ec58170 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java @@ -39,34 +39,36 @@ public class DataIteratorContext { public enum LookupResolutionType { - primaryKey(1, -1), // known that the use will always supply the pk value - alternateKey(-1, 1), // known that the use will never supply the pk value - alternateThenPrimaryKey(2, 1), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first + primaryKey(true, false, true), // known that the use will always supply the pk value + alternateKey(false, true, false), // known that the use will never supply the pk value + alternateThenPrimaryKey(true, true, false), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first // TODO do we need to support this? If not, we can change the properties here to just have a "supportsAlternateKey" since alternate will always be checked first. - primaryThenAlternateKey(1, 2); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (added for compatibility; prefer the other options) + primaryThenAlternateKey(true, true, true); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (added for compatibility; prefer the other options) - final int _primaryResolutionOrder; - final int _alternateResolutionOrder; + final boolean _useAlternateKey; + final boolean _usePrimaryKey; + final boolean _usePrimaryFirst; - LookupResolutionType(int primaryResolutionOrder, int alternateResolutionOrder) + LookupResolutionType(boolean usePrimaryKey, boolean useAlternateKey, boolean usePrimaryFirst) { - _primaryResolutionOrder = primaryResolutionOrder; - _alternateResolutionOrder = alternateResolutionOrder; + _usePrimaryKey = usePrimaryKey; + _useAlternateKey = useAlternateKey; + _usePrimaryFirst = usePrimaryFirst; } - public int getPrimaryResolutionOrder() + public boolean useAlternateKey() { - return _primaryResolutionOrder; + return _useAlternateKey; } - public int getAlternateResolutionOrder() + public boolean usePrimaryKey() { - return _alternateResolutionOrder; + return _usePrimaryKey; } - public boolean usesAlternateKey() + public boolean usePrimaryFirst() { - return _alternateResolutionOrder != -1; + return _usePrimaryFirst; } } From 93c5aea3760e8978ddc0c298f910d56cd49c19fc Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 14 Apr 2025 11:09:56 -0700 Subject: [PATCH 05/39] Always reload maps when changing includePkLookup --- api/src/org/labkey/api/dataiterator/SimpleTranslator.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 1e0738cc10f..a80b1527dfe 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -221,8 +221,7 @@ public RemapPostConvert(@NotNull TableInfo targetTable, boolean includeTitleColu public void setIncludePkLookup(boolean includePkLookup) { _includePkLookup = includePkLookup; - if (includePkLookup) - _maps = null; + _maps = null; } public ColumnInfo getPkColumn() From 9ef713c95bbbe489f59a0c50b1a95eadf2532a8a Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 14 Apr 2025 11:11:34 -0700 Subject: [PATCH 06/39] Remove unused method --- api/src/org/labkey/api/dataiterator/DataIteratorContext.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java index b306ec58170..bc9cf43f472 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java @@ -208,11 +208,6 @@ public void setLookupResolutionType(LookupResolutionType lookupResolutionType) _lookupResolutionType = lookupResolutionType; } - public boolean isAllowImportLookupByAlternateKey() - { - return getLookupResolutionType() == LookupResolutionType.alternateThenPrimaryKey; - } - /** When true, allow importing lookup columns by the lookup table's alternate key instead of by primary key. */ public void setAllowImportLookupByAlternateKey(boolean allowImportLookupByAlternateKey) { From 47eeba6f4a5520a2d4e705c78aadaed7bf794f8e Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 14 Apr 2025 12:56:28 -0700 Subject: [PATCH 07/39] Make getter method non-null --- api/src/org/labkey/api/query/AbstractQueryImportAction.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/query/AbstractQueryImportAction.java b/api/src/org/labkey/api/query/AbstractQueryImportAction.java index 66892055a0f..f76e6cb6eb0 100644 --- a/api/src/org/labkey/api/query/AbstractQueryImportAction.java +++ b/api/src/org/labkey/api/query/AbstractQueryImportAction.java @@ -338,9 +338,13 @@ protected boolean getOptionParamValue(Params p) return getOptionParamsMap().getOrDefault(p, false); } + @NotNull protected DataIteratorContext.LookupResolutionType getLookupResolutionType() { - return DataIteratorContext.LookupResolutionType.valueOf(getParam(Params.lookupResolutionType)); + String paramValue = getParam(Params.lookupResolutionType); + if (paramValue == null) + return DataIteratorContext.LookupResolutionType.primaryKey; + return DataIteratorContext.LookupResolutionType.valueOf(paramValue); } protected boolean skipInsertOptionValidation() From 2494e613502fcefbf62a8ca075a95fe133f82a15 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 14 Apr 2025 12:56:48 -0700 Subject: [PATCH 08/39] Update test expectations --- .../api/dataiterator/SimpleTranslator.java | 78 ++++++++++++++++++- 1 file changed, 75 insertions(+), 3 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index a80b1527dfe..ebf025bac0c 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -2255,11 +2255,11 @@ public void convertRemapTest() throws Exception public StringExpression getURL(ColumnInfo parent) { return null; } }; - // with remap with allowImportLookupByAlternateKey + // with remap with primary then alternate key // don't throw error if remap can't be resolved { DataIteratorContext context = new DataIteratorContext(); - context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.alternateThenPrimaryKey); + context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.primaryThenAlternateKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue); @@ -2285,7 +2285,79 @@ public void convertRemapTest() throws Exception // fourth row assertTrue(t.next()); assertEquals(4, t.get(0)); - assertNull(t.get(1)); // empty string converts to null + assertEquals("", t.get(1)); // since remapping to the original, returns the empty string + + // no more rows + assertFalse(t.next()); + } + + // with remap with primary then alternate key, missing behavior is null + // don't throw error if remap can't be resolved + { + DataIteratorContext context = new DataIteratorContext(); + context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.primaryThenAlternateKey); + simpleData.beforeFirst(); + SimpleTranslator t = new SimpleTranslator(simpleData, context); + t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null); + assertEquals(1, t.getColumnCount()); + assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); + assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); + + // first row + assertTrue(t.next()); + assertEquals(1, t.get(0)); + assertEquals(0, t.get(1)); // convert string "0" -> rowId ordinal 0 + + // second row + assertTrue(t.next()); + assertEquals(2, t.get(0)); + assertEquals(1, t.get(1)); // convert string "Two" -> rowId ordinal 1 + + // third row -- original value passed through + assertTrue(t.next()); + assertEquals(3, t.get(0)); + assertNull(t.get(1)); // fails to convert + + // fourth row + assertTrue(t.next()); + assertEquals(4, t.get(0)); + assertNull(t.get(1)); // missing returns null + + // no more rows + assertFalse(t.next()); + } + + // with remap with alternate then primary key, missing behavior is null + // don't throw error if remap can't be resolved + { + DataIteratorContext context = new DataIteratorContext(); + context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.alternateThenPrimaryKey); + simpleData.beforeFirst(); + SimpleTranslator t = new SimpleTranslator(simpleData, context); + t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null); + assertEquals(1, t.getColumnCount()); + assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); + assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); + + // first row + assertTrue(t.next()); + assertEquals(1, t.get(0)); + assertEquals(0, t.get(1)); // convert string "0" -> rowId ordinal 0 + + // second row + assertTrue(t.next()); + assertEquals(2, t.get(0)); + assertEquals(1, t.get(1)); // convert string "Two" -> rowId ordinal 1 + + // third row -- original value passed through + assertTrue(t.next()); + assertEquals(3, t.get(0)); + assertNull(t.get(1)); // fails to convert + + // fourth row + assertTrue(t.next()); + assertEquals(4, t.get(0)); + assertNull(t.get(1)); // missing returns null // no more rows assertFalse(t.next()); From b401519466e42a8ce21631721d527410658047c2 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 14 Apr 2025 12:57:13 -0700 Subject: [PATCH 09/39] Use non-null LookupResolutionType --- list/src/org/labkey/list/model/ListImporter.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/list/src/org/labkey/list/model/ListImporter.java b/list/src/org/labkey/list/model/ListImporter.java index 7766cd165f6..0091496cd2e 100644 --- a/list/src/org/labkey/list/model/ListImporter.java +++ b/list/src/org/labkey/list/model/ListImporter.java @@ -32,6 +32,7 @@ import org.labkey.api.data.SqlExecutor; import org.labkey.api.data.TableInfo; import org.labkey.api.data.dialect.SqlDialect; +import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.di.DataIntegrationService; import org.labkey.api.exceptions.OptimisticConflictException; import org.labkey.api.exp.ImportTypesHelper; @@ -258,7 +259,7 @@ private boolean processSingle(VirtualFile sourceDir, ListDefinition def, String } } - def.importListItems(user, c, loader, batchErrors, sourceDir.getDir(FileUtil.makeLegalName(def.getName())), null, supportAI, null, _importContext.useMerge() ? QueryUpdateService.InsertOption.MERGE : QueryUpdateService.InsertOption.IMPORT); + def.importListItems(user, c, loader, batchErrors, sourceDir.getDir(FileUtil.makeLegalName(def.getName())), null, supportAI, DataIteratorContext.LookupResolutionType.primaryKey, _importContext.useMerge() ? QueryUpdateService.InsertOption.MERGE : QueryUpdateService.InsertOption.IMPORT); } for (ValidationException v : batchErrors.getRowErrors()) From 53586cb662b78e6d6c84abddb78639251c3630d8 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 15 Apr 2025 12:37:48 -0700 Subject: [PATCH 10/39] Support (for now) deprecated importLookupByAlternateKey --- .../api/query/AbstractQueryImportAction.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/query/AbstractQueryImportAction.java b/api/src/org/labkey/api/query/AbstractQueryImportAction.java index f76e6cb6eb0..68784f8e250 100644 --- a/api/src/org/labkey/api/query/AbstractQueryImportAction.java +++ b/api/src/org/labkey/api/query/AbstractQueryImportAction.java @@ -308,6 +308,7 @@ public enum Params insertOption, crossTypeImport, allowCreateStorage, + importLookupByAlternateKey, // deprecated. Prefer lookupResolutionType crossFolderImport, useTransactionAuditCache, lookupResolutionType, @@ -343,7 +344,18 @@ protected DataIteratorContext.LookupResolutionType getLookupResolutionType() { String paramValue = getParam(Params.lookupResolutionType); if (paramValue == null) - return DataIteratorContext.LookupResolutionType.primaryKey; + { + paramValue = getParam(Params.importLookupByAlternateKey); + if (paramValue == null) + return DataIteratorContext.LookupResolutionType.primaryKey; + else + { + if (Boolean.valueOf(paramValue)) + return DataIteratorContext.LookupResolutionType.alternateThenPrimaryKey; + else + return DataIteratorContext.LookupResolutionType.primaryThenAlternateKey; + } + } return DataIteratorContext.LookupResolutionType.valueOf(paramValue); } From 82c091162662d6d0d9ff232b989589947e66e645 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 15 Apr 2025 15:34:49 -0700 Subject: [PATCH 11/39] Extract LookupResolutionType out of DataIteratorContext --- .../labkey/api/data/LookupResolutionType.java | 36 +++++++++++++++++++ .../api/dataiterator/DataIteratorContext.java | 36 +------------------ .../api/dataiterator/SimpleTranslator.java | 15 ++++---- .../labkey/api/exp/list/ListDefinition.java | 6 ++-- .../api/query/AbstractQueryImportAction.java | 17 ++++----- .../api/query/QueryImportPipelineJob.java | 7 ++-- .../labkey/experiment/api/LineageTest.java | 3 +- .../samples/AbstractExpFolderImporter.java | 3 +- .../labkey/list/model/ListDefinitionImpl.java | 8 ++--- .../org/labkey/list/model/ListImporter.java | 4 +-- .../list/model/ListQueryUpdateService.java | 3 +- .../labkey/query/QueryServiceImplTestCase.jsp | 3 +- .../study/assay/StudyPublishManager.java | 4 +-- .../org/labkey/study/model/StudyManager.java | 3 +- 14 files changed, 79 insertions(+), 69 deletions(-) create mode 100644 api/src/org/labkey/api/data/LookupResolutionType.java diff --git a/api/src/org/labkey/api/data/LookupResolutionType.java b/api/src/org/labkey/api/data/LookupResolutionType.java new file mode 100644 index 00000000000..092ff3bb0c4 --- /dev/null +++ b/api/src/org/labkey/api/data/LookupResolutionType.java @@ -0,0 +1,36 @@ +package org.labkey.api.data; + +public enum LookupResolutionType +{ + primaryKey(true, false, true), // known that the use will always supply the pk value + alternateKey(false, true, false), // known that the use will never supply the pk value + alternateThenPrimaryKey(true, true, false), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first + // TODO do we need to support this? If not, we can change the properties here to just have a "supportsAlternateKey" since alternate will always be checked first. + primaryThenAlternateKey(true, true, true); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (added for compatibility; prefer the other options) + + final boolean _useAlternateKey; + final boolean _usePrimaryKey; + final boolean _usePrimaryFirst; + + LookupResolutionType(boolean usePrimaryKey, boolean useAlternateKey, boolean usePrimaryFirst) + { + _usePrimaryKey = usePrimaryKey; + _useAlternateKey = useAlternateKey; + _usePrimaryFirst = usePrimaryFirst; + } + + public boolean useAlternateKey() + { + return _useAlternateKey; + } + + public boolean usePrimaryKey() + { + return _usePrimaryKey; + } + + public boolean usePrimaryFirst() + { + return _usePrimaryFirst; + } +} diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java index bc9cf43f472..91dff816e61 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java @@ -19,6 +19,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.collections.CaseInsensitiveHashSet; +import org.labkey.api.data.LookupResolutionType; import org.labkey.api.query.BatchValidationException; import org.labkey.api.query.QueryImportPipelineJob; import org.labkey.api.query.QueryUpdateService; @@ -37,41 +38,6 @@ */ public class DataIteratorContext { - public enum LookupResolutionType - { - primaryKey(true, false, true), // known that the use will always supply the pk value - alternateKey(false, true, false), // known that the use will never supply the pk value - alternateThenPrimaryKey(true, true, false), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first - // TODO do we need to support this? If not, we can change the properties here to just have a "supportsAlternateKey" since alternate will always be checked first. - primaryThenAlternateKey(true, true, true); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (added for compatibility; prefer the other options) - - final boolean _useAlternateKey; - final boolean _usePrimaryKey; - final boolean _usePrimaryFirst; - - LookupResolutionType(boolean usePrimaryKey, boolean useAlternateKey, boolean usePrimaryFirst) - { - _usePrimaryKey = usePrimaryKey; - _useAlternateKey = useAlternateKey; - _usePrimaryFirst = usePrimaryFirst; - } - - public boolean useAlternateKey() - { - return _useAlternateKey; - } - - public boolean usePrimaryKey() - { - return _usePrimaryKey; - } - - public boolean usePrimaryFirst() - { - return _usePrimaryFirst; - } - } - /* NOTE: DIC is not really meant to be a set up parameter block targetOption and selectIds should probably be moved out in a future diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index ebf025bac0c..01fcbf152d8 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -44,6 +44,7 @@ import org.labkey.api.data.EnumTableInfo; import org.labkey.api.data.ForeignKey; import org.labkey.api.data.JdbcType; +import org.labkey.api.data.LookupResolutionType; import org.labkey.api.data.MultiValuedForeignKey; import org.labkey.api.data.MvUtil; import org.labkey.api.data.SimpleFilter; @@ -958,11 +959,11 @@ protected class RemapPostConvertColumn extends SimpleConvertColumn final ColumnInfo _toCol; final RemapMissingBehavior _missing; final boolean _includeTitleColumn; - final DataIteratorContext.LookupResolutionType _lookupResolutionType; + final LookupResolutionType _lookupResolutionType; final private RemapPostConvert _remapper; - public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, final int fromIndex, final @NotNull ColumnInfo toCol, RemapMissingBehavior missing, boolean includeTitleColumn, @NotNull DataIteratorContext.LookupResolutionType lookupResolutionType) + public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, final int fromIndex, final @NotNull ColumnInfo toCol, RemapMissingBehavior missing, boolean includeTitleColumn, @NotNull LookupResolutionType lookupResolutionType) { super(convertCol.fieldName, convertCol.index, convertCol.type); _convertCol = convertCol; @@ -1384,7 +1385,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro c = new PropertyConvertColumn(name, fromIndex, mvIndex, mv, pt, type); ForeignKey fk = col.getFk(); - DataIteratorContext.LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); + LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); if (fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) { // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error @@ -1396,7 +1397,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro // For enum tables, we should not be using number names for the values, so we will look up by primary key first (resolving rowIds) then alternate. // This assures the type is an Integer when a rowId is given (e.g., from a row read from the database), not a string. if (fk.getLookupTableInfo() instanceof EnumTableInfo) - lookupResolutionType = DataIteratorContext.LookupResolutionType.primaryThenAlternateKey; + lookupResolutionType = LookupResolutionType.primaryThenAlternateKey; c = new RemapPostConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); } @@ -2259,7 +2260,7 @@ public void convertRemapTest() throws Exception // don't throw error if remap can't be resolved { DataIteratorContext context = new DataIteratorContext(); - context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.primaryThenAlternateKey); + context.setLookupResolutionType(LookupResolutionType.primaryThenAlternateKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue); @@ -2295,7 +2296,7 @@ public void convertRemapTest() throws Exception // don't throw error if remap can't be resolved { DataIteratorContext context = new DataIteratorContext(); - context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.primaryThenAlternateKey); + context.setLookupResolutionType(LookupResolutionType.primaryThenAlternateKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null); @@ -2331,7 +2332,7 @@ public void convertRemapTest() throws Exception // don't throw error if remap can't be resolved { DataIteratorContext context = new DataIteratorContext(); - context.setLookupResolutionType(DataIteratorContext.LookupResolutionType.alternateThenPrimaryKey); + context.setLookupResolutionType(LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null); diff --git a/api/src/org/labkey/api/exp/list/ListDefinition.java b/api/src/org/labkey/api/exp/list/ListDefinition.java index 0185bcf2e6a..9dd121c74ff 100644 --- a/api/src/org/labkey/api/exp/list/ListDefinition.java +++ b/api/src/org/labkey/api/exp/list/ListDefinition.java @@ -21,8 +21,8 @@ import org.labkey.api.data.ColumnInfo; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; +import org.labkey.api.data.LookupResolutionType; import org.labkey.api.data.TableInfo; -import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.exp.DomainNotFoundException; import org.labkey.api.exp.PropertyType; import org.labkey.api.exp.property.Domain; @@ -300,8 +300,8 @@ public static BodySetting getForValue(int value) ListItem getListItemForEntityId(String entityId, User user); int insertListItems(User user, Container container, List listItems) throws IOException; - int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType) throws IOException; - int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType, QueryUpdateService.InsertOption insertOption) throws IOException; + int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, LookupResolutionType lookupResolutionType) throws IOException; + int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, LookupResolutionType lookupResolutionType, QueryUpdateService.InsertOption insertOption) throws IOException; @Nullable TableInfo getTable(User user); @Nullable TableInfo getTable(User user, Container c); diff --git a/api/src/org/labkey/api/query/AbstractQueryImportAction.java b/api/src/org/labkey/api/query/AbstractQueryImportAction.java index 68784f8e250..9d3a4439bda 100644 --- a/api/src/org/labkey/api/query/AbstractQueryImportAction.java +++ b/api/src/org/labkey/api/query/AbstractQueryImportAction.java @@ -34,6 +34,7 @@ import org.labkey.api.data.Container; import org.labkey.api.data.DbSchema; import org.labkey.api.data.DbScope; +import org.labkey.api.data.LookupResolutionType; import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.TableInfo; import org.labkey.api.dataiterator.DataIterator; @@ -340,23 +341,23 @@ protected boolean getOptionParamValue(Params p) } @NotNull - protected DataIteratorContext.LookupResolutionType getLookupResolutionType() + protected LookupResolutionType getLookupResolutionType() { String paramValue = getParam(Params.lookupResolutionType); if (paramValue == null) { paramValue = getParam(Params.importLookupByAlternateKey); if (paramValue == null) - return DataIteratorContext.LookupResolutionType.primaryKey; + return LookupResolutionType.primaryKey; else { if (Boolean.valueOf(paramValue)) - return DataIteratorContext.LookupResolutionType.alternateThenPrimaryKey; + return LookupResolutionType.alternateThenPrimaryKey; else - return DataIteratorContext.LookupResolutionType.primaryThenAlternateKey; + return LookupResolutionType.primaryThenAlternateKey; } } - return DataIteratorContext.LookupResolutionType.valueOf(paramValue); + return LookupResolutionType.valueOf(paramValue); } protected boolean skipInsertOptionValidation() @@ -813,12 +814,12 @@ protected int importData(DataLoader dl, FileStream file, String originalName, Ba return importData(dl, _target, _updateService, _insertOption, getOptionParamsMap(), getLookupResolutionType(), errors, auditBehaviorType, auditEvent, auditUserComment, getUser(), getContainer(), null); } - public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, DataIteratorContext.LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container) + public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container) { return createDataIteratorContext(insertOption, optionParamsMap, lookupResolutionType, auditBehaviorType, auditUserComment, errors, logger, container, null); } - public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, DataIteratorContext.LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container, QueryImportPipelineJob importJob) + public static DataIteratorContext createDataIteratorContext(QueryUpdateService.InsertOption insertOption, Map optionParamsMap, LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String auditUserComment, BatchValidationException errors, @Nullable Logger logger, @Nullable Container container, QueryImportPipelineJob importJob) { boolean importIdentity = optionParamsMap.getOrDefault(AbstractQueryImportAction.Params.importIdentity, false); boolean crossTypeImport = optionParamsMap.getOrDefault(AbstractQueryImportAction.Params.crossTypeImport, false); @@ -852,7 +853,7 @@ public static DataIteratorContext createDataIteratorContext(QueryUpdateService.I return context; } - public static int importData(DataLoader dl, TableInfo target, QueryUpdateService updateService, QueryUpdateService.InsertOption insertOption, Map optionParamsMap, DataIteratorContext.LookupResolutionType lookupResolutionType, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, TransactionAuditProvider.@Nullable TransactionAuditEvent auditEvent, @Nullable String auditUserComment, User user, Container container, @Nullable Logger logger) throws IOException + public static int importData(DataLoader dl, TableInfo target, QueryUpdateService updateService, QueryUpdateService.InsertOption insertOption, Map optionParamsMap, LookupResolutionType lookupResolutionType, BatchValidationException errors, @Nullable AuditBehaviorType auditBehaviorType, TransactionAuditProvider.@Nullable TransactionAuditEvent auditEvent, @Nullable String auditUserComment, User user, Container container, @Nullable Logger logger) throws IOException { return importData(dl, target, updateService, createDataIteratorContext(insertOption, optionParamsMap, lookupResolutionType, auditBehaviorType, auditUserComment, errors, logger, container), auditEvent, user, container); } diff --git a/api/src/org/labkey/api/query/QueryImportPipelineJob.java b/api/src/org/labkey/api/query/QueryImportPipelineJob.java index 6cc2194f516..a6b40ae3373 100644 --- a/api/src/org/labkey/api/query/QueryImportPipelineJob.java +++ b/api/src/org/labkey/api/query/QueryImportPipelineJob.java @@ -4,6 +4,7 @@ import org.jetbrains.annotations.Nullable; import org.labkey.api.audit.TransactionAuditProvider; import org.labkey.api.data.Container; +import org.labkey.api.data.LookupResolutionType; import org.labkey.api.data.TableInfo; import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.gwt.client.AuditBehaviorType; @@ -60,7 +61,7 @@ public static class QueryImportAsyncContextBuilder String _auditUserComment = null; boolean _allowLineageColumns = false; Map _optionParamsMap = new HashMap<>(); - DataIteratorContext.LookupResolutionType _lookupResolutionType = null; + LookupResolutionType _lookupResolutionType = null; String _jobDescription; @@ -209,12 +210,12 @@ public QueryImportAsyncContextBuilder setOptionParamsMap(Map options = new HashMap<>(); try diff --git a/list/src/org/labkey/list/model/ListDefinitionImpl.java b/list/src/org/labkey/list/model/ListDefinitionImpl.java index 0b1c02adda2..bf4018c9260 100644 --- a/list/src/org/labkey/list/model/ListDefinitionImpl.java +++ b/list/src/org/labkey/list/model/ListDefinitionImpl.java @@ -25,11 +25,11 @@ import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DbScope; +import org.labkey.api.data.LookupResolutionType; import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; -import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.exp.DomainNotFoundException; import org.labkey.api.exp.Lsid; import org.labkey.api.exp.ObjectProperty; @@ -587,18 +587,18 @@ public int insertListItems(User user, Container container, List listIt MapLoader loader = new MapLoader(rows); // TODO: Find out the attachment directory? - return insertListItems(user, container, loader, ve, null, null, false, DataIteratorContext.LookupResolutionType.primaryKey); + return insertListItems(user, container, loader, ve, null, null, false, LookupResolutionType.primaryKey); } @Override - public int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType) + public int insertListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, LookupResolutionType lookupResolutionType) { return importListItems(user, container, loader, errors, attachmentDir, progress, supportAutoIncrementKey, lookupResolutionType, QueryUpdateService.InsertOption.INSERT); } @Override - public int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, DataIteratorContext.LookupResolutionType lookupResolutionType, QueryUpdateService.InsertOption insertOption) + public int importListItems(User user, Container container, DataLoader loader, @NotNull BatchValidationException errors, @Nullable VirtualFile attachmentDir, @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, LookupResolutionType lookupResolutionType, QueryUpdateService.InsertOption insertOption) { ListQuerySchema schema = new ListQuerySchema(user, container); TableInfo table = schema.getTable(_def.getName()); diff --git a/list/src/org/labkey/list/model/ListImporter.java b/list/src/org/labkey/list/model/ListImporter.java index 0091496cd2e..a1da940aaa5 100644 --- a/list/src/org/labkey/list/model/ListImporter.java +++ b/list/src/org/labkey/list/model/ListImporter.java @@ -27,12 +27,12 @@ import org.labkey.api.data.Container; import org.labkey.api.data.DbScope; import org.labkey.api.data.JdbcType; +import org.labkey.api.data.LookupResolutionType; import org.labkey.api.data.PHI; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SqlExecutor; import org.labkey.api.data.TableInfo; import org.labkey.api.data.dialect.SqlDialect; -import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.di.DataIntegrationService; import org.labkey.api.exceptions.OptimisticConflictException; import org.labkey.api.exp.ImportTypesHelper; @@ -259,7 +259,7 @@ private boolean processSingle(VirtualFile sourceDir, ListDefinition def, String } } - def.importListItems(user, c, loader, batchErrors, sourceDir.getDir(FileUtil.makeLegalName(def.getName())), null, supportAI, DataIteratorContext.LookupResolutionType.primaryKey, _importContext.useMerge() ? QueryUpdateService.InsertOption.MERGE : QueryUpdateService.InsertOption.IMPORT); + def.importListItems(user, c, loader, batchErrors, sourceDir.getDir(FileUtil.makeLegalName(def.getName())), null, supportAI, LookupResolutionType.primaryKey, _importContext.useMerge() ? QueryUpdateService.InsertOption.MERGE : QueryUpdateService.InsertOption.IMPORT); } for (ValidationException v : batchErrors.getRowErrors()) diff --git a/list/src/org/labkey/list/model/ListQueryUpdateService.java b/list/src/org/labkey/list/model/ListQueryUpdateService.java index 0bd1890acd0..eb4b357cc5c 100644 --- a/list/src/org/labkey/list/model/ListQueryUpdateService.java +++ b/list/src/org/labkey/list/model/ListQueryUpdateService.java @@ -26,6 +26,7 @@ import org.labkey.api.data.ColumnInfo; import org.labkey.api.data.Container; import org.labkey.api.data.DbScope; +import org.labkey.api.data.LookupResolutionType; import org.labkey.api.data.Selector.ForEachBatchBlock; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.TableInfo; @@ -205,7 +206,7 @@ private User getListUser(User user, Container container) } public int insertUsingDataIterator(DataLoader loader, User user, Container container, BatchValidationException errors, @Nullable VirtualFile attachmentDir, - @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, InsertOption insertOption, DataIteratorContext.LookupResolutionType lookupResolutionType) + @Nullable ListImportProgress progress, boolean supportAutoIncrementKey, InsertOption insertOption, LookupResolutionType lookupResolutionType) { if (!_list.isVisible(user)) throw new UnauthorizedException("You do not have permission to insert data into this table."); diff --git a/query/src/org/labkey/query/QueryServiceImplTestCase.jsp b/query/src/org/labkey/query/QueryServiceImplTestCase.jsp index 651639daa1e..3cbd08a46fd 100644 --- a/query/src/org/labkey/query/QueryServiceImplTestCase.jsp +++ b/query/src/org/labkey/query/QueryServiceImplTestCase.jsp @@ -30,6 +30,7 @@ <%@ page import="java.util.Arrays" %> <%@ page import="java.util.List" %> <%@ page import="org.labkey.api.dataiterator.DataIteratorContext" %> +<%@ page import="org.labkey.api.data.LookupResolutionType" %> <%@ page extends="org.labkey.api.jsp.JspTest.DRT" %> <%! final static String tableName = "testGetSelectSqlSort"; @@ -44,7 +45,7 @@ ListDefinition list = s.getList(c, tableName); var data = new ReaderInputStream(new StringReader("A,B,C\n6,4,3\n1,8,6\n7,1,9\n2,5,1\n8,9,4\n3,2,7\n9,6,10\n4,10,2\n10,3,5\n5,7,8\n")); - list.insertListItems(user, c, new TabLoader.CsvFactory().createLoader(data,true), new BatchValidationException(), null, null, false, DataIteratorContext.LookupResolutionType.primaryKey); + list.insertListItems(user, c, new TabLoader.CsvFactory().createLoader(data,true), new BatchValidationException(), null, null, false, LookupResolutionType.primaryKey); // wrap this list so we can mangle the sort properties UserSchema schema = (UserSchema)DefaultSchema.get(user,c,"lists"); diff --git a/study/src/org/labkey/study/assay/StudyPublishManager.java b/study/src/org/labkey/study/assay/StudyPublishManager.java index b784f7a2ecd..1764fdb0872 100644 --- a/study/src/org/labkey/study/assay/StudyPublishManager.java +++ b/study/src/org/labkey/study/assay/StudyPublishManager.java @@ -50,6 +50,7 @@ import org.labkey.api.data.Filter; import org.labkey.api.data.ILineageDisplayColumn; import org.labkey.api.data.JdbcType; +import org.labkey.api.data.LookupResolutionType; import org.labkey.api.data.RenderContext; import org.labkey.api.data.Results; import org.labkey.api.data.SQLFragment; @@ -58,7 +59,6 @@ import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; -import org.labkey.api.dataiterator.DataIteratorContext; import org.labkey.api.exp.ChangePropertyDescriptorException; import org.labkey.api.exp.ExperimentException; import org.labkey.api.exp.PropertyDescriptor; @@ -985,7 +985,7 @@ public UploadLog saveUploadData(User user, Dataset dsd, FileStream tsv, String f * Return an array of LSIDs from the newly created dataset entries, * along with the upload log. */ - public Pair, UploadLog> importDatasetTSV(User user, StudyImpl study, DatasetDefinition dsd, DataLoader dl, DataIteratorContext.LookupResolutionType lookupResolutionType, FileStream fileIn, String originalFileName, Map columnMap, BatchValidationException errors, QueryUpdateService.InsertOption insertOption, @Nullable AuditBehaviorType auditBehaviorType) + public Pair, UploadLog> importDatasetTSV(User user, StudyImpl study, DatasetDefinition dsd, DataLoader dl, LookupResolutionType lookupResolutionType, FileStream fileIn, String originalFileName, Map columnMap, BatchValidationException errors, QueryUpdateService.InsertOption insertOption, @Nullable AuditBehaviorType auditBehaviorType) { DbScope scope = StudySchema.getInstance().getScope(); diff --git a/study/src/org/labkey/study/model/StudyManager.java b/study/src/org/labkey/study/model/StudyManager.java index 69b0350d832..3dda92f57ef 100644 --- a/study/src/org/labkey/study/model/StudyManager.java +++ b/study/src/org/labkey/study/model/StudyManager.java @@ -58,6 +58,7 @@ import org.labkey.api.data.DbScope.Transaction; import org.labkey.api.data.Filter; import org.labkey.api.data.JdbcType; +import org.labkey.api.data.LookupResolutionType; import org.labkey.api.data.PHI; import org.labkey.api.data.PropertyManager; import org.labkey.api.data.PropertyManager.WritablePropertyMap; @@ -3443,7 +3444,7 @@ public List importDatasetData(User user, DatasetDefinition def, @Nullable DataState defaultQCState, QueryUpdateService.InsertOption insertOption, Logger logger, - DataIteratorContext.LookupResolutionType lookupResolutionType, + LookupResolutionType lookupResolutionType, @Nullable AuditBehaviorType auditBehaviorType) throws IOException { From 5b961bb875421c2bbf55a17e732fa3d720261508 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 15 Apr 2025 19:37:00 -0700 Subject: [PATCH 12/39] Throw exception if conversion fails --- .../api/dataiterator/SimpleTranslator.java | 16 +++++++++++++--- .../api/SampleTypeUpdateServiceDI.java | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 01fcbf152d8..69bacc4b8bc 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -1027,15 +1027,25 @@ protected Object convert(Object o) return null; boolean triedPrimary = false; - Object value = o; - value = convertWithPrimaryColumn(o, triedPrimary); - triedPrimary = true; + Object value = convertWithPrimaryColumn(o, triedPrimary); + if (value != null) return value; + triedPrimary = true; value = convertWithRemapper(o); if (value != null) return value; value = convertWithPrimaryColumn(o, triedPrimary); + if (value == null) + { + return switch (_missing) + { + case Null -> null; + case OriginalValue -> o; + default -> + throw new ConversionExceptionWithMessage("Invalid value '" + String.valueOf(o) + "' for " + _toCol.getName() + "."); + }; + } return value; } } diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index d65975b1c31..c6fd2ce62d2 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -1938,7 +1938,7 @@ else if (name.equalsIgnoreCase("StoredAmount")) if (isScopedField) _addConvertColumn(name, i, to.getJdbcType(), to.getFk(), aliquotedFromDataColInd, scopedFields.get(name)); else - addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), RemapMissingBehavior.OriginalValue); + addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), to.getRemapMissingBehavior()); } } else From b33f92da7d7a8a93459ee5e629617f7f7d954613 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 15 Apr 2025 19:44:10 -0700 Subject: [PATCH 13/39] Simplify message --- api/src/org/labkey/api/dataiterator/SimpleTranslator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 69bacc4b8bc..742dea53ba7 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -1043,7 +1043,7 @@ protected Object convert(Object o) case Null -> null; case OriginalValue -> o; default -> - throw new ConversionExceptionWithMessage("Invalid value '" + String.valueOf(o) + "' for " + _toCol.getName() + "."); + throw new ConversionExceptionWithMessage("Invalid " + _toCol.getName() + " value '" + o + "'."); }; } return value; From 97bc2a38c0e2047f469a0bf9e51e918ab9b69a78 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Wed, 16 Apr 2025 09:57:27 -0700 Subject: [PATCH 14/39] Remove som unneeded code and parameters --- api/src/org/labkey/api/data/RemapCache.java | 2 +- .../api/dataiterator/SimpleTranslator.java | 91 +++---------------- 2 files changed, 12 insertions(+), 81 deletions(-) diff --git a/api/src/org/labkey/api/data/RemapCache.java b/api/src/org/labkey/api/data/RemapCache.java index 710fad8f02e..ea6fb94b6de 100644 --- a/api/src/org/labkey/api/data/RemapCache.java +++ b/api/src/org/labkey/api/data/RemapCache.java @@ -144,7 +144,7 @@ private SimpleTranslator.RemapPostConvert remapper(Key key, Map { TableInfo table = key.getTable(); - return new SimpleTranslator.RemapPostConvert(table, true, SimpleTranslator.RemapMissingBehavior.Null, _allowBulkLoads, includePkLookup); + return new SimpleTranslator.RemapPostConvert(table, true, _allowBulkLoads, includePkLookup); }); } diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 742dea53ba7..30fb83bb5f7 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -195,7 +195,6 @@ public static class RemapPostConvert { private final TableInfo _targetTable; private final boolean _includeTitleColumn; - private final RemapMissingBehavior _missing; private boolean _includePkLookup; // if true, will perform an initial PK lookup before attempting the AK lookup private final boolean _allowBulkLoads; @@ -205,16 +204,10 @@ public static class RemapPostConvert private Triple> _titleColumnLookupMap = null; private Pair> _pkColumnLookupMap = null; - public RemapPostConvert(@NotNull TableInfo targetTable, boolean includeTitleColumn, RemapMissingBehavior missing, boolean allowBulkLoads) - { - this(targetTable, includeTitleColumn, missing, allowBulkLoads, false); - } - - public RemapPostConvert(@NotNull TableInfo targetTable, boolean includeTitleColumn, RemapMissingBehavior missing, boolean allowBulkLoads, boolean includePkLookup) + public RemapPostConvert(@NotNull TableInfo targetTable, boolean includeTitleColumn, boolean allowBulkLoads, boolean includePkLookup) { _targetTable = targetTable; _includeTitleColumn = includeTitleColumn; - _missing = missing; _allowBulkLoads = allowBulkLoads; _includePkLookup = includePkLookup; } @@ -309,18 +302,10 @@ public Object mappedValue(Object k) if (_titleColumnLookupMap != null) { - Object v = fetch(_titleColumnLookupMap, String.valueOf(k)); - if (v != null) - return v; + return fetch(_titleColumnLookupMap, String.valueOf(k)); } - switch (_missing) - { - case Null: return null; - case OriginalValue: return k; - case Error: - default: throw new ConversionException("Could not translate value: " + String.valueOf(k)); - } + return null; } private final Object MISS = new Object(); @@ -970,17 +955,17 @@ public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, fin _toCol = toCol; _missing = missing; _includeTitleColumn = includeTitleColumn; - _remapper = new RemapPostConvert(_toCol.getFkTableInfo(), _includeTitleColumn, _missing, false, true); + _remapper = new RemapPostConvert(_toCol.getFkTableInfo(), _includeTitleColumn, false, true); _lookupResolutionType = lookupResolutionType; } - protected Object convertWithPrimaryColumn(Object o, boolean primaryTried) + private Object convertWithPrimaryColumn(Object o, boolean primaryTried) { if (_lookupResolutionType.usePrimaryKey() && _lookupResolutionType.usePrimaryFirst() != primaryTried) { try { - // _convertCol here will be the column for the primary key type (unless type inference has gone wrong?) + // _convertCol here will be the column for the primary key type Object value = _convertCol.convert(o); ForeignKey fk = _toCol.getFk(); // issue 40909 : allow String columns to resolve lookups by alternate key if the raw lookup fails to resolve @@ -1003,7 +988,7 @@ protected Object convertWithPrimaryColumn(Object o, boolean primaryTried) return null; } - protected Object convertWithRemapper(Object o) + private Object convertWithRemapper(Object o) { if (_lookupResolutionType.useAlternateKey()) { @@ -1026,16 +1011,15 @@ protected Object convert(Object o) if (o == null) return null; - boolean triedPrimary = false; - Object value = convertWithPrimaryColumn(o, triedPrimary); - + Object value = convertWithPrimaryColumn(o, false); if (value != null) return value; - triedPrimary = true; + value = convertWithRemapper(o); if (value != null) return value; - value = convertWithPrimaryColumn(o, triedPrimary); + + value = convertWithPrimaryColumn(o, true); if (value == null) { return switch (_missing) @@ -1050,46 +1034,6 @@ protected Object convert(Object o) } } - protected class RemapColumn implements Supplier - { - final Supplier _inputColumn; - final Map _map; - final RemapMissingBehavior _missing; - - public RemapColumn(final int index, Map map, RemapMissingBehavior missing) - { - _inputColumn = _data.getSupplier(index); - _map = map; - _missing = missing; - } - - public RemapColumn(Supplier call, Map map, RemapMissingBehavior missing) - { - _inputColumn = call; - _map = map; - _missing = missing; - } - - @Override - public Object get() - { - Object k = _inputColumn.get(); - if (null == k) - return null; - Object v = _map.get(k); - if (null != v || _map.containsKey(k)) - return v; - switch (_missing) - { - case Null: return null; - case OriginalValue: return k; - case Error: - default: throw new ConversionException("Could not translate value: " + String.valueOf(k)); - } - } - } - - protected class NullColumn implements Supplier { @Override @@ -1456,19 +1400,6 @@ public int addTimestampColumn(String name) return addColumn(col, new TimestampColumn()); } - /** - * Translate values from the source data iterator to those contained in the in-memory map. - * @param fromIndex Source column to wrap. - * @param map Mapping from source to value. - * @param missing Tell me how to handle incoming values not present in the map. - */ - public int addRemapColumn(int fromIndex, @NotNull Map map, RemapMissingBehavior missing) - { - ColumnInfo col = new BaseColumnInfo(_data.getColumnInfo(fromIndex)); - RemapColumn remap = new RemapColumn(fromIndex, map, missing); - return addColumn(col, remap); - } - public int addSharedTableLookupColumn(int fromIndex, @Nullable FieldKey extraColumnFieldKey, @Nullable ForeignKey fk, @NotNull Map dataspaceTableIdMap) { From e4726acb485584f7e384e60540f48ae3656408df Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Wed, 16 Apr 2025 11:39:32 -0700 Subject: [PATCH 15/39] Reorder assert arguments --- .../labkey/api/dataiterator/SimpleTranslator.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 30fb83bb5f7..18ab943a441 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -2130,9 +2130,9 @@ public void convertTest() throws Exception simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); t.addConvertColumn("Text", 2, JdbcType.INTEGER, null, null); - assertEquals(t.getColumnCount(), 1); - assertEquals(t.getColumnInfo(0).getJdbcType(), JdbcType.INTEGER); - assertEquals(t.getColumnInfo(1).getJdbcType(), JdbcType.INTEGER); + assertEquals(1, t.getColumnCount()); + assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); + assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); try { assertFalse(t.next()); @@ -2151,9 +2151,9 @@ public void convertTest() throws Exception simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); t.addConvertColumn("Text", 2, JdbcType.INTEGER, null, null); - assertEquals(t.getColumnCount(), 1); - assertEquals(t.getColumnInfo(0).getJdbcType(), JdbcType.INTEGER); - assertEquals(t.getColumnInfo(1).getJdbcType(), JdbcType.INTEGER); + assertEquals(1, t.getColumnCount()); + assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); + assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); for (int i=1 ; i<=4 ; i++) { assertTrue(t.next()); From f21ad3055df79487b9bf8da300152140625da38a Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Wed, 16 Apr 2025 13:18:52 -0700 Subject: [PATCH 16/39] Update lookupResolutionType once a conversion has happened --- .../org/labkey/api/dataiterator/SimpleTranslator.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 18ab943a441..45f91ce0c16 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -944,7 +944,7 @@ protected class RemapPostConvertColumn extends SimpleConvertColumn final ColumnInfo _toCol; final RemapMissingBehavior _missing; final boolean _includeTitleColumn; - final LookupResolutionType _lookupResolutionType; + LookupResolutionType _lookupResolutionType; final private RemapPostConvert _remapper; @@ -1013,13 +1013,20 @@ protected Object convert(Object o) Object value = convertWithPrimaryColumn(o, false); if (value != null) + { + _lookupResolutionType = LookupResolutionType.primaryKey; // having converted, let's not try to do it again return value; + } value = convertWithRemapper(o); if (value != null) + { + _lookupResolutionType = LookupResolutionType.primaryKey; // having converted, let's not try to do it again return value; + } value = convertWithPrimaryColumn(o, true); + _lookupResolutionType = LookupResolutionType.primaryKey; // having converted, let's not try to do it again if (value == null) { return switch (_missing) From af70abeb35440bfcc50dcdea72b9cf591bc46508 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Wed, 16 Apr 2025 13:19:21 -0700 Subject: [PATCH 17/39] Start of conversion for Assay data loader settings --- .../api/assay/AbstractAssayTsvDataHandler.java | 6 +++--- .../org/labkey/api/qc/DataLoaderSettings.java | 17 ++++++++++++----- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/api/src/org/labkey/api/assay/AbstractAssayTsvDataHandler.java b/api/src/org/labkey/api/assay/AbstractAssayTsvDataHandler.java index 0cffb184810..d111fb55845 100644 --- a/api/src/org/labkey/api/assay/AbstractAssayTsvDataHandler.java +++ b/api/src/org/labkey/api/assay/AbstractAssayTsvDataHandler.java @@ -303,7 +303,7 @@ else if (mvIndicatorColumns.contains(column.name)) { // Allow String values through if the column is a lookup and the settings allow lookups by alternate key. // The lookup table unique indices or display column value will be used to convert the column to the lookup value. - if (!(settings.isAllowLookupByAlternateKey() && column.clazz == String.class && prop.getLookup() != null)) + if (!(settings.getLookupResolutionType().useAlternateKey() && column.clazz == String.class && prop.getLookup() != null)) { // Otherwise, just use the expected PropertyDescriptor's column type column.clazz = prop.getPropertyDescriptor().getPropertyType().getJavaType(); @@ -771,7 +771,7 @@ else if (sampleLookup.isLookup()) } } - if (dataTable != null && settings.isAllowLookupByAlternateKey()) + if (dataTable != null && settings.getLookupResolutionType().useAlternateKey()) { ColumnInfo column = dataTable.getColumn(pd.getName()); ForeignKey fk = column != null ? column.getFk() : null; @@ -993,7 +993,7 @@ else if (o != remapped) // If allowLookupByAlternateKey is true or the sample lookup is by name, we call findExpMaterial which will attempt to resolve by name first and then rowId. // If allowLookupByAlternateKey is false, we will only try resolving by the rowId. ExpMaterial material = null; - if (settings.isAllowLookupByAlternateKey() || isSampleLookupByName) + if (settings.getLookupResolutionType().useAlternateKey() || isSampleLookupByName) { String materialName = o.toString(); if (inputMaterials.containsKey(materialName)) diff --git a/api/src/org/labkey/api/qc/DataLoaderSettings.java b/api/src/org/labkey/api/qc/DataLoaderSettings.java index 8d27f664360..edba3eb83ab 100644 --- a/api/src/org/labkey/api/qc/DataLoaderSettings.java +++ b/api/src/org/labkey/api/qc/DataLoaderSettings.java @@ -15,6 +15,8 @@ */ package org.labkey.api.qc; +import org.labkey.api.data.LookupResolutionType; + /** * User: klum * Date: Oct 9, 2011 @@ -26,7 +28,7 @@ public class DataLoaderSettings private boolean _allowEmptyData; private boolean _throwOnErrors; private boolean _allowUnexpectedColumns; // don't load columns not in target domain - private boolean _allowLookupByAlternateKey = true; // import lookup column by unique index on target column or by title display column (if unique) + private LookupResolutionType _lookupResolutionType = LookupResolutionType.alternateThenPrimaryKey; public boolean isBestEffortConversion() { @@ -68,13 +70,18 @@ public void setAllowUnexpectedColumns(boolean allowUnexpectedColumns) _allowUnexpectedColumns = allowUnexpectedColumns; } - public boolean isAllowLookupByAlternateKey() + public void setAllowLookupByAlternateKey(boolean allowLookupByAlternateKey) { - return _allowLookupByAlternateKey; + _lookupResolutionType = allowLookupByAlternateKey ? LookupResolutionType.alternateThenPrimaryKey : LookupResolutionType.primaryKey; } - public void setAllowLookupByAlternateKey(boolean allowLookupByAlternateKey) + public LookupResolutionType getLookupResolutionType() + { + return _lookupResolutionType; + } + + public void setLookupResolutionType(LookupResolutionType lookupResolutionType) { - _allowLookupByAlternateKey = allowLookupByAlternateKey; + _lookupResolutionType = lookupResolutionType; } } From 30d75852671bf577a46fd47142a6d6dcc2c37596 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Wed, 7 May 2025 09:04:09 -0700 Subject: [PATCH 18/39] Add hasBeenCoerced property to context so we can not try coercion again. --- .../labkey/api/dataiterator/DataIteratorContext.java | 11 +++++++++++ .../labkey/api/dataiterator/EmbargoDataIterator.java | 2 +- .../org/labkey/api/dataiterator/SimpleTranslator.java | 5 +---- .../src/org/labkey/experiment/ExpDataIterators.java | 4 +--- .../experiment/api/ExpDataClassDataTableImpl.java | 2 +- .../experiment/api/SampleTypeUpdateServiceDI.java | 3 ++- 6 files changed, 17 insertions(+), 10 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java index f164d7c9d26..35efb32d9d4 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java @@ -61,6 +61,7 @@ public class DataIteratorContext private final Set _dontUpdateColumnNames = new CaseInsensitiveHashSet(); private final Set _alternateKeys = new CaseInsensitiveHashSet(); private String _dataSource; + private boolean _hasBeenCoerced = false; private final Map _responseInfo = new HashMap<>(); // information from the import/loadRows context to be passed back to the API response object private Logger _logger; @@ -174,6 +175,16 @@ public void setLookupResolutionType(LookupResolutionType lookupResolutionType) _lookupResolutionType = lookupResolutionType; } + public boolean hasBeenCoerced() + { + return _hasBeenCoerced; + } + + public void setHasBeenCoerced(boolean hasBeenCoerced) + { + _hasBeenCoerced = hasBeenCoerced; + } + /** When true, allow importing lookup columns by the lookup table's alternate key instead of by primary key. */ public void setAllowImportLookupByAlternateKey(boolean allowImportLookupByAlternateKey) { diff --git a/api/src/org/labkey/api/dataiterator/EmbargoDataIterator.java b/api/src/org/labkey/api/dataiterator/EmbargoDataIterator.java index 9e67db9f97f..4d546f8cbcd 100644 --- a/api/src/org/labkey/api/dataiterator/EmbargoDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/EmbargoDataIterator.java @@ -13,7 +13,7 @@ /** * StatementDataIterator is already complicated enough, so the cache-ahead functionality is implemented by a separate class called * EmbargoDataIterator. This class is similar to CachingDataIterator, however, where CachingDataIterator - * caches rows that have have already been seen, EmbargoDataIterator caches rows _ahead_ and holds them back until + * caches rows that have already been seen, EmbargoDataIterator caches rows _ahead_ and holds them back until * the StatementDataIterator indicates that they have been 'saved' and may be released. */ diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index e6887f53710..bd05c45a908 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -1014,19 +1014,16 @@ protected Object convert(Object o) Object value = convertWithPrimaryColumn(o, false); if (value != null) { - _lookupResolutionType = LookupResolutionType.primaryKey; // having converted, let's not try to do it again return value; } value = convertWithRemapper(o); if (value != null) { - _lookupResolutionType = LookupResolutionType.primaryKey; // having converted, let's not try to do it again return value; } value = convertWithPrimaryColumn(o, true); - _lookupResolutionType = LookupResolutionType.primaryKey; // having converted, let's not try to do it again if (value == null) { return switch (_missing) @@ -1349,7 +1346,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro ForeignKey fk = col.getFk(); LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); - if (fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) + if (!_context.hasBeenCoerced() && fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) { // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 7bb10bfa53f..597df232c4e 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -19,7 +19,6 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import org.json.JSONArray; import org.labkey.api.assay.AssayFileWriter; import org.labkey.api.attachments.AttachmentFile; import org.labkey.api.collections.CaseInsensitiveHashMap; @@ -132,7 +131,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.Date; import java.util.HashMap; import java.util.HashSet; @@ -177,9 +175,9 @@ import static org.labkey.api.exp.query.ExpMaterialTable.Column.Units; import static org.labkey.api.query.AbstractQueryImportAction.configureLoader; import static org.labkey.experiment.api.SampleTypeServiceImpl.SampleChangeType.insert; +import static org.labkey.experiment.api.SampleTypeUpdateServiceDI.PARENT_RECOMPUTE_NAME_COL; import static org.labkey.experiment.api.SampleTypeUpdateServiceDI.PARENT_RECOMPUTE_NAME_SET; import static org.labkey.experiment.api.SampleTypeUpdateServiceDI.ROOT_RECOMPUTE_ROWID_COL; -import static org.labkey.experiment.api.SampleTypeUpdateServiceDI.PARENT_RECOMPUTE_NAME_COL; import static org.labkey.experiment.api.SampleTypeUpdateServiceDI.ROOT_RECOMPUTE_ROWID_SET; diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java index 2e6baf738de..2a19188ccb5 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java @@ -1077,7 +1077,7 @@ else if (Column.ClassId.name().equalsIgnoreCase(name)) // CoerceDataIterator to handle the lookup/alternatekeys functionality of loadRows(), // TODO check if this covers all the functionality, in particular how is alternateKeyCandidates used? di = LoggingDataIterator.wrap(new CoerceDataIterator(di, context, ExpDataClassDataTableImpl.this, false)); - + context.setHasBeenCoerced(true); TableInfo dataClassTInfo = ExpDataClassDataTableImpl.this; if (c.hasProductFolders() && !c.isProject()) { diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 3866722cba9..72e1f507548 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -1504,13 +1504,14 @@ public DataIterator getDataIterator(DataIteratorContext context) var addRequiredColsDI = new SampleUpdateAddColumnsDataIterator(new CachingDataIterator(addAliquotedFrom), materialTable, sampleType.getRowId(), columnNameMap.containsKey("lsid")); SimpleTranslator c = new _SamplesCoerceDataIterator(addRequiredColsDI, context, sampleType, materialTable); + context.setHasBeenCoerced(true); return LoggingDataIterator.wrap(c); } // CoerceDataIterator to handle the lookup/alternatekeys functionality of loadRows(), // TODO: check if this covers all the functionality, in particular how is alternateKeyCandidates used? DataIterator c = LoggingDataIterator.wrap(new _SamplesCoerceDataIterator(source, context, sampleType, materialTable)); - + context.setHasBeenCoerced(true); SimpleTranslator addColumns = new SimpleTranslator(c, context); addColumns.setDebugName("add genId and other requried columns"); Set idColNames = Sets.newCaseInsensitiveHashSet("genId"); From 1973843fc21d0d8d3ad2a2444a6d05c29653272f Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Wed, 7 May 2025 09:11:27 -0700 Subject: [PATCH 19/39] Remove unused option --- api/src/org/labkey/api/data/LookupResolutionType.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/src/org/labkey/api/data/LookupResolutionType.java b/api/src/org/labkey/api/data/LookupResolutionType.java index 092ff3bb0c4..7c210a7889b 100644 --- a/api/src/org/labkey/api/data/LookupResolutionType.java +++ b/api/src/org/labkey/api/data/LookupResolutionType.java @@ -3,9 +3,7 @@ public enum LookupResolutionType { primaryKey(true, false, true), // known that the use will always supply the pk value - alternateKey(false, true, false), // known that the use will never supply the pk value alternateThenPrimaryKey(true, true, false), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first - // TODO do we need to support this? If not, we can change the properties here to just have a "supportsAlternateKey" since alternate will always be checked first. primaryThenAlternateKey(true, true, true); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (added for compatibility; prefer the other options) final boolean _useAlternateKey; From b71bd510846b225a32051298f40eb0e197cbae7c Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 8 May 2025 16:36:36 -0700 Subject: [PATCH 20/39] If we've coerced our data, let's first try primary key but still allow for alternate key (for trigger scripts?) --- api/src/org/labkey/api/dataiterator/SimpleTranslator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index bd05c45a908..f350aa4928d 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -1346,7 +1346,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro ForeignKey fk = col.getFk(); LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); - if (!_context.hasBeenCoerced() && fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) + if (fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) { // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); @@ -1356,7 +1356,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro missing = col.isRequired() || hasValidator || !lookupResolutionType.usePrimaryKey() ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; // For enum tables, we should not be using number names for the values, so we will look up by primary key first (resolving rowIds) then alternate. // This assures the type is an Integer when a rowId is given (e.g., from a row read from the database), not a string. - if (fk.getLookupTableInfo() instanceof EnumTableInfo) + if (fk.getLookupTableInfo() instanceof EnumTableInfo || _context.hasBeenCoerced()) lookupResolutionType = LookupResolutionType.primaryThenAlternateKey; c = new RemapPostConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); } From 3619f14b37d0eac34c9f4d3e99d3ce831da9095d Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 8 May 2025 16:43:30 -0700 Subject: [PATCH 21/39] typo --- api/src/org/labkey/api/data/LookupResolutionType.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/data/LookupResolutionType.java b/api/src/org/labkey/api/data/LookupResolutionType.java index 7c210a7889b..84995dfab2c 100644 --- a/api/src/org/labkey/api/data/LookupResolutionType.java +++ b/api/src/org/labkey/api/data/LookupResolutionType.java @@ -2,7 +2,7 @@ public enum LookupResolutionType { - primaryKey(true, false, true), // known that the use will always supply the pk value + primaryKey(true, false, true), // known that the user will always supply the pk value alternateThenPrimaryKey(true, true, false), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first primaryThenAlternateKey(true, true, true); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (added for compatibility; prefer the other options) From 75496006061adb8ab7a384162dd7eede0adb19d0 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 8 May 2025 16:45:30 -0700 Subject: [PATCH 22/39] Don't need usePrimaryKey --- .../org/labkey/api/data/LookupResolutionType.java | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/api/src/org/labkey/api/data/LookupResolutionType.java b/api/src/org/labkey/api/data/LookupResolutionType.java index 84995dfab2c..21ce0123a84 100644 --- a/api/src/org/labkey/api/data/LookupResolutionType.java +++ b/api/src/org/labkey/api/data/LookupResolutionType.java @@ -2,17 +2,15 @@ public enum LookupResolutionType { - primaryKey(true, false, true), // known that the user will always supply the pk value - alternateThenPrimaryKey(true, true, false), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first - primaryThenAlternateKey(true, true, true); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (added for compatibility; prefer the other options) + primaryKey(false, true), // known that the user will always supply the pk value + alternateThenPrimaryKey(true, false), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first + primaryThenAlternateKey(true, true); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (used for internal purposes) final boolean _useAlternateKey; - final boolean _usePrimaryKey; final boolean _usePrimaryFirst; - LookupResolutionType(boolean usePrimaryKey, boolean useAlternateKey, boolean usePrimaryFirst) + LookupResolutionType(boolean useAlternateKey, boolean usePrimaryFirst) { - _usePrimaryKey = usePrimaryKey; _useAlternateKey = useAlternateKey; _usePrimaryFirst = usePrimaryFirst; } @@ -22,11 +20,6 @@ public boolean useAlternateKey() return _useAlternateKey; } - public boolean usePrimaryKey() - { - return _usePrimaryKey; - } - public boolean usePrimaryFirst() { return _usePrimaryFirst; From af15eeb898ae02c974e66ee54b6894fb37cffb72 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 8 May 2025 19:05:53 -0700 Subject: [PATCH 23/39] Remove reference to deleted method. --- api/src/org/labkey/api/dataiterator/SimpleTranslator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index f350aa4928d..886cbb26053 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -961,7 +961,7 @@ public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, fin private Object convertWithPrimaryColumn(Object o, boolean primaryTried) { - if (_lookupResolutionType.usePrimaryKey() && _lookupResolutionType.usePrimaryFirst() != primaryTried) + if (_lookupResolutionType.usePrimaryFirst() != primaryTried) { try { @@ -1353,7 +1353,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro RemapMissingBehavior missing = remapMissingBehavior; if (missing == null) - missing = col.isRequired() || hasValidator || !lookupResolutionType.usePrimaryKey() ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; + missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; // For enum tables, we should not be using number names for the values, so we will look up by primary key first (resolving rowIds) then alternate. // This assures the type is an Integer when a rowId is given (e.g., from a row read from the database), not a string. if (fk.getLookupTableInfo() instanceof EnumTableInfo || _context.hasBeenCoerced()) From 42afcfcda236c252f8d6cc7902a8004ce6b6624e Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Fri, 9 May 2025 09:25:58 -0700 Subject: [PATCH 24/39] Add new setHasBeenCoerced --- .../org/labkey/api/dataiterator/TriggerDataBuilderHelper.java | 1 + 1 file changed, 1 insertion(+) diff --git a/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java b/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java index 9ee30d5c45b..3f5e5e927d7 100644 --- a/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java +++ b/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java @@ -145,6 +145,7 @@ public DataIterator getDataIterator(DataIteratorContext context) boolean skipExistingRecord = !context.getInsertOption().allowUpdate || mergeKeys == null || isNewFolderImport; DataIterator coerce = new CoerceDataIterator(pre, context, _target, !context.getInsertOption().updateOnly); + context.setHasBeenCoerced(true); coerce = LoggingDataIterator.wrap(coerce); if (skipExistingRecord) From 248866f351b4030c2e86d880e9f436a0fb47e2e3 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Wed, 14 May 2025 10:04:08 -0700 Subject: [PATCH 25/39] Update logic to fail if remapping before trigger does not work --- .../api/dataiterator/CoerceDataIterator.java | 2 +- .../api/dataiterator/DataIteratorContext.java | 10 ++-- .../api/dataiterator/SimpleTranslator.java | 48 ++++++++----------- .../TriggerDataBuilderHelper.java | 2 +- .../api/ExpDataClassDataTableImpl.java | 2 +- .../api/SampleTypeUpdateServiceDI.java | 6 +-- 6 files changed, 31 insertions(+), 39 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java b/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java index c3dc5414389..7841977adca 100644 --- a/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java @@ -68,7 +68,7 @@ void init(TableInfo target, boolean useImportAliases, boolean outputAllColumns) else if (to.getFk() instanceof MultiValuedForeignKey) addColumn(to.getName(), i); // pass-through multi-value columns -- converting will stringify a collection else - addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), RemapMissingBehavior.OriginalValue); + addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), RemapMissingBehavior.Error); } else { diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java index 35efb32d9d4..da01ca24dad 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java @@ -61,7 +61,7 @@ public class DataIteratorContext private final Set _dontUpdateColumnNames = new CaseInsensitiveHashSet(); private final Set _alternateKeys = new CaseInsensitiveHashSet(); private String _dataSource; - private boolean _hasBeenCoerced = false; + private boolean _hasBeenRemapped = false; private final Map _responseInfo = new HashMap<>(); // information from the import/loadRows context to be passed back to the API response object private Logger _logger; @@ -175,14 +175,14 @@ public void setLookupResolutionType(LookupResolutionType lookupResolutionType) _lookupResolutionType = lookupResolutionType; } - public boolean hasBeenCoerced() + public boolean hasBeenRemapped() { - return _hasBeenCoerced; + return _hasBeenRemapped; } - public void setHasBeenCoerced(boolean hasBeenCoerced) + public void setHasBeenRemapped(boolean hasBeenRemapped) { - _hasBeenCoerced = hasBeenCoerced; + _hasBeenRemapped = hasBeenRemapped; } /** When true, allow importing lookup columns by the lookup table's alternate key instead of by primary key. */ diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 886cbb26053..48c87f96480 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -959,33 +959,29 @@ public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, fin _lookupResolutionType = lookupResolutionType; } - private Object convertWithPrimaryColumn(Object o, boolean primaryTried) + private Object convertWithPrimaryColumn(Object o) { - if (_lookupResolutionType.usePrimaryFirst() != primaryTried) + try { - try + // _convertCol here will be the column for the primary key type + Object value = _convertCol.convert(o); + ForeignKey fk = _toCol.getFk(); + // issue 40909 : allow String columns to resolve lookups by alternate key if the raw lookup fails to resolve + if (fk != null && Objects.equals(o, value) && _toCol.getJdbcType().isText()) { - // _convertCol here will be the column for the primary key type - Object value = _convertCol.convert(o); - ForeignKey fk = _toCol.getFk(); - // issue 40909 : allow String columns to resolve lookups by alternate key if the raw lookup fails to resolve - if (fk != null && Objects.equals(o, value) && _toCol.getJdbcType().isText()) + if (_remapper.getPkColumn().getJdbcType().isText()) { - if (_remapper.getPkColumn().getJdbcType().isText()) - { - _remapper.setIncludePkLookup(true); - Object remappedValue = _remapper.mappedValue(o); - value = remappedValue != null ? remappedValue : value; - } + _remapper.setIncludePkLookup(true); + Object remappedValue = _remapper.mappedValue(o); + value = remappedValue != null ? remappedValue : value; } - return value; - } - catch (ConversionException ex) - { - return null; } + return value; + } + catch (ConversionException ex) + { + return null; } - return null; } private Object convertWithRemapper(Object o) @@ -1011,11 +1007,7 @@ protected Object convert(Object o) if (o == null) return null; - Object value = convertWithPrimaryColumn(o, false); - if (value != null) - { - return value; - } + Object value; value = convertWithRemapper(o); if (value != null) @@ -1023,7 +1015,7 @@ protected Object convert(Object o) return value; } - value = convertWithPrimaryColumn(o, true); + value = convertWithPrimaryColumn(o); if (value == null) { return switch (_missing) @@ -1346,7 +1338,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro ForeignKey fk = col.getFk(); LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); - if (fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) + if (!_context.hasBeenRemapped() && fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) { // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); @@ -1356,7 +1348,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; // For enum tables, we should not be using number names for the values, so we will look up by primary key first (resolving rowIds) then alternate. // This assures the type is an Integer when a rowId is given (e.g., from a row read from the database), not a string. - if (fk.getLookupTableInfo() instanceof EnumTableInfo || _context.hasBeenCoerced()) + if (fk.getLookupTableInfo() instanceof EnumTableInfo) lookupResolutionType = LookupResolutionType.primaryThenAlternateKey; c = new RemapPostConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); } diff --git a/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java b/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java index 3f5e5e927d7..e547e9f34b7 100644 --- a/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java +++ b/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java @@ -145,7 +145,7 @@ public DataIterator getDataIterator(DataIteratorContext context) boolean skipExistingRecord = !context.getInsertOption().allowUpdate || mergeKeys == null || isNewFolderImport; DataIterator coerce = new CoerceDataIterator(pre, context, _target, !context.getInsertOption().updateOnly); - context.setHasBeenCoerced(true); + context.setHasBeenRemapped(true); coerce = LoggingDataIterator.wrap(coerce); if (skipExistingRecord) diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java index 2a19188ccb5..81333be39bd 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java @@ -1077,7 +1077,7 @@ else if (Column.ClassId.name().equalsIgnoreCase(name)) // CoerceDataIterator to handle the lookup/alternatekeys functionality of loadRows(), // TODO check if this covers all the functionality, in particular how is alternateKeyCandidates used? di = LoggingDataIterator.wrap(new CoerceDataIterator(di, context, ExpDataClassDataTableImpl.this, false)); - context.setHasBeenCoerced(true); + context.setHasBeenRemapped(true); TableInfo dataClassTInfo = ExpDataClassDataTableImpl.this; if (c.hasProductFolders() && !c.isProject()) { diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index f2f8c2f2606..2a031f5eb1d 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -1499,14 +1499,14 @@ public DataIterator getDataIterator(DataIteratorContext context) var addRequiredColsDI = new SampleUpdateAddColumnsDataIterator(new CachingDataIterator(addAliquotedFrom), materialTable, sampleType.getRowId(), columnNameMap.containsKey("lsid")); SimpleTranslator c = new _SamplesCoerceDataIterator(addRequiredColsDI, context, sampleType, materialTable); - context.setHasBeenCoerced(true); + context.setHasBeenRemapped(true); return LoggingDataIterator.wrap(c); } // CoerceDataIterator to handle the lookup/alternatekeys functionality of loadRows(), // TODO: check if this covers all the functionality, in particular how is alternateKeyCandidates used? DataIterator c = LoggingDataIterator.wrap(new _SamplesCoerceDataIterator(source, context, sampleType, materialTable)); - context.setHasBeenCoerced(true); + context.setHasBeenRemapped(true); SimpleTranslator addColumns = new SimpleTranslator(c, context); addColumns.setDebugName("add genId and other requried columns"); Set idColNames = Sets.newCaseInsensitiveHashSet("genId"); @@ -1932,7 +1932,7 @@ private void _addConvertColumn(String name, int fromIndex, JdbcType toType, Fore private void _addConvertColumn(ColumnInfo col, int fromIndex, int derivationDataColInd, boolean isAliquotField) { - SimpleConvertColumn c = createConvertColumn(col, fromIndex, RemapMissingBehavior.OriginalValue); + SimpleConvertColumn c = createConvertColumn(col, fromIndex, RemapMissingBehavior.Error); c = new DerivationScopedConvertColumn(fromIndex, c, derivationDataColInd, isAliquotField, String.format(INVALID_ALIQUOT_PROPERTY, col.getName()), String.format(INVALID_NONALIQUOT_PROPERTY, col.getName())); addColumn(col, c); From 31d50b0eb17a962bd2c9079a3e3e0f496edcc3e9 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 15 May 2025 09:19:44 -0700 Subject: [PATCH 26/39] Reset remapping flag for cross-folder and cross-type imports --- experiment/src/org/labkey/experiment/ExpDataIterators.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index c86320a3efe..c9525c584cf 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -2598,6 +2598,7 @@ private int _importPartition(TypeData typeData) return totalRowCount; } totalRowCount += _importSplitFile(typeData, containerSplitFile.getValue(), splitContainer, dataTable); + _context.setHasBeenRemapped(false); } return totalRowCount; } @@ -2632,7 +2633,7 @@ public boolean next() throws BatchValidationException for (TypeData typeData : typeFolderData.values()) { writeRowsToFile(typeData); // write the last rows that have been collected since the last write, if any - + _context.setHasBeenRemapped(false); if (!_context.getErrors().hasErrors()) // Issue 48402: Stop early since the transaction may have been aborted _importPartition(typeData); } From 0069e009341aee580fb31a550a4bb2107273d9a0 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 15 May 2025 11:21:19 -0700 Subject: [PATCH 27/39] Move where we set and use the `hasBeenRemapped` value --- .../api/dataiterator/CoerceDataIterator.java | 2 +- .../api/dataiterator/SimpleTranslator.java | 33 ++++++++++--------- .../StandardDataIteratorBuilder.java | 2 +- .../labkey/experiment/ExpDataIterators.java | 2 -- .../api/SampleTypeUpdateServiceDI.java | 2 +- .../model/DatasetDataIteratorBuilder.java | 2 +- 6 files changed, 22 insertions(+), 21 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java b/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java index 7841977adca..6431d03cceb 100644 --- a/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java @@ -68,7 +68,7 @@ void init(TableInfo target, boolean useImportAliases, boolean outputAllColumns) else if (to.getFk() instanceof MultiValuedForeignKey) addColumn(to.getName(), i); // pass-through multi-value columns -- converting will stringify a collection else - addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), RemapMissingBehavior.Error); + addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), RemapMissingBehavior.Error, false); } else { diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 48c87f96480..d51d1aafcf0 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -1261,10 +1261,11 @@ public int addConvertColumn(ColumnInfo col, int fromIndex) * @param fromIndex Source column to create the output column from. * @param mvIndex Missing value column index. * @param remapMissingBehavior The behavior desired when remapping fails. If null, indicate an error if the column is required or null if not required. + * @param hasBeenRemapped Indicates if remapping of lookup columns has happened or not */ - public int addConvertColumn(ColumnInfo col, int fromIndex, int mvIndex, @Nullable RemapMissingBehavior remapMissingBehavior) + private int addConvertColumn(ColumnInfo col, int fromIndex, int mvIndex, @Nullable RemapMissingBehavior remapMissingBehavior, boolean hasBeenRemapped) { - SimpleConvertColumn c = createConvertColumn(col, fromIndex, mvIndex, null, null, col.getJdbcType(), remapMissingBehavior); + SimpleConvertColumn c = createConvertColumn(col, fromIndex, mvIndex, null, null, col.getJdbcType(), remapMissingBehavior, hasBeenRemapped); return addColumn(col, c); } @@ -1283,8 +1284,9 @@ public int addConvertColumn(ColumnInfo col, int fromIndex, int mvIndex, @Nullabl * @param toType Convert the source data values to this type. * @param toFk When isAllowImportLookupByAlternateKey is turned on, remap lookup values using the foreign key if there is a conversion failure. * @param remapMissingBehavior The behavior desired when remapping fails. If null, indicate an error if the column is required or null if not required. + * @param hasBeenRemapped Indicates if remapping of lookup columns has happened or not */ - public int addConvertColumn(String name, int fromIndex, JdbcType toType, @Nullable ForeignKey toFk, @Nullable RemapMissingBehavior remapMissingBehavior) + public int addConvertColumn(String name, int fromIndex, JdbcType toType, @Nullable ForeignKey toFk, @Nullable RemapMissingBehavior remapMissingBehavior, boolean hasBeenRemapped) { var col = new BaseColumnInfo(_data.getColumnInfo(fromIndex)); col.setName(name); @@ -1292,7 +1294,7 @@ public int addConvertColumn(String name, int fromIndex, JdbcType toType, @Nullab if (toFk != null) col.setFk(toFk); - return addConvertColumn(col, fromIndex, fromIndex, remapMissingBehavior); + return addConvertColumn(col, fromIndex, fromIndex, remapMissingBehavior, hasBeenRemapped); } /** @@ -1310,19 +1312,20 @@ public int addConvertColumn(String name, int fromIndex, JdbcType toType, @Nullab * @param pd PropertyDescriptor used for missing value enabled-ness. * @param pt Convert the source data values to this type. * @param remapMissingBehavior The behavior desired when remapping fails. If null, indicate an error if the column is required or null if not required. + * @param hasBeenRemapped Indicates if remapping of lookup columns has happened or not */ - public int addConvertColumn(@NotNull ColumnInfo col, int fromIndex, int mvIndex, @Nullable PropertyDescriptor pd, @Nullable PropertyType pt, @Nullable RemapMissingBehavior remapMissingBehavior) + public int addConvertColumn(@NotNull ColumnInfo col, int fromIndex, int mvIndex, @Nullable PropertyDescriptor pd, @Nullable PropertyType pt, @Nullable RemapMissingBehavior remapMissingBehavior, boolean hasBeenRemapped) { - SimpleConvertColumn c = createConvertColumn(col, fromIndex, mvIndex, pd, pt, col.getJdbcType(), remapMissingBehavior); + SimpleConvertColumn c = createConvertColumn(col, fromIndex, mvIndex, pd, pt, col.getJdbcType(), remapMissingBehavior, hasBeenRemapped); return addColumn(col, c); } public SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fromIndex, @Nullable RemapMissingBehavior remapMissingBehavior) { - return createConvertColumn(col, fromIndex, NO_MV_INDEX, null, col.getPropertyType(), col.getJdbcType(), remapMissingBehavior); + return createConvertColumn(col, fromIndex, NO_MV_INDEX, null, col.getPropertyType(), col.getJdbcType(), remapMissingBehavior, false); } - private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fromIndex, int mvIndex, @Nullable PropertyDescriptor pd, @Nullable PropertyType pt, @Nullable JdbcType type, @Nullable RemapMissingBehavior remapMissingBehavior) + private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fromIndex, int mvIndex, @Nullable PropertyDescriptor pd, @Nullable PropertyType pt, @Nullable JdbcType type, @Nullable RemapMissingBehavior remapMissingBehavior, boolean hasBeenRemapped) { final String name = col.getName(); @@ -1338,7 +1341,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro ForeignKey fk = col.getFk(); LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); - if (!_context.hasBeenRemapped() && fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) + if (!hasBeenRemapped && fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) { // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); @@ -2107,7 +2110,7 @@ public void convertTest() throws Exception DataIteratorContext context = new DataIteratorContext(); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("IntNotNull", 1, JdbcType.INTEGER, null, null); + t.addConvertColumn("IntNotNull", 1, JdbcType.INTEGER, null, null, false); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2127,7 +2130,7 @@ public void convertTest() throws Exception context.setVerbose(true); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Text", 2, JdbcType.INTEGER, null, null); + t.addConvertColumn("Text", 2, JdbcType.INTEGER, null, null, false); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2148,7 +2151,7 @@ public void convertTest() throws Exception context.setVerbose(true); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Text", 2, JdbcType.INTEGER, null, null); + t.addConvertColumn("Text", 2, JdbcType.INTEGER, null, null, false); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2202,7 +2205,7 @@ public void convertRemapTest() throws Exception context.setLookupResolutionType(LookupResolutionType.primaryThenAlternateKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue); + t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue, false); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2238,7 +2241,7 @@ public void convertRemapTest() throws Exception context.setLookupResolutionType(LookupResolutionType.primaryThenAlternateKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null); + t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null, false); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2274,7 +2277,7 @@ public void convertRemapTest() throws Exception context.setLookupResolutionType(LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null); + t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null, false); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); diff --git a/api/src/org/labkey/api/dataiterator/StandardDataIteratorBuilder.java b/api/src/org/labkey/api/dataiterator/StandardDataIteratorBuilder.java index 6b262774b99..696c4a756ba 100644 --- a/api/src/org/labkey/api/dataiterator/StandardDataIteratorBuilder.java +++ b/api/src/org/labkey/api/dataiterator/StandardDataIteratorBuilder.java @@ -280,7 +280,7 @@ public DataIterator getDataIterator(DataIteratorContext context) if (null == pair.target || isAttachment) convert.addColumn(pair.indexFrom); else - convert.addConvertColumn(pair.target, pair.indexFrom, pair.indexMv, pd, pt, pair.target.getRemapMissingBehavior()); + convert.addConvertColumn(pair.target, pair.indexFrom, pair.indexMv, pd, pt, pair.target.getRemapMissingBehavior(), context.hasBeenRemapped()); } // diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index c9525c584cf..3d6e6f5102f 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -2598,7 +2598,6 @@ private int _importPartition(TypeData typeData) return totalRowCount; } totalRowCount += _importSplitFile(typeData, containerSplitFile.getValue(), splitContainer, dataTable); - _context.setHasBeenRemapped(false); } return totalRowCount; } @@ -2633,7 +2632,6 @@ public boolean next() throws BatchValidationException for (TypeData typeData : typeFolderData.values()) { writeRowsToFile(typeData); // write the last rows that have been collected since the last write, if any - _context.setHasBeenRemapped(false); if (!_context.getErrors().hasErrors()) // Issue 48402: Stop early since the transaction may have been aborted _importPartition(typeData); } diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 2a031f5eb1d..f09965259c8 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -1886,7 +1886,7 @@ else if (name.equalsIgnoreCase("StoredAmount")) if (isScopedField) _addConvertColumn(name, i, to.getJdbcType(), to.getFk(), aliquotedFromDataColInd, scopedFields.get(name)); else - addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), to.getRemapMissingBehavior()); + addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), to.getRemapMissingBehavior(), false); } } else diff --git a/study/src/org/labkey/study/model/DatasetDataIteratorBuilder.java b/study/src/org/labkey/study/model/DatasetDataIteratorBuilder.java index 9ef718edd83..0a98b7e68ed 100644 --- a/study/src/org/labkey/study/model/DatasetDataIteratorBuilder.java +++ b/study/src/org/labkey/study/model/DatasetDataIteratorBuilder.java @@ -238,7 +238,7 @@ public DataIterator getDataIterator(DataIteratorContext context) else if (match == keyColumn && _datasetDefinition.getKeyManagementType() == Dataset.KeyManagementType.None) { // usually we let DataIterator handle convert, but we need to convert for consistent _key/lsid generation - out = it.addConvertColumn(match.getName(), in, match.getJdbcType(), null, null != match.getMvColumnName() ? SimpleTranslator.RemapMissingBehavior.OriginalValue : null); + out = it.addConvertColumn(match.getName(), in, match.getJdbcType(), null, null != match.getMvColumnName() ? SimpleTranslator.RemapMissingBehavior.OriginalValue : null, false); } else if (match.getPropertyType() == PropertyType.FILE_LINK) { From 9ea95464e9bcc14aa2a311f38f7a970ccb3ac8c2 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 15 May 2025 16:41:46 -0700 Subject: [PATCH 28/39] Use non-deprecated method --- api/src/org/labkey/api/dataiterator/SimpleTranslator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index d51d1aafcf0..80336963320 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -184,7 +184,7 @@ protected Object addConversionException(String fieldName, @Nullable Object value else if (null != value && null != target) msg = ConvertHelper.getStandardConversionErrorMessage(value, fieldName, target.getJavaClass()); else if (null != x) - msg = StringUtils.defaultString(x.getMessage(), x.toString()); + msg = Objects.toString(x.getMessage(), x.toString()); else msg = "Could not convert value"; addFieldError(fieldName, msg); From 00281294afee0720e41a4718bfc637294141c0d4 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 15 May 2025 16:42:02 -0700 Subject: [PATCH 29/39] Don't override `addConversionException` --- api/src/org/labkey/api/dataiterator/CoerceDataIterator.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java b/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java index 6431d03cceb..f3895d25b58 100644 --- a/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java @@ -88,10 +88,4 @@ else if (to.getFk() instanceof MultiValuedForeignKey) } } } - - @Override - protected Object addConversionException(String fieldName, Object value, JdbcType target, Exception x) - { - return value; - } } From 42eaf3f178b3c9a81aa34128bd94156e57f2c1b6 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Fri, 16 May 2025 06:46:45 -0700 Subject: [PATCH 30/39] Remove unnecessary primaryThenAlternate option --- .../org/labkey/api/data/LookupResolutionType.java | 13 +++---------- .../labkey/api/dataiterator/SimpleTranslator.java | 6 +++--- .../labkey/api/query/AbstractQueryImportAction.java | 2 +- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/api/src/org/labkey/api/data/LookupResolutionType.java b/api/src/org/labkey/api/data/LookupResolutionType.java index 21ce0123a84..7801cbe77b5 100644 --- a/api/src/org/labkey/api/data/LookupResolutionType.java +++ b/api/src/org/labkey/api/data/LookupResolutionType.java @@ -2,17 +2,14 @@ public enum LookupResolutionType { - primaryKey(false, true), // known that the user will always supply the pk value - alternateThenPrimaryKey(true, false), // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first - primaryThenAlternateKey(true, true); // this most closely matches previous behavior when allowImportLookupByAlternateKey was true (used for internal purposes) + primaryKey(false), // known that the user will always supply the pk value + alternateThenPrimaryKey(true); // If there is a situation where it's sometimes a primary and sometimes an alternate key, check for the alternate key first final boolean _useAlternateKey; - final boolean _usePrimaryFirst; - LookupResolutionType(boolean useAlternateKey, boolean usePrimaryFirst) + LookupResolutionType(boolean useAlternateKey) { _useAlternateKey = useAlternateKey; - _usePrimaryFirst = usePrimaryFirst; } public boolean useAlternateKey() @@ -20,8 +17,4 @@ public boolean useAlternateKey() return _useAlternateKey; } - public boolean usePrimaryFirst() - { - return _usePrimaryFirst; - } } diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 80336963320..950a320b9c6 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -1352,7 +1352,7 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro // For enum tables, we should not be using number names for the values, so we will look up by primary key first (resolving rowIds) then alternate. // This assures the type is an Integer when a rowId is given (e.g., from a row read from the database), not a string. if (fk.getLookupTableInfo() instanceof EnumTableInfo) - lookupResolutionType = LookupResolutionType.primaryThenAlternateKey; + lookupResolutionType = LookupResolutionType.alternateThenPrimaryKey; c = new RemapPostConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); } @@ -2202,7 +2202,7 @@ public void convertRemapTest() throws Exception // don't throw error if remap can't be resolved { DataIteratorContext context = new DataIteratorContext(); - context.setLookupResolutionType(LookupResolutionType.primaryThenAlternateKey); + context.setLookupResolutionType(LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue, false); @@ -2238,7 +2238,7 @@ public void convertRemapTest() throws Exception // don't throw error if remap can't be resolved { DataIteratorContext context = new DataIteratorContext(); - context.setLookupResolutionType(LookupResolutionType.primaryThenAlternateKey); + context.setLookupResolutionType(LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null, false); diff --git a/api/src/org/labkey/api/query/AbstractQueryImportAction.java b/api/src/org/labkey/api/query/AbstractQueryImportAction.java index 9766086d2ed..bbd97e36b83 100644 --- a/api/src/org/labkey/api/query/AbstractQueryImportAction.java +++ b/api/src/org/labkey/api/query/AbstractQueryImportAction.java @@ -354,7 +354,7 @@ protected LookupResolutionType getLookupResolutionType() if (Boolean.valueOf(paramValue)) return LookupResolutionType.alternateThenPrimaryKey; else - return LookupResolutionType.primaryThenAlternateKey; + return LookupResolutionType.primaryKey; } } return LookupResolutionType.valueOf(paramValue); From 0dd62da54dd808f29be08459cecefe4d11e5f404 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 19 May 2025 15:10:53 -0700 Subject: [PATCH 31/39] hasBeenRemapped -> withLookupRemapping and don't use Null as RemapMissingBehavior --- .../api/dataiterator/CoerceDataIterator.java | 3 +- .../api/dataiterator/DataIteratorContext.java | 10 ++--- .../api/dataiterator/SimpleTranslator.java | 44 +++++++++---------- .../StandardDataIteratorBuilder.java | 2 +- .../TriggerDataBuilderHelper.java | 2 +- .../api/ExpDataClassDataTableImpl.java | 2 +- .../api/SampleTypeUpdateServiceDI.java | 6 +-- .../model/DatasetDataIteratorBuilder.java | 2 +- 8 files changed, 33 insertions(+), 38 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java b/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java index f3895d25b58..f900733f5eb 100644 --- a/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java +++ b/api/src/org/labkey/api/dataiterator/CoerceDataIterator.java @@ -17,7 +17,6 @@ import org.labkey.api.collections.CaseInsensitiveHashSet; import org.labkey.api.data.ColumnInfo; -import org.labkey.api.data.JdbcType; import org.labkey.api.data.MultiValuedForeignKey; import org.labkey.api.data.TableInfo; import org.labkey.api.exp.PropertyType; @@ -68,7 +67,7 @@ void init(TableInfo target, boolean useImportAliases, boolean outputAllColumns) else if (to.getFk() instanceof MultiValuedForeignKey) addColumn(to.getName(), i); // pass-through multi-value columns -- converting will stringify a collection else - addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), RemapMissingBehavior.Error, false); + addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), RemapMissingBehavior.Error, true); } else { diff --git a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java index da01ca24dad..12713d9605b 100644 --- a/api/src/org/labkey/api/dataiterator/DataIteratorContext.java +++ b/api/src/org/labkey/api/dataiterator/DataIteratorContext.java @@ -61,7 +61,7 @@ public class DataIteratorContext private final Set _dontUpdateColumnNames = new CaseInsensitiveHashSet(); private final Set _alternateKeys = new CaseInsensitiveHashSet(); private String _dataSource; - private boolean _hasBeenRemapped = false; + private boolean _withLookupRemapping = true; private final Map _responseInfo = new HashMap<>(); // information from the import/loadRows context to be passed back to the API response object private Logger _logger; @@ -175,14 +175,14 @@ public void setLookupResolutionType(LookupResolutionType lookupResolutionType) _lookupResolutionType = lookupResolutionType; } - public boolean hasBeenRemapped() + public boolean isWithLookupRemapping() { - return _hasBeenRemapped; + return _withLookupRemapping; } - public void setHasBeenRemapped(boolean hasBeenRemapped) + public void setWithLookupRemapping(boolean withLookupRemapping) { - _hasBeenRemapped = hasBeenRemapped; + _withLookupRemapping = withLookupRemapping; } /** When true, allow importing lookup columns by the lookup table's alternate key instead of by primary key. */ diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 2ff26c40c42..bbd05c7ec0e 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -1261,11 +1261,11 @@ public int addConvertColumn(ColumnInfo col, int fromIndex) * @param fromIndex Source column to create the output column from. * @param mvIndex Missing value column index. * @param remapMissingBehavior The behavior desired when remapping fails. If null, indicate an error if the column is required or null if not required. - * @param hasBeenRemapped Indicates if remapping of lookup columns has happened or not + * @param withLookupRemapping Indicates if remapping of lookup columns should be attempted or not */ - private int addConvertColumn(ColumnInfo col, int fromIndex, int mvIndex, @Nullable RemapMissingBehavior remapMissingBehavior, boolean hasBeenRemapped) + private int addConvertColumn(ColumnInfo col, int fromIndex, int mvIndex, @Nullable RemapMissingBehavior remapMissingBehavior, boolean withLookupRemapping) { - SimpleConvertColumn c = createConvertColumn(col, fromIndex, mvIndex, null, null, col.getJdbcType(), remapMissingBehavior, hasBeenRemapped); + SimpleConvertColumn c = createConvertColumn(col, fromIndex, mvIndex, null, null, col.getJdbcType(), remapMissingBehavior, withLookupRemapping); return addColumn(col, c); } @@ -1283,10 +1283,10 @@ private int addConvertColumn(ColumnInfo col, int fromIndex, int mvIndex, @Nullab * @param fromIndex Source column to create the output column from and pull data from. * @param toType Convert the source data values to this type. * @param toFk When isAllowImportLookupByAlternateKey is turned on, remap lookup values using the foreign key if there is a conversion failure. - * @param remapMissingBehavior The behavior desired when remapping fails. If null, indicate an error if the column is required or null if not required. - * @param hasBeenRemapped Indicates if remapping of lookup columns has happened or not + * @param remapMissingBehavior The behavior desired when remapping fails. If null, indicate an error if the column is required or original value if not required. + * @param withLookupRemapping Indicates if we should attempt to resolve lookups or not */ - public int addConvertColumn(String name, int fromIndex, JdbcType toType, @Nullable ForeignKey toFk, @Nullable RemapMissingBehavior remapMissingBehavior, boolean hasBeenRemapped) + public int addConvertColumn(String name, int fromIndex, JdbcType toType, @Nullable ForeignKey toFk, @Nullable RemapMissingBehavior remapMissingBehavior, boolean withLookupRemapping) { var col = new BaseColumnInfo(_data.getColumnInfo(fromIndex)); col.setName(name); @@ -1294,7 +1294,7 @@ public int addConvertColumn(String name, int fromIndex, JdbcType toType, @Nullab if (toFk != null) col.setFk(toFk); - return addConvertColumn(col, fromIndex, fromIndex, remapMissingBehavior, hasBeenRemapped); + return addConvertColumn(col, fromIndex, fromIndex, remapMissingBehavior, withLookupRemapping); } /** @@ -1312,20 +1312,20 @@ public int addConvertColumn(String name, int fromIndex, JdbcType toType, @Nullab * @param pd PropertyDescriptor used for missing value enabled-ness. * @param pt Convert the source data values to this type. * @param remapMissingBehavior The behavior desired when remapping fails. If null, indicate an error if the column is required or null if not required. - * @param hasBeenRemapped Indicates if remapping of lookup columns has happened or not + * @param withLookupRemapping Indicates if we should try to remap lookups during the conversion or not. */ - public int addConvertColumn(@NotNull ColumnInfo col, int fromIndex, int mvIndex, @Nullable PropertyDescriptor pd, @Nullable PropertyType pt, @Nullable RemapMissingBehavior remapMissingBehavior, boolean hasBeenRemapped) + public int addConvertColumn(@NotNull ColumnInfo col, int fromIndex, int mvIndex, @Nullable PropertyDescriptor pd, @Nullable PropertyType pt, @Nullable RemapMissingBehavior remapMissingBehavior, boolean withLookupRemapping) { - SimpleConvertColumn c = createConvertColumn(col, fromIndex, mvIndex, pd, pt, col.getJdbcType(), remapMissingBehavior, hasBeenRemapped); + SimpleConvertColumn c = createConvertColumn(col, fromIndex, mvIndex, pd, pt, col.getJdbcType(), remapMissingBehavior, withLookupRemapping); return addColumn(col, c); } public SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fromIndex, @Nullable RemapMissingBehavior remapMissingBehavior) { - return createConvertColumn(col, fromIndex, NO_MV_INDEX, null, col.getPropertyType(), col.getJdbcType(), remapMissingBehavior, false); + return createConvertColumn(col, fromIndex, NO_MV_INDEX, null, col.getPropertyType(), col.getJdbcType(), remapMissingBehavior, true); } - private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fromIndex, int mvIndex, @Nullable PropertyDescriptor pd, @Nullable PropertyType pt, @Nullable JdbcType type, @Nullable RemapMissingBehavior remapMissingBehavior, boolean hasBeenRemapped) + private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fromIndex, int mvIndex, @Nullable PropertyDescriptor pd, @Nullable PropertyType pt, @Nullable JdbcType type, @Nullable RemapMissingBehavior remapMissingBehavior, boolean withLookupRemapping) { final String name = col.getName(); @@ -1341,18 +1341,14 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro ForeignKey fk = col.getFk(); LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); - if (!hasBeenRemapped && fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) + if (withLookupRemapping && fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) { // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); RemapMissingBehavior missing = remapMissingBehavior; if (missing == null) - missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; - // For enum tables, we should not be using number names for the values, so we will look up by primary key first (resolving rowIds) then alternate. - // This assures the type is an Integer when a rowId is given (e.g., from a row read from the database), not a string. - if (fk.getLookupTableInfo() instanceof EnumTableInfo) - lookupResolutionType = LookupResolutionType.alternateThenPrimaryKey; + missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.OriginalValue; c = new RemapPostConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); } @@ -2110,7 +2106,7 @@ public void convertTest() throws Exception DataIteratorContext context = new DataIteratorContext(); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("IntNotNull", 1, JdbcType.INTEGER, null, null, false); + t.addConvertColumn("IntNotNull", 1, JdbcType.INTEGER, null, null, true); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2130,7 +2126,7 @@ public void convertTest() throws Exception context.setVerbose(true); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Text", 2, JdbcType.INTEGER, null, null, false); + t.addConvertColumn("Text", 2, JdbcType.INTEGER, null, null, true); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2151,7 +2147,7 @@ public void convertTest() throws Exception context.setVerbose(true); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Text", 2, JdbcType.INTEGER, null, null, false); + t.addConvertColumn("Text", 2, JdbcType.INTEGER, null, null, true); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2205,7 +2201,7 @@ public void convertRemapTest() throws Exception context.setLookupResolutionType(LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue, false); + t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue, true); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2241,7 +2237,7 @@ public void convertRemapTest() throws Exception context.setLookupResolutionType(LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null, false); + t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null, true); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2277,7 +2273,7 @@ public void convertRemapTest() throws Exception context.setLookupResolutionType(LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null, false); + t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null, true); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); diff --git a/api/src/org/labkey/api/dataiterator/StandardDataIteratorBuilder.java b/api/src/org/labkey/api/dataiterator/StandardDataIteratorBuilder.java index 696c4a756ba..d4f7d0e697a 100644 --- a/api/src/org/labkey/api/dataiterator/StandardDataIteratorBuilder.java +++ b/api/src/org/labkey/api/dataiterator/StandardDataIteratorBuilder.java @@ -280,7 +280,7 @@ public DataIterator getDataIterator(DataIteratorContext context) if (null == pair.target || isAttachment) convert.addColumn(pair.indexFrom); else - convert.addConvertColumn(pair.target, pair.indexFrom, pair.indexMv, pd, pt, pair.target.getRemapMissingBehavior(), context.hasBeenRemapped()); + convert.addConvertColumn(pair.target, pair.indexFrom, pair.indexMv, pd, pt, pair.target.getRemapMissingBehavior(), context.isWithLookupRemapping()); } // diff --git a/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java b/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java index e547e9f34b7..6a70fc30932 100644 --- a/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java +++ b/api/src/org/labkey/api/dataiterator/TriggerDataBuilderHelper.java @@ -145,7 +145,7 @@ public DataIterator getDataIterator(DataIteratorContext context) boolean skipExistingRecord = !context.getInsertOption().allowUpdate || mergeKeys == null || isNewFolderImport; DataIterator coerce = new CoerceDataIterator(pre, context, _target, !context.getInsertOption().updateOnly); - context.setHasBeenRemapped(true); + context.setWithLookupRemapping(false); coerce = LoggingDataIterator.wrap(coerce); if (skipExistingRecord) diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java index 81333be39bd..c53cee90f69 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java @@ -1077,7 +1077,7 @@ else if (Column.ClassId.name().equalsIgnoreCase(name)) // CoerceDataIterator to handle the lookup/alternatekeys functionality of loadRows(), // TODO check if this covers all the functionality, in particular how is alternateKeyCandidates used? di = LoggingDataIterator.wrap(new CoerceDataIterator(di, context, ExpDataClassDataTableImpl.this, false)); - context.setHasBeenRemapped(true); + context.setWithLookupRemapping(false); TableInfo dataClassTInfo = ExpDataClassDataTableImpl.this; if (c.hasProductFolders() && !c.isProject()) { diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index f09965259c8..d771d7c3b08 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -1499,14 +1499,14 @@ public DataIterator getDataIterator(DataIteratorContext context) var addRequiredColsDI = new SampleUpdateAddColumnsDataIterator(new CachingDataIterator(addAliquotedFrom), materialTable, sampleType.getRowId(), columnNameMap.containsKey("lsid")); SimpleTranslator c = new _SamplesCoerceDataIterator(addRequiredColsDI, context, sampleType, materialTable); - context.setHasBeenRemapped(true); + context.setWithLookupRemapping(false); return LoggingDataIterator.wrap(c); } // CoerceDataIterator to handle the lookup/alternatekeys functionality of loadRows(), // TODO: check if this covers all the functionality, in particular how is alternateKeyCandidates used? DataIterator c = LoggingDataIterator.wrap(new _SamplesCoerceDataIterator(source, context, sampleType, materialTable)); - context.setHasBeenRemapped(true); + context.setWithLookupRemapping(false); SimpleTranslator addColumns = new SimpleTranslator(c, context); addColumns.setDebugName("add genId and other requried columns"); Set idColNames = Sets.newCaseInsensitiveHashSet("genId"); @@ -1886,7 +1886,7 @@ else if (name.equalsIgnoreCase("StoredAmount")) if (isScopedField) _addConvertColumn(name, i, to.getJdbcType(), to.getFk(), aliquotedFromDataColInd, scopedFields.get(name)); else - addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), to.getRemapMissingBehavior(), false); + addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), to.getRemapMissingBehavior(), true); } } else diff --git a/study/src/org/labkey/study/model/DatasetDataIteratorBuilder.java b/study/src/org/labkey/study/model/DatasetDataIteratorBuilder.java index 1eed3f55464..1072cc04e89 100644 --- a/study/src/org/labkey/study/model/DatasetDataIteratorBuilder.java +++ b/study/src/org/labkey/study/model/DatasetDataIteratorBuilder.java @@ -235,7 +235,7 @@ public DataIterator getDataIterator(DataIteratorContext context) else if (match == keyColumn && _datasetDefinition.getKeyManagementType() == Dataset.KeyManagementType.None) { // usually we let DataIterator handle convert, but we need to convert for consistent _key/lsid generation - out = it.addConvertColumn(match.getName(), in, match.getJdbcType(), null, null != match.getMvColumnName() ? SimpleTranslator.RemapMissingBehavior.OriginalValue : null, false); + out = it.addConvertColumn(match.getName(), in, match.getJdbcType(), null, null != match.getMvColumnName() ? SimpleTranslator.RemapMissingBehavior.OriginalValue : null, true); } else if (match.getPropertyType() == PropertyType.FILE_LINK) { From 75b4479cbf6905c132707d56894ab254e6b2356f Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 19 May 2025 15:41:30 -0700 Subject: [PATCH 32/39] Add comment about when check for assignableFrom is needed --- .../api/dataiterator/SimpleTranslator.java | 63 ++++--------------- 1 file changed, 12 insertions(+), 51 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index bbd05c7ec0e..4e2ca003280 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -339,6 +339,10 @@ private Object fetch(Triple> triple, // ArrayListValuedHashMap returns an empty collection if 'k' is not in the map. if (bulkLoaded == null || bulkLoaded.isEmpty() ) { + // when the given key (e.g., a rowId value) cannot be assigned to the alternate key value, + // don't attempt to select the value from the database, lest a syntax error result. + // This can happen for system fields like createdBy or modifiedBy that are added to a + // row during the insert or update process already mapped to their primary keys. if (altKeyCol.getJdbcType().getJavaClass().isAssignableFrom(k.getClass())) { TableSelector ts = createSelector(pkCol, altKeyCol, k); @@ -349,7 +353,6 @@ private Object fetch(Triple> triple, { vs = Collections.emptyList(); } - } else { @@ -931,9 +934,6 @@ public enum RemapMissingBehavior /** Every incoming value must have an entry in the map. */ Error, - /** Incoming values without a map entry will be replaced with null. */ - Null, - /** Incoming values without a map entry will pass through. */ OriginalValue } @@ -1018,13 +1018,10 @@ protected Object convert(Object o) value = convertWithPrimaryColumn(o); if (value == null) { - return switch (_missing) - { - case Null -> null; - case OriginalValue -> o; - default -> - throw new ConversionExceptionWithMessage("Value '" + o + "' not found for field " + _toCol.getName() + " in the current context."); - }; + if (_missing == RemapMissingBehavior.OriginalValue) + return o; + else + throw new ConversionExceptionWithMessage("Value '" + o + "' not found for field " + _toCol.getName() + " in the current context."); } return value; } @@ -2230,50 +2227,14 @@ public void convertRemapTest() throws Exception assertFalse(t.next()); } - // with remap with primary then alternate key, missing behavior is null - // don't throw error if remap can't be resolved - { - DataIteratorContext context = new DataIteratorContext(); - context.setLookupResolutionType(LookupResolutionType.alternateThenPrimaryKey); - simpleData.beforeFirst(); - SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null, true); - assertEquals(1, t.getColumnCount()); - assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); - assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); - - // first row - assertTrue(t.next()); - assertEquals(1, t.get(0)); - assertEquals(0, t.get(1)); // convert string "0" -> rowId ordinal 0 - - // second row - assertTrue(t.next()); - assertEquals(2, t.get(0)); - assertEquals(1, t.get(1)); // convert string "Two" -> rowId ordinal 1 - - // third row -- original value passed through - assertTrue(t.next()); - assertEquals(3, t.get(0)); - assertNull(t.get(1)); // fails to convert - - // fourth row - assertTrue(t.next()); - assertEquals(4, t.get(0)); - assertNull(t.get(1)); // missing returns null - - // no more rows - assertFalse(t.next()); - } - - // with remap with alternate then primary key, missing behavior is null + // with remap with alternate then primary key, missing behavior is OriginalValue // don't throw error if remap can't be resolved { DataIteratorContext context = new DataIteratorContext(); context.setLookupResolutionType(LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Null, true); + t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue, true); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2291,12 +2252,12 @@ public void convertRemapTest() throws Exception // third row -- original value passed through assertTrue(t.next()); assertEquals(3, t.get(0)); - assertNull(t.get(1)); // fails to convert + assertEquals("FAIL", t.get(1)); // fails to convert // fourth row assertTrue(t.next()); assertEquals(4, t.get(0)); - assertNull(t.get(1)); // missing returns null + assertEquals("", t.get(1)); // missing returns original value // no more rows assertFalse(t.next()); From 266a411b97246b91e88ceb9456ec9b37ee565e5c Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 19 May 2025 16:24:03 -0700 Subject: [PATCH 33/39] Remove unused import --- query/src/org/labkey/query/QueryServiceImplTestCase.jsp | 1 - 1 file changed, 1 deletion(-) diff --git a/query/src/org/labkey/query/QueryServiceImplTestCase.jsp b/query/src/org/labkey/query/QueryServiceImplTestCase.jsp index 3cbd08a46fd..8653053f38f 100644 --- a/query/src/org/labkey/query/QueryServiceImplTestCase.jsp +++ b/query/src/org/labkey/query/QueryServiceImplTestCase.jsp @@ -29,7 +29,6 @@ <%@ page import="java.sql.SQLException" %> <%@ page import="java.util.Arrays" %> <%@ page import="java.util.List" %> -<%@ page import="org.labkey.api.dataiterator.DataIteratorContext" %> <%@ page import="org.labkey.api.data.LookupResolutionType" %> <%@ page extends="org.labkey.api.jsp.JspTest.DRT" %> <%! From 1ba5c2a4d7739127b947dcdd1ea0e5643e2ab67b Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Mon, 19 May 2025 19:01:47 -0700 Subject: [PATCH 34/39] Set remap missing behavior to Error unless explicitly told otherwise --- api/src/org/labkey/api/dataiterator/SimpleTranslator.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 4e2ca003280..46c4514373a 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -1345,7 +1345,8 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro RemapMissingBehavior missing = remapMissingBehavior; if (missing == null) - missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.OriginalValue; +// missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.OriginalValue; + missing = RemapMissingBehavior.Error; c = new RemapPostConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); } From 1bd30ed7d610d865f31a561cfa7c6f6bbb48fb06 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 20 May 2025 09:00:31 -0700 Subject: [PATCH 35/39] Add deprecated feature flag and rename classes --- api/src/org/labkey/api/data/RemapCache.java | 8 ++-- .../api/dataiterator/SimpleTranslator.java | 37 +++++++++++++------ core/src/org/labkey/core/CoreModule.java | 5 +++ 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/api/src/org/labkey/api/data/RemapCache.java b/api/src/org/labkey/api/data/RemapCache.java index ea6fb94b6de..4a5716f3898 100644 --- a/api/src/org/labkey/api/data/RemapCache.java +++ b/api/src/org/labkey/api/data/RemapCache.java @@ -33,7 +33,7 @@ */ public class RemapCache { - Map remaps = new HashMap<>(); + Map remaps = new HashMap<>(); private final boolean _allowBulkLoads; public RemapCache() @@ -140,17 +140,17 @@ private Key key(@NotNull TableInfo table) return new Key(table); } - private SimpleTranslator.RemapPostConvert remapper(Key key, Map remapCache, boolean includePkLookup) + private SimpleTranslator.RemapConverter remapper(Key key, Map remapCache, boolean includePkLookup) { return remapCache.computeIfAbsent(key, (k) -> { TableInfo table = key.getTable(); - return new SimpleTranslator.RemapPostConvert(table, true, _allowBulkLoads, includePkLookup); + return new SimpleTranslator.RemapConverter(table, true, _allowBulkLoads, includePkLookup); }); } private V remap(Key key, String value, boolean includePkLookup) { - SimpleTranslator.RemapPostConvert remap = remapper(key, remaps, includePkLookup); + SimpleTranslator.RemapConverter remap = remapper(key, remaps, includePkLookup); if (remap == null) throw new NotFoundException("Failed to create remap: " + key._schemaKey.toString() + "." + key._queryName); //noinspection unchecked diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 46c4514373a..bfa969c0621 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -68,6 +68,7 @@ import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.settings.OptionalFeatureService; import org.labkey.api.util.GUID; import org.labkey.api.util.JunitUtil; import org.labkey.api.util.Pair; @@ -115,6 +116,7 @@ */ public class SimpleTranslator extends AbstractDataIterator implements DataIterator, ScrollableDataIterator { + public static final String DEPRECATED_NULL_MISSING_VALUE_RESOLUTION = "deprecatedNullMissingValueResolution"; private static final Logger LOG = LogManager.getLogger(SimpleTranslator.class); /** @@ -191,7 +193,7 @@ else if (null != x) return null; } - public static class RemapPostConvert + public static class RemapConverter { private final TableInfo _targetTable; private final boolean _includeTitleColumn; @@ -204,7 +206,7 @@ public static class RemapPostConvert private Triple> _titleColumnLookupMap = null; private Pair> _pkColumnLookupMap = null; - public RemapPostConvert(@NotNull TableInfo targetTable, boolean includeTitleColumn, boolean allowBulkLoads, boolean includePkLookup) + public RemapConverter(@NotNull TableInfo targetTable, boolean includeTitleColumn, boolean allowBulkLoads, boolean includePkLookup) { _targetTable = targetTable; _includeTitleColumn = includeTitleColumn; @@ -934,11 +936,14 @@ public enum RemapMissingBehavior /** Every incoming value must have an entry in the map. */ Error, + /** @Deprecated Prefer Error instead. Incoming values without a map entry will be replaced with null. */ + Null, + /** Incoming values without a map entry will pass through. */ OriginalValue } - protected class RemapPostConvertColumn extends SimpleConvertColumn + protected class RemappingConvertColumn extends SimpleConvertColumn { final SimpleConvertColumn _convertCol; final ColumnInfo _toCol; @@ -946,16 +951,16 @@ protected class RemapPostConvertColumn extends SimpleConvertColumn final boolean _includeTitleColumn; LookupResolutionType _lookupResolutionType; - final private RemapPostConvert _remapper; + final private RemapConverter _remapper; - public RemapPostConvertColumn(final @NotNull SimpleConvertColumn convertCol, final int fromIndex, final @NotNull ColumnInfo toCol, RemapMissingBehavior missing, boolean includeTitleColumn, @NotNull LookupResolutionType lookupResolutionType) + public RemappingConvertColumn(final @NotNull SimpleConvertColumn convertCol, final int fromIndex, final @NotNull ColumnInfo toCol, RemapMissingBehavior missing, boolean includeTitleColumn, @NotNull LookupResolutionType lookupResolutionType) { super(convertCol.fieldName, convertCol.index, convertCol.type); _convertCol = convertCol; _toCol = toCol; _missing = missing; _includeTitleColumn = includeTitleColumn; - _remapper = new RemapPostConvert(_toCol.getFkTableInfo(), _includeTitleColumn, false, true); + _remapper = new RemapConverter(_toCol.getFkTableInfo(), _includeTitleColumn, false, true); _lookupResolutionType = lookupResolutionType; } @@ -1020,6 +1025,8 @@ protected Object convert(Object o) { if (_missing == RemapMissingBehavior.OriginalValue) return o; + else if (_missing == RemapMissingBehavior.Null) + return null; else throw new ConversionExceptionWithMessage("Value '" + o + "' not found for field " + _toCol.getName() + " in the current context."); } @@ -1340,14 +1347,20 @@ private SimpleConvertColumn createConvertColumn(@NotNull ColumnInfo col, int fro LookupResolutionType lookupResolutionType = _context.getLookupResolutionType(); if (withLookupRemapping && fk != null && lookupResolutionType.useAlternateKey() && fk.allowImportByAlternateKey()) { - // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error - boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); - RemapMissingBehavior missing = remapMissingBehavior; if (missing == null) -// missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.OriginalValue; - missing = RemapMissingBehavior.Error; - c = new RemapPostConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); + { + if (OptionalFeatureService.get().isFeatureEnabled(DEPRECATED_NULL_MISSING_VALUE_RESOLUTION)) + { + // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error + boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); + + missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; + } + else + missing = RemapMissingBehavior.Error; + } + c = new RemappingConvertColumn(c, fromIndex, col, missing, true, lookupResolutionType); } boolean multiValue = fk instanceof MultiValuedForeignKey; diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index a6f8be9a8cb..3827bffe7c9 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -80,6 +80,7 @@ import org.labkey.api.data.dialect.SqlDialectManager; import org.labkey.api.data.dialect.SqlDialectRegistry; import org.labkey.api.data.statistics.StatsService; +import org.labkey.api.dataiterator.SimpleTranslator; import org.labkey.api.exp.property.PropertyService; import org.labkey.api.exp.property.TestDomainKind; import org.labkey.api.external.tools.ExternalToolsViewService; @@ -534,6 +535,10 @@ public QuerySchema createSchema(DefaultSchema schema, Module module) "Restore Object-Level Discussions", "This option and all support for Object-Level Discussions will be removed in LabKey Server v25.7.", false, false, FeatureType.Deprecated)); + AdminConsole.addOptionalFeatureFlag(new OptionalFeatureFlag(SimpleTranslator.DEPRECATED_NULL_MISSING_VALUE_RESOLUTION, + "Resolve Missing Lookup Values to Null", + "When Lookup Validation for a field is not selected and lookup by alternate key is enabled, resolves missing lookup values to null instead of throwing an error. This option will be removed in LabKey Server v25.11.", + false, false, OptionalFeatureService.FeatureType.Deprecated)); SiteValidationService svc = SiteValidationService.get(); if (null != svc) From efee1fb448e253b28e2aba4b24e3d8bfceef7d1c Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 20 May 2025 13:11:06 -0700 Subject: [PATCH 36/39] Simplify check since the type of the alternate key is known to be a string --- api/src/org/labkey/api/dataiterator/SimpleTranslator.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index bfa969c0621..8192c347d56 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -345,7 +345,8 @@ private Object fetch(Triple> triple, // don't attempt to select the value from the database, lest a syntax error result. // This can happen for system fields like createdBy or modifiedBy that are added to a // row during the insert or update process already mapped to their primary keys. - if (altKeyCol.getJdbcType().getJavaClass().isAssignableFrom(k.getClass())) + // Alternate keys must be of type String. + if (k instanceof String) { TableSelector ts = createSelector(pkCol, altKeyCol, k); ts.fillMultiValuedMap(map); From a5480b2de5c6c5f9369a681d4e1d931859099d70 Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Tue, 20 May 2025 15:42:22 -0700 Subject: [PATCH 37/39] Update unit tests --- .../api/dataiterator/SimpleTranslator.java | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 8192c347d56..5207cc42439 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -2206,7 +2206,7 @@ public void convertRemapTest() throws Exception public StringExpression getURL(ColumnInfo parent) { return null; } }; - // with remap with primary then alternate key + // with remap with alternate key then primary key // don't throw error if remap can't be resolved { DataIteratorContext context = new DataIteratorContext(); @@ -2249,7 +2249,7 @@ public void convertRemapTest() throws Exception context.setLookupResolutionType(LookupResolutionType.alternateThenPrimaryKey); simpleData.beforeFirst(); SimpleTranslator t = new SimpleTranslator(simpleData, context); - t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.OriginalValue, true); + t.addConvertColumn("Lookup", 5, JdbcType.INTEGER, fk, RemapMissingBehavior.Error, true); assertEquals(1, t.getColumnCount()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(0).getJdbcType()); assertEquals(JdbcType.INTEGER, t.getColumnInfo(1).getJdbcType()); @@ -2264,18 +2264,15 @@ public void convertRemapTest() throws Exception assertEquals(2, t.get(0)); assertEquals(1, t.get(1)); // convert string "Two" -> rowId ordinal 1 - // third row -- original value passed through - assertTrue(t.next()); - assertEquals(3, t.get(0)); - assertEquals("FAIL", t.get(1)); // fails to convert - - // fourth row - assertTrue(t.next()); - assertEquals(4, t.get(0)); - assertEquals("", t.get(1)); // missing returns original value - - // no more rows - assertFalse(t.next()); + // third row -- fails to resolve + try + { + t.next(); + fail("Should have thrown a conversion exception."); + } catch (BatchValidationException x) + { + assertTrue((x.getMessage().contains("Lookup: Value 'FAIL' not found for field Lookup in the current context."))); + } } } From df33246f9208eb025ad2332eedafcef6a8875b00 Mon Sep 17 00:00:00 2001 From: Matthew Bellew Date: Wed, 21 May 2025 14:39:52 -0700 Subject: [PATCH 38/39] Add AK to vehicle.Manufacturers don't show required validation error after conversion error for the same field --- api/src/org/labkey/api/dataiterator/ValidatorIterator.java | 4 +++- api/src/org/labkey/api/query/ValidationException.java | 6 ++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/ValidatorIterator.java b/api/src/org/labkey/api/dataiterator/ValidatorIterator.java index 53343da40dc..e42921963aa 100644 --- a/api/src/org/labkey/api/dataiterator/ValidatorIterator.java +++ b/api/src/org/labkey/api/dataiterator/ValidatorIterator.java @@ -168,7 +168,8 @@ public boolean next() throws BatchValidationException if (null != msg) { - addFieldError(_data.getColumnInfo(i).getName(), msg); + if (!getRowError().hasFieldErrors(_data.getColumnInfo(i).getName())) + addFieldError(_data.getColumnInfo(i).getName(), msg); validRow = false; break; } @@ -192,6 +193,7 @@ public boolean next() throws BatchValidationException if (!validRow) { + assert hasErrors(); // we'll never return true once we hit a validation error hasValidationErrors = true; checkShouldCancel(); diff --git a/api/src/org/labkey/api/query/ValidationException.java b/api/src/org/labkey/api/query/ValidationException.java index 69bf3032325..3254e6e5836 100644 --- a/api/src/org/labkey/api/query/ValidationException.java +++ b/api/src/org/labkey/api/query/ValidationException.java @@ -513,10 +513,8 @@ public boolean hasFieldErrors() public boolean hasFieldErrors(String name) { - if (_fieldErrors.containsKey(name) && _fieldErrors.get(name).size() > 0) - return true; - - return false; + var fieldErrors = _fieldErrors.get(name); + return null != fieldErrors && !fieldErrors.isEmpty(); } public boolean hasErrors() From 313679317148e82bc8a5de512d2bea899db4370a Mon Sep 17 00:00:00 2001 From: labkey-susanh Date: Thu, 22 May 2025 14:06:48 -0700 Subject: [PATCH 39/39] Use case-insensitive map for containerMap --- experiment/src/org/labkey/experiment/ExpDataIterators.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experiment/src/org/labkey/experiment/ExpDataIterators.java b/experiment/src/org/labkey/experiment/ExpDataIterators.java index 3d6e6f5102f..41c56fdd812 100644 --- a/experiment/src/org/labkey/experiment/ExpDataIterators.java +++ b/experiment/src/org/labkey/experiment/ExpDataIterators.java @@ -2448,7 +2448,7 @@ record TypeData( private final Map> _idsPerType = new HashMap<>(); private final Map> _parentIdsPerType = new HashMap<>(); - private final Map _containerMap = new HashMap<>(); + private final Map _containerMap = new CaseInsensitiveHashMap<>(); private final boolean _isCrossFolderUpdate;