Skip to content

feat(rag): add StorageBase interface for encrypted storage#13458

Open
Achieve3318 wants to merge 3 commits intoinfiniflow:mainfrom
Achieve3318:fix/encrypted-storage-base
Open

feat(rag): add StorageBase interface for encrypted storage#13458
Achieve3318 wants to merge 3 commits intoinfiniflow:mainfrom
Achieve3318:fix/encrypted-storage-base

Conversation

@Achieve3318
Copy link
Contributor

@Achieve3318 Achieve3318 commented Mar 6, 2026

Design Purpose

  • Define a formal storage abstraction
    Introduce StorageBase in rag/utils/storage_base.py as the canonical interface for storage backends used with encrypted storage. It declares the required operations (put, get, rm, obj_exist, health) instead of relying on ad-hoc hasattr checks.

  • Make EncryptedStorageWrapper safer and self-documenting
    EncryptedStorageWrapper now expects a StorageBase implementation (with a defensive runtime check). This clarifies what backends must provide, reduces the risk of partially implemented or incompatible backends, and makes it easier for new backends (MinIO, S3, filesystem, etc.) to integrate by implementing one interface.

  • Enable local, dependency-free tests for storage behavior
    Add an in-memory InMemoryStorage implementation and unit tests in test_storage_base.py so the storage contract can be validated without external services or configuration.

File Changes

Modified files

rag/utils/encrypted_storage.py (+3, -3)

  • Import: from rag.utils.storage_base import StorageBase.
  • EncryptedStorageWrapper.__init__: parameter type hint storage_impl: StorageBase.
  • Updated comment: replaced TODO with note that backends should implement StorageBase, with defensive runtime checks for duck typing.

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. 💞 feature Feature request, pull request that fullfill a new feature. labels Mar 6, 2026
@Achieve3318
Copy link
Contributor Author

Hi, @yingfeng , Please review my PR. Thanks

@Achieve3318
Copy link
Contributor Author

Hi, @KevinHuSh , Could you review my PR if you have time? Thanks

@yingfeng
Copy link
Member

yingfeng commented Mar 9, 2026

What's the design purpose of this PR?

@Achieve3318
Copy link
Contributor Author

What's the design purpose of this PR?

Hi, @yingfeng , Thank you for your review. I updated description of this PR. Please check again.

@yingfeng
Copy link
Member

yingfeng commented Mar 9, 2026

I meant why introducing a base class for EncryptedStorage? It's just a utility to wrap up other S3 storage. There does not exist a so-called in-memory storage, and then adding new storage type as MinIO, S3,...,etc because those storage types already exists and they are not based on your newly defined StorageBase.

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Mar 9, 2026
@Achieve3318
Copy link
Contributor Author

@yingfeng , Thanks for the feedback. I’ve removed the StorageBase abstraction and the in-memory storage type.

The EncryptedStorageWrapper is now kept as a thin, duck-typed utility around the existing storage backends (MinIO/S3, etc.). It simply validates at runtime that the wrapped implementation exposes the minimal method set it relies on (put, get, rm, obj_exist, health), without introducing a new base class or new storage type.

@Achieve3318
Copy link
Contributor Author

Hi, @yingfeng , Could you review my PR again? Thank you

@yingfeng
Copy link
Member

So, what's left in this PR?

@Achieve3318
Copy link
Contributor Author

So, what's left in this PR?

At this point the PR is very small

  • EncryptedStorageWrapper is still just a thin, duck-typed wrapper over the existing storage implementations (MinIO/S3/etc.).
  • The only behavior it enforces is a runtime check that the wrapped storage has the minimal method set it already relies on: put/get/rm/obj_exist/health.
  • The previous StorageBase abstraction and in-memory storage/test have been removed, so there are no new storage types or base classes introduced.

@Achieve3318
Copy link
Contributor Author

I replaced TODO with note that backends should implement StorageBase, with defensive runtime checks for duck typing.

@yingfeng
Copy link
Member

I can only see some comments

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Mar 10, 2026
@Achieve3318
Copy link
Contributor Author

I can only see some comments

oh, sorry, I had mistake. Please review again. Thank you for your review

@Achieve3318
Copy link
Contributor Author

@yingfeng , If you have time, please check my updates again. Thank you

@Achieve3318
Copy link
Contributor Author

@yingfeng . Please check my PR if you have time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💞 feature Feature request, pull request that fullfill a new feature. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants