Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Greptile OverviewGreptile SummaryThis PR refactors the attachment storage system using the Strategy and Factory design patterns, creating a proper service layer architecture. The refactor successfully separates storage concerns from the data model and provides a unified interface for S3, GCS, and local storage backends. Key Improvements:
Architecture Quality: Process Note: Confidence Score: 4/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/fides/api/models/attachment.py
Outdated
| """Creates a new attachment record in the database.""" | ||
| return super().create(db=db, data=data, check_name=check_name) |
| # Mapping of storage types to their uploader functions | ||
| # These functions handle DSR-specific data transformation before upload | ||
| _DSR_UPLOADERS: Dict[str, Any] = {} # Populated lazily | ||
|
|
There was a problem hiding this comment.
Does this do anything? I didn't see it referenced anywhere in the changes or code.
There was a problem hiding this comment.
Good catch, it was unused. The actual routing happens inline in _get_uploader_from_config_type. Removed it.
| db = self._require_db() | ||
| return Attachment.get_by_key_or_id(db, data={"id": attachment_id}) | ||
|
|
||
| def _get_provider_and_bucket(self, attachment: Attachment) -> Tuple[Any, str]: |
There was a problem hiding this comment.
Is Any right or should it be StorageProvider?
| def _get_provider_and_bucket(self, attachment: Attachment) -> Tuple[Any, str]: | |
| def _get_provider_and_bucket(self, attachment: Attachment) -> Tuple[StorageProvider, str]: |
There was a problem hiding this comment.
Updated to Tuple[StorageProvider, str] and added the import. Also changed the return type of retrieve_url from Tuple[int, AnyHttpUrlString] to Tuple[int, str] since generate_presigned_url returns str across all providers (the local provider returns a file path, not a valid HTTP URL).
|
|
||
| try: | ||
| blob.delete() | ||
| except (AttributeError, RuntimeError) as e: |
There was a problem hiding this comment.
The GCS sdk has some specific exceptions for not finding a file - i think its google.api.core.exceptions.NotFound or a broader google.api.core.exceptions.GoogleAPICallError. Could we perhaps use those here so we arent accidentally swallowing other unexpected errors?
There was a problem hiding this comment.
Good call. Replaced (AttributeError, RuntimeError) with NotFound and GoogleAPICallError from google.api_core.exceptions in delete, delete_prefix, and generate_presigned_url. NotFound is logged at debug level since it's expected for cleanup operations.
JadeCara
left a comment
JadeCara
left a comment
There was a problem hiding this comment.
Tested DSRs with attachments using S3 and worked as expected.
2 participants
Description Of Changes
Refactors the attachment storage system to use a proper service layer architecture with the Strategy and Factory design patterns. This improves code organization, testability, and maintainability by separating storage concerns from the data model.
Key architectural improvements:
StorageProviderabstract base class that defines a consistent interface for all storage backends (S3, GCS, Local)StorageProviderFactoryfor clean provider instantiation based on configurationAttachmentServicein the service layer to handle all storage operations, following the project's layered architecture (API routes → Services → Models)Attachmentmodel, keeping it focused on data representation onlyCode Changes
StorageProviderabstract base class (src/fides/api/service/storage/providers/base.py) defining unified interface for upload, download, delete, presigned URLs, and file operationsS3StorageProviderimplementation (src/fides/api/service/storage/providers/s3_provider.py)GCSStorageProviderimplementation (src/fides/api/service/storage/providers/gcs_provider.py)LocalStorageProviderimplementation (src/fides/api/service/storage/providers/local_provider.py)StorageProviderFactory(src/fides/api/service/storage/providers/factory.py) for creating providers from configurationAttachmentService(src/fides/service/attachment_service.py) for attachment storage operations with methods for upload, retrieve, delete, and reference managementAttachmentmodel to remove embedded storage logicexternal_data_storage.pyto use new provider architecturepolling_attachment_handler.pyto useAttachmentServiceSteps to Confirm
pytest tests/ops/service/storage/providers/pytest tests/ctl/models/test_attachment.pyPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works