Skip to content

Issue 52098, 49422: when looking up by alternate keys do that first so number names will resolve well #6642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 48 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
02303d0
Issue 52098: Switch lookupByAlternateKey to enum with three choices
labkey-susanh Apr 9, 2025
8e172a1
Issue 52098: Switch lookupByAlternateKey to enum with three choices
labkey-susanh Apr 9, 2025
3f315e2
Clear maps if we are to include the PK lookup so we'll reload to incl…
labkey-susanh Apr 14, 2025
f7d2cf6
Different parameterization of LookupResolutionType
labkey-susanh Apr 14, 2025
046b1b4
Merge remote-tracking branch 'origin/fb_lookupResolutionType' into fb…
labkey-susanh Apr 14, 2025
93c5aea
Always reload maps when changing includePkLookup
labkey-susanh Apr 14, 2025
9ef713c
Remove unused method
labkey-susanh Apr 14, 2025
47eeba6
Make getter method non-null
labkey-susanh Apr 14, 2025
2494e61
Update test expectations
labkey-susanh Apr 14, 2025
b401519
Use non-null LookupResolutionType
labkey-susanh Apr 14, 2025
70f2ca1
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh Apr 14, 2025
02599e2
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh Apr 14, 2025
53586cb
Support (for now) deprecated importLookupByAlternateKey
labkey-susanh Apr 15, 2025
82c0911
Extract LookupResolutionType out of DataIteratorContext
labkey-susanh Apr 15, 2025
5b961bb
Throw exception if conversion fails
labkey-susanh Apr 16, 2025
b33f92d
Simplify message
labkey-susanh Apr 16, 2025
97bc2a3
Remove som unneeded code and parameters
labkey-susanh Apr 16, 2025
e4726ac
Reorder assert arguments
labkey-susanh Apr 16, 2025
f21ad30
Update lookupResolutionType once a conversion has happened
labkey-susanh Apr 16, 2025
af70abe
Start of conversion for Assay data loader settings
labkey-susanh Apr 16, 2025
4dcb853
Merge from develop
labkey-susanh May 6, 2025
026e828
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh May 6, 2025
30d7585
Add hasBeenCoerced property to context so we can not try coercion again.
labkey-susanh May 7, 2025
1973843
Remove unused option
labkey-susanh May 7, 2025
809ebce
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh May 8, 2025
b71bd51
If we've coerced our data, let's first try primary key but still allo…
labkey-susanh May 8, 2025
3619f14
typo
labkey-susanh May 8, 2025
7549600
Don't need usePrimaryKey
labkey-susanh May 8, 2025
af15eeb
Remove reference to deleted method.
labkey-susanh May 9, 2025
42afcfc
Add new setHasBeenCoerced
labkey-susanh May 9, 2025
a4ce940
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh May 9, 2025
ebaacaf
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh May 12, 2025
5291b8a
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh May 14, 2025
248866f
Update logic to fail if remapping before trigger does not work
labkey-susanh May 14, 2025
31d50b0
Reset remapping flag for cross-folder and cross-type imports
labkey-susanh May 15, 2025
0069e00
Move where we set and use the `hasBeenRemapped` value
labkey-susanh May 15, 2025
136bd44
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh May 15, 2025
9ea9546
Use non-deprecated method
labkey-susanh May 15, 2025
0028129
Don't override `addConversionException`
labkey-susanh May 15, 2025
42eaf3f
Remove unnecessary primaryThenAlternate option
labkey-susanh May 16, 2025
77f3978
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh May 16, 2025
46339ed
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh May 19, 2025
0dd62da
hasBeenRemapped -> withLookupRemapping and don't use Null as RemapMis…
labkey-susanh May 19, 2025
75b4479
Add comment about when check for assignableFrom is needed
labkey-susanh May 19, 2025
810a392
Merge remote-tracking branch 'origin/develop' into fb_lookupResolutio…
labkey-susanh May 19, 2025
266a411
Remove unused import
labkey-susanh May 19, 2025
1ba5c2a
Set remap missing behavior to Error unless explicitly told otherwise
labkey-susanh May 20, 2025
1bd30ed
Add deprecated feature flag and rename classes
labkey-susanh May 20, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions api/src/org/labkey/api/assay/AbstractAssayTsvDataHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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))
Expand Down
20 changes: 20 additions & 0 deletions api/src/org/labkey/api/data/LookupResolutionType.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package org.labkey.api.data;

public enum LookupResolutionType
{
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;

LookupResolutionType(boolean useAlternateKey)
{
_useAlternateKey = useAlternateKey;
}

public boolean useAlternateKey()
{
return _useAlternateKey;
}

}
8 changes: 4 additions & 4 deletions api/src/org/labkey/api/data/RemapCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
*/
public class RemapCache
{
Map<Key, SimpleTranslator.RemapPostConvert> remaps = new HashMap<>();
Map<Key, SimpleTranslator.RemapConverter> remaps = new HashMap<>();
private final boolean _allowBulkLoads;

public RemapCache()
Expand Down Expand Up @@ -140,17 +140,17 @@ private Key key(@NotNull TableInfo table)
return new Key(table);
}

private SimpleTranslator.RemapPostConvert remapper(Key key, Map<Key, SimpleTranslator.RemapPostConvert> remapCache, boolean includePkLookup)
private SimpleTranslator.RemapConverter remapper(Key key, Map<Key, SimpleTranslator.RemapConverter> remapCache, boolean includePkLookup)
{
return remapCache.computeIfAbsent(key, (k) -> {
TableInfo table = key.getTable();
return new SimpleTranslator.RemapPostConvert(table, true, SimpleTranslator.RemapMissingBehavior.Null, _allowBulkLoads, includePkLookup, null);
return new SimpleTranslator.RemapConverter(table, true, _allowBulkLoads, includePkLookup);
});
}

private <V> 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
Expand Down
9 changes: 1 addition & 8 deletions api/src/org/labkey/api/dataiterator/CoerceDataIterator.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.OriginalValue);
addConvertColumn(to.getName(), i, to.getJdbcType(), to.getFk(), RemapMissingBehavior.Error, true);
}
else
{
Expand All @@ -88,10 +87,4 @@ else if (to.getFk() instanceof MultiValuedForeignKey)
}
}
}

@Override
protected Object addConversionException(String fieldName, Object value, JdbcType target, Exception x)
{
return value;
}
}
26 changes: 22 additions & 4 deletions api/src/org/labkey/api/dataiterator/DataIteratorContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -50,7 +51,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;
Expand All @@ -60,6 +61,7 @@ public class DataIteratorContext
private final Set<String> _dontUpdateColumnNames = new CaseInsensitiveHashSet();
private final Set<String> _alternateKeys = new CaseInsensitiveHashSet();
private String _dataSource;
private boolean _withLookupRemapping = true;

private final Map<String, Object> _responseInfo = new HashMap<>(); // information from the import/loadRows context to be passed back to the API response object
private Logger _logger;
Expand Down Expand Up @@ -162,15 +164,31 @@ public void setSupportAutoIncrementKey(boolean supportAutoIncrementKey)
_supportAutoIncrementKey = supportAutoIncrementKey;
}

public boolean isAllowImportLookupByAlternateKey()
@NotNull
public LookupResolutionType getLookupResolutionType()
{
return _lookupResolutionType == null ? LookupResolutionType.primaryKey : _lookupResolutionType;
}

public void setLookupResolutionType(LookupResolutionType lookupResolutionType)
{
_lookupResolutionType = lookupResolutionType;
}

public boolean isWithLookupRemapping()
{
return _withLookupRemapping;
}

public void setWithLookupRemapping(boolean withLookupRemapping)
{
return _allowImportLookupByAlternateKey;
_withLookupRemapping = withLookupRemapping;
}

/** 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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/

Expand Down
Loading