Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
jest.mock('@aws-sdk/client-s3', () => ({
S3Client: jest.fn(),
GetObjectCommand: jest.fn(),
ListObjectsV2Command: jest.fn().mockImplementation((input) => ({ input }))
}))

describe('S3Directory', () => {
const s3Client = { send: jest.fn() }
let getS3FileKeys: (s3Client: any, bucketName: string, prefix: string) => Promise<string[]>
let ListObjectsV2Command: jest.Mock

beforeEach(async () => {
jest.clearAllMocks()
s3Client.send.mockReset()

const s3Module = require('@aws-sdk/client-s3')
ListObjectsV2Command = s3Module.ListObjectsV2Command

const module = (await import('./S3Directory')) as any
getS3FileKeys = module.getS3FileKeys
})

it('loads keys from every S3 list page', async () => {
s3Client.send
.mockResolvedValueOnce({
Contents: [{ Key: 'docs/0001.txt', ETag: 'etag-1' }],
IsTruncated: true,
NextContinuationToken: 'next-page'
})
.mockResolvedValueOnce({
Contents: [{ Key: 'docs/1001.txt', ETag: 'etag-2' }],
IsTruncated: false
})

await expect(getS3FileKeys(s3Client, 'documents', 'docs/')).resolves.toEqual(['docs/0001.txt', 'docs/1001.txt'])
expect(ListObjectsV2Command).toHaveBeenNthCalledWith(1, {
Bucket: 'documents',
Prefix: 'docs/',
ContinuationToken: undefined
})
expect(ListObjectsV2Command).toHaveBeenNthCalledWith(2, {
Bucket: 'documents',
Prefix: 'docs/',
ContinuationToken: 'next-page'
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,27 @@ import { TextSplitter } from '@langchain/textsplitters'
import { CSVLoader } from '../Csv/CsvLoader'
import { LoadOfSheet } from '../MicrosoftExcel/ExcelLoader'
import { PowerpointLoader } from '../MicrosoftPowerpoint/PowerpointLoader'

const getS3FileKeys = async (s3Client: S3Client, bucketName: string, prefix: string): Promise<string[]> => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The prefix parameter is optional in the node inputs (line 104) and can be undefined at runtime. However, the getS3FileKeys function signature types prefix as a required string. To maintain type safety and prevent potential compilation or runtime issues under strict null checks, update the parameter type to string | undefined or make it optional.

Suggested change
const getS3FileKeys = async (s3Client: S3Client, bucketName: string, prefix: string): Promise<string[]> => {
const getS3FileKeys = async (s3Client: S3Client, bucketName: string, prefix?: string): Promise<string[]> => {

const keys: string[] = []
let continuationToken: string | undefined

do {
const listObjectsOutput: ListObjectsV2Output = await s3Client.send(
new ListObjectsV2Command({
Bucket: bucketName,
Prefix: prefix,
ContinuationToken: continuationToken
})
)

keys.push(...(listObjectsOutput.Contents ?? []).filter((item) => item.Key && item.ETag).map((item) => item.Key!))
continuationToken = listObjectsOutput.IsTruncated ? listObjectsOutput.NextContinuationToken : undefined
} while (continuationToken)

return keys
}

class S3_DocumentLoaders implements INode {
label: string
name: string
Expand Down Expand Up @@ -178,14 +199,7 @@ class S3_DocumentLoaders implements INode {
try {
const s3Client = new S3Client(s3Config)

const listObjectsOutput: ListObjectsV2Output = await s3Client.send(
new ListObjectsV2Command({
Bucket: bucketName,
Prefix: prefix
})
)

const keys: string[] = (listObjectsOutput?.Contents ?? []).filter((item) => item.Key && item.ETag).map((item) => item.Key!)
const keys = await getS3FileKeys(s3Client, bucketName, prefix)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

With the introduction of pagination in getS3FileKeys, the number of retrieved keys is no longer capped at 1,000 and can potentially be much larger (e.g., tens of thousands of files). Passing all these keys directly to Promise.all for concurrent downloading and writing to disk can lead to severe issues such as:

  1. Memory Exhaustion: Buffering thousands of file contents in memory simultaneously.
  2. File Descriptor Limits: Exceeding the OS limit for open files (EMFILE).
  3. Rate Limiting: Getting throttled by the S3 API due to too many concurrent requests.

Consider implementing a concurrency limit (e.g., processing in batches of 10-50 files) to make the download process robust and scalable.


await Promise.all(
keys.map(async (key) => {
Expand Down Expand Up @@ -291,4 +305,4 @@ class S3_DocumentLoaders implements INode {
}
}
}
module.exports = { nodeClass: S3_DocumentLoaders }
module.exports = { nodeClass: S3_DocumentLoaders, getS3FileKeys }