Removing confusing field from CosmosDBStorageConfig#377
Removing confusing field from CosmosDBStorageConfig#377rodrigobr-msft wants to merge 1 commit intomainfrom
Conversation
|
Awaiting completion of end-to-end test harness before merging this. |
There was a problem hiding this comment.
Pull request overview
This PR refactors Cosmos DB storage configuration by removing the ambiguous url field in favor of cosmos_db_endpoint, and tightens Cosmos client creation by validating required configuration up-front.
Changes:
- Removes
urlfromCosmosDBStorageConfigand updates configuration usage tocosmos_db_endpoint. - Updates
CosmosDBStorage._create_client()to requirecosmos_db_endpointand eithercredentialorauth_key, with clearerValueErrors. - Updates Cosmos storage tests to use
cosmos_db_endpoint.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/storage_cosmos/test_cosmos_db_storage.py | Updates Azure-credential test setup to pass cosmos_db_endpoint instead of the removed url. |
| libraries/microsoft-agents-storage-cosmos/microsoft_agents/storage/cosmos/cosmos_db_storage_config.py | Removes the url config field/param and its docstring mention. |
| libraries/microsoft-agents-storage-cosmos/microsoft_agents/storage/cosmos/cosmos_db_storage.py | Adds stricter client initialization validation and improved configuration error handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __init__( | ||
| self, | ||
| cosmos_db_endpoint: str = "", | ||
| auth_key: str = "", | ||
| database_id: str = "", | ||
| container_id: str = "", | ||
| cosmos_client_options: dict = None, | ||
| container_throughput: int | None = None, | ||
| key_suffix: str = "", | ||
| compatibility_mode: bool = False, | ||
| url: str = "", | ||
| credential: Union[AsyncTokenCredential, None] = None, | ||
| **kwargs, | ||
| ): |
There was a problem hiding this comment.
url was removed from the explicit constructor params, but **kwargs still allows callers (or JSON config files) to pass url without any error. Because the initializer no longer reads kwargs.get("url"), this legacy value is silently ignored and later surfaces as a missing cosmos_db_endpoint error. Consider either (a) supporting url as a deprecated alias for cosmos_db_endpoint (preferably with a warning), or (b) detecting "url" in kwargs and raising a clear ValueError telling users to use cosmos_db_endpoint.
| max key length of 255. | ||
| :param url: The URL to the CosmosDB resource. | ||
| :param credential: The TokenCredential to use for authentication. | ||
| :return CosmosDBConfig: |
There was a problem hiding this comment.
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.
| :return CosmosDBConfig: |
| 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." | ||
| ) | ||
| ) |
There was a problem hiding this comment.
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).
| 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." | ||
| ) | ||
| ) |
There was a problem hiding this comment.
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.
| url = os.environ.get("TEST_COSMOS_DB_ENDPOINT", "") | ||
| config = CosmosDBStorageConfig( | ||
| url=url, | ||
| cosmos_db_endpoint=url, | ||
| credential=cred, |
There was a problem hiding this comment.
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.
This pull request refactors the Cosmos DB storage configuration and client initialization to improve clarity and error handling. The main changes include removing the ambiguous
urlparameter in favor of a more clearly namedcosmos_db_endpoint, updating client initialization logic to enforce required parameters, and improving error messages for missing configuration.Configuration and parameter changes:
urlparameter from theCosmosDBStorageConfigclass and replaced all usages withcosmos_db_endpointfor clearer intent. [1] [2] [3] [4]Client initialization and validation improvements:
_create_clientmethod incosmos_db_storage.pyto requirecosmos_db_endpointand eithercredentialorauth_key, raising clearValueErrors if these are missing. This ensures that clients are only created with valid configurations and provides more helpful error messages. [1] [2]Test updates:
cosmos_db_endpointparameter instead of the removedurlparameter.