Skip to content

Removing confusing field from CosmosDBStorageConfig #377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
rodrigobr-msft wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Removing confusing field from CosmosDBStorageConfig #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 31 additions & 23 deletions ...ries/microsoft-agents-storage-cosmos/microsoft_agents/storage/cosmos/cosmos_db_storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,33 +53,41 @@ def __init__(self, config: CosmosDBStorageConfig):
self._lock: asyncio.Lock = asyncio.Lock()

def _create_client(self) -> CosmosClient:
if self._config.url:
if not self._config.credential:
raise ValueError(
storage_errors.InvalidConfiguration.format(
"Credential is required when using a custom service URL"
)

if not self._config.cosmos_db_endpoint:
raise ValueError(
storage_errors.InvalidConfiguration.format(
"Cosmos DB Endpoint is required."
)
return CosmosClient(
url=self._config.url, credential=self._config.credential
)

connection_policy = self._config.cosmos_client_options.get(
"connection_policy", documents.ConnectionPolicy()
)
if self._config.credential or self._config.auth_key:
cred = (
self._config.credential
if self._config.credential
else self._config.auth_key
)

# kwargs 'connection_verify' is to handle CosmosClient overwriting the
# ConnectionPolicy.DisableSSLVerification value.
return CosmosClient(
self._config.cosmos_db_endpoint,
self._config.auth_key,
consistency_level=self._config.cosmos_client_options.get(
"consistency_level", None
),
**{
"connection_policy": connection_policy,
"connection_verify": not connection_policy.DisableSSLVerification,
},
connection_policy = self._config.cosmos_client_options.get(
"connection_policy", documents.ConnectionPolicy()
)

return CosmosClient(
url=self._config.cosmos_db_endpoint,
credential=cred,
consistency_level=self._config.cosmos_client_options.get(
"consistency_level", None
),
**{
"connection_policy": connection_policy,
"connection_verify": not connection_policy.DisableSSLVerification,
},
)

raise ValueError(
storage_errors.InvalidConfiguration.format(
"Either Cosmos DB Credential or Auth Key is required."
)
)
Comment on lines +57 to 91
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new missing-endpoint/auth validation uses storage_errors.InvalidConfiguration (error code -61012), but this package already defines dedicated CosmosDbEndpointRequired (-61001) and CosmosDbAuthKeyRequired (-61002) messages. Using the dedicated resources would preserve more specific error codes and align with how other required fields are reported (e.g., database_id/container_id).

Copilot uses AI. Check for mistakes.
Comment on lines +57 to 91
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior: _create_client now raises when cosmos_db_endpoint is missing, and when neither credential nor auth_key is provided. There don’t appear to be unit tests asserting these failure modes/messages; adding tests that instantiate CosmosDBStorage with invalid configs and assert the raised ValueError would prevent regressions.

Copilot uses AI. Check for mistakes.

def _sanitize(self, key: str) -> str:
Expand Down
3 changes: 0 additions & 3 deletions ...crosoft-agents-storage-cosmos/microsoft_agents/storage/cosmos/cosmos_db_storage_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ def __init__(
container_throughput: int | None = None,
key_suffix: str = "",
compatibility_mode: bool = False,
url: str = "",
credential: Union[AsyncTokenCredential, None] = None,
**kwargs,
):
Expand All @@ -37,7 +36,6 @@ def __init__(
key characters. (e.g. not: '\\', '?', '/', '#', '*')
:param compatibility_mode: True if keys should be truncated in order to support previous CosmosDb
max key length of 255.
:param url: The URL to the CosmosDB resource.
:param credential: The TokenCredential to use for authentication.
:return CosmosDBConfig:
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring :return CosmosDBConfig: doesn’t match the actual class name (CosmosDBStorageConfig) and the constructor doesn’t explicitly “return” a different type. Consider updating/removing this line to avoid misleading generated docs.

Suggested change
:return CosmosDBConfig:

Copilot uses AI. Check for mistakes.
"""
Expand All @@ -61,7 +59,6 @@ def __init__(
self.compatibility_mode: bool = compatibility_mode or kwargs.get(
"compatibility_mode", False
)
self.url = url or kwargs.get("url", "")
self.credential: Union[AsyncTokenCredential, None] = credential

@staticmethod
Expand Down
4 changes: 2 additions & 2 deletions tests/storage_cosmos/test_cosmos_db_storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,9 @@ async def test_cosmos_db_from_azure_cred(self):
load_dotenv()

cred = DefaultAzureCredential()
url = os.environ.get("TEST_COSMOS_DB_ENDPOINT")
url = os.environ.get("TEST_COSMOS_DB_ENDPOINT", "")
config = CosmosDBStorageConfig(
url=url,
cosmos_db_endpoint=url,
credential=cred,
Comment on lines +256 to 259
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local variable url now holds the Cosmos DB endpoint; renaming it to cosmos_db_endpoint (or endpoint) would better match the new config parameter name and avoid reintroducing the old terminology.

Copilot uses AI. Check for mistakes.
database_id="test-db",
container_id="bot-storage",
Expand Down
Loading