Skip to content

GitHub Issue 915: Bulk edit doesn't completely remove attachments for sources #7543

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

Open
XingY wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

GitHub Issue 915: Bulk edit doesn't completely remove attachments for sources #7543

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
3cf91e7
GitHub Issue 915: Bulk edit doesn't completely remove attachments for…
XingY Apr 3, 2026
e75f427
CRLF
XingY Apr 3, 2026
d86537c
fix
XingY Apr 3, 2026
0150443
fix import
XingY Apr 3, 2026
1b338f1
Merge remote-tracking branch 'origin/develop' into fb_attachmentDIB
XingY Apr 6, 2026
ddc907a
Selenium test for GitHub Issue 915: Bulk edit doesn't completely remo…
XingY Apr 6, 2026
d024751
Merge remote-tracking branch 'origin/develop' into fb_attachmentDIB
XingY Apr 6, 2026
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
66 changes: 61 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 @@ -15,6 +15,7 @@
*/
package org.labkey.api.dataiterator;

import org.apache.commons.lang3.StringUtils;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.labkey.api.attachments.AttachmentFile;
Expand Down Expand Up @@ -42,13 +43,17 @@
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class AttachmentDataIterator extends WrapperDataIterator
{
final VirtualFile attachmentDir;
final BatchValidationException errors;
final int entityIdIndex;
final ArrayList<_AttachmentUploadHelper> attachmentColumns;
final Map<String, String> attachmentColumnsAliases;
final QueryUpdateService.InsertOption insertOption;
final User user;
final Container container;
Expand All @@ -60,6 +65,7 @@ public class AttachmentDataIterator extends WrapperDataIterator
@Nullable VirtualFile attachmentDir,
int entityIdIndex,
ArrayList<_AttachmentUploadHelper> attachmentColumns,
Map<String, String> attachmentColumnsAliases,
QueryUpdateService.InsertOption insertOption,
Container container,
AttachmentParentFactory parentFactory)
Expand All @@ -69,6 +75,7 @@ public class AttachmentDataIterator extends WrapperDataIterator
this.errors = errors;
this.entityIdIndex = entityIdIndex;
this.attachmentColumns = attachmentColumns;
this.attachmentColumnsAliases = attachmentColumnsAliases;
this.insertOption = insertOption;
this.user = user;
this.container = container;
Expand All @@ -83,19 +90,53 @@ public boolean next() throws BatchValidationException
return false;

ArrayList<AttachmentFile> attachmentFiles = null;
List<String> oldAttachments = new ArrayList<>();

try
{
Map<String, Object> existing = getExistingRecord();

for (_AttachmentUploadHelper p : attachmentColumns)
{
Object attachmentValue = get(p.index);
String oldAttachmentValue = null;
if (insertOption.allowUpdate && existing != null)
{
// GitHub Issue 915: Bulk edit doesn't completely remove attachments for sources
Object oldValue = existing.get(p.domainProperty.getName());
if (oldValue == null)
oldValue = existing.get(attachmentColumnsAliases.get(p.domainProperty.getName()));
if (oldValue != null)
oldAttachmentValue = oldValue.toString();
}

if (null == attachmentValue)
{
// Remove existing attachment
if (!StringUtils.isEmpty(oldAttachmentValue))
oldAttachments.add(oldAttachmentValue);
continue;
}

String filename;
AttachmentFile attachmentFile;

if (attachmentValue instanceof String str)
{
if (StringUtils.isEmpty(str))
{
// Remove existing attachment
if (!StringUtils.isEmpty(oldAttachmentValue))
oldAttachments.add(oldAttachmentValue);
continue;
}

if (str.equals(oldAttachmentValue)) // no change
continue;

if (!StringUtils.isEmpty(oldAttachmentValue)) // replace old attachment with new attachment, so mark old attachment for deletion
oldAttachments.add(oldAttachmentValue);

if (null == attachmentDir)
{
errors.addRowError(propertyValidationException(p.domainProperty, attachmentValue));
Expand All @@ -115,11 +156,15 @@ else if (attachmentValue instanceof AttachmentFile file)
{
attachmentFile = file;
filename = attachmentFile.getFilename();
if (!StringUtils.isEmpty(oldAttachmentValue))
oldAttachments.add(oldAttachmentValue);
}
else if (attachmentValue instanceof File file)
{
attachmentFile = new FileAttachmentFile(file);
filename = attachmentFile.getFilename();
if (!StringUtils.isEmpty(oldAttachmentValue))
oldAttachments.add(oldAttachmentValue);
}
else
{
Expand All @@ -141,11 +186,18 @@ else if (attachmentValue instanceof File file)
attachmentFiles.add(attachmentFile);
}

if ((null == attachmentFiles || attachmentFiles.isEmpty()) && oldAttachments.isEmpty())
Copy link
Copy Markdown
Contributor

@labkey-susanh labkey-susanh Apr 3, 2026

Choose a reason for hiding this comment

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

Could simplify here by initializing attachmentFiles to an empty list.

return ret;

String entityId = String.valueOf(get(entityIdIndex));
var attachmentParent = getAttachmentParent(entityId, container);

if (!oldAttachments.isEmpty())
AttachmentService.get().deleteAttachments(attachmentParent, oldAttachments, user);

if (null != attachmentFiles && !attachmentFiles.isEmpty())
{
String entityId = String.valueOf(get(entityIdIndex));
AttachmentService.get().addAttachments(getAttachmentParent(entityId, container), attachmentFiles, user);
}
AttachmentService.get().addAttachments(attachmentParent, attachmentFiles, user);

return ret;
}
catch (AttachmentService.DuplicateFilenameException | AttachmentService.FileTooLargeException e)
Expand Down Expand Up @@ -212,6 +264,7 @@ public static DataIteratorBuilder getAttachmentDataIteratorBuilder(TableInfo ti,
// find attachment columns
int entityIdIndex = 0;
final ArrayList<_AttachmentUploadHelper> attachmentColumns = new ArrayList<>();
final Map<String, String> attachmentColumnsAliases = new HashMap<>();

for (int c = 1; c <= it.getColumnCount(); c++)
{
Expand All @@ -229,14 +282,17 @@ public static DataIteratorBuilder getAttachmentDataIteratorBuilder(TableInfo ti,
continue;

attachmentColumns.add(new _AttachmentUploadHelper(c,domainProperty));
attachmentColumnsAliases.put(domainProperty.getName(), col.getAlias().getId());
}
catch (IndexOutOfBoundsException ignored) // Until issue is resolved between StatementDataIterator.getColumnCount() and SimpleTranslator.getColumnCount()
{
}
}

if (!attachmentColumns.isEmpty())
return new AttachmentDataIterator(it, context.getErrors(), user, attachmentDir, entityIdIndex, attachmentColumns, context.getInsertOption(), container, parentFactory );
{
return new AttachmentDataIterator(it, context.getErrors(), user, attachmentDir, entityIdIndex, attachmentColumns, attachmentColumnsAliases, context.getInsertOption(), container, parentFactory);
}

return it;
};
Expand Down
7 changes: 6 additions & 1 deletion api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.labkey.api.module.Module;
import org.labkey.api.module.ModuleLoader;
import org.labkey.api.query.BatchValidationException;
import org.labkey.api.query.DefaultQueryUpdateService;
import org.labkey.api.query.InvalidKeyException;
import org.labkey.api.query.QueryUpdateService;
import org.labkey.api.query.QueryUpdateServiceException;
Expand Down Expand Up @@ -238,10 +239,14 @@ public static DataIteratorBuilder createBuilder(DataIteratorBuilder dib, TableIn
QueryUpdateService.InsertOption option = context.getInsertOption();
if (option.allowUpdate)
{
boolean hasAttachmentProperties = false;
QueryUpdateService qus = target.getUpdateService();
if (qus instanceof DefaultQueryUpdateService dQus)
hasAttachmentProperties = dQus.hasAttachmentProperties(); // if true, we need to fetch existing records to properly handle old attachment delete
AuditBehaviorType auditType = AuditBehaviorType.NONE;
if (target.supportsAuditTracking())
auditType = target.getEffectiveAuditBehavior((AuditBehaviorType) context.getConfigParameter(DetailedAuditLogDataIterator.AuditConfigs.AuditBehavior));
boolean detailed = auditType == DETAILED;
boolean detailed = auditType == DETAILED || hasAttachmentProperties;
if (useGetRows)
return new ExistingDataIteratorsGetRows(new CachingDataIterator(di), target, keys, sharedKeys, context, detailed);
else
Expand Down
2 changes: 1 addition & 1 deletion api/src/org/labkey/api/query/DefaultQueryUpdateService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,7 +893,7 @@ protected void setSpecialColumns(Container container, Map<String,Object> row, Us
}
}

protected boolean hasAttachmentProperties()
public boolean hasAttachmentProperties()
{
Domain domain = getDomain();
if (null != domain)
Expand Down
Loading