Skip to content

Issue 53625: remap assay lookup keys before validation so that validators can validate the remapped value #7100

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.

Sign up for GitHub

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

Merged
cnathe merged 7 commits into from
Oct 7, 2025
Merged

Issue 53625: remap assay lookup keys before validation so that validators can validate the remapped value #7100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4ce9a71
Issue 53625: remap assay lookup keys before validation so that valida…
cnathe Oct 3, 2025
eeb60e5
AssayTest.testAssayLookupValidatorConversion test case for issue 53625
cnathe Oct 3, 2025
c7f7860
don't try/catch within LookupValidator.validate, handle that in the A…
cnathe Oct 3, 2025
c00d0c3
update test case for the issue found when just a numeric value is pro…
cnathe Oct 3, 2025
05b3a22
Merge branch 'develop' into fb_assayLookupValidator53625
cnathe Oct 6, 2025
d0de01d
CR feedback - remove unnecessary null check and try/catch
cnathe Oct 7, 2025
bd2b5b8
Merge branch 'develop' into fb_assayLookupValidator53625
cnathe Oct 7, 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
69 changes: 36 additions & 33 deletions api/src/org/labkey/api/assay/AbstractAssayTsvDataHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,17 +842,48 @@ else if (entry.getKey().equalsIgnoreCase(ProvenanceService.PROVENANCE_INPUT_PROP
map.put(pd.getName(), o);
}

// validate the data value for the non-sample lookup fields
// note that sample lookup mapping and validation will happen separately below in the code which handles populating materialInputs
boolean isSampleLookupById = lookupToAllSamplesById.contains(pd) || lookupToSampleTypeById.containsKey(pd);
boolean isSampleLookupByName = lookupToAllSamplesByName.contains(pd) || lookupToSampleTypeByName.containsKey(pd);

// If we have a String value for a lookup column, attempt to use the table's unique indices or display value to convert the String into the lookup value
// See similar conversion performed in SimpleTranslator.RemapPostConvertColumn
// Issue 47509: if the value is a string and is for a SampleId lookup field, let the code below which handles populating materialInputs take care of the remapping.
// Issue 53625: remap before validation so that validators can validate the remapped value
if (o != null && remappableLookup.containsKey(pd) && !isSampleLookupById)
{
String s = o instanceof String ? (String) o : o.toString();
TableInfo lookupTable = remappableLookup.get(pd);
Object remapped = cache.remap(lookupTable, s, true);
if (remapped == null)
{
if (SAMPLE_CONCEPT_URI.equals(pd.getConceptURI()))
errors.add(new PropertyValidationError(o + " not found in the current context.", pd.getName()));
else
errors.add(new PropertyValidationError("Failed to convert '" + pd.getName() + "': Could not translate value: " + o, pd.getName()));
}
else if (o != remapped)
{
o = remapped;
map.put(pd.getName(), remapped);
}
}

// validate the data value for the non-sample lookup fields
// note that sample lookup mapping and validation will happen separately below in the code which handles populating materialInputs
if (validatorMap.containsKey(pd) && !isSampleLookupById && !isSampleLookupByName)
{
for (ColumnValidator validator : validatorMap.get(pd))
{
String error = validator.validate(rowNum, o, validatorContext);
if (error != null)
errors.add(new PropertyValidationError(error, pd.getName()));
try
{
String error = validator.validate(rowNum, o, validatorContext);
if (error != null)
errors.add(new PropertyValidationError(error, pd.getName()));
}
catch (ConversionException e)
{
errors.add(new PropertyValidationError(e.getMessage(), pd.getName()));
}
}
}

Expand Down Expand Up @@ -944,34 +975,6 @@ else if (o instanceof MvFieldWrapper mvWrapper)
}
}

// If we have a String value for a lookup column, attempt to use the table's unique indices or display value to convert the String into the lookup value
// See similar conversion performed in SimpleTranslator.RemapPostConvertColumn
// Issue 47509: if the value is a string and is for a SampleId lookup field, let the code below which handles populating materialInputs take care of the remapping.
if (o instanceof String s && remappableLookup.containsKey(pd) && !isSampleLookupById)
{
TableInfo lookupTable = remappableLookup.get(pd);
try
{
Object remapped = cache.remap(lookupTable, s, true);
if (remapped == null)
{
if (pd.getConceptURI() != null && SAMPLE_CONCEPT_URI.equals(pd.getConceptURI()))
errors.add(new PropertyValidationError(o + " not found in the current context.", pd.getName()));
else
errors.add(new PropertyValidationError("Failed to convert '" + pd.getName() + "': Could not translate value: " + o, pd.getName()));
}
else if (o != remapped)
{
o = remapped;
map.put(pd.getName(), remapped);
}
}
catch (ConversionException e)
{
errors.add(new PropertyValidationError(e.getMessage(), pd.getName()));
}
}

if (!valueMissing && o == ERROR_VALUE && !wrongTypes.contains(pd.getName()))
{
wrongTypes.add(pd.getName());
Expand Down
108 changes: 102 additions & 6 deletions study/test/src/org/labkey/test/tests/study/AssayTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,24 @@
import org.labkey.test.pages.ReactAssayDesignerPage;
import org.labkey.test.pages.assay.AssayBeginPage;
import org.labkey.test.params.FieldDefinition;
import org.labkey.test.params.FieldInfo;
import org.labkey.test.params.experiment.SampleTypeDefinition;
import org.labkey.test.tests.AbstractAssayTest;
import org.labkey.test.tests.AuditLogTest;
import org.labkey.test.util.AuditLogHelper;
import org.labkey.test.util.DataRegionTable;
import org.labkey.test.util.DomainUtils;
import org.labkey.test.util.LogMethod;
import org.labkey.test.util.SampleTypeHelper;
import org.labkey.test.util.StudyHelper;
import org.labkey.test.util.TestDataGenerator;
import org.labkey.test.util.data.TestDataUtils;

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
Expand All @@ -58,6 +63,8 @@ public class AssayTest extends AbstractAssayTest
private static final String INVESTIGATOR = "Dr. No";
private static final String GRANT = "SPECTRE";
private static final String DESCRIPTION = "World Domination.";
private static final String ISSUE_53625_ASSAY = TestDataGenerator.randomDomainName("Issue53625", DomainUtils.DomainKind.Assay);
private static final String ISSUE_53625_PROJECT = "Issue53625Project";
private static final String ISSUE_53616_ASSAY = "Issue53616Assay";
private static final String ISSUE_53616_PROJECT = "Issue53616Project" + TRICKY_CHARACTERS_FOR_PROJECT_NAMES;
private static final String SAMPLE_FIELD_TEST_ASSAY = "SampleFieldTestAssay";
Expand All @@ -79,6 +86,7 @@ protected void doCleanup(boolean afterTest) throws TestTimeoutException
_containerHelper.deleteProject(getProjectName(), false);
_containerHelper.deleteProject(SAMPLE_FIELD_PROJECT_NAME, false);
_containerHelper.deleteProject(ISSUE_53616_PROJECT, false);
_containerHelper.deleteProject(ISSUE_53625_PROJECT, false);

_userHelper.deleteUsers(false, TEST_ASSAY_USR_PI1, TEST_ASSAY_USR_TECH1);
}
Expand Down Expand Up @@ -176,7 +184,7 @@ public void testSampleFieldUpdate()
ssHelper = SampleTypeHelper.beginAtSampleTypesList(this, getCurrentContainerPath());
ssHelper.createSampleType(otherDefinition, "Name\nOS_1\nOS_2");

importSampleAssayData(TEST_RUN1, "OS_1");
importAssayData(SAMPLE_FIELD_TEST_ASSAY, TEST_RUN1, "SampleField\nOS_1");
goToManageAssays().clickAndWait(Locator.linkWithText(SAMPLE_FIELD_TEST_ASSAY));
clickAndWait(Locator.linkWithText("view results"));
assertElementPresent("Sample lookup failed for: OS_1", new Locator.LinkLocator("OS_1"), 1);
Expand All @@ -193,7 +201,7 @@ public void testSampleFieldUpdate()

log("Verify updates saved successfully");
assertEquals("Error saving initial assay", 0, checker().errorsSinceMark());
importSampleAssayData(TEST_RUN2, "S_1");
importAssayData(SAMPLE_FIELD_TEST_ASSAY, TEST_RUN2, "SampleField\nS_1");
goToManageAssays().clickAndWait(Locator.linkWithText(SAMPLE_FIELD_TEST_ASSAY));
clickAndWait(Locator.linkWithText("view results"));
// assertElementPresent("Sample lookup failed for: OS_1", new Locator.LinkLocator("OS_1"), 1); //TODO this becomes the RowId "<123>" after change. Issue #40047
Expand All @@ -211,7 +219,7 @@ public void testSampleFieldUpdate()
assertEquals("Error saving updated sample field", 0, checker().errorsSinceMark());

log("Verify updates saved successfully");
importSampleAssayData(TEST_RUN3, "S_2\nOS_2");
importAssayData(SAMPLE_FIELD_TEST_ASSAY, TEST_RUN3, "SampleField\nS_2\nOS_2");
assertEquals("Error importing data after assay sample field update", 0, checker().errorsSinceMark());

goToManageAssays().clickAndWait(Locator.linkWithText(SAMPLE_FIELD_TEST_ASSAY));
Expand All @@ -223,14 +231,14 @@ public void testSampleFieldUpdate()

}

private void importSampleAssayData(String runName, String sampleId)
private void importAssayData(String assayName, String runName, String runDataStr)
{
goToManageAssays();
clickAndWait(Locator.linkWithText(SAMPLE_FIELD_TEST_ASSAY));
clickAndWait(Locator.linkWithText(assayName));
clickButton("Import Data", "Run Data");
setFormElement(AssayConstants.ASSAY_NAME_FIELD_LOCATOR, runName);
click(AssayConstants.TEXT_AREA_DATA_PROVIDER_LOCATOR);
setFormElement(AssayConstants.TEXT_AREA_DATA_COLLECTOR_LOCATOR, "SampleField\n" + sampleId);
setFormElement(AssayConstants.TEXT_AREA_DATA_COLLECTOR_LOCATOR, runDataStr);
clickButton("Save and Finish");

}
Expand Down Expand Up @@ -996,6 +1004,94 @@ private void verifySpecimensPresent(int aaa07Count, int controlCount, int baq000
assertTextPresent("BAQ00051", baq00051Count * 2);
}

@Test // Issue 53625
public void testAssayLookupValidatorConversion()
{
_containerHelper.createProject(ISSUE_53625_PROJECT, "Assay");
goToProjectHome(ISSUE_53625_PROJECT);

log("Create a list with an int key and a string value");
String lookToListName = TestDataGenerator.randomDomainName("lookToList", DomainUtils.DomainKind.IntList);
String keyName = TestDataGenerator.randomFieldName("key", null, DomainUtils.DomainKind.IntList);
FieldInfo valueField = FieldInfo.random("value", FieldDefinition.ColumnType.String, DomainUtils.DomainKind.IntList);
_listHelper.createList(ISSUE_53625_PROJECT, lookToListName, keyName, valueField.getFieldDefinition());
_listHelper.bulkImportData(TestDataUtils.tsvStringFromRowMaps(List.of(
Map.of(valueField.getName(), "One"),
Map.of(valueField.getName(), "Two"),
Map.of(valueField.getName(), "123")
), List.of(valueField.getName()), true));

log("Create an assay with a results lookup field to the list, with lookup validator set");
goToProjectHome(ISSUE_53625_PROJECT);
FieldInfo lookupField = new FieldInfo("lookup", new FieldDefinition.IntLookup(null, "lists", lookToListName));
ReactAssayDesignerPage designerPage = _assayHelper.createAssayDesign("General", ISSUE_53625_ASSAY);
designerPage.goToBatchFields()
.removeAllFields(false);
designerPage.goToResultsFields()
.removeAllFields(false)
.manuallyDefineFields(lookupField.getFieldDefinition().setLookupValidatorEnabled(true));
designerPage.clickFinish();

log("Verify importing an assay run with valid and invalid values for the lookup field");
verifyAssayImportForLookupValidator(ISSUE_53625_ASSAY, lookupField, "RunWithLookupValidator", true);

log("Turn off lookup field validator and test the imports again");
designerPage = _assayHelper.clickEditAssayDesign();
designerPage.goToResultsFields()
.getField(lookupField.getName())
.setLookupValidatorEnabled(false);
designerPage.clickFinish();
verifyAssayImportForLookupValidator(ISSUE_53625_ASSAY, lookupField, "RunWithoutLookupValidator", false);
}

private void verifyAssayImportForLookupValidator(String assayName, FieldInfo lookupField, String runName, boolean validatorOn)
{
String runDataStr = TestDataUtils.tsvStringFromRowMaps(List.of(
Map.of(lookupField.getName(), "One"), // valid
Map.of(lookupField.getName(), "99")), // invalid
List.of(lookupField.getName()), true
);
importAssayData(assayName, runName, runDataStr);
assertTextPresent("Could not translate value: 99");
if (validatorOn) assertTextPresent("Value '99' was not present in lookup target");
else assertTextNotPresent("Value '99' was not present in lookup target");
clickButton("Cancel");

runDataStr = TestDataUtils.tsvStringFromRowMaps(List.of(
Map.of(lookupField.getName(), "Three"), // invalid
Map.of(lookupField.getName(), "2")), // valid
List.of(lookupField.getName()), true
);
importAssayData(assayName, runName, runDataStr);
assertTextPresent("Failed to convert");
assertTextPresent("Could not translate value: Three");
clickButton("Cancel");

runDataStr = TestDataUtils.tsvStringFromRowMaps(List.of(
Map.of(lookupField.getName(), "One"), // valid
Map.of(lookupField.getName(), "2"), // valid
Map.of(lookupField.getName(), "123")), // valid
List.of(lookupField.getName()), true
);
importAssayData(assayName, runName, runDataStr);
clickAndWait(Locator.linkWithText(runName));
DataRegionTable dataTable = new DataRegionTable("Data", getDriver());
checker().verifyEquals("Incorrect number of results shown.", 3, dataTable.getDataRowCount());
checker().verifyEquals("Lookup values not as expected.", List.of("One", "Two", "123"), dataTable.getColumnDataAsText(lookupField.getLabel()));

// test with just the numeric value since that was causing issues during manual testing
runName = runName + "NumericOnly";
runDataStr = TestDataUtils.tsvStringFromRowMaps(List.of(
Map.of(lookupField.getName(), "123")), // valid
List.of(lookupField.getName()), true
);
importAssayData(assayName, runName, runDataStr);
clickAndWait(Locator.linkWithText(runName));
dataTable = new DataRegionTable("Data", getDriver());
checker().verifyEquals("Incorrect number of results shown.", 1, dataTable.getDataRowCount());
checker().verifyEquals("Lookup values not as expected.", List.of("123"), dataTable.getColumnDataAsText(lookupField.getLabel()));
}

@Override
protected BrowserType bestBrowser()
{
Expand Down