From dec98f6cb79f9504856a6dc30350d06766dc4b1d Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Fri, 11 Sep 2026 10:13:57 -0700 Subject: [PATCH 1/4] Optimize SimpleTranslator caching --- api/src/org/labkey/api/ApiModule.java | 1 + .../api/dataiterator/SimpleTranslator.java | 267 ++++++++++++++++++ 2 files changed, 268 insertions(+) diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index b687e91f753..b4fa61792df 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -568,6 +568,7 @@ public void registerServlets(ServletContext servletCtx) RoleSet.TestCase.class, RowTrackingResultSetWrapper.TestCase.class, SecurityManager.TestCase.class, + SimpleTranslator.RemapCollisionTestCase.class, SimpleTranslator.TranslateTestCase.class, SqlSelectorTestCase.class, StandardDialectStringHandler.TestCase.class, diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index b17fc536299..79ebffd6cd5 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -26,7 +26,9 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.json.JSONArray; +import org.junit.AfterClass; import org.junit.Assert; +import org.junit.BeforeClass; import org.junit.Test; import org.labkey.api.action.ApiUsageException; import org.labkey.api.attachments.AttachmentFile; @@ -49,6 +51,7 @@ import org.labkey.api.data.MultiValuedForeignKey; import org.labkey.api.data.MvUtil; import org.labkey.api.data.NowTimestamp; +import org.labkey.api.data.PropertyStorageSpec; import org.labkey.api.data.SimpleConvert; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.TableDescription; @@ -58,7 +61,13 @@ import org.labkey.api.data.TestSchema; import org.labkey.api.exp.MvFieldWrapper; import org.labkey.api.exp.PropertyType; +import org.labkey.api.exp.api.ExperimentService; +import org.labkey.api.exp.api.SampleTypeService; +import org.labkey.api.exp.list.ListDefinition; +import org.labkey.api.exp.list.ListService; +import org.labkey.api.exp.property.Domain; import org.labkey.api.files.FileContentService; +import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; import org.labkey.api.ontology.Unit; import org.labkey.api.query.AbstractQueryUpdateService; import org.labkey.api.query.BatchValidationException; @@ -69,7 +78,10 @@ import org.labkey.api.query.SimpleValidationError; import org.labkey.api.query.UserSchema; import org.labkey.api.query.ValidationException; +import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.ValidEmail; import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.util.GUID; import org.labkey.api.util.IntegerUtils; @@ -2374,4 +2386,259 @@ public void builtinColumns() TableInfo t = TestSchema.getInstance().getTableInfoTestTable(); } } + + /** + * Deterministic coverage for the rowId/name collision hazard. + * + * RemapConverter resolves an FK value through the lookup table's title column whenever that table exposes no + * single-column unique text index -- which AbstractTableInfo.getUniqueIndices() makes the default, so it is the + * common case rather than an edge case. A value that is already a primary key then matches whatever row is TITLED + * with that key's decimal string and resolves to the wrong entity with no error, surfacing far downstream as + * "does not exist", an empty grid, or data written under the wrong parent. + * + * Every fixture here derives the colliding name from a rowId read back from the server, so the collision fires on + * any database whatever state its sequences are in. A hardcoded numeric name is not a fixture for this: it collides + * only when the sequence happens to hand out that value, which is why this defect stayed latent for years and then + * appeared as an intermittent, misattributed CI flake. + */ + public static class RemapCollisionTestCase extends Assert + { + private static final String LIST_TITLE_COLUMN = "Label"; + private static final int MAX_LOOKUPS = 8; + + private static User _user; + private static Container _container; + private static final List _createdUserIds = new ArrayList<>(); + + /** Creates one row in a lookup table under the given name. */ + private interface Named + { + void create(String name) throws Exception; + } + + @BeforeClass + public static void doSetup() + { + JunitUtil.deleteTestContainer(); + _user = TestContext.get().getUser(); + _container = JunitUtil.getTestContainer(); + } + + @AfterClass + public static void doCleanup() throws Exception + { + for (Integer userId : _createdUserIds) + UserManager.deleteUser(userId); + _createdUserIds.clear(); + JunitUtil.deleteTestContainer(); + } + + /** An integer primary key with a text title column is the generic shape of this hazard; the surfaces below are instances of it. */ + @Test + public void listPkIsNotResolvedThroughACollidingLabel() throws Exception + { + TableInfo list = createList("RemapCollisionList"); + assertPkKeyBeatsTitleCollision(list, name -> insertListRow(list, name)); + } + + @Test + public void sampleTypeIdIsNotResolvedThroughACollidingName() throws Exception + { + assertPkKeyBeatsTitleCollision(lookupTargetOf("Materials", "MaterialSourceId"), this::createSampleType); + } + + @Test + public void dataClassIdIsNotResolvedThroughACollidingName() throws Exception + { + assertPkKeyBeatsTitleCollision(lookupTargetOf("Data", "DataClass"), this::createDataClass); + } + + /** fetch()'s "alternate keys must be of type String" rule names createdBy/modifiedBy as the hazard, and a user whose display name is a number is exactly it. */ + @Test + public void userIdIsNotResolvedThroughACollidingDisplayName() throws Exception + { + assertPkKeyBeatsTitleCollision(lookupTargetOf("Materials", "CreatedBy"), this::createUserWithDisplayName); + } + + @Test + public void ambiguousTitleIsReportedRatherThanPicked() throws Exception + { + TableInfo list = createList("RemapAmbiguousTitleList"); + String shared = uniqueName("shared"); + insertListRow(list, shared); + insertListRow(list, shared); + + RemapConverter converter = new RemapConverter(list, true, false, true); + converter.setIncludePkLookup(false); + + for (int lookup = 0; lookup <= MAX_LOOKUPS; lookup++) + { + try + { + Object resolved = converter.mappedValue(shared); + fail("two rows titled \"" + shared + "\" resolved to " + resolved + " rather than reporting the ambiguity"); + } + catch (ConversionException x) + { + if (x.getMessage() != null && x.getMessage().contains("Found 2 values")) + return; + } + } + fail("two rows titled \"" + shared + "\" never produced the ambiguity error"); + } + + /** + * Manufactures a rowId/name collision on one FK target and pins which interpretation wins: an integer that is + * already a primary key resolves to itself and never to a title match, while a String key still resolves by + * title. + * + * @param lookup the FK target as the production caller sees it, so the container filter under test is the real one + * @param factory creates one row in that table under a given name + */ + private void assertPkKeyBeatsTitleCollision(TableInfo lookup, Named factory) throws Exception + { + RemapConverter converter = new RemapConverter(lookup, true, false, true); + // convertWithRemapper's setting, and what lets a key reach the title column at all + converter.setIncludePkLookup(false); + converter.getMaps(); + assertNotNull(lookup.getName() + " does not resolve through a title column, so this fixture proves nothing", + converter._titleColumnLookupMap); + + // Read the columns off the converter, so the fixture matches on whatever the resolver itself matches on + ColumnInfo pkCol = converter.getPkColumn(); + ColumnInfo titleCol = converter._titleColumnLookupMap.getMiddle(); + + String subjectName = uniqueName("collide"); + factory.create(subjectName); + int subjectPk = onlyPkTitled(pkCol, titleCol, subjectName); + + // The whole point: the colliding name is the rowId the server actually assigned, not a guess + String collidingName = String.valueOf(subjectPk); + factory.create(collidingName); + int collidingPk = onlyPkTitled(pkCol, titleCol, collidingName); + assertNotEquals("the colliding row has to be a different row than the subject", subjectPk, collidingPk); + + String wrongEntity = "integer pk " + subjectPk + " resolved through the title column to \"" + collidingName + + "\" (pk " + collidingPk + ") instead of being left as itself"; + assertNull(wrongEntity, resolvePastFirstMiss(converter, subjectPk)); + assertEquals("a String key must still resolve against the title column", + collidingPk, resolvePastFirstMiss(converter, collidingName)); + + // RemappingConvertColumn flips this before every row, so neither answer may depend on it + converter.setIncludePkLookup(true); + assertEquals("with the pk lookup on, an integer key must resolve to itself", + subjectPk, resolvePastFirstMiss(converter, subjectPk)); + converter.setIncludePkLookup(false); + + assertNull(wrongEntity + ", on a later lookup", resolvePastFirstMiss(converter, subjectPk)); + assertEquals("title resolution drifted between lookups", + collidingPk, resolvePastFirstMiss(converter, collidingName)); + } + + /** + * The primary key of the one row titled {@code title}, read back through the resolver's own columns and so its + * own container filter. + * + * Requiring exactly one matters as much as the name: with two, the resolver reports an ambiguity instead of + * resolving, the import code swallows that error, and the collision silently disarms while the test passes. + */ + private int onlyPkTitled(ColumnInfo pkCol, ColumnInfo titleCol, String title) + { + Integer[] pks = new TableSelector(pkCol, new SimpleFilter(titleCol.getFieldKey(), title), null).getArray(Integer.class); + assertEquals("expected exactly one row titled \"" + title + "\" in the resolver's scope", 1, pks.length); + return pks[0]; + } + + /** + * What the converter resolves once it gets past its throwing first-miss path. + * + * RemapConverter memoizes per key, and fetch() stores a MISS marker but then calls getSingleValue() with the + * still-empty collection: the first miss in a map throws, while a later lookup of the same key returns null and + * falls through to the next resolution strategy. convertWithRemapper discards the exception and the import moves + * on to the next row, so the title-column fallback is only ever reached from a later row. Pushing a single value + * through a memoizing resolver exercises the throwing path alone and never reaches the fallback where this defect + * lives, which is how it hid -- so do not reduce these to one lookup. + */ + private Object resolvePastFirstMiss(RemapConverter converter, Object k) + { + for (int lookup = 0; lookup <= MAX_LOOKUPS; lookup++) + { + try + { + return converter.mappedValue(k); + } + catch (ConversionException x) + { + // what convertWithRemapper does with it; the next row tries again + } + } + throw new AssertionError("every lookup of " + k + " threw, so no resolution strategy was ever reached"); + } + + /** Truncated because the user fixture builds an email address from this and core.Principals.Name is VARCHAR(64). */ + private String uniqueName(String prefix) + { + return prefix + GUID.makeHash().substring(0, 8); + } + + /** The table an FK actually resolves against, container filter included, rather than a stand-in for it. */ + private TableInfo lookupTargetOf(String tableName, String columnName) + { + TableInfo table = QueryService.get().getUserSchema(_user, _container, "exp").getTable(tableName); + ColumnInfo col = table.getColumn(columnName); + assertNotNull("exp." + tableName + " has no " + columnName + " column", col); + ForeignKey fk = col.getFk(); + assertNotNull(columnName + " carries no foreign key", fk); + assertTrue(columnName + " cannot be imported by alternate key, so the title column is never consulted", + fk.allowImportByAlternateKey()); + return fk.getLookupTableInfo(); + } + + private TableInfo createList(String name) throws Exception + { + ListDefinition list = ListService.get().createList(_container, name, ListDefinition.KeyType.AutoIncrementInteger); + list.setKeyName("RowId"); + Domain domain = list.getDomain(); + assertNotNull("new list has no domain", domain); + domain.addProperty(new PropertyStorageSpec(LIST_TITLE_COLUMN, JdbcType.VARCHAR, 100)); + // Pin the title column rather than relying on "the first string column wins" + list.setTitleColumn(LIST_TITLE_COLUMN); + list.save(_user); + + return QueryService.get().getUserSchema(_user, _container, "lists").getTable(name); + } + + private void insertListRow(TableInfo list, String label) throws Exception + { + QueryUpdateService qus = list.getUpdateService(); + assertNotNull("list " + list.getName() + " is not updatable", qus); + + BatchValidationException errors = new BatchValidationException(); + qus.insertRows(_user, _container, + List.of(CaseInsensitiveHashMap.of(LIST_TITLE_COLUMN, (Object) label)), errors, null, null); + if (errors.hasErrors()) + throw errors; + } + + private void createSampleType(String name) throws Exception + { + SampleTypeService.get().createSampleType(_container, _user, name, null, + List.of(new GWTPropertyDescriptor("name", "string")), List.of(), -1, -1, -1, -1, null); + } + + private void createDataClass(String name) throws Exception + { + ExperimentService.get().createDataClass(_container, _user, name, null, + List.of(new GWTPropertyDescriptor("name", "string")), List.of(), null, null); + } + + private void createUserWithDisplayName(String displayName) throws Exception + { + ValidEmail email = new ValidEmail("remapcollision_" + displayName.replaceAll("\\W", "") + "@test.labkey.com"); + User user = SecurityManager.addUser(email, null).getUser(); + _createdUserIds.add(user.getUserId()); + user.setDisplayName(displayName); + UserManager.updateUser(user, user); + } + } } From 4ce2eba86e8ed7d4004b308daaa99478a9ac6e02 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Sun, 13 Sep 2026 07:23:49 -0700 Subject: [PATCH 2/4] Add the fix --- .../api/dataiterator/SimpleTranslator.java | 53 ++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 79ebffd6cd5..c576a79f6ee 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -325,7 +325,9 @@ public Object mappedValue(Object k) if (_titleColumnLookupMap != null) { - return fetch(_titleColumnLookupMap, String.valueOf(k)); + // Pass the key as-is so fetch() can apply its "alternate keys must be String" rule. Stringifying here + // turned an integer pk into a title match, so rowId 2 resolved to whatever row is titled "2". + return fetch(_titleColumnLookupMap, k); } return null; @@ -2258,6 +2260,55 @@ private EnumTableInfo remapLookupTable() return new EnumTableInfo<>(LookupValues.class, core, "fake enum", true); } + /** + * Same fixture with the unique indices dropped, so the title column is not already claimed as an alternate key. + * That is the shape of exp.MaterialSource (integer pk, Name carries no single-column unique index) and the only + * one that builds the title-column map. + */ + private EnumTableInfo remapTitleColumnLookupTable() + { + var core = QueryService.get().getUserSchema(TestContext.get().getUser(), JunitUtil.getTestContainer(), "core"); + return new EnumTableInfo<>(LookupValues.class, core, "fake enum", true) + { + @Override + public @NotNull List getUniqueIndices() + { + return List.of(); + } + }; + } + + @Test + public void remapTitleColumnIgnoresNonStringKey() + { + RemapConverter converter = new RemapConverter(remapTitleColumnLookupTable(), true, false, true); + + // convertWithRemapper turns the pk lookup off, which is what lets the key reach the title column at all + converter.setIncludePkLookup(false); + assertTrue("expected no alternate-key maps, so the title column is the only match left", converter.getMaps().isEmpty()); + + // A row whose title is the decimal string of a different row's pk, e.g. a sample type named "2" + MultiValuedMap titleCache = converter._titleColumnLookupMap.getRight(); + Integer rowTitledTwo = 42; + titleCache.put("2", rowTitledTwo); + + assertEquals("a String key should still resolve against the title column", rowTitledTwo, converter.mappedValue("2")); + assertNull("integer pk 2 was stringified into a title match, resolving to the row titled \"2\"", resolve(converter, 2)); + } + + /** mappedValue throws on a first-time miss instead of returning null, so mirror what convertWithRemapper does with that. */ + private Object resolve(RemapConverter converter, Object k) + { + try + { + return converter.mappedValue(k); + } + catch (ConversionException x) + { + return null; + } + } + @Test public void remapCacheSurvivesPkLookupToggle() { From 0a4655ad37e170ce2813caab130b4f328de40caa Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Sun, 13 Sep 2026 11:50:29 -0700 Subject: [PATCH 3/4] Docs and more --- .../api/dataiterator/SimpleTranslator.java | 250 ++++++++++++------ 1 file changed, 169 insertions(+), 81 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index c576a79f6ee..58eb3fcb577 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -66,6 +66,8 @@ import org.labkey.api.exp.list.ListDefinition; import org.labkey.api.exp.list.ListService; import org.labkey.api.exp.property.Domain; +import org.labkey.api.exp.property.DomainProperty; +import org.labkey.api.exp.property.Lookup; import org.labkey.api.files.FileContentService; import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; import org.labkey.api.ontology.Unit; @@ -297,6 +299,18 @@ public ColumnInfo getPkColumn() return _maps; } + /** + * Resolves {@code k} to the target table's primary key, trying in order: the primary key itself (only while + * includePkLookup is on), each single-column unique text index, then the title column. The first strategy that + * matches wins, and null means none did. + * + * Only a String can resolve by alternate key or title, so a value that arrives already typed as the pk -- a + * rowId, or a system column like createdBy -- is never matched against a name. Without that rule an integer + * would match whatever row is TITLED with its decimal string and resolve to an unrelated entity. + * + * Resolutions and misses are both cached per key, so repeated values cost one query at most. A key matching + * more than one row throws ConversionException rather than picking one. + */ public Object mappedValue(Object k) { if (null == k) @@ -325,8 +339,7 @@ public Object mappedValue(Object k) if (_titleColumnLookupMap != null) { - // Pass the key as-is so fetch() can apply its "alternate keys must be String" rule. Stringifying here - // turned an integer pk into a title match, so rowId 2 resolved to whatever row is titled "2". + // Pass the key as-is; fetch() applies the "alternate keys must be String" rule return fetch(_titleColumnLookupMap, k); } @@ -335,9 +348,16 @@ public Object mappedValue(Object k) private final Object MISS = new Object(); - // While there should be at most one matching value for lookup targets with a true unique constraint, - // using a multi-valued map allows us to also work with things that are almost always unique, like - // exp.Material names, when only a single value matches + /** + * The primary key of the row whose alternate key or title column holds {@code k}, or null when no row does. + * Throws ConversionException when more than one row matches. + * + * Only a String is looked up; anything else misses without querying, since a value already typed as the pk + * cannot be an alternate key and filtering a text column on it can fail in the database. + * + * While a target with a true unique constraint has at most one matching row, the multi-valued map lets this + * also serve columns that are only almost always unique, such as exp.Material names. + */ private Object fetch(Triple> triple, Object k) { final ColumnInfo pkCol = triple.getLeft(); @@ -389,7 +409,12 @@ private Object fetch(Triple> triple, // If there are no values in the database, stash a MISS marker to avoid re-fetching. assert vs != null; if (vs.isEmpty()) + { + // Return here: 'vs' is still the empty collection, so getSingleValue() below would throw rather + // than report the miss map.put(k, MISS); + return null; + } } Object v = getSingleValue(k, vs); @@ -1009,6 +1034,14 @@ private Object convertWithRemapper(Object o) return null; } + /** + * The value written to the column: whatever {@code o} resolves to by alternate key or title, else {@code o} + * coerced to the column's own type, else the configured RemapMissingBehavior. + * + * A String may take either route, so a name that reads as a number resolves to the row carrying that name + * rather than to the row with that rowId. A value already typed as the pk only takes the second, and is + * written through as the key it is -- no check that it names an existing row happens here. + */ @Override protected Object convert(Object o) { @@ -2260,11 +2293,7 @@ private EnumTableInfo remapLookupTable() return new EnumTableInfo<>(LookupValues.class, core, "fake enum", true); } - /** - * Same fixture with the unique indices dropped, so the title column is not already claimed as an alternate key. - * That is the shape of exp.MaterialSource (integer pk, Name carries no single-column unique index) and the only - * one that builds the title-column map. - */ + /** Unique indices dropped, so the title column isn't already claimed as an alternate key -- the shape of exp.MaterialSource, and the only shape that builds the title-column map. */ private EnumTableInfo remapTitleColumnLookupTable() { var core = QueryService.get().getUserSchema(TestContext.get().getUser(), JunitUtil.getTestContainer(), "core"); @@ -2287,26 +2316,15 @@ public void remapTitleColumnIgnoresNonStringKey() converter.setIncludePkLookup(false); assertTrue("expected no alternate-key maps, so the title column is the only match left", converter.getMaps().isEmpty()); + assertNotNull("fixture resolves through no title column, so it proves nothing", converter._titleColumnLookupMap); + // A row whose title is the decimal string of a different row's pk, e.g. a sample type named "2" MultiValuedMap titleCache = converter._titleColumnLookupMap.getRight(); Integer rowTitledTwo = 42; titleCache.put("2", rowTitledTwo); assertEquals("a String key should still resolve against the title column", rowTitledTwo, converter.mappedValue("2")); - assertNull("integer pk 2 was stringified into a title match, resolving to the row titled \"2\"", resolve(converter, 2)); - } - - /** mappedValue throws on a first-time miss instead of returning null, so mirror what convertWithRemapper does with that. */ - private Object resolve(RemapConverter converter, Object k) - { - try - { - return converter.mappedValue(k); - } - catch (ConversionException x) - { - return null; - } + assertNull("integer pk 2 reached the title column and resolved to the row titled \"2\"", converter.mappedValue(2)); } @Test @@ -2439,23 +2457,18 @@ public void builtinColumns() } /** - * Deterministic coverage for the rowId/name collision hazard. - * - * RemapConverter resolves an FK value through the lookup table's title column whenever that table exposes no - * single-column unique text index -- which AbstractTableInfo.getUniqueIndices() makes the default, so it is the - * common case rather than an edge case. A value that is already a primary key then matches whatever row is TITLED - * with that key's decimal string and resolves to the wrong entity with no error, surfacing far downstream as - * "does not exist", an empty grid, or data written under the wrong parent. + * Pins how RemapConverter reads an FK value when the lookup target resolves through its title column, which + * AbstractTableInfo.getUniqueIndices() makes the default: a String resolves to the row carrying that name, while a + * value already typed as the pk stays the key it is and is never matched against a name. * - * Every fixture here derives the colliding name from a rowId read back from the server, so the collision fires on - * any database whatever state its sequences are in. A hardcoded numeric name is not a fixture for this: it collides - * only when the sequence happens to hand out that value, which is why this defect stayed latent for years and then - * appeared as an intermittent, misattributed CI flake. + * Derive every colliding name from a rowId read back from the server, never a hardcoded number: a literal name + * collides only when the sequence happens to hand out that value. */ public static class RemapCollisionTestCase extends Assert { private static final String LIST_TITLE_COLUMN = "Label"; - private static final int MAX_LOOKUPS = 8; + private static final String LIST_TAG_COLUMN = "Tag"; + private static final String LIST_LOOKUP_COLUMN = "LookupCol"; private static User _user; private static Container _container; @@ -2522,26 +2535,83 @@ public void ambiguousTitleIsReportedRatherThanPicked() throws Exception RemapConverter converter = new RemapConverter(list, true, false, true); converter.setIncludePkLookup(false); - for (int lookup = 0; lookup <= MAX_LOOKUPS; lookup++) + try { - try - { - Object resolved = converter.mappedValue(shared); - fail("two rows titled \"" + shared + "\" resolved to " + resolved + " rather than reporting the ambiguity"); - } - catch (ConversionException x) - { - if (x.getMessage() != null && x.getMessage().contains("Found 2 values")) - return; - } + Object resolved = converter.mappedValue(shared); + fail("two rows titled \"" + shared + "\" resolved to " + resolved + " rather than reporting the ambiguity"); + } + catch (ConversionException x) + { + assertTrue("expected an ambiguity error, got: " + x.getMessage(), + x.getMessage() != null && x.getMessage().contains("Found 2 values")); } - fail("two rows titled \"" + shared + "\" never produced the ambiguity error"); + } + + /** + * The rule is "a non-String key is never eligible for title resolution", not merely "a valid pk outranks a + * title match" -- a key that is no row's pk still must not reach the title column. + */ + @Test + public void nonStringKeyIsNeverResolvedByTitle() throws Exception + { + TableInfo list = createList("RemapNonStringKeyList"); + insertListRow(list, uniqueName("seed")); + + RemapConverter converter = new RemapConverter(list, true, false, true); + converter.setIncludePkLookup(false); + converter.getMaps(); + assertNotNull(list.getName() + " does not resolve through a title column, so this fixture proves nothing", + converter._titleColumnLookupMap); + ColumnInfo pkCol = converter.getPkColumn(); + ColumnInfo titleCol = converter._titleColumnLookupMap.getMiddle(); + + int absentPk = maxPk(pkCol) + 1000; + String absentPkName = String.valueOf(absentPk); + insertListRow(list, absentPkName); + int labelledPk = onlyPkTitled(pkCol, titleCol, absentPkName); + // The last assertion below only means anything while absentPk is no row's pk + assertEquals("absentPk is a real pk, so the pk lookup would legitimately resolve it", 0, + new TableSelector(pkCol, new SimpleFilter(pkCol.getFieldKey(), absentPk), null).getArray(Integer.class).length); + + String wrongEntity = "integer " + absentPk + " reached the title column and resolved to the row labelled \"" + + absentPkName + "\" (pk " + labelledPk + ")"; + assertNull(wrongEntity, converter.mappedValue(absentPk)); + assertEquals("the same value as a String must still resolve by title", + labelledPk, converter.mappedValue(absentPkName)); + + // With the pk lookup on the integer misses there too, and still must not fall through to a title match + converter.setIncludePkLookup(true); + assertNull(wrongEntity + ", with the pk lookup on", converter.mappedValue(absentPk)); + } + + /** + * The same rule as seen by an API caller: RemappingConvertColumn decides whether a value resolves by display + * value or falls through to convertWithPrimaryColumn and is taken as a rowId, and only an import exercises that. + */ + @Test + public void importedIntegerIsNotResolvedByTitle() throws Exception + { + TableInfo target = createList("RemapImportTargetList"); + insertListRow(target, uniqueName("seed")); + + ColumnInfo pkCol = target.getPkColumns().getFirst(); + int absentPk = maxPk(pkCol) + 1000; + String absentPkName = String.valueOf(absentPk); + insertListRow(target, absentPkName); + int labelledPk = onlyPkTitled(pkCol, target.getColumn(LIST_TITLE_COLUMN), absentPkName); + + TableInfo source = createListWithLookup("RemapImportSourceList", target.getName()); + importRows(source, List.of( + CaseInsensitiveHashMap.of(LIST_TAG_COLUMN, (Object) "asString", LIST_LOOKUP_COLUMN, absentPkName), + CaseInsensitiveHashMap.of(LIST_TAG_COLUMN, (Object) "asInteger", LIST_LOOKUP_COLUMN, absentPk))); + + assertEquals("a String must still import by display value", (Integer) labelledPk, importedLookup(source, "asString")); + assertEquals("an integer must import as a rowId, not by display value", (Integer) absentPk, importedLookup(source, "asInteger")); } /** * Manufactures a rowId/name collision on one FK target and pins which interpretation wins: an integer that is - * already a primary key resolves to itself and never to a title match, while a String key still resolves by - * title. + * already a primary key resolves to itself, never to a title match, while a String key still resolves by title. * * @param lookup the FK target as the production caller sees it, so the container filter under test is the real one * @param factory creates one row in that table under a given name @@ -2571,26 +2641,25 @@ private void assertPkKeyBeatsTitleCollision(TableInfo lookup, Named factory) thr String wrongEntity = "integer pk " + subjectPk + " resolved through the title column to \"" + collidingName + "\" (pk " + collidingPk + ") instead of being left as itself"; - assertNull(wrongEntity, resolvePastFirstMiss(converter, subjectPk)); + assertNull(wrongEntity, converter.mappedValue(subjectPk)); assertEquals("a String key must still resolve against the title column", - collidingPk, resolvePastFirstMiss(converter, collidingName)); + collidingPk, converter.mappedValue(collidingName)); // RemappingConvertColumn flips this before every row, so neither answer may depend on it converter.setIncludePkLookup(true); assertEquals("with the pk lookup on, an integer key must resolve to itself", - subjectPk, resolvePastFirstMiss(converter, subjectPk)); + subjectPk, converter.mappedValue(subjectPk)); converter.setIncludePkLookup(false); - assertNull(wrongEntity + ", on a later lookup", resolvePastFirstMiss(converter, subjectPk)); + assertNull(wrongEntity + ", on a later lookup", converter.mappedValue(subjectPk)); assertEquals("title resolution drifted between lookups", - collidingPk, resolvePastFirstMiss(converter, collidingName)); + collidingPk, converter.mappedValue(collidingName)); } /** - * The primary key of the one row titled {@code title}, read back through the resolver's own columns and so its - * own container filter. + * The primary key of the one row titled {@code title}, read through the resolver's own columns and container filter. * - * Requiring exactly one matters as much as the name: with two, the resolver reports an ambiguity instead of + * Requiring exactly one matters as much as the name: with two the resolver reports an ambiguity instead of * resolving, the import code swallows that error, and the collision silently disarms while the test passes. */ private int onlyPkTitled(ColumnInfo pkCol, ColumnInfo titleCol, String title) @@ -2600,30 +2669,19 @@ private int onlyPkTitled(ColumnInfo pkCol, ColumnInfo titleCol, String title) return pks[0]; } - /** - * What the converter resolves once it gets past its throwing first-miss path. - * - * RemapConverter memoizes per key, and fetch() stores a MISS marker but then calls getSingleValue() with the - * still-empty collection: the first miss in a map throws, while a later lookup of the same key returns null and - * falls through to the next resolution strategy. convertWithRemapper discards the exception and the import moves - * on to the next row, so the title-column fallback is only ever reached from a later row. Pushing a single value - * through a memoizing resolver exercises the throwing path alone and never reaches the fallback where this defect - * lives, which is how it hid -- so do not reduce these to one lookup. - */ - private Object resolvePastFirstMiss(RemapConverter converter, Object k) + private int maxPk(ColumnInfo pkCol) { - for (int lookup = 0; lookup <= MAX_LOOKUPS; lookup++) - { - try - { - return converter.mappedValue(k); - } - catch (ConversionException x) - { - // what convertWithRemapper does with it; the next row tries again - } - } - throw new AssertionError("every lookup of " + k + " threw, so no resolution strategy was ever reached"); + Integer[] pks = new TableSelector(pkCol).getArray(Integer.class); + return Arrays.stream(pks).mapToInt(Integer::intValue).max().orElse(0); + } + + /** The lookup value actually stored for the row tagged {@code tag}, so the assertion is on what the import wrote. */ + private Integer importedLookup(TableInfo table, String tag) + { + Integer[] values = new TableSelector(table.getColumn(LIST_LOOKUP_COLUMN), + new SimpleFilter(table.getColumn(LIST_TAG_COLUMN).getFieldKey(), tag), null).getArray(Integer.class); + assertEquals("expected exactly one row tagged \"" + tag + "\"", 1, values.length); + return values[0]; } /** Truncated because the user fixture builds an email address from this and core.Principals.Name is VARCHAR(64). */ @@ -2659,6 +2717,36 @@ private TableInfo createList(String name) throws Exception return QueryService.get().getUserSchema(_user, _container, "lists").getTable(name); } + /** A list with an integer FK to {@code targetListName}, so importing into it runs RemappingConvertColumn. */ + private TableInfo createListWithLookup(String name, String targetListName) throws Exception + { + ListDefinition list = ListService.get().createList(_container, name, ListDefinition.KeyType.AutoIncrementInteger); + list.setKeyName("RowId"); + Domain domain = list.getDomain(); + assertNotNull("new list has no domain", domain); + domain.addProperty(new PropertyStorageSpec(LIST_TAG_COLUMN, JdbcType.VARCHAR, 100)); + DomainProperty lookup = domain.addProperty(new PropertyStorageSpec(LIST_LOOKUP_COLUMN, JdbcType.INTEGER)); + lookup.setLookup(new Lookup(_container, "lists", targetListName)); + list.setTitleColumn(LIST_TAG_COLUMN); + list.save(_user); + + return QueryService.get().getUserSchema(_user, _container, "lists").getTable(name); + } + + /** Alternate-key resolution is off by default, and without it the lookup column never reaches RemappingConvertColumn. */ + private void importRows(TableInfo table, List> rows) throws Exception + { + QueryUpdateService qus = table.getUpdateService(); + assertNotNull("list " + table.getName() + " is not updatable", qus); + + DataIteratorContext context = new DataIteratorContext(); + context.setAllowImportLookupByAlternateKey(true); + qus.loadRows(_user, _container, + new ListofMapsDataIterator.Builder(Set.of(LIST_TAG_COLUMN, LIST_LOOKUP_COLUMN), rows), context, null); + if (context.getErrors().hasErrors()) + throw context.getErrors(); + } + private void insertListRow(TableInfo list, String label) throws Exception { QueryUpdateService qus = list.getUpdateService(); From 73c42fb5fd1fe0981339196cca4932e5b972813d Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 14 Sep 2026 11:43:12 -0700 Subject: [PATCH 4/4] Test the first-lookup miss, bound the test timeout, harden cleanup --- .../api/dataiterator/SimpleTranslator.java | 31 ++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 58eb3fcb577..d12c52670d4 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -85,6 +85,7 @@ import org.labkey.api.security.UserManager; import org.labkey.api.security.ValidEmail; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.test.TestTimeout; import org.labkey.api.util.GUID; import org.labkey.api.util.IntegerUtils; import org.labkey.api.util.JunitUtil; @@ -2464,6 +2465,7 @@ public void builtinColumns() * Derive every colliding name from a rowId read back from the server, never a hardcoded number: a literal name * collides only when the sequence happens to hand out that value. */ + @TestTimeout(120) public static class RemapCollisionTestCase extends Assert { private static final String LIST_TITLE_COLUMN = "Label"; @@ -2491,10 +2493,16 @@ public static void doSetup() @AfterClass public static void doCleanup() throws Exception { - for (Integer userId : _createdUserIds) - UserManager.deleteUser(userId); - _createdUserIds.clear(); - JunitUtil.deleteTestContainer(); + try + { + for (Integer userId : _createdUserIds) + UserManager.deleteUser(userId); + } + finally + { + _createdUserIds.clear(); + JunitUtil.deleteTestContainer(); + } } /** An integer primary key with a text title column is the generic shape of this hazard; the surfaces below are instances of it. */ @@ -2547,6 +2555,21 @@ public void ambiguousTitleIsReportedRatherThanPicked() throws Exception } } + /** The first lookup of an unmatched key must report the miss, not throw: RemapCache's callers already read null as unresolved. */ + @Test + public void unmatchedKeyMissesOnEveryLookup() throws Exception + { + TableInfo list = createList("RemapUnmatchedKeyList"); + insertListRow(list, uniqueName("seed")); + + RemapConverter converter = new RemapConverter(list, true, false, true); + converter.setIncludePkLookup(false); + + String absent = uniqueName("nobody"); + assertNull("no row is titled \"" + absent + "\", so the first lookup must miss", converter.mappedValue(absent)); + assertNull("the cached miss must read the same as the uncached one", converter.mappedValue(absent)); + } + /** * The rule is "a non-String key is never eligible for title resolution", not merely "a valid pk outranks a * title match" -- a key that is no row's pk still must not reach the title column.