From c7029d1cdad8fe3cd6e73f1b5d72d1f3c4b0cbfa Mon Sep 17 00:00:00 2001 From: Sanjai Kumar <161328623+sanjaikumar-bruno@users.noreply.github.com> Date: Tue, 30 Sep 2025 22:47:09 +0530 Subject: [PATCH] fix: improve file upload handling in prepare-request to use streaming (#5637) * fix: improve file upload handling in prepare-request to use streaming * feat: add unit tests --------- Co-authored-by: Bijin Bruno --- .../bruno-cli/src/runner/prepare-request.js | 15 +++- packages/bruno-cli/src/utils/filesystem.js | 19 ++++- .../tests/runner/prepare-request.spec.js | 80 +++++++++++++++---- .../bruno-cli/tests/utils/filesystem.spec.js | 50 ++++++++++++ .../src/ipc/network/prepare-request.js | 15 +++- .../bruno-electron/src/utils/filesystem.js | 18 ++++- .../src/utils/filesystem.test.js | 51 +++++++++++- 7 files changed, 222 insertions(+), 26 deletions(-) create mode 100644 packages/bruno-cli/tests/utils/filesystem.spec.js diff --git a/packages/bruno-cli/src/runner/prepare-request.js b/packages/bruno-cli/src/runner/prepare-request.js index abaed8bf7..e45cee592 100644 --- a/packages/bruno-cli/src/runner/prepare-request.js +++ b/packages/bruno-cli/src/runner/prepare-request.js @@ -4,10 +4,13 @@ const filter = require('lodash/filter'); const find = require('lodash/find'); const decomment = require('decomment'); const crypto = require('node:crypto'); -const fs = require('node:fs/promises'); +const fs = require('node:fs'); const { mergeHeaders, mergeScripts, mergeVars, mergeAuth, getTreePathFromCollectionToItem } = require('../utils/collection'); const { buildFormUrlEncodedPayload } = require('../utils/form-data'); const path = require('node:path'); +const { isLargeFile } = require('../utils/filesystem'); + +const STREAMING_FILE_SIZE_THRESHOLD = 20 * 1024 * 1024; // 20MB const prepareRequest = async (item = {}, collection = {}) => { const request = item?.request; @@ -311,8 +314,14 @@ const prepareRequest = async (item = {}, collection = {}) => { } try { - const fileContent = await fs.readFile(filePath); - axiosRequest.data = fileContent; + // Large files can cause "JavaScript heap out of memory" errors when loaded entirely into memory. + if (isLargeFile(filePath, STREAMING_FILE_SIZE_THRESHOLD)) { + // For large files: Use streaming to avoid memory issues + axiosRequest.data = fs.createReadStream(filePath); + } else { + // For smaller files: Use synchronous read for better performance + axiosRequest.data = fs.readFileSync(filePath); + } } catch (error) { console.error('Error reading file:', error); } diff --git a/packages/bruno-cli/src/utils/filesystem.js b/packages/bruno-cli/src/utils/filesystem.js index 46aa6c797..e06d962c8 100644 --- a/packages/bruno-cli/src/utils/filesystem.js +++ b/packages/bruno-cli/src/utils/filesystem.js @@ -158,6 +158,22 @@ const validateName = (name) => { ); }; +/** + * Checks if a file is larger than a given threshold. + * @param {string} filePath - The path to the file. + * @param {number} threshold - The threshold in bytes. Default is 10MB. + * @returns {boolean} True if the file is larger than the threshold, false otherwise. + */ +const isLargeFile = (filePath, threshold = 10 * 1024 * 1024) => { + if (!isFile(filePath)) { + throw new Error(`File ${filePath} is not a file`); + } + + const size = fs.statSync(filePath).size; + + return size > threshold; +}; + module.exports = { exists, isSymbolicLink, @@ -173,5 +189,6 @@ module.exports = { stripExtension, getSubDirectories, sanitizeName, - validateName + validateName, + isLargeFile }; diff --git a/packages/bruno-cli/tests/runner/prepare-request.spec.js b/packages/bruno-cli/tests/runner/prepare-request.spec.js index 8056f17bb..0d25fc3b3 100644 --- a/packages/bruno-cli/tests/runner/prepare-request.spec.js +++ b/packages/bruno-cli/tests/runner/prepare-request.spec.js @@ -1,4 +1,8 @@ const { describe, it, expect, beforeEach } = require('@jest/globals'); +jest.mock('../../src/utils/filesystem', () => ({ + isLargeFile: jest.fn() +})); +const filesystemUtils = require('../../src/utils/filesystem'); const prepareRequest = require('../../src/runner/prepare-request'); describe('prepare-request: prepareRequest', () => { @@ -521,21 +525,23 @@ describe('prepare-request: prepareRequest', () => { }); describe('Request file body mode', () => { - it('reads the uploaded file and applies correct headers', async () => { - const fsPromises = require('node:fs/promises'); - // Mock fs.readFile to avoid actual file system dependency - jest.spyOn(fsPromises, 'readFile').mockResolvedValue(Buffer.from('dummy file content')); + const fs = require('node:fs'); + let readFileSyncSpy; + let createReadStreamSpy; - const body = { - mode: 'file', - file: [ - { - contentType: 'text/plain', - filePath: '/absolute/path/to/file.txt', - selected: true, - }, - ], - }; + beforeEach(() => { + readFileSyncSpy = jest.spyOn(fs, 'readFileSync'); + createReadStreamSpy = jest.spyOn(fs, 'createReadStream'); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('should use readFileSync to read small files', async () => { + const fileContent = Buffer.from('small file content'); + filesystemUtils.isLargeFile.mockReturnValue(false); + readFileSyncSpy.mockReturnValue(fileContent); const item = { name: 'File Request', @@ -545,13 +551,53 @@ describe('prepare-request: prepareRequest', () => { headers: [], params: [], url: 'https://example.com/upload', - body, + body: { + mode: 'file', + file: [{ + contentType: 'text/plain', + filePath: '/path/to/file.txt', + selected: true + }] + } }, }; const result = await prepareRequest(item); - expect(result.data).toBeInstanceOf(Buffer); - expect(result.headers['content-type']).toBe('text/plain'); + + expect(result.data).toBe(fileContent); + expect(readFileSyncSpy).toHaveBeenCalled(); + expect(createReadStreamSpy).not.toHaveBeenCalled(); + }); + + it('should use createReadStream to read large files', async () => { + const mockStream = { pipe: jest.fn() }; + filesystemUtils.isLargeFile.mockReturnValue(true); + createReadStreamSpy.mockReturnValue(mockStream); + + const item = { + name: 'File Request', + type: 'http-request', + request: { + method: 'POST', + headers: [], + params: [], + url: 'https://example.com/upload', + body: { + mode: 'file', + file: [{ + contentType: 'application/octet-stream', + filePath: '/path/to/large-file.bin', + selected: true + }] + } + } + }; + + const result = await prepareRequest(item); + + expect(result.data).toBe(mockStream); + expect(createReadStreamSpy).toHaveBeenCalled(); + expect(readFileSyncSpy).not.toHaveBeenCalled(); }); }); }); diff --git a/packages/bruno-cli/tests/utils/filesystem.spec.js b/packages/bruno-cli/tests/utils/filesystem.spec.js new file mode 100644 index 000000000..f6badc683 --- /dev/null +++ b/packages/bruno-cli/tests/utils/filesystem.spec.js @@ -0,0 +1,50 @@ +const { isLargeFile } = require('../../src/utils/filesystem'); +const fs = require('fs-extra'); + +describe('isLargeFile', () => { + let existsSyncSpy; + let lstatSyncSpy; + let statSyncSpy; + + beforeEach(() => { + existsSyncSpy = jest.spyOn(fs, 'existsSync'); + lstatSyncSpy = jest.spyOn(fs, 'lstatSync'); + statSyncSpy = jest.spyOn(fs, 'statSync'); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('should return false when file size is below default threshold (10MB)', () => { + existsSyncSpy.mockReturnValue(true); + lstatSyncSpy.mockReturnValue({ isFile: () => true }); + statSyncSpy.mockReturnValue({ size: 5 * 1024 * 1024 }); // 5MB + + expect(isLargeFile('/path/small.bin')).toBe(false); + }); + + it('should return true when file size is above default threshold (10MB)', () => { + existsSyncSpy.mockReturnValue(true); + lstatSyncSpy.mockReturnValue({ isFile: () => true }); + statSyncSpy.mockReturnValue({ size: 15 * 1024 * 1024 }); // 15MB + + expect(isLargeFile('/path/large.bin')).toBe(true); + }); + + it('should respect custom threshold (args true or false)', () => { + existsSyncSpy.mockReturnValue(true); + lstatSyncSpy.mockReturnValue({ isFile: () => true }); + statSyncSpy.mockReturnValue({ size: 50 }); + + expect(isLargeFile('/path/file.bin', 100)).toBe(false); // 50 < 100 + expect(isLargeFile('/path/file.bin', 10)).toBe(true); // 50 > 10 + }); + + it('should throw on invalid values (not a file)', () => { + existsSyncSpy.mockReturnValue(false); + lstatSyncSpy.mockReturnValue({ isFile: () => false }); + + expect(() => isLargeFile('/path/not-a-file.bin')).toThrow('File /path/not-a-file.bin is not a file'); + }); +}); diff --git a/packages/bruno-electron/src/ipc/network/prepare-request.js b/packages/bruno-electron/src/ipc/network/prepare-request.js index 09d03c85f..c6a785018 100644 --- a/packages/bruno-electron/src/ipc/network/prepare-request.js +++ b/packages/bruno-electron/src/ipc/network/prepare-request.js @@ -1,10 +1,13 @@ const { get, each, filter, find } = require('lodash'); const decomment = require('decomment'); const crypto = require('node:crypto'); -const fs = require('node:fs/promises'); +const fs = require('node:fs'); const { getTreePathFromCollectionToItem, mergeHeaders, mergeScripts, mergeVars, getFormattedCollectionOauth2Credentials, mergeAuth } = require('../../utils/collection'); const { buildFormUrlEncodedPayload } = require('../../utils/form-data'); const path = require('node:path'); +const { isLargeFile } = require('../../utils/filesystem'); + +const STREAMING_FILE_SIZE_THRESHOLD = 20 * 1024 * 1024; // 20MB const setAuthHeaders = (axiosRequest, request, collectionRoot) => { const collectionAuth = get(collectionRoot, 'request.auth'); @@ -398,8 +401,14 @@ const prepareRequest = async (item, collection = {}, abortController) => { } try { - const fileContent = await fs.readFile(filePath); - axiosRequest.data = fileContent; + // Large files can cause "JavaScript heap out of memory" errors when loaded entirely into memory. + if (isLargeFile(filePath, STREAMING_FILE_SIZE_THRESHOLD)) { + // For large files: Use streaming to avoid memory issues + axiosRequest.data = fs.createReadStream(filePath); + } else { + // For smaller files: Use synchronous read for better performance + axiosRequest.data = fs.readFileSync(filePath); + } } catch (error) { console.error('Error reading file:', error); } diff --git a/packages/bruno-electron/src/utils/filesystem.js b/packages/bruno-electron/src/utils/filesystem.js index 28b479013..00892e903 100644 --- a/packages/bruno-electron/src/utils/filesystem.js +++ b/packages/bruno-electron/src/utils/filesystem.js @@ -343,6 +343,21 @@ const getPaths = async (source) => { return paths; } +/** + * Checks if a file is larger than a given threshold. + * @param {string} filePath - The path to the file. + * @param {number} threshold - The threshold in bytes. Default is 10MB. + * @returns {boolean} True if the file is larger than the threshold, false otherwise. + */ +const isLargeFile = (filePath, threshold = 10 * 1024 * 1024) => { + if (!isFile(filePath)) { + throw new Error(`File ${filePath} is not a file`); + } + + const size = fs.statSync(filePath).size; + + return size > threshold; +}; module.exports = { isValidPathname, @@ -373,5 +388,6 @@ module.exports = { safeWriteFileSync, copyPath, removePath, - getPaths + getPaths, + isLargeFile }; diff --git a/packages/bruno-electron/src/utils/filesystem.test.js b/packages/bruno-electron/src/utils/filesystem.test.js index a0f0f018d..c6dc6bc99 100644 --- a/packages/bruno-electron/src/utils/filesystem.test.js +++ b/packages/bruno-electron/src/utils/filesystem.test.js @@ -1,4 +1,5 @@ -const { sanitizeName, isWSLPath, normalizeWSLPath, normalizeAndResolvePath } = require('./filesystem.js'); +const { sanitizeName, isWSLPath, normalizeWSLPath, normalizeAndResolvePath, isLargeFile } = require('./filesystem.js'); +const fs = require('fs-extra'); describe('sanitizeName', () => { it('should replace invalid characters with hyphens', () => { @@ -29,6 +30,54 @@ describe('sanitizeName', () => { }); }); +describe('isLargeFile', () => { + let existsSyncSpy; + let lstatSyncSpy; + let statSyncSpy; + + beforeEach(() => { + existsSyncSpy = jest.spyOn(fs, 'existsSync'); + lstatSyncSpy = jest.spyOn(fs, 'lstatSync'); + statSyncSpy = jest.spyOn(fs, 'statSync'); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('should return false when file size is below default threshold (10MB)', () => { + existsSyncSpy.mockReturnValue(true); + lstatSyncSpy.mockReturnValue({ isFile: () => true }); + statSyncSpy.mockReturnValue({ size: 5 * 1024 * 1024 }); // 5MB + + expect(isLargeFile('/path/small.bin')).toBe(false); + }); + + it('should return true when file size is above default threshold (10MB)', () => { + existsSyncSpy.mockReturnValue(true); + lstatSyncSpy.mockReturnValue({ isFile: () => true }); + statSyncSpy.mockReturnValue({ size: 15 * 1024 * 1024 }); // 15MB + + expect(isLargeFile('/path/large.bin')).toBe(true); + }); + + it('should respect custom threshold (args true or false)', () => { + existsSyncSpy.mockReturnValue(true); + lstatSyncSpy.mockReturnValue({ isFile: () => true }); + statSyncSpy.mockReturnValue({ size: 50 }); + + expect(isLargeFile('/path/file.bin', 100)).toBe(false); // 50 < 100 + expect(isLargeFile('/path/file.bin', 10)).toBe(true); // 50 > 10 + }); + + it('should throw on invalid values (not a file)', () => { + existsSyncSpy.mockReturnValue(false); + lstatSyncSpy.mockReturnValue({ isFile: () => false }); + + expect(() => isLargeFile('/path/not-a-file.bin')).toThrow('File /path/not-a-file.bin is not a file'); + }); +}); + describe('WSL Path Utilities', () => { describe('isWSLPath', () => { it('should identify WSL paths starting with double backslash', () => {