Skip to content

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented May 22, 2025

Description

The AzureBlobStorageContainerSettings specialized class is preventing the distinction between ConnectionString and ServiceUri from happening. In the case of Azure deployment it would then assign a service uri to the ConnectionString property and the wrong client constructor was invoked.

Fixes #9454

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@Copilot Copilot AI review requested due to automatic review settings May 22, 2025 19:45
@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label May 22, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes how AzureBlobStorageContainerSettings distinguishes between a service URI and an emulator connection string when parsing, ensuring the correct client constructor is used.

  • Introduce ParseConnectionStringInternal to centralize parsing logic.
  • Update AzureBlobStorageContainerSettings to call the internal parser on the endpoint value.
  • Add unit tests covering service URI, emulator connection string, and invalid URI scenarios.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/Aspire.Azure.Storage.Blobs.Tests/AzureBlobStorageContainerSettingsTests.cs Added tests for parsing service URI, emulator connection string, and non-URI endpoints.
src/Components/Aspire.Azure.Storage.Blobs/AzureStorageBlobsSettings.cs Exposed ParseConnectionStringInternal and hooked up the interface implementation.
src/Components/Aspire.Azure.Storage.Blobs/AzureBlobStorageContainerSettings.cs Changed logic to call ParseConnectionStringInternal instead of assigning directly to ConnectionString.
playground/AzureStorageEndToEnd/AzureStorageEndToEnd.AppHost/Program.cs Commented-out RunAsEmulator calls in example host to reflect new defaults.
Comments suppressed due to low confidence (2)

src/Components/Aspire.Azure.Storage.Blobs/AzureBlobStorageContainerSettings.cs:29

  • [nitpick] The constant 'ContainerName' is shadowing the property name and can be confusing; consider renaming it to 'ContainerNameKey' or similar to clarify its purpose.
const string ContainerName = nameof(ContainerName);

tests/Aspire.Azure.Storage.Blobs.Tests/AzureBlobStorageContainerSettingsTests.cs:49

  • Add an assertion that settings.ConnectionString is null when a service URI is provided to ensure only ServiceUri is set and ConnectionString remains unset.
public void ParseConnectionString_With_ServiceUri(string connectionString)

@sebastienros sebastienros requested review from davidfowl and removed request for DamianEdwards May 22, 2025 19:46
@sebastienros sebastienros requested a review from eerhardt May 23, 2025 21:05
else
{
builder.Append($"Endpoint={ConnectionStringExpression}");
}

if (!string.IsNullOrEmpty(blobContainerName))
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed cuz it is already checked above.

@@ -231,10 +231,11 @@ public async Task AddBlobContainer_ConnectionString_resolved_expected_RunAsEmula
var blobs = storage.AddBlobs("blob");
Copy link
Member

Choose a reason for hiding this comment

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

Can we also update this test to verify blob containers work end-to-end?

[Fact]
[RequiresDocker]
public async Task VerifyAzureStorageEmulatorResource()
{
using var builder = TestDistributedApplicationBuilder.Create().WithTestAndResourceLogging(testOutputHelper);
var storage = builder.AddAzureStorage("storage").RunAsEmulator().AddBlobs("BlobConnection");
using var app = builder.Build();
await app.StartAsync();
var hb = Host.CreateApplicationBuilder();
hb.Configuration["ConnectionStrings:BlobConnection"] = await storage.Resource.ConnectionStringExpression.GetValueAsync(CancellationToken.None);
hb.AddAzureBlobClient("BlobConnection");
using var host = hb.Build();
await host.StartAsync();
var serviceClient = host.Services.GetRequiredService<BlobServiceClient>();
var blobContainer = (await serviceClient.CreateBlobContainerAsync("container")).Value;
var blobClient = blobContainer.GetBlobClient("testKey");
await blobClient.UploadAsync(BinaryData.FromString("testValue"));
var downloadResult = (await blobClient.DownloadContentAsync()).Value;
Assert.Equal("testValue", downloadResult.Content.ToString());
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand, update AddBlobContainer_ConnectionString_resolved_expected_RunAsEmulator or VerifyAzureStorageEmulatorResource. The first one is a unit test, the second is already testing the container client with the emulator.

Copy link
Member

Choose a reason for hiding this comment

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

Update the VerifyAzureStorageEmulatorResource functional e2e test to also ensure that Hosting builder.AddBlobContainer("container"); and Client side builder.AddAzureBlobContainerClient("container"); work together end-to-end.

@sebastienros sebastienros merged commit 2ba6e8a into main May 23, 2025
254 checks passed
@sebastienros sebastienros deleted the sebros/blobcontainer branch May 23, 2025 23:19
@sebastienros
Copy link
Member Author

/backport to release/9.3

Copy link
Contributor

Started backporting to release/9.3: https://github.com/dotnet/aspire/actions/runs/15220833760

@RussKie RussKie mentioned this pull request May 26, 2025
16 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Jun 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blob Container Connection String Format Exception
3 participants