Add support for azure blob storage #5604
Conversation
Summary of ChangesHello @chiragjagga, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the platform's file storage capabilities by integrating Azure Blob Storage. It provides a new, robust option for managing files, complementing the existing local, S3, and Google Cloud Storage solutions. The changes ensure that all standard file operations, including uploading, downloading, listing, and deleting, are fully supported for Azure Blob Storage, offering greater flexibility and scalability for file management. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for Azure Blob Storage, adding the @azure/storage-blob dependency and implementing Azure-specific logic across various file management functions such as uploading, downloading, listing, and deleting files. It also includes new environment variable flags for Azure configuration. The review comments highlight several areas for refactoring to improve code maintainability and reduce duplication, specifically suggesting the abstraction of fallback logic for retrieving files without an orgId, extracting base64 data URI parsing into a helper function, standardizing path construction, and consolidating duplicated folder/file removal logic across different storage providers.
| try { | ||
| const blockBlobClient = containerClient.getBlockBlobClient(blobName) | ||
| const buffer = await blockBlobClient.downloadToBuffer() | ||
| return buffer | ||
| } catch (error) { | ||
| // Fallback: Check if file exists without the first path element (likely orgId) | ||
| if (paths.length > 1) { | ||
| const fallbackPaths = paths.slice(1) | ||
| let fallbackBlobName = fallbackPaths.reduce((acc, cur) => acc + '/' + cur, '') + '/' + sanitizedFilename | ||
| if (fallbackBlobName.startsWith('/')) { | ||
| fallbackBlobName = fallbackBlobName.substring(1) | ||
| } | ||
|
|
||
| try { | ||
| const fallbackBlobClient = containerClient.getBlockBlobClient(fallbackBlobName) | ||
| const buffer = await fallbackBlobClient.downloadToBuffer() | ||
|
|
||
| // Move to correct location with orgId | ||
| const blockBlobClient = containerClient.getBlockBlobClient(blobName) | ||
| await blockBlobClient.upload(buffer, buffer.length) | ||
|
|
||
| // Delete the old file | ||
| await fallbackBlobClient.delete() | ||
|
|
||
| return buffer | ||
| } catch (fallbackError) { | ||
| // Throw the original error since the fallback also failed | ||
| throw error | ||
| } | ||
| } else { | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
The fallback logic to handle files stored without an orgId is duplicated across S3, GCS, and now Azure implementations. This makes the code difficult to maintain and prone to errors. This is a great opportunity for a refactor to abstract this logic.
Consider refactoring this into a higher-level function or using a strategy pattern where each storage provider implements a common interface for getFile and moveFile. This would significantly reduce code duplication.
A simplified abstraction could look like this:
async function getFileWithFallback(primaryPath, fallbackPath, storage) {
try {
return await storage.download(primaryPath);
} catch (error) {
if (fallbackPath) {
try {
const buffer = await storage.download(fallbackPath);
await storage.upload(primaryPath, buffer);
await storage.delete(fallbackPath);
return buffer;
} catch (fallbackError) {
throw error; // Throw original error
}
}
throw error;
}
}This logic is also duplicated in streamStorageFile and could benefit from the same refactoring.
| const splitDataURI = fileBase64.split(',') | ||
| const filename = splitDataURI.pop()?.split(':')[1] ?? '' | ||
| const bf = Buffer.from(splitDataURI.pop() || '', 'base64') | ||
| const mime = splitDataURI[0].split(':')[1].split(';')[0] |
There was a problem hiding this comment.
The logic for parsing a base64 data URI is duplicated across the s3, gcs, and now azure branches of this function. To improve maintainability and reduce redundancy, this logic should be extracted into a separate helper function.
For example:
const parseDataURI = (dataURI: string) => {
const splitDataURI = dataURI.split(',');
const filename = splitDataURI.pop()?.split(':')[1] ?? '';
const buffer = Buffer.from(splitDataURI.pop() || '', 'base64');
const mime = splitDataURI[0].split(':')[1].split(';')[0];
return { filename, buffer, mime };
}This would make addBase64FilesToStorage cleaner and easier to read.
| let blobName = paths.reduce((acc, cur) => acc + '/' + cur, '') + '/' + sanitizedFilename | ||
| if (blobName.startsWith('/')) { | ||
| blobName = blobName.substring(1) | ||
| } |
There was a problem hiding this comment.
This path construction logic is duplicated across several functions (addArrayFilesToStorage, addSingleFileToStorage, getFileFromStorage, etc.) and storage providers. For consistency and clarity, you could adopt the same approach used for GCS, which is cleaner and more idiomatic for joining path segments.
Using [...paths, sanitizedFilename].join('/') is more readable than reduce and chaining substring.
| let blobName = paths.reduce((acc, cur) => acc + '/' + cur, '') + '/' + sanitizedFilename | |
| if (blobName.startsWith('/')) { | |
| blobName = blobName.substring(1) | |
| } | |
| const blobName = [...paths, sanitizedFilename].join('/'); |
| } else if (storageType === 'azure') { | ||
| const { containerClient } = getAzureBlobConfig() | ||
|
|
||
| let prefix = paths.reduce((acc, cur) => acc + '/' + cur, '') | ||
| if (prefix.startsWith('/')) { | ||
| prefix = prefix.substring(1) | ||
| } | ||
|
|
||
| // Delete all blobs with the prefix | ||
| for await (const blob of containerClient.listBlobsFlat({ prefix })) { | ||
| await containerClient.deleteBlob(blob.name) | ||
| } | ||
|
|
||
| const totalSize = await getAzureBlobStorageSize(paths[0]) | ||
| return { totalSize: totalSize / 1024 / 1024 } | ||
| } else { |
There was a problem hiding this comment.
The implementation for removeFolderFromStorage for Azure is identical to removeFilesFromStorage. This duplication also exists for S3 and GCS. If their behavior for cloud storage is indeed the same, consider consolidating them to avoid maintaining duplicate code. For example, removeFolderFromStorage could simply call removeFilesFromStorage.
|
Hi @HenryHengZJ,
Would love to get your input on this before proceeding as this would be a significant change. |
chiragjagga
changed the title
1 participant
DRAFT PR:
Pending: