Skip to content

Issue 53431: Data Class and Sample Type data doesn't round-trip via folder export/import for field names with special char #6840

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
Jul 15, 2025
Merged

Issue 53431: Data Class and Sample Type data doesn't round-trip via folder export/import for field names with special char #6840

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
b0365ca
PlateManager.exportPlateData to use ColumnHeaderType.ImportField
cnathe Jul 11, 2025
9764864
Issue 53431: Data Class and Sample Type data doesn't round-trip via f…
cnathe Jul 11, 2025
abb1457
remove comment re: ColumnHeaderType.FieldKey being "useful for import…
cnathe Jul 14, 2025
cdf8fbb
DisplayFieldKey does not round-trip for import. Use ImportField instead
labkey-susanh Jul 14, 2025
6c1ac61
ListWriter add issue number comment
cnathe Jul 14, 2025
295134b
DatasetDataWriter does not round-trip fields with special chars on im…
cnathe Jul 14, 2025
defdee0
StudyDatasetsTest test case for 53431
cnathe Jul 14, 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
2 changes: 1 addition & 1 deletion api/src/org/labkey/api/data/ColumnHeaderType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public String getText(DisplayColumn dc)
}
},

// Use the ColumnInfo's FieldKey with FieldKey escaping. Useful for import/export round-tripping, but can lead to ugly names.
// Use the ColumnInfo's FieldKey with FieldKey escaping.
FieldKey("Field Key", "The column name rendered with FieldKey encoding; unambiguous and canonical, useful for exporting and re-importing.") {
@Override
public String getText(DisplayColumn dc)
Expand Down
2 changes: 1 addition & 1 deletion assay/src/org/labkey/assay/plate/PlateManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3768,7 +3768,7 @@ public List<PlateFileBytes> exportPlateData(Container c, User user, ContainerFil
try (TSVGridWriter writer = new TSVGridWriter(plateQueryView::getResults, displayColumns, Collections.singletonMap(sampleIdNameFieldKey.toString(), "Sample ID")))
{
writer.setDelimiterCharacter(delim);
writer.setColumnHeaderType(ColumnHeaderType.FieldKey);
Copy link
Contributor

@labkey-nicka labkey-nicka Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main question is about the usage of these different ColumnHeaderType. Reading through the comments that surround them I'm still not sure which one to choose in which situation.

It looks like up to this point all the usages of ImportField align with usage in import templates, however, there are other usages of ColumnHeaderType.FieldKey that would seem to align with what you're using them for here.

Is ImportField just the new and improved FieldKey that we should be using more widely (I assume not)? I may be off-base here as you may have already discussed with @labkey-susanh who looks to familiarity with this enumeration.

Copy link
Contributor Author

@cnathe cnathe Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. This enum is confusing. The comment for FieldKey, which has been there since 2015 saying "Useful for import/export round-tripping" is wrong. We do not support FieldKey on import (see my attempt to add it here #6270). Maybe it is time to remove that part of the comment.

Copy link
Contributor

@labkey-susanh labkey-susanh Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImportField was added specifically because, as Cory said, round-tripping with FieldKey doesn't work currently. Given this, I'm not sure when FieldKey is useful as a column header type. Perhaps it worked at some point, but I doubt it. Since this targets 25.7, we shouldn't remove it altogether here, I think, but might consider that for develop.

writer.setColumnHeaderType(ColumnHeaderType.ImportField); // Issue 53431
writer.write(plateFileBytes.bytes);
}

Expand Down
2 changes: 1 addition & 1 deletion experiment/src/org/labkey/experiment/samples/AbstractExpFolderWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ protected void writeTsv(TableInfo tinfo, Collection<ColumnInfo> columns, SimpleF
try (TSVGridWriter tsvWriter = new TSVGridWriter(factory))
{
tsvWriter.setApplyFormats(false);
tsvWriter.setColumnHeaderType(ColumnHeaderType.FieldKey);
tsvWriter.setColumnHeaderType(ColumnHeaderType.ImportField); // Issue 53431
PrintWriter out = dir.getPrintWriter(baseName + ".tsv");
tsvWriter.write(out);
}
Expand Down
2 changes: 1 addition & 1 deletion list/src/org/labkey/list/model/ListWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ private void writeLists(List<Pair<String, ListDefinition>> listsToExport, Virtua
try (TSVGridWriter tsvWriter = new TSVGridWriter(()->QueryService.get().getSelectBuilder(ti).columns(columns).sort(sort).select(null, false), displayColumns))
{
tsvWriter.setApplyFormats(false);
tsvWriter.setColumnHeaderType(ColumnHeaderType.DisplayFieldKey); // CONSIDER: Use FieldKey instead
tsvWriter.setColumnHeaderType(ColumnHeaderType.ImportField); // Issue 53431
PrintWriter out = listsDir.getPrintWriter( def.getName() + ".tsv");
tsvWriter.write(out);
}
Expand Down
2 changes: 1 addition & 1 deletion study/src/org/labkey/study/writer/DatasetDataWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ private void writeResultsToTSV(ResultsFactory factory, VirtualFile vf, String fi
try (TSVGridWriter tsvWriter = new TSVGridWriter(factory))
{
tsvWriter.setApplyFormats(false);
tsvWriter.setColumnHeaderType(ColumnHeaderType.DisplayFieldKey); // CONSIDER: Use FieldKey instead
tsvWriter.setColumnHeaderType(ColumnHeaderType.ImportField); // Issue 53431
PrintWriter out = vf.getPrintWriter(fileName);
tsvWriter.write(out);
}
Expand Down
36 changes: 36 additions & 0 deletions study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.labkey.test.pages.ImportDataPage;
import org.labkey.test.pages.TimeChartWizard;
import org.labkey.test.pages.study.DatasetDesignerPage;
import org.labkey.test.params.FieldInfo;
import org.labkey.test.util.AuditLogHelper;
import org.labkey.test.util.DataRegionTable;
import org.labkey.test.util.LogMethod;
Expand All @@ -41,6 +42,7 @@
import org.labkey.test.util.TestDataGenerator;
import org.openqa.selenium.WebElement;

import java.io.File;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -212,6 +214,40 @@ public void testDatasetSubjectId()
clickTab("Overview");
}

@Test
public void testDatasetRoundTripWithSpecialChars() // Issue 53431
{
goToManageStudy();
String datasetName = "Issue 53431";
FieldInfo fieldInfo = FieldInfo.random("test,./field");
DatasetDesignerPage definitionPage = _studyHelper.goToManageDatasets()
.clickCreateNewDataset()
.setName(datasetName);
DomainFormPanel panel = definitionPage.getFieldsPanel();
panel.manuallyDefineFields(fieldInfo.getFieldDefinition());
definitionPage.clickSave();
importDatasetData(datasetName, "mouseId\tsequenceNum\t\"" + fieldInfo.getName() + "\"\n", "a1\t1\ttest123", "All data");

File exportedFolder = exportFolderAsZip(null, false, false, false, false);
deleteStudy();
importFolderFromZip(exportedFolder, false, 2, false);
_studyHelper.goToManageDatasets()
.selectDatasetByName(datasetName)
.clickViewData();
DataRegionTable drt = new DataRegionTable("Dataset", getDriver());
checker().verifyEquals("Field data didn't import as expected", "test123",
drt.getDataAsText(0, fieldInfo));
}

private void deleteStudy()
{
clickTab("Manage");
clickButton("Delete Study");
checkCheckbox(Locator.checkboxByName("confirm"));
clickButton("Delete", WAIT_FOR_PAGE * 2);
}


@LogMethod
protected void createDataset(@LoggedParam String name, @Nullable String error)
{
Expand Down