Skip to content

patch: file upload errors #1351

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
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

patch: file upload errors #1351

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
741d35c
file upload errors
kitttang Aug 18, 2025
d8b14d4
Update uploadService.ts
kitttang Sep 17, 2025
1535832
Update handleUpload.ts
kitttang Sep 18, 2025
62759cd
Update handleUpload.ts
kitttang Sep 22, 2025
2763306
Changing to plural and removing field name for GDS
kitttang Oct 9, 2025
f2df74e
Merge branch 'main' into file_error_fix
kitttang Oct 9, 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
20 changes: 9 additions & 11 deletions runner/src/server/plugins/engine/pluginHandlers/files/prehandlers/handleUpload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,13 @@ export async function handleUpload(
}

let response;

const errors = new Set();
try {
response = await uploadService.uploadDocuments(streams);
} catch (err) {
if (err.data?.res) {
const { error } = uploadService.parsedDocumentUploadResponse(err.data);
request.pre.errors = [
Copy link
Contributor

Choose a reason for hiding this comment

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

please keep since we don't want to lose any errors that have accumulated up to this point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jenbutongit Is it safe to delete error writing in the if (error) on line 102 instead?

And is keeping "response = uploadService.parsedDocumentUploadResponse(err.data);" okay? I've done this so "const { location, warning, error } = response;" can unpack

Copy link
Member

Choose a reason for hiding this comment

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

Doing it in this way will result in the error being added to request.pre.errors twice (when an error is thrown) - you add response.error in the catch, then unpack the response and add it again in the if(error) block. Do you need the response when an error is thrown? (e.g. are you using location or warning?)

Copy link
Contributor

@jenbutongit jenbutongit Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@superafroman Do you want to resolve/review since the implementation has changed? My initial comment is also redundant.

As a side note - I've noticed an issue with the code (not introduced by this change). uploadService.parsedDocumentUploadResponse is called internally in uploadService, but is called again on err.data. We have httpService.request (and httpService.post) which looks like it wants to suppress the thrown error and return it in an object instead

Looking at uploadService.uploadDocuments - I think the intention is that it returns an object containing { location, error, warning } if there was a document upload API error but is never called since post request throws.

I'm fine to resolve this in another PR/issue though.

...request.pre.errors,
parsedError(fieldName, error),
];
response = uploadService.parsedDocumentUploadResponse(err.data);
errors.add(response.error);
} else if (err.code === "EPIPE") {
// ignore this error, it happens when the request is responded to by the doc upload service before the
// body has finished being sent. A valid response is still received.
Expand All @@ -63,10 +60,7 @@ export async function handleUpload(
{ ...loggerIdentifier, err },
`Error uploading document: ${err.message}`
);
request.pre.errors = [
...(h.request.pre.errors || []),
parsedError(fieldName, err),
];
errors.add(err);
}
}

Expand Down Expand Up @@ -108,9 +102,13 @@ export async function handleUpload(
loggerIdentifier,
`Document upload API responded with an error ${error}`
);
errors.add(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be omitted, since it's handled in l51-53. I think that should remove the need for the set as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you have an error which doesn't go into the catch block, then it will not be added to the errors list (which is how the file upload tests work) so it needs to be added here

}

if(errors){
request.pre.errors = [
...(request.pre.errors || []),
parsedError(fieldName, error),
...Array.from(errors).map(e => parsedError(fieldName, String(e)))
];
}
}
Expand Down
24 changes: 22 additions & 2 deletions runner/src/server/services/upload/uploadService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const parsedError = (key: string, error?: string) => {
const ERRORS = {
fileSizeError: 'The selected file for "%s" is too large',
fileTypeError: "Invalid file type. Upload a PNG, JPG or PDF",
fileCountError: 'You have selected too many files for "%s"',
virusError: 'The selected file for "%s" contained a virus',
default: "There was an error uploading your file",
};
Expand Down Expand Up @@ -123,7 +124,18 @@ export class UploadService {
}

parsedDocumentUploadResponse({ res, payload }) {
const warning = payload?.toString?.();
const payloadString = payload?.toString?.();
let payloadJson: any;
let warning: string | undefined;
let errorCode: string | undefined;

try {
payloadJson = payloadString ? JSON.parse(payloadString) : undefined;
errorCode = payloadJson?.errorCode;
warning = payloadJson?.warning;
} catch (e) {
warning = payloadString;
}
let error: string | undefined;
let location: string | undefined;
switch (res.statusCode) {
Expand All @@ -134,7 +146,15 @@ export class UploadService {
error = ERRORS.fileTypeError;
break;
case 413:
error = ERRORS.fileSizeError;
if(errorCode === "TOO_MANY_FILES") {
if (payloadJson?.maxFilesPerUpload) {
error = `You can only select up to ${payloadJson?.maxFilesPerUpload} files at the same time for "%s"`;
} else {
error = ERRORS.fileCountError;
}
} else {
error = ERRORS.fileSizeError;
}
break;
case 422:
error = ERRORS.virusError;
Expand Down
Loading