Add TryGetByName to IResourceCollection#16389
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16389Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16389" |
|
What are your thoughts on TryGetByName vs GetByName. IResource is a reference type so returning null is fine. The other thing to think about is whether a type check should be included, e.g. var dashboardResource = model.Resources.GetByName("aspire-dashboard") as IResourceWithEndpoints;Any strong opinions on using a DIM? If we don't like it then we could have an extension method for |
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
JamesNK
requested review from
danegsta,
davidfowl,
karolz-ms,
mitchdenny and
sebastienros
as code owners
There was a problem hiding this comment.
Pull request overview
This PR introduces a standardized, case-insensitive resource lookup API (TryGetByName) on IResourceCollection to replace repeated linear scans across the hosting codebase, with an internal ResourceCollection implementation optimized via a dictionary for O(1) lookups.
Changes:
- Added
IResourceCollection.TryGetByName(string, out IResource?)as a default interface method (enumerator-based fallback). - Implemented an O(1) override in
ResourceCollectionusing a name-indexed dictionary kept in sync with mutations. - Updated multiple call sites to use
TryGetByNameinstead ofFirstOrDefault/SingleOrDefault+ name comparisons; added unit tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/ApplicationModel/ResourceCollectionTests.cs | Adds unit tests for TryGetByName behavior and dictionary sync across mutations. |
| src/Aspire.Hosting/ApplicationModel/IResourceCollection.cs | Adds the public TryGetByName API as a default interface method fallback. |
| src/Aspire.Hosting/ApplicationModel/ResourceCollection.cs | Adds dictionary-backed lookup and updates mutation paths to keep the index in sync. |
| src/Aspire.Hosting/Exec/ExecResourceManager.cs | Switches exec target lookup to TryGetByName. |
| src/Aspire.Hosting/DistributedApplicationBuilderExtensions.cs | Uses TryGetByName for builder resource lookup helpers. |
| src/Aspire.Hosting/DistributedApplicationBuilder.cs | Uses TryGetByName to detect duplicate resource names during adds. |
| src/Aspire.Hosting/Dcp/OtlpEndpointReferenceGatherer.cs | Uses TryGetByName to locate the dashboard resource. |
| src/Aspire.Hosting/Dashboard/DashboardEventHandlers.cs | Uses TryGetByName for dashboard resource lookup. |
| src/Aspire.Hosting/BuiltInDistributedApplicationEventSubscriptionHandlers.cs | Uses TryGetByName and removes an unused using. |
| src/Aspire.Hosting/Backchannel/DashboardUrlsHelper.cs | Uses TryGetByName when resolving dashboard endpoints. |
| src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs | Uses TryGetByName for resource resolution fallback. |
| src/Aspire.Hosting/Ats/ModelExports.cs | Updates ATS export lookup helper to use TryGetByName. |
| src/Aspire.Hosting.PostgreSQL/PostgresBuilderExtensions.cs | Uses TryGetByName to locate PostgresDatabaseResource instances by name. |
Comments suppressed due to low confidence (1)
src/Aspire.Hosting/Exec/ExecResourceManager.cs:169
_execOptions.ResourceName!can still be null at runtime (e.g., config binding missingExec:ResourceName), andTryGetByName/Dictionary.TryGetValuewill throwArgumentNullException. This changes behavior from the previousEqualscheck (which safely returned false for null) and can result in a less actionable failure. Consider validatingResourceName(null/empty) before lookup and returning the existing “not found” error path with a clear message.
var targetResource = _model.Resources.TryGetByName(_execOptions.ResourceName!, out var resource) ? resource : null;
if (targetResource is null)
{
_logger.LogError("Target resource '{ResourceName}' not found in the model resources.", _execOptions.ResourceName);
throw new InvalidOperationException($"Target resource {_execOptions.ResourceName} not found in the model resources");
I like having TryGetByName ... I think it leads to better coding patterns. If we have GetByName it show throw if that name is not found. |
|
Not for this commit but this work probably lines us up to do something like ResourceAdded/ResourceRemoved events. |
…esourceCollection - Add TryGetByName as a DIM on IResourceCollection with dictionary-backed O(1) override in ResourceCollection - Enforce unique resource names on Add, Insert, indexer set, and constructor in ResourceCollection - Replace LINQ-based resource name lookups with TryGetByName across the codebase - Remove redundant duplicate check from AddResource (now handled by ResourceCollection) - Keep Build() duplicate check as safety net for non-ResourceCollection implementations
JamesNK
force-pushed
the
copilot/add-trygetbyname-method
branch
from
816e65c to
8edc87d
Compare
|
@copilot fix build |
mitchdenny
left a comment
mitchdenny
left a comment
There was a problem hiding this comment.
2 issues found (1 critical build failure, 1 minor diagnostic regression).
| [DoesNotReturn] | ||
| private static void ThrowDuplicateResource(IResource newResource) | ||
| { | ||
| throw new DistributedApplicationException($"Cannot add resource of type '{newResource.GetType()}' with name '{newResource.Name}' because a resource with that name already exists. Resource names are case-insensitive."); |
There was a problem hiding this comment.
Minor diagnostic regression: The old duplicate check in AddResource included both resource types in the error message:
Cannot add resource of type '{new}' with name '...' because resource of type '{existing.GetType()}' with that name already exists.
This new ThrowDuplicateResource helper only mentions the new resource's type, losing the useful info about which existing resource conflicts. Consider accepting an optional IResource? existing parameter so callers that have it (like AddResource) can produce the richer message.
There was a problem hiding this comment.
Fixed in 19d6b79. ThrowDuplicateResource now accepts an optional IResource? existingResource parameter, and all call sites pass _resourcesByName[item.Name] so the error message includes the existing resource's type (e.g. "resource of type 'TestResource'"), matching the original AddResource message.
Agent-Logs-Url: https://github.com/microsoft/aspire/sessions/0e8cee65-e801-4340-896c-5087be5d35c0 Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
Fixed in 28362a9. The Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Agent-Logs-Url: https://github.com/microsoft/aspire/sessions/88102b0f-6ac2-4b14-81e8-20175cbb76b6 Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…re and AzureResourcePreparer
| if (appModel.Resources.TryGetByName("workload", out var existingResource) && existingResource is AksNodePoolResource existingPool) | ||
| { | ||
| return existingPool; | ||
| } |
There was a problem hiding this comment.
@mitchdenny Is this correct? Multiple workload resources were being added so this was needed to stop MultiEnv_ResourcesMatchCorrectEnvironment from failing.
Also, workload is a pretty generic name. Someone could have a resource call workload in their own app and they'll get weird behavior.
There was a problem hiding this comment.
Leave it as workload for now, I'll probably revisit with a different name for the resource vs. the Azure sub-resource name in AKS (which we'll want to align to the common default for AKS deployments)
| if (appModel.Resources.TryGetByName(roleAssignmentResource.Name, out _)) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
@eerhardt Is this correct? Multiple role assignment resources with the same name were being added so this was needed to stop AddFoundry_GeneratesValidBicep from failing.
|
🎬 CLI E2E Test Recordings — 74 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24821426898 |
|
No documentation PR is required for this change. Reason: This PR adds
|
Description
Resource lookup by name was verbose, duplicated across the codebase, and O(n):
Adds
TryGetByNameas a default interface method onIResourceCollectionwith a dictionary-backed O(1) override inResourceCollection:Changes:
IResourceCollection— addedTryGetByName(string, out IResource?)as a DIM with enumerator-based fallback usingStringComparisons.ResourceName, including<remarks>documentationResourceCollection— addedDictionary<string, IResource>(keyed withStringComparers.ResourceName) kept in sync across all mutation methods (Add, Remove, RemoveAt, Clear, Insert, indexer set); overridesTryGetByNamefor O(1) lookup with null-safe handling; enforces unique resource names across all mutation pathsThrowDuplicateResourceincludes both the new and existing resource types in the error message (e.g."Cannot add resource of type 'X' with name '...' because resource of type 'Y' with that name already exists.")SingleOrDefault/FirstOrDefault+ lambda name comparisons withTryGetByNamecalls acrossOtlpConfigurationExtensions,ContainerCreator,DistributedApplicationHostingTestingExtensions,PostgresBuilderExtensions,DistributedApplicationBuilderChecklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: