-
Notifications
You must be signed in to change notification settings - Fork 761
Fix duplicate key exception when using WithExplicitStart() on resources with environment callbacks #12604
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.
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
Conversation
…enarios Co-authored-by: davidfowl <[email protected]>
|
@copilot We need to add unit tests that evaluate these callbacks using GetEnvironmentVariableValuesAsync multiple times to make sure we are idempotent for these resources. |
Co-authored-by: davidfowl <[email protected]>
Added idempotency tests for all resources with environment callbacks. Each test calls Tests added in commit a6209f3:
All tests pass ✅ |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12604Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12604" |
There was a problem hiding this 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 environment callback idempotency issues where calling GetEnvironmentVariableValuesAsync() multiple times would fail due to duplicate key additions. The fix changes environment variable setting from .Add() to indexer assignment [] in multiple hosting extension methods, making callbacks idempotent.
- Changes environment variable assignments from
.Add()to[]indexer syntax across multiple hosting extensions (Redis, PostgreSQL, MySQL, MongoDB, Kafka, Milvus, Azure Service Bus, Azure Event Hubs) - Adds comprehensive test coverage verifying idempotent behavior when
GetEnvironmentVariableValuesAsync()is called multiple times
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs | Changes RedisInsight environment variable assignments to use indexer syntax |
| src/Aspire.Hosting.PostgreSQL/PostgresBuilderExtensions.cs | Changes PgAdmin environment variable assignments to use indexer syntax |
| src/Aspire.Hosting.MySql/MySqlBuilderExtensions.cs | Changes PhpMyAdmin environment variable assignments to use indexer syntax |
| src/Aspire.Hosting.MongoDB/MongoDBBuilderExtensions.cs | Changes MongoExpress environment variable assignments to use indexer syntax |
| src/Aspire.Hosting.Milvus/MilvusBuilderExtensions.cs | Changes Attu environment variable assignments to use indexer syntax |
| src/Aspire.Hosting.Kafka/KafkaBuilderExtensions.cs | Changes Kafka and KafkaUI environment variable assignments to use indexer syntax |
| src/Aspire.Hosting.Azure.ServiceBus/AzureServiceBusExtensions.cs | Changes Service Bus emulator environment variable assignments to use indexer syntax |
| src/Aspire.Hosting.Azure.EventHubs/AzureEventHubsExtensions.cs | Changes Event Hubs emulator environment variable assignments to use indexer syntax |
| tests/Aspire.Hosting.Redis.Tests/AddRedisTests.cs | Adds test verifying RedisInsight environment callbacks are idempotent |
| tests/Aspire.Hosting.PostgreSQL.Tests/AddPostgresTests.cs | Adds test verifying PostgreSQL environment callbacks are idempotent |
| tests/Aspire.Hosting.MySql.Tests/AddMySqlTests.cs | Adds test verifying PhpMyAdmin environment callbacks are idempotent |
| tests/Aspire.Hosting.MongoDB.Tests/AddMongoDBTests.cs | Adds test verifying MongoExpress environment callbacks are idempotent |
| tests/Aspire.Hosting.Kafka.Tests/AddKafkaTests.cs | Adds tests verifying Kafka and KafkaUI environment callbacks are idempotent |
|
/backport to release/13.0 |
|
Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/19051646441 |
Environment callbacks in hosting builder extensions were using
Dictionary.Add(), which throws when invoked multiple times with the same key. Resources configured withWithExplicitStart()trigger these callbacks repeatedly during their lifecycle, causing crashes.Example failure:
Changes:
context.EnvironmentVariables.Add(key, value)with indexer syntaxcontext.EnvironmentVariables[key] = valueto allow idempotent callback executionGetEnvironmentVariableValuesAsynccan be called multiple times without errors:KafkaEnvironmentCallbackIsIdempotentKafkaUIEnvironmentCallbackIsIdempotentMongoExpressEnvironmentCallbackIsIdempotentPostgresEnvironmentCallbackIsIdempotentRedisInsightEnvironmentCallbackIsIdempotentPhpMyAdminEnvironmentCallbackIsIdempotentThis aligns with the existing pattern used in
ProjectResourceBuilderExtensionsandDashboardEventHandlers.Fixes #12516
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.