Adding Storage wrappers for internal use cases#159
Adding Storage wrappers for internal use cases#159rodrigobr-msft wants to merge 8 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the storage subsystem by reorganizing transcript-related modules into a new _transcript internal package, introducing storage wrapper classes for caching and namespacing functionality, and modernizing type annotations to Python 3.9+ standards.
Key Changes:
- Moved transcript modules into a new
_transcriptsubpackage with updated imports - Added new storage wrapper classes (
_CachedStorage,_StorageNamespace,_ItemStorage) for enhanced functionality - Renamed
AsyncStorageBaseto_AsyncStorageBaseand replacedpassstatements withraise NotImplementedError()in abstract methods
Reviewed Changes
Copilot reviewed 17 out of 21 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
tests/hosting_core/storage/wrappers/test_storage_namespace.py |
New test file for _StorageNamespace wrapper functionality |
tests/hosting_core/storage/test_transcript_store_memory.py |
Updated import paths for transcript modules |
tests/hosting_core/storage/test_transcript_logger_middleware.py |
Updated import paths for transcript modules |
libraries/microsoft-agents-storage-cosmos/microsoft_agents/storage/cosmos/cosmos_db_storage.py |
Updated to use renamed _AsyncStorageBase |
libraries/microsoft-agents-storage-blob/microsoft_agents/storage/blob/blob_storage.py |
Updated to use renamed _AsyncStorageBase |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/storage.py |
Renamed base class and replaced pass with raise NotImplementedError() |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_wrappers/_storage_namespace.py |
New storage wrapper for namespacing keys |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_wrappers/_memory_cache.py |
New cache implementations for storage wrappers |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_wrappers/_cached_storage.py |
New storage wrapper adding caching layer |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_wrappers/__init__.py |
Exports for new wrapper classes |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_transcript/transcript_store.py |
Updated type annotations and moved to _transcript package |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_transcript/transcript_memory_store.py |
Updated type annotations and moved to _transcript package |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_transcript/transcript_logger.py |
Updated type annotations and moved to _transcript package |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_transcript/transcript_file_store.py |
Updated type annotations, removed _to_plain_dict helper, moved to _transcript package |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_transcript/__init__.py |
New __init__.py for _transcript package |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/__init__.py |
Updated exports for reorganized structure |
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/_oauth/_flow_storage_client.py |
Added commented placeholder code |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| StoreItemT = TypeVar("StoreItemT", bound=StoreItem) | ||
|
|
||
|
|
||
| class _DummyCache(Storage): |
There was a problem hiding this comment.
Missing import for Storage class. The file imports MemoryStorage and StoreItem but not Storage, which is required as the base class for _DummyCache.
|
|
||
| class _DummyCache(Storage): | ||
|
|
||
| async def read(self, keys: list[str], **kwargs) -> dict[str, _FlowState]: |
There was a problem hiding this comment.
Missing import for _FlowState. The return type references _FlowState which is not imported in this file.
| async def read(self, keys: list[str], **kwargs) -> dict[str, _FlowState]: | ||
| return {} | ||
|
|
||
| async def write(self, changes: dict[str, _FlowState]) -> None: |
There was a problem hiding this comment.
The parameter type references _FlowState which is not imported. This should use a generic type variable like StoreItemT to match the file's existing generic pattern.
| async def read(self, keys: list[str], **kwargs) -> dict[str, _FlowState]: | |
| return {} | |
| async def write(self, changes: dict[str, _FlowState]) -> None: | |
| async def read(self, keys: list[str], **kwargs) -> dict[str, StoreItemT]: | |
| return {} | |
| async def write(self, changes: dict[str, StoreItemT]) -> None: |
| self.clear() | ||
| return await super().read(keys, target_cls=target_cls, **kwargs) | ||
|
|
||
| async def write(self, changes: dict[str, StoreItem]): | ||
| self.clear() | ||
| return await super().write(changes) | ||
|
|
||
| async def delete(self, keys: list[str]): | ||
| self.clear() |
There was a problem hiding this comment.
Missing await keyword. The clear() method is defined as async on line 36 but is called without await here and on lines 51 and 55.
| self.clear() | |
| return await super().read(keys, target_cls=target_cls, **kwargs) | |
| async def write(self, changes: dict[str, StoreItem]): | |
| self.clear() | |
| return await super().write(changes) | |
| async def delete(self, keys: list[str]): | |
| self.clear() | |
| await self.clear() | |
| return await super().read(keys, target_cls=target_cls, **kwargs) | |
| async def write(self, changes: dict[str, StoreItem]): | |
| await self.clear() | |
| return await super().write(changes) | |
| async def delete(self, keys: list[str]): | |
| await self.clear() |
|
|
||
| def key(self, vkey: str) -> str: | ||
| """Creates a storage key for a specific sign-in handler.""" | ||
| return f"{self._base_key}:{vkey}" |
There was a problem hiding this comment.
Key construction uses a colon separator but the test in test_storage_namespace.py line 34 expects a forward slash separator (e.g., auth/{channel_id}/{user_id}/). This inconsistency will cause the key construction to produce keys like auth/channel/user/:handler instead of auth/channel/user/handler.
| channel_id: used to create the prefix | ||
| user_id: used to create the prefix | ||
| storage: the backing storage | ||
| cache_class: the cache class to use (defaults to DummyCache, which performs no caching). | ||
| This cache's lifetime is tied to the FlowStorageClient instance. |
There was a problem hiding this comment.
The docstring describes parameters (channel_id, user_id, cache_class) that don't match the actual method signature, which only takes namespace and storage. This appears to be copy-pasted from _FlowStorageClient and needs to be updated to reflect the actual parameters.
| channel_id: used to create the prefix | |
| user_id: used to create the prefix | |
| storage: the backing storage | |
| cache_class: the cache class to use (defaults to DummyCache, which performs no caching). | |
| This cache's lifetime is tied to the FlowStorageClient instance. | |
| namespace: The key prefix used for storage operations to isolate state. | |
| storage: The backing Storage instance. |
| @@ -0,0 +1,47 @@ | |||
| from typing import TypeVar, Generic | |||
|
|
|||
| from strenum import StrEnum | |||
There was a problem hiding this comment.
Unused import. The StrEnum class is imported but never used in this file.
| from strenum import StrEnum |
| @@ -0,0 +1,47 @@ | |||
| from typing import TypeVar, Generic | |||
There was a problem hiding this comment.
Unused import. The Generic class is imported but never used in this file.
| from typing import TypeVar, Generic | |
| from typing import TypeVar |
| from ._cached_storage import _CachedStorage, _ClearableCachedStorage | ||
| from ._item_storage import _ItemStorage | ||
| from ._storage_namespace import _StorageNamespace | ||
|
|
||
| __all__ = [ | ||
| "_CachedStorage", | ||
| "_ClearableCachedStorage", | ||
| "_ItemStorage", | ||
| "_StorageNamespace", | ||
| ] |
There was a problem hiding this comment.
Import error. _ClearableCachedStorage is imported but does not exist in _cached_storage.py. Only _CachedStorage is defined in that file.
| from ._cached_storage import _CachedStorage, _ClearableCachedStorage | |
| from ._item_storage import _ItemStorage | |
| from ._storage_namespace import _StorageNamespace | |
| __all__ = [ | |
| "_CachedStorage", | |
| "_ClearableCachedStorage", | |
| "_ItemStorage", | |
| "_StorageNamespace", | |
| ] | |
| from ._cached_storage import _CachedStorage | |
| from ._item_storage import _ItemStorage | |
| from ._storage_namespace import _StorageNamespace | |
| __all__ = [ | |
| "_CachedStorage", | |
| "_ItemStorage", | |
| "_StorageNamespace", | |
| ] |
| @@ -0,0 +1,10 @@ | |||
| from ._cached_storage import _CachedStorage, _ClearableCachedStorage | |||
| from ._item_storage import _ItemStorage | |||
There was a problem hiding this comment.
Import error. The file _item_storage.py is referenced but was not included in the PR changes, suggesting this import will fail unless the file exists elsewhere.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 25 out of 30 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| channel_id: used to create the prefix | ||
| user_id: used to create the prefix | ||
| storage: the backing storage | ||
| cache_class: the cache class to use (defaults to DummyCache, which performs no caching). | ||
| This cache's lifetime is tied to the FlowStorageClient instance. |
There was a problem hiding this comment.
The docstring describes incorrect parameters. It mentions channel_id, user_id, and cache_class, but the actual parameters are namespace and storage. The docstring should be updated to accurately describe the actual parameters.
| channel_id: used to create the prefix | |
| user_id: used to create the prefix | |
| storage: the backing storage | |
| cache_class: the cache class to use (defaults to DummyCache, which performs no caching). | |
| This cache's lifetime is tied to the FlowStorageClient instance. | |
| namespace: The key prefix used for storage operations to isolate data. | |
| storage: The backing Storage instance. |
| return self._base_key | ||
|
|
||
| def key(self, vkey: str) -> str: | ||
| """Creates a storage key for a specific sign-in handler.""" |
There was a problem hiding this comment.
The docstring mentions 'sign-in handler' which is too specific. This is a generic method that creates a namespaced storage key for any virtual key, not just sign-in handlers. Update the docstring to reflect its generic purpose.
| """Creates a storage key for a specific sign-in handler.""" | |
| """Creates a namespaced storage key for the given virtual key.""" |
| ) | ||
| from microsoft_agents.hosting.core.storage import ( | ||
| _ItemNamespace, | ||
| Namespaces |
There was a problem hiding this comment.
Importing Namespaces but the constant class is named _Namespaces (with underscore prefix) as shown in the _namespaces.py file. This import will fail. Should be _Namespaces.
| if sign_in_response.tag == _FlowStateTag.COMPLETE: | ||
| if self._sign_in_success_handler: | ||
| await self._sign_in_success_handler(context, state, auth_handler_id) | ||
| await self._sign_in_state_store.delete(self._sign_in_state_vkey(context)) |
There was a problem hiding this comment.
The sign-in state is being deleted twice - once through the new _sign_in_state_store.delete() on line 245, and again through the existing _delete_sign_in_state() method on line 246. This appears to be redundant. Either remove one of these calls or verify if both are intentionally needed for different purposes.
| await self._sign_in_state_store.delete(self._sign_in_state_vkey(context)) |
| def _utc_iso_now() -> str: | ||
| return datetime.now(timezone.utc).isoformat() |
There was a problem hiding this comment.
The function _utc_iso_now() is defined but never used in the file. The removed _to_plain_dict() function was also unused. If this new function is not used elsewhere, it should be removed as part of the cleanup effort.
| def _utc_iso_now() -> str: | |
| return datetime.now(timezone.utc).isoformat() |
1 participant
This pull request refactors and reorganizes the storage subsystem, with a focus on transcript storage and storage wrappers. Key changes include moving transcript-related modules into a new internal
_transcriptpackage, introducing new storage wrapper classes for caching and namespacing, and modernizing type annotations. The updates improve code organization, extensibility, and type safety.Transcript storage refactor and type annotation improvements:
transcript_info,transcript_logger,transcript_store,transcript_file_store) into a new_transcriptsubpackage, updating all import paths and__all__definitions accordingly. This improves modularity and encapsulation of transcript functionality. [1] [2]list[Activity]instead ofList[Activity]), and standardized the use ofOptionaland return types for consistency and clarity. [1] [2] [3] [4] [5] [6] [7] [8] [9]_to_plain_dicthelper fromtranscript_file_store.py, cleaning up legacy code.Storage wrapper enhancements:
_CachedStorageand_ClearableCachedStoragefor adding a caching layer to storage,_ItemStoragefor type-safe item access, and_StorageNamespacefor key namespacing based on user/channel. These wrappers are now exported from the_wrapperspackage. [1] [2] [3] [4]Base storage class improvements:
AsyncStorageBaseto_AsyncStorageBaseand updated its import/export. Replaced placeholderpassstatements withraise NotImplementedError()in all unimplemented methods ofStorageand_AsyncStorageBaseto enforce correct subclassing. [1] [2] [3] [4]These changes collectively modernize and modularize the storage layer, making it easier to extend and maintain.