Skip to content

Revise file/attachment exception handling #6949

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
labkey-nicka merged 1 commit into from
Aug 20, 2025
Merged

Revise file/attachment exception handling #6949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/assay/AbstractAssayTsvDataHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,7 @@ else if (o instanceof MvFieldWrapper mvWrapper)
}
catch (ConvertHelper.FileConversionException e)
{
throw new ApiUsageException(e);
errors.add(new PropertyValidationError(e.getMessage(), pd.getName()));
Copy link
Contributor

@XingY XingY Aug 20, 2025

Choose a reason for hiding this comment

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

Would this result in inconsistent error msg for FileLink? Can we just do errors.add(e.gerMessage())?

Copy link
Contributor Author

@labkey-nicka labkey-nicka Aug 20, 2025

Choose a reason for hiding this comment

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

I looked into the exact place and its trivial to handle in a similar manner BUT it causes the exception chaining that we're trying to avoid with those ApiUsageException opt-outs. I'm going to investigate a better way so we could do this.

In the meantime, I plan on merging these changes as-is assuming test runs look good.

}
}
}
Expand Down
22 changes: 12 additions & 10 deletions api/src/org/labkey/api/assay/DefaultAssayRunCreator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,23 +482,25 @@ public ExpExperiment saveExperimentRun(

return batch;
}
catch (ExperimentException | IOException | ConvertHelper.FileConversionException e)
catch (ExperimentException | IOException | ConvertHelper.FileConversionException | BatchValidationException e)
{
// TODO: This is better done as a post-rollback task on the transaction
// clean up the run results file dir here if it was created, for non-async imports
AssayResultsFileWriter<?> resultsFileWriter = new AssayResultsFileWriter<>(context.getProtocol(), run, null);
resultsFileWriter.cleanupPostedFiles(context.getContainer(), false);

cleanPrimaryFile(context);

if (e instanceof ExperimentException)
throw (ExperimentException)e;
else if (e instanceof ConvertHelper.FileConversionException)
throw new ApiUsageException(e.getMessage(), e);
else
throw new ExperimentException(e);
}
catch (BatchValidationException e)
{
if (e instanceof ExperimentException ee)
throw ee;

// HACK: Rethrowing these as ApiUsageException avoids any upstream consequences of wrapping them in ExperimentException.
// Namely, that they are logged to the server/mothership. There has to be a better way.
if (e instanceof ConvertHelper.FileConversionException fce)
throw new ApiUsageException(fce.getMessage(), fce);
else if (e instanceof BatchValidationException bve)
throw new ApiUsageException(bve.getMessage(), bve);

throw new ExperimentException(e);
}
}
Expand Down
38 changes: 23 additions & 15 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 @@ -43,10 +43,6 @@
import java.io.InputStream;
import java.util.ArrayList;

/**
* Moved from ListQueryUpdateService.java
* by iansigmon on 2/16/16.
*/
public class AttachmentDataIterator extends WrapperDataIterator
{
final VirtualFile attachmentDir;
Expand Down Expand Up @@ -92,19 +88,21 @@ public boolean next() throws BatchValidationException
for (_AttachmentUploadHelper p : attachmentColumns)
{
Object attachmentValue = get(p.index);
if (null == attachmentValue)
continue;

String filename;
AttachmentFile attachmentFile;

if (null == attachmentValue)
continue;
else if (attachmentValue instanceof String str)
if (attachmentValue instanceof String str)
{
if (null == attachmentDir)
{
errors.addRowError(new ValidationException("Row " + get(0) + ": " + "Can't upload '" + str + "' to field " + p.domainProperty.getName() + " with type " + p.domainProperty.getType().getLabel() + "."));
errors.addRowError(propertyValidationException(p.domainProperty, attachmentValue));
return false;
}
filename = (String) attachmentValue;

filename = str;
InputStream aIS = attachmentDir.getDir(p.domainProperty.getName()).getInputStream(p.uniquifier.uniquify(filename));
if (aIS == null)
{
Expand All @@ -113,25 +111,25 @@ else if (attachmentValue instanceof String str)
}
attachmentFile = new InputStreamAttachmentFile(aIS, filename);
}
else if (attachmentValue instanceof AttachmentFile)
else if (attachmentValue instanceof AttachmentFile file)
{
attachmentFile = (AttachmentFile) attachmentValue;
attachmentFile = file;
filename = attachmentFile.getFilename();
}
else if (attachmentValue instanceof File)
else if (attachmentValue instanceof File file)
{
attachmentFile = new FileAttachmentFile((File) attachmentValue);
attachmentFile = new FileAttachmentFile(file);
filename = attachmentFile.getFilename();
}
else
{
errors.addRowError(new ValidationException("Row " + get(0) + ": " + "Unable to create attachment file."));
errors.addRowError(propertyValidationException(p.domainProperty, attachmentValue));
return false;
}

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

Expand Down Expand Up @@ -172,6 +170,16 @@ else if (attachmentValue instanceof File)
}
}

private ValidationException propertyValidationException(DomainProperty property, Object value)
{
return rowValidationException(String.format("Can't upload '%s' to field %s with type %s.", value, property.getName(), property.getType().getLabel()));
}

private ValidationException rowValidationException(String message)
{
return new ValidationException("Row " + get(0) + ": " + message);
}

public static DataIteratorBuilder getAttachmentDataIteratorBuilder(TableInfo ti, @NotNull final DataIteratorBuilder builder,
final User user,
@Nullable final VirtualFile attachmentDir,
Expand Down