feat: support minio as file storage #120
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR adds support for MinIO-based file storage by introducing a storage abstraction layer, integrating a MinIO client, and refactoring existing endpoints and registration logic to delegate file operations to the new storage manager; it also updates deployment configurations and dependencies to include MinIO services. Sequence diagram for storing a file using MinIO strategysequenceDiagram
participant API as TRS Filer API
participant FSM as FileStorageManager
participant MS as MinIOStorage
participant MC as MinIOClient
API->>FSM: store_file(file_content, file_path, file_type)
FSM->>MS: store_file(file_content, file_path, file_type)
MS->>MC: upload_file(object_name, file_content, content_type)
MC-->>MS: object_name
MS-->>FSM: {minio_path: object_name}
FSM-->>API: {minio_path: object_name}
Sequence diagram for retrieving a file's download URL using MinIOsequenceDiagram
participant API as TRS Filer API
participant FSM as FileStorageManager
participant MS as MinIOStorage
participant MC as MinIOClient
API->>FSM: get_file_url(file_wrapper)
FSM->>MS: get_file_url(file_wrapper)
MS->>MC: get_presigned_url(object_name)
MC-->>MS: presigned_url
MS-->>FSM: presigned_url
FSM-->>API: presigned_url
Class diagram for file storage abstraction and MinIO clientclassDiagram
class FileStorageManager {
+store_file(file_content, file_path, file_type)
+retrieve_file_content(file_wrapper)
+get_file_url(file_wrapper)
+delete_file(file_wrapper)
-strategy: FileStorageStrategy
}
class FileStorageStrategy {
<<abstract>>
+store_file(file_content, file_path, file_type)
+retrieve_file_content(file_wrapper)
+get_file_url(file_wrapper)
+delete_file(file_wrapper)
}
class MongoDBStorage {
+store_file(file_content, file_path, file_type)
+retrieve_file_content(file_wrapper)
+get_file_url(file_wrapper)
+delete_file(file_wrapper)
}
class MinIOStorage {
+store_file(file_content, file_path, file_type)
+retrieve_file_content(file_wrapper)
+get_file_url(file_wrapper)
+delete_file(file_wrapper)
-minio_client: MinIOClient
}
class MinIOClient {
+upload_file(object_name, file_content, content_type)
+get_presigned_url(object_name)
+download_file(object_name)
+delete_file(object_name)
+file_exists(object_name)
}
FileStorageManager --> FileStorageStrategy
FileStorageStrategy <|-- MongoDBStorage
FileStorageStrategy <|-- MinIOStorage
MinIOStorage --> MinIOClient
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `trs_filer/ga4gh/trs/server.py:528` </location>
<code_context>
- 'content' not in _file['file_wrapper']
- ):
- _w = _file['file_wrapper']
- try:
- _w['content'] = requests.get(_w['url']).text
- except (
</code_context>
<issue_to_address>
Catching all exceptions when retrieving file content may mask unexpected errors.
Catching 'Exception' may hide programming or system errors. Please catch only specific exceptions relevant to file retrieval.
</issue_to_address>
### Comment 2
<location> `trs_filer/minio_client.py:43` </location>
<code_context>
+ self.bucket = os.environ.get('MINIO_BUCKET', "trs-files")
+ self._ensure_bucket()
+
+ def _parse_endpoint(self, endpoint_url: str) -> tuple[str, bool]:
+ """Parse endpoint URL to extract host:port and determine if secure."""
+ if endpoint_url.startswith(('http://', 'https://')):
+ parsed = urlparse(endpoint_url)
+ secure = parsed.scheme == 'https'
+ # Extract host:port and path
+ path = parsed.path.rstrip('/')
+ endpoint = parsed.netloc + path
+ return endpoint, secure
+ else:
</code_context>
<issue_to_address>
Concatenating netloc and path may result in malformed endpoints.
For MinIO, the endpoint should be host:port only; including the path can break client connections. Use parsed.netloc instead of concatenating with the path.
</issue_to_address>
### Comment 3
<location> `trs_filer/file_storage.py:44` </location>
<code_context>
+ def store_file(self, file_content: str | bytes, file_path: str,
+ file_type: str) -> Dict[str, Any]:
+ """Store file content directly in the file wrapper."""
+ if isinstance(file_content, bytes):
+ file_content = file_content.decode('utf-8')
+ return {"content": file_content}
+
+ def retrieve_file_content(self, file_wrapper: Dict[str, Any]) -> bytes:
</code_context>
<issue_to_address>
Decoding bytes as UTF-8 may fail for non-text files.
Decoding will fail for non-UTF-8 or binary files. Consider adding error handling or supporting binary formats, such as base64 encoding.
</issue_to_address>
### Comment 4
<location> `trs_filer/file_storage.py:116` </location>
<code_context>
+
+ def __init__(self):
+ """Initialize storage manager."""
+ strategy = os.environ.get('FILE_STORAGE_STRATEGY', 'minio')
+ if strategy == "minio":
+ self.strategy = MinIOStorage()
+ elif strategy == "mongodb":
+ self.strategy = MongoDBStorage()
+ else:
+ raise ValueError(f"Unsupported file storage strategy: {strategy}")
+
+ def store_file(self, file_content: str | bytes, file_path: str,
</code_context>
<issue_to_address>
Defaulting to 'minio' may cause unexpected failures if MinIO is not configured.
Defaulting to 'minio' may lead to runtime errors if MinIO is not available. Recommend specifying the strategy in deployment settings or choosing a safer default.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def __init__(self):
"""Initialize storage manager."""
strategy = os.environ.get('FILE_STORAGE_STRATEGY', 'minio')
if strategy == "minio":
self.strategy = MinIOStorage()
elif strategy == "mongodb":
self.strategy = MongoDBStorage()
else:
raise ValueError(f"Unsupported file storage strategy: {strategy}")
=======
def __init__(self):
"""Initialize storage manager."""
strategy = os.environ.get('FILE_STORAGE_STRATEGY')
if not strategy:
raise ValueError(
"FILE_STORAGE_STRATEGY environment variable must be set to 'minio' or 'mongodb'."
)
if strategy == "minio":
self.strategy = MinIOStorage()
elif strategy == "mongodb":
self.strategy = MongoDBStorage()
else:
raise ValueError(f"Unsupported file storage strategy: {strategy}. Please set FILE_STORAGE_STRATEGY to 'minio' or 'mongodb'.")
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ret = _d['file_wrapper'] | ||
| ret = _prepare_file_wrapper(_d['file_wrapper']) | ||
| except (IndexError, KeyError, TypeError): | ||
| raise NotFound |
There was a problem hiding this comment.
suggestion (bug_risk): Catching all exceptions when retrieving file content may mask unexpected errors.
Catching 'Exception' may hide programming or system errors. Please catch only specific exceptions relevant to file retrieval.
Comment on lines
+44
to
+46
| if isinstance(file_content, bytes): | ||
| file_content = file_content.decode('utf-8') | ||
| return {"content": file_content} |
There was a problem hiding this comment.
issue: Decoding bytes as UTF-8 may fail for non-text files.
Decoding will fail for non-UTF-8 or binary files. Consider adding error handling or supporting binary formats, such as base64 encoding.
Comment on lines
+81
to
+83
| object_name = file_wrapper.get("minio_path") | ||
| if not object_name: | ||
| raise ValueError("No MinIO path specified in file wrapper") |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Lift code into else after jump in control flow (
reintroduce-else) - Swap if/else branches (
swap-if-else-branches)
Comment on lines
413
to
414
| except (IndexError, KeyError, TypeError): | ||
| raise NotFound |
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
| except (IndexError, KeyError, TypeError): | |
| raise NotFound | |
| except (IndexError, KeyError, TypeError) as e: | |
| raise NotFound from e |
Comment on lines
474
to
475
| except (IndexError, KeyError, TypeError): | ||
| raise NotFound |
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
| except (IndexError, KeyError, TypeError): | |
| raise NotFound | |
| except (IndexError, KeyError, TypeError) as e: | |
| raise NotFound from e |
| raise NotFound | ||
| del_obj_tools = db_coll_tools.delete_one({'id': id}) | ||
|
|
||
| if del_obj_tools.deleted_count: |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Swap if/else branches (
swap-if-else-branches) - Remove unnecessary else after guard condition (
remove-unnecessary-else)
Comment on lines
+45
to
+54
| if endpoint_url.startswith(('http://', 'https://')): | ||
| parsed = urlparse(endpoint_url) | ||
| secure = parsed.scheme == 'https' | ||
| # Extract host:port and path | ||
| path = parsed.path.rstrip('/') | ||
| endpoint = parsed.netloc + path | ||
| return endpoint, secure | ||
| else: | ||
| # Assume it's already in host:port format | ||
| return endpoint_url, False |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Swap if/else branches (
swap-if-else-branches) - Remove unnecessary else after guard condition (
remove-unnecessary-else)
Comment on lines
+102
to
+106
| presigned_url = public_client.presigned_get_object( | ||
| bucket_name=self.bucket, | ||
| object_name=object_name, | ||
| expires=timedelta(hours=1) | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
uniqueg
reviewed
Aug 20, 2025
Comment on lines
+387
to
+391
| # # validate image file types | ||
| # elif _file['tool_file']['file_type'] == "CONTAINERFILE": | ||
| # if _file['type'] not in self.image_types: | ||
| # logger.error("Missing or invalid image file type.") | ||
| # raise BadRequest |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist:
Summary by Sourcery
Enable external file storage support by introducing a FileStorageManager with MinIO and MongoDB strategies, refactor all file-related endpoints to store, retrieve, and delete files via MinIO, add a bulk import endpoint, and update build and deployment configurations to include a MinIO service.
New Features:
Enhancements:
Build:
Deployment: