Add support for azure blob storage and storage refactor#5604
Add support for azure blob storage and storage refactor#5604chiragjagga wants to merge 12 commits intoFlowiseAI:mainfrom
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
|
hey @chiragjagga thank you so much for the PR! Yes, I'm open to the changes you mentioned. Having said that, once you have made the changes, we need to test all storage methods to make sure nothing breaks. File storage is always the fallback option, because not every open source users will setup different blob storage method, for starters its always easier to just use file storage when no specific blob storage option is specified. |
chiragjagga
force-pushed
the
feat/support-azure-storage
branch
from
af30fe4 to
cf7ac20
Compare
chiragjagga
changed the title
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Azure Blob Storage and refactors the storage logic into a provider-based architecture using a factory pattern. This is a significant improvement in modularity and extensibility. However, there are several critical issues in the new providers, particularly regarding Multer integration for Azure and incorrect API usage in the S3 provider that will lead to runtime errors. Additionally, there are inconsistencies in file size reporting across providers that should be addressed to ensure a unified experience.
Hi @HenryHengZJ , the mentioned changes are now completed. I have updated the PR description to add the relevant change summary.
Sorry for the confusion, I meant the fallback option in specific code-path in functions where it first checks for file path orgId/chatflowId/chatId/fileName and if the file doesn't exist it tries a legacy path without orgId and if the file exists there it migrates it. Example - Fallback local storage |
chiragjagga
force-pushed
the
feat/support-azure-storage
branch
from
3b0c14d to
c938ba4
Compare
|
Hi @HenryHengZJ , I have cleaned up the commits and resolved the new merge conflicts. The newer validations that we added have been added to the BaseStorageProvider in the relevant validation functions already being called by the individual functions. |
|
thanks @chiragjagga ! Verified working correctly for Local Storage, S3, GCS and Azure:
cc @yau-wd to have a second pair of eyes to help testing |
# Conflicts: # pnpm-lock.yaml
2 participants
Changes (Azure Storage):
Changes (Storage Refactor):
Adding a new storage option should be pretty straightforward by adding another storage provider class and all changes including logger and multer changes are in a single place.
GCS Path Bug:
GCSPathBug_Compressed.mp4
Testing:
Azure Storage:
AzureStorage.mp4
Local Storage:
LocalStorageClean.mp4
GCS:
GCSStorage.mp4
S3 Storage:
S3StorageClean.mp4