Fix Supabase storage driver list operation (#22322)

Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>

.changeset/seven-brooms-applaud.md

@@ -0,0 +1,5 @@
+---
+'@directus/storage-driver-supabase': patch
+---
+
+Fixed `list` method to recursively list all files under a prefix

packages/storage-driver-supabase/src/index.test.ts

@@ -10,6 +10,7 @@ import {
 	randPastDate,
 	randText,
 	randGitShortSha as randUnique,
+	randFileName,
 } from '@ngneat/falso';
 import { Response, fetch } from 'undici';
 import { Readable } from 'node:stream';
@@ -49,7 +50,7 @@ beforeEach(() => {
 			serviceRole: randAlphaNumeric({ length: 40 }).join(''),
 			bucket: randBucket(),
 			projectId: randAlphaNumeric({ length: 10 }).join(''),
-			root: randDirectoryPath(),
+			root: randUnique() + randDirectoryPath(),
 			endpoint: randDomainName(),
 		},
 		path: {
@@ -104,7 +105,8 @@ describe('#constructor', () => {
 
 	test('Saves passed config to local property', () => {
 		const driver = new DriverSupabase(sample.config);
-		expect(driver['config']).toBe(sample.config);
+
+		expect(driver['config']).toStrictEqual(sample.config);
 	});
 
 	test('Creates shared client', () => {
@@ -114,7 +116,7 @@ describe('#constructor', () => {
 	});
 
 	test('Defaults root to empty string', () => {
-		expect(driver['config'].root).toBe(undefined);
+		expect(driver['config'].root).toBe('');
 	});
 });
 
@@ -170,7 +172,7 @@ describe('#getClient', () => {
 	});
 });
 
-describe('#getFullPath', () => {
+describe('#fullPath', () => {
 	test('Returns the input value if no root is given', () => {
 		const driver = new DriverSupabase({
 			serviceRole: sample.config.serviceRole,
@@ -178,7 +180,7 @@ describe('#getFullPath', () => {
 			endpoint: sample.config.endpoint,
 		});
 
-		const result = driver['getFullPath'](sample.path.input);
+		const result = driver['fullPath'](sample.path.input);
 		expect(result).toBe(sample.path.input);
 	});
 
@@ -190,7 +192,7 @@ describe('#getFullPath', () => {
 			root: sample.config.root,
 		});
 
-		const result = driver['getFullPath'](sample.path.input);
+		const result = driver['fullPath'](sample.path.input);
 		expect(result).toBe(`${sample.config.root}/${sample.path.input}`);
 	});
 });
@@ -462,11 +464,11 @@ describe('#write', () => {
 		});
 	});
 
-	test('Ensures input is passed to getFullPath', async () => {
-		driver['getFullPath'] = vi.fn();
+	test('Ensures input is passed to fullPath', async () => {
+		driver['fullPath'] = vi.fn();
 
 		await driver.write(sample.path.input, sample.stream);
-		expect(driver['getFullPath']).toHaveBeenCalledWith(sample.path.input);
+		expect(driver['fullPath']).toHaveBeenCalledWith(sample.path.input);
 	});
 
 	test('Optionally sets ContentType', async () => {
@@ -482,44 +484,49 @@ describe('#write', () => {
 });
 
 describe('#delete', () => {
-	test('Ensures input is passed to getFullPath', async () => {
+	test('Ensures input is passed to fullPath', async () => {
 		driver['bucket'] = {
 			remove: vi.fn(),
 		} as any;
 
-		driver['getFullPath'] = vi.fn();
+		driver['fullPath'] = vi.fn();
 
 		await driver.delete(sample.path.input);
-		expect(driver['getFullPath']).toHaveBeenCalledWith(sample.path.input);
+		expect(driver['fullPath']).toHaveBeenCalledWith(sample.path.input);
 	});
 });
 
 describe('#list', () => {
 	test('Constructs list objects params based on input prefix', async () => {
+		const sampleFile = randFileName();
+		const sampleDirectory = randDirectoryPath();
+		const fullSample = `${sampleDirectory}/${sampleFile}`;
+
 		// TODO: Probably a better way to do this?
 		driver['bucket'] = {
 			list: vi.fn().mockReturnValue({ data: [], error: null }),
 		} as any;
 
-		await driver.list(sample.path.input).next();
+		await driver.list(fullSample)[Symbol.asyncIterator]().next();
 
-		expect(driver['bucket'].list).toHaveBeenCalledWith(undefined, {
+		expect(driver['bucket'].list).toHaveBeenCalledWith(sampleDirectory, {
+			search: sampleFile,
 			limit: 1000,
 			offset: 0,
-			search: sample.path.input,
 		});
 	});
 
-	test('Yields file name omitting root', async () => {
+	test('Yields file name omitting root if prefix is the full file path', async () => {
 		const sampleRoot = randDirectoryPath();
-		const sampleFile = randFilePath();
+		const sampleFile = randFileName();
+		const sampleFull = `${sample.path.input}/${sampleFile}`;
 
-		// TODO: Probably a better way to do this?
 		driver['bucket'] = {
-			list: vi.fn().mockReturnValue({
+			list: vi.fn().mockResolvedValueOnce({
 				data: [
 					{
 						name: sampleFile,
+						id: randUnique(),
 					},
 				],
 				error: null,
@@ -528,20 +535,142 @@ describe('#list', () => {
 
 		driver['config'].root = sampleRoot;
 
-		const iterator = driver.list(sample.path.input);
+		const iterator = driver.list(sampleFull);
 		const output: string[] = [];
 
 		for await (const filepath of iterator) {
 			output.push(filepath);
 		}
 
-		expect(driver['bucket'].list).toHaveBeenCalledWith(sampleRoot, {
-			limit: 1000,
-			offset: 0,
-			search: sample.path.input,
-		});
+		expect(output).toStrictEqual([sampleFull]);
+	});
 
-		expect(output).toStrictEqual([sampleFile]);
+	test('Yields file name omitting root if prefix is the parent directory', async () => {
+		const sampleRoot = randDirectoryPath();
+		const sampleFile = randFileName();
+		const sampleParentDir = randUnique();
+		const sampleInput = `${sample.path.input}/${sampleParentDir}`;
+		const sampleFull = `${sampleInput}/${sampleFile}`;
+
+		driver['bucket'] = {
+			list: vi
+				.fn()
+				.mockResolvedValueOnce({
+					data: [
+						{
+							name: sampleParentDir,
+							id: null,
+						},
+					],
+					error: null,
+				})
+				.mockResolvedValueOnce({
+					data: [
+						{
+							name: sampleFile,
+							id: randUnique(),
+						},
+					],
+					error: null,
+				}),
+		} as any;
+
+		driver['config'].root = sampleRoot;
+
+		const iterator = driver.list(sampleInput);
+		const output: string[] = [];
+
+		for await (const filepath of iterator) {
+			output.push(filepath);
+		}
+
+		expect(driver['bucket'].list).toHaveBeenCalledTimes(2);
+		expect(output).toStrictEqual([sampleFull]);
+	});
+
+	test('Yields file name omitting root if prefix is part of the file name', async () => {
+		const sampleRoot = randDirectoryPath();
+		const sampleFilePrefix = randFileName();
+		const sampleFiles = [1, 2, 3].map((i) => `${sampleFilePrefix}_postfix${i}`);
+		const sampleInput = `${sample.path.input}/${sampleFilePrefix}`;
+		const sampleFilesFull = sampleFiles.map((name) => `${sample.path.input}/${name}`);
+
+		driver['bucket'] = {
+			list: vi.fn().mockResolvedValueOnce({
+				data: sampleFiles.map((name) => ({
+					name,
+					id: randUnique(),
+				})),
+				error: null,
+			}),
+		} as any;
+
+		driver['config'].root = sampleRoot;
+
+		const iterator = driver.list(sampleInput);
+		const output: string[] = [];
+
+		for await (const filepath of iterator) {
+			output.push(filepath);
+		}
+
+		expect(output).toStrictEqual(sampleFilesFull);
+	});
+
+	test('Recursively fetches all nested directories and yields only the files', async () => {
+		const sampleRoot = randUnique() + randDirectoryPath();
+		const samplePrefixBase = randUnique() + randDirectoryPath();
+		const samplePrefixLastDir = randUnique();
+		const samplePrefix = `${samplePrefixBase}/${samplePrefixLastDir}`;
+
+		/*
+		sampleFile
+		sampleDirectory/
+		├─ sampleFileNested
+		 */
+		const sampleDirectory = randUnique();
+		const sampleFile = randFileName();
+		const sampleFileNested = randFileName();
+
+		const fullSampleDirectory = `${samplePrefix}/${sampleDirectory}`;
+		const fullSampleFile = `${samplePrefix}/${sampleFile}`;
+		const fullSampleFileNested = `${fullSampleDirectory}/${sampleFileNested}`;
+
+		driver['bucket'] = {
+			list: vi.fn(async (path, options): Promise<any> => {
+				// query for parent dir, return the contained dir
+				if (path === `${sampleRoot}/${samplePrefixBase}` && options?.search === samplePrefixLastDir)
+					return { data: [{ name: samplePrefixLastDir, id: null }], error: null };
+				// query for the contents of the samplePrefix, return file and directory
+				if (path === `${sampleRoot}/${samplePrefix}/` && options?.search === '')
+					return {
+						data: [
+							{ name: sampleDirectory, id: null },
+							{ name: sampleFile, id: randUnique() },
+						],
+						error: null,
+					};
+				// query for the contents of the sampleDirectory, return the nested file
+				if (path === `${sampleRoot}/${fullSampleDirectory}/` && options?.search === '')
+					return {
+						data: [{ name: sampleFileNested, id: randUnique() }],
+						error: null,
+					};
+				throw Error();
+			}),
+		} as any;
+
+		driver['config'].root = sampleRoot;
+
+		const iterator = driver.list(samplePrefix);
+		const output: string[] = [];
+
+		for await (const filepath of iterator) {
+			output.push(filepath);
+		}
+
+		expect(driver['bucket'].list).toHaveBeenCalledTimes(3);
+		expect(output).toStrictEqual([fullSampleFileNested, fullSampleFile]);
 	});
 
 	test('Continuously fetches until all pages are returned', async () => {

packages/storage-driver-supabase/src/index.ts

@@ -10,18 +10,22 @@ export type DriverSupabaseConfig = {
 	bucket: string;
 	serviceRole: string;
 	projectId?: string;
-	// Allows a custom Supabase endpoint incase self-hosting
+	/** Allows a custom Supabase endpoint for self-hosting */
 	endpoint?: string;
 	root?: string;
 };
 
 export class DriverSupabase implements Driver {
-	private config: DriverSupabaseConfig;
+	private config: DriverSupabaseConfig & { root: string };
 	private client: StorageClient;
 	private bucket: ReturnType<StorageClient['from']>;
 
 	constructor(config: DriverSupabaseConfig) {
-		this.config = config;
+		this.config = {
+			...config,
+			root: normalizePath(config.root ?? '', { removeLeading: true }),
+		};
+
 		this.client = this.getClient();
 		this.bucket = this.getBucket();
 	}
@@ -53,12 +57,15 @@ export class DriverSupabase implements Driver {
 		return this.client.from(this.config.bucket);
 	}
 
-	private getFullPath(filepath: string) {
-		return this.config.root ? normalizePath(join(this.config.root, filepath)) : filepath;
+	private fullPath(filepath: string) {
+		const path = join(this.config.root, filepath);
+		// Supabase expects an empty string for current directory
+		if (path === '.') return '';
+		return normalizePath(path);
 	}
 
 	private getAuthenticatedUrl(filepath: string) {
-		return `${this.endpoint}/${join('object/authenticated', this.config.bucket, this.getFullPath(filepath))}`;
+		return `${this.endpoint}/${join('object/authenticated', this.config.bucket, this.fullPath(filepath))}`;
 	}
 
 	async read(filepath: string, range?: Range) {
@@ -115,15 +122,15 @@ export class DriverSupabase implements Driver {
 	}
 
 	async move(src: string, dest: string) {
-		await this.bucket.move(this.getFullPath(src), this.getFullPath(dest));
+		await this.bucket.move(this.fullPath(src), this.fullPath(dest));
 	}
 
 	async copy(src: string, dest: string) {
-		await this.bucket.copy(this.getFullPath(src), this.getFullPath(dest));
+		await this.bucket.copy(this.fullPath(src), this.fullPath(dest));
 	}
 
 	async write(filepath: string, content: Readable, type?: string) {
-		await this.bucket.upload(this.getFullPath(filepath), content, {
+		await this.bucket.upload(this.fullPath(filepath), content, {
 			contentType: type ?? '',
 			cacheControl: '3600',
 			upsert: true,
@@ -132,16 +139,52 @@ export class DriverSupabase implements Driver {
 	}
 
 	async delete(filepath: string) {
-		await this.bucket.remove([this.getFullPath(filepath)]);
+		await this.bucket.remove([this.fullPath(filepath)]);
 	}
 
-	async *list(prefix = '') {
+	list(prefix = ''): AsyncIterable<string> {
+		const fullPrefix = this.fullPath(prefix);
+		return this.listGenerator(fullPrefix);
+	}
+
+	async *listGenerator(prefix: string): AsyncIterable<string> {
 		const limit = 1000;
 		let offset = 0;
 		let itemCount = 0;
 
+		/*
+		 *	The Supabase API only returns the directories and files directly within the queried location
+		 *	(called prefix in their API) matching the search query. Directories can be identified by the id being null.
+		 *
+		 *	Since it's unknown whether the last part of the prefix param is a directory, a file or part of a directory or file name,
+		 *	the first query will be the largest common denominator (the parent directory part of the prefix)
+		 *	and the results then filtered with the remaining part of the query.
+		 *
+		 *	This can lead to the following outcomes:
+		 *	1. The full prefix is a file and is split up into parentdir/filename, the API is queried with
+		 *	   { prefix: parentdir, search: filename } and returns one result { name: filename, id: "..." }.
+		 *	   The filename is yielded in that case.
+		 *	2. The full prefix is a directory and is split up into parentdir/dir, the API is queried with
+		 *	   { prefix: parentdir, search: dir } and returns one result { name: dir, id: null } and importantly, not the
+		 *	   contents of the directory. Then  the contents of the directory must be listed recursively by calling this
+		 *	   function with the prefix and a trailing "/".
+		 *	   2.1. The prefix ends with a "/", the API is queried with { prefix: prefix, search: "" }
+		 *	        and returns all files and directories within the prefix, which are yielded and recursively listed.
+		 *	3. Special case of 1. and 2.: The prefix is part of a filename/directory name and could result in more than one result
+		 *	   that all match the search query. The API is queried with { prefix: parentdir, search: part of filename }
+		 *	   and returns all files and directories in parentdir that match the search query. Filenames are yielded and
+		 *	   directories are recursively listed.
+		 */
+		const isDirectory = prefix.endsWith('/');
+		const prefixDirectory = isDirectory ? prefix : dirname(prefix);
+		const search = isDirectory ? '' : prefix.split('/').pop() ?? '';
+
 		do {
-			const { data, error } = await this.bucket.list(this.config.root, { limit, offset, search: prefix });
+			const { data, error } = await this.bucket.list(prefixDirectory, {
+				limit,
+				offset,
+				search,
+			});
 
 			if (!data || error) {
 				break;
@@ -151,10 +194,26 @@ export class DriverSupabase implements Driver {
 			offset += itemCount;
 
 			for (const item of data) {
-				yield item.name;
+				// Since the API only returns the filename, the file path is constructed by joining the prefix with the item name
+				const filePath = normalizePath(join(prefixDirectory, item.name));
+
+				if (item.id !== null) {
+					// Remove the root from the path and yield the filename
+					yield filePath.substring(this.config.root ? this.config.root.length + 1 : 0);
+				} else {
+					// This is a directory, recursively list it
+					yield* this.listGenerator(`${filePath}/`);
+				}
 			}
 		} while (itemCount === limit);
 	}
 }
 
 export default DriverSupabase;
+
+/**
+ * dirname implementation that always uses '/' to split and returns '' in case of no separator present.
+ */
+function dirname(path: string) {
+	return path.split('/').slice(0, -1).join('/');
+}