Skip to content

Issue 53498: Sample Manager: Attachment field is lost after export/import round trip #6894

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
XingY merged 2 commits into from
Aug 4, 2025
Merged

Issue 53498: Sample Manager: Attachment field is lost after export/import round trip #6894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22b9705
Issue 53498: Sample Manager: Attachment field is lost after export/im…
XingY Jul 31, 2025
a30e622
Fix attachment field with underscore
XingY Aug 1, 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
16 changes: 11 additions & 5 deletions api/src/org/labkey/api/dataiterator/AttachmentDataIterator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,11 @@ public boolean next()

if (null == attachmentValue)
continue;
else if (attachmentValue instanceof String)
else if (attachmentValue instanceof String str)
{
if (null == attachmentDir)
{
errors.addRowError(new ValidationException("Row " + get(0) + ": " + "Can't upload to field " + p.domainProperty.getName() + " with type " + p.domainProperty.getType().getLabel() + "."));
errors.addRowError(new ValidationException("Row " + get(0) + ": " + "Can't upload '" + str + "' to field " + p.domainProperty.getName() + " with type " + p.domainProperty.getType().getLabel() + "."));
return false;
}
filename = (String) attachmentValue;
Expand All @@ -124,7 +124,13 @@ else if (attachmentValue instanceof File)
}
else
{
errors.addRowError(new ValidationException("Row " + get(0) + ": " + "Unable to create attachament file."));
errors.addRowError(new ValidationException("Row " + get(0) + ": " + "Unable to create attachment file."));
return false;
}

if (entityIdIndex == 0)
{
errors.addRowError(new ValidationException("Row " + get(0) + ": " + "Unable to create attachment file."));
return false;
}

Expand All @@ -139,7 +145,7 @@ else if (attachmentValue instanceof File)
if (null != attachmentFiles && !attachmentFiles.isEmpty())
{
String entityId = String.valueOf(get(entityIdIndex));
AttachmentService.get().addAttachments(getAttachmentParent(entityId, container) , attachmentFiles, user);
AttachmentService.get().addAttachments(getAttachmentParent(entityId, container), attachmentFiles, user);
}
return ret;
}
Expand Down Expand Up @@ -217,7 +223,7 @@ public static DataIteratorBuilder getAttachmentDataIteratorBuilder(TableInfo ti,
}
}

if (!attachmentColumns.isEmpty() && 0 != entityIdIndex)
if (!attachmentColumns.isEmpty())
return new AttachmentDataIterator(it, context.getErrors(), user, attachmentDir, entityIdIndex, attachmentColumns, context.getInsertOption(), container, parentFactory );

return it;
Expand Down
4 changes: 3 additions & 1 deletion api/src/org/labkey/api/dataiterator/StandardDataIteratorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,8 +277,10 @@ public DataIterator getDataIterator(DataIteratorContext context)
PropertyType pt = pd == null ? null : pd.getPropertyType();
boolean isAttachment = pt == PropertyType.ATTACHMENT || pt == PropertyType.FILE_LINK;

if (null == pair.target || isAttachment)
if (null == pair.target)
convert.addColumn(pair.indexFrom);
else if (isAttachment) // Issue 53498: attachment is blank after update from file, if the field name contains underscore
convert.addColumn(pair.target, pair.indexFrom);
else
convert.addConvertColumn(pair.target, pair.indexFrom, pair.indexMv, pd, pt, pair.target.getRemapMissingBehavior(), context.isWithLookupRemapping());
}
Expand Down
75 changes: 57 additions & 18 deletions api/src/org/labkey/api/reader/DataLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.labkey.api.dataiterator.ScrollableDataIterator;
import org.labkey.api.exp.MvColumn;
import org.labkey.api.exp.MvFieldWrapper;
import org.labkey.api.exp.PropertyType;
import org.labkey.api.iterator.CloseableFilteredIterator;
import org.labkey.api.iterator.CloseableIterator;
import org.labkey.api.query.BatchValidationException;
Expand Down Expand Up @@ -327,7 +328,7 @@ private void inferColumnInfo(@NotNull Map<String, String> renamedColumns) throws
for (int f = 0; f < nCols; f++)
{
List<Class> classesToTest = new ArrayList<>(Arrays.asList(CONVERT_CLASSES));
Class knownColumnClass = null;
Class knownColumnClass;

int classIndex = -1;
//NOTE: this means we have a header row
Expand All @@ -337,27 +338,21 @@ private void inferColumnInfo(@NotNull Map<String, String> renamedColumns) throws
{
String name = lineFields[0][f];
name = StringUtilsLabKey.sanitizeSeparatorsAndTrim(name);
if (_columnInfoMap.containsKey(name))
{
//preferentially use this class if it matches
knownColumnClass = _columnInfoMap.get(name).getJavaClass();
classesToTest.add(0, knownColumnClass);
}
else if (renamedColumns.containsKey(name) && _columnInfoMap.containsKey(renamedColumns.get(name)))

ColumnInfo knownColumn = getKnownColumn(name, renamedColumns);

if (knownColumn != null)
{
knownColumnClass = _columnInfoMap.get(renamedColumns.get(name)).getJavaClass();
knownColumnClass = knownColumn.getJavaClass();
classesToTest.add(0, knownColumnClass);

// Issue 49830: if we know the column class is File based on the columnInfoMap, use it instead of trying to infer the class based on the data
if (setFileColDescriptor(colDescs[f], knownColumn))
Copy link
Contributor

@labkey-susanh labkey-susanh Jul 31, 2025

Choose a reason for hiding this comment

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

Seems like this could come before the setting of knownColumnClass.

continue;
}
}
}

// Issue 49830: if we know the column class is File based on the columnInfoMap, use it instead of trying to infer the class based on the data
if (File.class.equals(knownColumnClass))
{
colDescs[f].clazz = knownColumnClass;
continue;
}

for (int line = inferStartLine; line < numLines; line++)
{
if (f >= lineFields[line].length)
Expand Down Expand Up @@ -458,7 +453,8 @@ else if (renamedColumns.containsKey(name) && _columnInfoMap.containsKey(renamedC
Set<String> columnNames = new HashSet<>();
for (ColumnDescriptor colDesc : colDescs)
{
if (!columnNames.add(colDesc.name) && isThrowOnErrors())
String name = colDesc.name;
if (!columnNames.add(name) && isThrowOnErrors())
{
// TODO: This should be refactored to not throw this here, but rather, have the callers check themselves. It
// is not in the interest of inferring columns that we validate duplicate columns.
Expand All @@ -468,11 +464,45 @@ else if (renamedColumns.containsKey(name) && _columnInfoMap.containsKey(renamedC
ExceptionUtil.decorateException(e, ExceptionUtil.ExceptionInfo.SkipMothershipLogging, "true", true);
throw e;
}

// use File converter for known file fields even if inferTypes = false. If inferTypes, this is already done.
if (!getInferTypes())
Copy link
Contributor

@labkey-susanh labkey-susanh Jul 31, 2025

Choose a reason for hiding this comment

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

Seems like you could leave off the if statement here for a cleaner read since setting again won't hurt, but this is slightly more efficient.

setFileColDescriptor(colDesc, getKnownColumn(name, renamedColumns));
}

_columns = colDescs;
}

private boolean setFileColDescriptor(ColumnDescriptor colDesc, @Nullable ColumnInfo knownColumn)
{
if (knownColumn == null)
return false;

Class knownColumnClass = knownColumn.getJavaClass();

if (File.class.equals(knownColumnClass))
{
if (PropertyType.ATTACHMENT.equals(knownColumn.getPropertyType()))
colDesc.clazz = String.class;
else
colDesc.clazz = knownColumnClass;

return true;
}

return false;
}

private ColumnInfo getKnownColumn(String name, @NotNull Map<String, String> renamedColumns)
{
ColumnInfo knownColumn = null;
if (_columnInfoMap.containsKey(name))
knownColumn = _columnInfoMap.get(name);
else if (renamedColumns.containsKey(name) && _columnInfoMap.containsKey(renamedColumns.get(name)))
knownColumn = _columnInfoMap.get(renamedColumns.get(name));
return knownColumn;
}

protected String getDefaultColumnName(int col)
{
return "column" + col;
Expand Down Expand Up @@ -955,7 +985,16 @@ public DataIterator getDataIterator(final DataIteratorContext context)
/** Actually create an instance of DataIterator to use, which might be subclass-specific */
protected DataIterator createDataIterator(DataIteratorContext context) throws IOException
{
return new _DataIterator(context, getActiveColumns(), isScrollable());
ColumnDescriptor[] columnDescriptors = getActiveColumns();
if (context.isCrossFolderImport() || context.isCrossTypeImport())
{
for (ColumnDescriptor columnDescriptor : columnDescriptors)
{
if (columnDescriptor.clazz.equals(File.class))
columnDescriptor.clazz = String.class; // defer file path validation for cross type/folder import
}
}
return new _DataIterator(context, columnDescriptors, isScrollable());
}

protected class _DataIterator implements ScrollableDataIterator, MapDataIterator
Expand Down