From c529c241d15bf157a4fd6b6227a7f5e46f8db33e Mon Sep 17 00:00:00 2001 From: dibahlfi <106994927+dibahlfi@users.noreply.github.com> Date: Fri, 30 Jan 2026 08:28:46 -0600 Subject: [PATCH 1/4] fix - passing None as a ccount should handle it gracefully --- .../azure/cosmos/_global_endpoint_manager.py | 4 + .../aio/_global_endpoint_manager_async.py | 4 +- .../azure-cosmos/tests/test_health_check.py | 63 +++++++++++ .../tests/test_health_check_async.py | 106 ++++++++++++++++++ 4 files changed, 176 insertions(+), 1 deletion(-) diff --git a/sdk/cosmos/azure-cosmos/azure/cosmos/_global_endpoint_manager.py b/sdk/cosmos/azure-cosmos/azure/cosmos/_global_endpoint_manager.py index 5e6ffb142400..26358cab63f0 100644 --- a/sdk/cosmos/azure-cosmos/azure/cosmos/_global_endpoint_manager.py +++ b/sdk/cosmos/azure-cosmos/azure/cosmos/_global_endpoint_manager.py @@ -142,6 +142,10 @@ def _refresh_endpoint_list_private(self, database_account=None, **kwargs): # background full refresh (database account + health checks) self._start_background_refresh(self._refresh_database_account_and_health, kwargs) else: + # Fetch database account if not provided or explicitly None + # This ensures callers can pass None and still get correct behavior + if database_account is None: + database_account = self._GetDatabaseAccount(**kwargs) self.location_cache.perform_on_database_account_read(database_account) self._start_background_refresh(self._endpoints_health_check, kwargs) self.startup = False diff --git a/sdk/cosmos/azure-cosmos/azure/cosmos/aio/_global_endpoint_manager_async.py b/sdk/cosmos/azure-cosmos/azure/cosmos/aio/_global_endpoint_manager_async.py index 6494e6e2a626..d883e01028a2 100644 --- a/sdk/cosmos/azure-cosmos/azure/cosmos/aio/_global_endpoint_manager_async.py +++ b/sdk/cosmos/azure-cosmos/azure/cosmos/aio/_global_endpoint_manager_async.py @@ -145,7 +145,9 @@ async def _refresh_endpoint_list_private(self, database_account=None, **kwargs): # in background self.refresh_task = asyncio.create_task(self._refresh_database_account_and_health()) else: - if not self._aenter_used: + # Fetch database account if not provided via async with pattern OR if explicitly None + # This ensures callers can pass None and still get correct behavior + if not self._aenter_used or database_account is None: database_account = await self._GetDatabaseAccount(**kwargs) self.location_cache.perform_on_database_account_read(database_account) # this will perform only calls to check endpoint health diff --git a/sdk/cosmos/azure-cosmos/tests/test_health_check.py b/sdk/cosmos/azure-cosmos/tests/test_health_check.py index 584db862e76a..6f31c621cf66 100644 --- a/sdk/cosmos/azure-cosmos/tests/test_health_check.py +++ b/sdk/cosmos/azure-cosmos/tests/test_health_check.py @@ -155,5 +155,68 @@ def __call__(self, endpoint): db_acc.ConsistencyPolicy = {"defaultConsistencyLevel": "Session"} return db_acc + + def test_force_refresh_on_startup_with_none_should_fetch_database_account(self, setup): + """Verifies that calling force_refresh_on_startup(None) fetches the database account + instead of crashing with AttributeError on NoneType._WritableLocations. + """ + self.original_getDatabaseAccountStub = _global_endpoint_manager._GlobalEndpointManager._GetDatabaseAccountStub + mock_get_db_account = self.MockGetDatabaseAccount(REGIONS) + _global_endpoint_manager._GlobalEndpointManager._GetDatabaseAccountStub = mock_get_db_account + + try: + client = CosmosClient(self.host, self.masterKey, preferred_locations=REGIONS) + gem = client.client_connection._global_endpoint_manager + + # Simulate the startup state + gem.startup = True + gem.refresh_needed = True + + # This should NOT crash - it should fetch the database account + gem.force_refresh_on_startup(None) + + # Verify the location cache was properly populated + read_contexts = gem.location_cache.read_regional_routing_contexts + assert len(read_contexts) > 0, "Location cache should have read endpoints after startup refresh" + + finally: + _global_endpoint_manager._GlobalEndpointManager._GetDatabaseAccountStub = self.original_getDatabaseAccountStub + + def test_force_refresh_on_startup_with_valid_account_uses_provided_account(self, setup): + """Verifies that when a valid database account is provided to force_refresh_on_startup, + it uses that account directly without making another network call. + """ + self.original_getDatabaseAccountStub = _global_endpoint_manager._GlobalEndpointManager._GetDatabaseAccountStub + call_counter = {'count': 0} + + def counting_mock(self_gem, endpoint, **kwargs): + call_counter['count'] += 1 + return self.MockGetDatabaseAccount(REGIONS)(endpoint) + + _global_endpoint_manager._GlobalEndpointManager._GetDatabaseAccountStub = counting_mock + + try: + client = CosmosClient(self.host, self.masterKey, preferred_locations=REGIONS) + gem = client.client_connection._global_endpoint_manager + + # Get a valid database account first + db_account = gem._GetDatabaseAccount() + initial_call_count = call_counter['count'] + + # Reset startup state + gem.startup = True + gem.refresh_needed = True + + # Call with valid account - should NOT make another network call + gem.force_refresh_on_startup(db_account) + + # Since we provided a valid account, no additional GetDatabaseAccount call should be made + assert call_counter['count'] == initial_call_count, \ + "Should not call _GetDatabaseAccount when valid account is provided" + + finally: + _global_endpoint_manager._GlobalEndpointManager._GetDatabaseAccountStub = self.original_getDatabaseAccountStub + + if __name__ == '__main__': unittest.main() diff --git a/sdk/cosmos/azure-cosmos/tests/test_health_check_async.py b/sdk/cosmos/azure-cosmos/tests/test_health_check_async.py index 716556b0fda2..d69a10734bc9 100644 --- a/sdk/cosmos/azure-cosmos/tests/test_health_check_async.py +++ b/sdk/cosmos/azure-cosmos/tests/test_health_check_async.py @@ -277,5 +277,111 @@ async def __call__(self, endpoint): db_acc.ConsistencyPolicy = {"defaultConsistencyLevel": "Session"} return db_acc + + async def test_force_refresh_on_startup_with_none_should_fetch_database_account(self, setup): + """Verifies that calling force_refresh_on_startup(None) fetches the database account + instead of crashing with AttributeError on NoneType._WritableLocations. + """ + self.original_getDatabaseAccountStub = _global_endpoint_manager_async._GlobalEndpointManager._GetDatabaseAccountStub + mock_get_db_account = self.MockGetDatabaseAccount(REGIONS) + _global_endpoint_manager_async._GlobalEndpointManager._GetDatabaseAccountStub = mock_get_db_account + + try: + client = CosmosClient(self.host, self.masterKey, preferred_locations=REGIONS) + await client.__aenter__() + gem = client.client_connection._global_endpoint_manager + + # Simulate the startup state + gem.startup = True + gem.refresh_needed = True + gem._aenter_used = True # Simulate that __aenter__ was used + + # This should NOT crash - it should fetch the database account + await gem.force_refresh_on_startup(None) + + # Verify the location cache was properly populated + read_contexts = gem.location_cache.read_regional_routing_contexts + assert len(read_contexts) > 0, "Location cache should have read endpoints after startup refresh" + + await client.close() + finally: + _global_endpoint_manager_async._GlobalEndpointManager._GetDatabaseAccountStub = self.original_getDatabaseAccountStub + + async def test_force_refresh_on_startup_with_valid_account_uses_provided_account(self, setup): + """Verifies that when a valid database account is provided to force_refresh_on_startup, + it uses that account directly without making another network call. + """ + self.original_getDatabaseAccountStub = _global_endpoint_manager_async._GlobalEndpointManager._GetDatabaseAccountStub + call_counter = {'count': 0} + + async def counting_mock(self_gem, endpoint, **kwargs): + call_counter['count'] += 1 + return await self.MockGetDatabaseAccount(REGIONS)(endpoint) + + _global_endpoint_manager_async._GlobalEndpointManager._GetDatabaseAccountStub = counting_mock + + try: + client = CosmosClient(self.host, self.masterKey, preferred_locations=REGIONS) + await client.__aenter__() + gem = client.client_connection._global_endpoint_manager + + # Get a valid database account first + db_account = await gem._GetDatabaseAccount() + initial_call_count = call_counter['count'] + + # Reset startup state + gem.startup = True + gem.refresh_needed = True + gem._aenter_used = True + + # Call with valid account - should NOT make another network call + await gem.force_refresh_on_startup(db_account) + + # Since we provided a valid account, no additional GetDatabaseAccount call should be made + + assert call_counter['count'] == initial_call_count, \ + "Should not call _GetDatabaseAccount when valid account is provided" + + await client.close() + finally: + _global_endpoint_manager_async._GlobalEndpointManager._GetDatabaseAccountStub = self.original_getDatabaseAccountStub + + async def test_aenter_used_flag_with_none_still_fetches_account(self, setup): + """Verifies that even when _aenter_used=True, passing None to force_refresh_on_startup + still fetches the database account. + """ + self.original_getDatabaseAccountStub = _global_endpoint_manager_async._GlobalEndpointManager._GetDatabaseAccountStub + fetch_was_called = {'called': False} + + async def tracking_mock(self_gem, endpoint, **kwargs): + fetch_was_called['called'] = True + return await self.MockGetDatabaseAccount(REGIONS)(endpoint) + + _global_endpoint_manager_async._GlobalEndpointManager._GetDatabaseAccountStub = tracking_mock + + try: + client = CosmosClient(self.host, self.masterKey, preferred_locations=REGIONS) + await client.__aenter__() + gem = client.client_connection._global_endpoint_manager + + # Reset state to simulate the buggy scenario + gem.startup = True + gem.refresh_needed = True + gem._aenter_used = True # This was causing the bug to skip fetching + fetch_was_called['called'] = False # Reset tracking + + # Call with None - should still fetch database account (this is the fix) + await gem.force_refresh_on_startup(None) + + # This ensures that even with _aenter_used=True, if database_account is None, + # it fetches the database account + assert fetch_was_called['called'], \ + "With _aenter_used=True and database_account=None, should still fetch database account" + + await client.close() + finally: + _global_endpoint_manager_async._GlobalEndpointManager._GetDatabaseAccountStub = self.original_getDatabaseAccountStub + + if __name__ == '__main__': unittest.main() From 2b08e163df1459788fc21f4717c53e6974c23c10 Mon Sep 17 00:00:00 2001 From: dibahlfi <106994927+dibahlfi@users.noreply.github.com> Date: Fri, 30 Jan 2026 10:24:24 -0600 Subject: [PATCH 2/4] updating chnage log --- sdk/cosmos/azure-cosmos/CHANGELOG.md | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/sdk/cosmos/azure-cosmos/CHANGELOG.md b/sdk/cosmos/azure-cosmos/CHANGELOG.md index ce0fdee5a729..5d6e2d77beb2 100644 --- a/sdk/cosmos/azure-cosmos/CHANGELOG.md +++ b/sdk/cosmos/azure-cosmos/CHANGELOG.md @@ -2,13 +2,8 @@ ### 4.14.6 (Unreleased) -#### Features Added - -#### Breaking Changes - #### Bugs Fixed - -#### Other Changes +* Fixed async client crash (`AttributeError: 'NoneType' object has no attribute '_WritableLocations'`) during region discovery when `database_account` was `None`. See [PR 44939](https://github.com/Azure/azure-sdk-for-python/pull/44939) ### 4.14.5 (2026-01-15) From 53f726f6c0acdb4e720c5a313bacfeaff0557aee Mon Sep 17 00:00:00 2001 From: dibahlfi <106994927+dibahlfi@users.noreply.github.com> Date: Fri, 30 Jan 2026 12:23:50 -0600 Subject: [PATCH 3/4] Update sdk/cosmos/azure-cosmos/CHANGELOG.md Co-authored-by: Simon Moreno <30335873+simorenoh@users.noreply.github.com> --- sdk/cosmos/azure-cosmos/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/cosmos/azure-cosmos/CHANGELOG.md b/sdk/cosmos/azure-cosmos/CHANGELOG.md index 5d6e2d77beb2..24dba99a0d9e 100644 --- a/sdk/cosmos/azure-cosmos/CHANGELOG.md +++ b/sdk/cosmos/azure-cosmos/CHANGELOG.md @@ -1,6 +1,6 @@ ## Release History -### 4.14.6 (Unreleased) +### 4.14.6 (2026-01-30) #### Bugs Fixed * Fixed async client crash (`AttributeError: 'NoneType' object has no attribute '_WritableLocations'`) during region discovery when `database_account` was `None`. See [PR 44939](https://github.com/Azure/azure-sdk-for-python/pull/44939) From 08bb45a486525f467eb10e4c2f97ec0ade8f674d Mon Sep 17 00:00:00 2001 From: Azure SDK Bot <53356347+azure-sdk@users.noreply.github.com> Date: Thu, 11 Dec 2025 11:43:56 -0800 Subject: [PATCH 4/4] Sync eng/common directory with azure-sdk-tools for PR 13235 (#44391) * Remove ApiKey usage * Add -TestAuth flag to verify Bearer token authentication * TEMP: Enable TestAuthOnly for pipeline testing * Remove testing logs * Additional clean up * Keep apikey fallback while migrating * Keep migration to new endpoint * Keep migration to new endpoint * Feedback --------- Co-authored-by: Alitzel Mendez --- .../templates/steps/create-apireview.yml | 52 +++++++++---------- eng/common/scripts/Create-APIReview.ps1 | 49 ++++++++++++++--- 2 files changed, 67 insertions(+), 34 deletions(-) diff --git a/eng/common/pipelines/templates/steps/create-apireview.yml b/eng/common/pipelines/templates/steps/create-apireview.yml index 6942abd0baa6..d93d5b44f6dd 100644 --- a/eng/common/pipelines/templates/steps/create-apireview.yml +++ b/eng/common/pipelines/templates/steps/create-apireview.yml @@ -37,29 +37,29 @@ steps: parameters: WorkingDirectory: ${{ parameters.SourceRootPath }} - - task: Powershell@2 - inputs: - filePath: ${{ parameters.SourceRootPath }}/eng/common/scripts/Create-APIReview.ps1 - arguments: > - -PackageInfoFiles ('${{ convertToJson(parameters.PackageInfoFiles) }}' | ConvertFrom-Json -NoEnumerate) - -ArtifactList ('${{ convertToJson(parameters.Artifacts) }}' | ConvertFrom-Json | Select-Object Name) - -ArtifactPath '${{parameters.ArtifactPath}}' - -ArtifactName ${{ parameters.ArtifactName }} - -APIKey '$(azuresdk-apiview-apikey)' - -PackageName '${{parameters.PackageName}}' - -SourceBranch '$(Build.SourceBranchName)' - -DefaultBranch '$(DefaultBranch)' - -ConfigFileDir '${{parameters.ConfigFileDir}}' - -BuildId '$(Build.BuildId)' - -RepoName '$(Build.Repository.Name)' - -MarkPackageAsShipped $${{parameters.MarkPackageAsShipped}} - pwsh: true - displayName: Create API Review - condition: >- - and( - succeededOrFailed(), - ne(variables['Skip.CreateApiReview'], 'true'), - ne(variables['Build.Reason'],'PullRequest'), - eq(variables['System.TeamProject'], 'internal'), - not(endsWith(variables['Build.Repository.Name'], '-pr')) - ) + - ${{ if and(eq(variables['System.TeamProject'], 'internal'), ne(variables['Build.Reason'], 'PullRequest'), not(endsWith(variables['Build.Repository.Name'], '-pr'))) }}: + - task: AzureCLI@2 + inputs: + azureSubscription: 'APIView prod deployment' + scriptType: pscore + scriptLocation: scriptPath + scriptPath: ${{ parameters.SourceRootPath }}/eng/common/scripts/Create-APIReview.ps1 + # PackageInfoFiles example: @('a/file1.json','a/file2.json') + arguments: > + -PackageInfoFiles @('${{ join(''',''', parameters.PackageInfoFiles) }}') + -ArtifactList ('${{ convertToJson(parameters.Artifacts) }}' | ConvertFrom-Json | Select-Object Name) + -ArtifactPath '${{parameters.ArtifactPath}}' + -ArtifactName ${{ parameters.ArtifactName }} + -PackageName '${{parameters.PackageName}}' + -SourceBranch '$(Build.SourceBranchName)' + -DefaultBranch '$(DefaultBranch)' + -ConfigFileDir '${{parameters.ConfigFileDir}}' + -BuildId '$(Build.BuildId)' + -RepoName '$(Build.Repository.Name)' + -MarkPackageAsShipped $${{parameters.MarkPackageAsShipped}} + displayName: Create API Review + condition: >- + and( + succeededOrFailed(), + ne(variables['Skip.CreateApiReview'], 'true') + ) \ No newline at end of file diff --git a/eng/common/scripts/Create-APIReview.ps1 b/eng/common/scripts/Create-APIReview.ps1 index 5d112d71a0a4..55f897624806 100644 --- a/eng/common/scripts/Create-APIReview.ps1 +++ b/eng/common/scripts/Create-APIReview.ps1 @@ -4,15 +4,13 @@ Param ( [array] $ArtifactList, [Parameter(Mandatory=$True)] [string] $ArtifactPath, - [Parameter(Mandatory=$True)] - [string] $APIKey, [string] $SourceBranch, [string] $DefaultBranch, [string] $RepoName, [string] $BuildId, [string] $PackageName = "", [string] $ConfigFileDir = "", - [string] $APIViewUri = "https://apiview.dev/AutoReview", + [string] $APIViewUri = "https://apiview.dev/autoreview", [string] $ArtifactName = "packages", [bool] $MarkPackageAsShipped = $false, [Parameter(Mandatory=$False)] @@ -20,9 +18,28 @@ Param ( ) Set-StrictMode -Version 3 + . (Join-Path $PSScriptRoot common.ps1) . (Join-Path $PSScriptRoot Helpers ApiView-Helpers.ps1) +# Get Bearer token for APIView authentication +# In Azure DevOps, this uses the service connection's Managed Identity/Service Principal +function Get-ApiViewBearerToken() +{ + try { + $tokenResponse = az account get-access-token --resource "api://apiview" --output json 2>&1 + if ($LASTEXITCODE -ne 0) { + Write-Error "Failed to acquire access token: $tokenResponse" + return $null + } + return ($tokenResponse | ConvertFrom-Json).accessToken + } + catch { + Write-Error "Failed to acquire access token: $($_.Exception.Message)" + return $null + } +} + # Submit API review request and return status whether current revision is approved or pending or failed to create review function Upload-SourceArtifact($filePath, $apiLabel, $releaseStatus, $packageVersion, $packageType) { @@ -78,9 +95,17 @@ function Upload-SourceArtifact($filePath, $apiLabel, $releaseStatus, $packageVer Write-Host "Request param, compareAllRevisions: true" } - $uri = "${APIViewUri}/UploadAutoReview" + $uri = "${APIViewUri}/upload" + + # Get Bearer token for authentication + $bearerToken = Get-ApiViewBearerToken + if (-not $bearerToken) { + Write-Error "Failed to acquire Bearer token for APIView authentication." + return [System.Net.HttpStatusCode]::Unauthorized + } + $headers = @{ - "ApiKey" = $apiKey; + "Authorization" = "Bearer $bearerToken"; "content-type" = "multipart/form-data" } @@ -115,20 +140,28 @@ function Upload-ReviewTokenFile($packageName, $apiLabel, $releaseStatus, $review if($MarkPackageAsShipped) { $params += "&setReleaseTag=true" } - $uri = "${APIViewUri}/CreateApiReview?${params}" + $uri = "${APIViewUri}/create?${params}" if ($releaseStatus -and ($releaseStatus -ne "Unreleased")) { $uri += "&compareAllRevisions=true" } Write-Host "Request to APIView: $uri" + + # Get Bearer token for authentication + $bearerToken = Get-ApiViewBearerToken + if (-not $bearerToken) { + Write-Error "Failed to acquire Bearer token for APIView authentication." + return [System.Net.HttpStatusCode]::Unauthorized + } + $headers = @{ - "ApiKey" = $APIKey; + "Authorization" = "Bearer $bearerToken" } try { - $Response = Invoke-WebRequest -Method 'GET' -Uri $uri -Headers $headers + $Response = Invoke-WebRequest -Method 'POST' -Uri $uri -Headers $headers Write-Host "API review: $($Response.Content)" $StatusCode = $Response.StatusCode }