Address feedback to WithUrls() (#8602)

* Address WithUrls feedback

Fixes #8587

* Add more WithUrl tests

* More tests

* Fix test failure

* Fix test

* PR feedback

Author: DamianEdwards

src/Aspire.Hosting/Dcp/ResourceSnapshotBuilder.cs

@@ -20,7 +20,7 @@ public ResourceSnapshotBuilder(DcpResourceState resourceState)
     public CustomResourceSnapshot ToSnapshot(Container container, CustomResourceSnapshot previous)
     {
         var containerId = container.Status?.ContainerId;
-        var urls = GetUrls(container);
+        var urls = GetUrls(container, container.Status?.State);
         var volumes = GetVolumes(container);
 
         var environment = GetEnvironmentVariables(container.Status?.EffectiveEnv ?? container.Spec.Env, container.Spec.Env);
@@ -99,7 +99,7 @@ public CustomResourceSnapshot ToSnapshot(Executable executable, CustomResourceSn
 
         var state = executable.AppModelInitialState is "Hidden" ? "Hidden" : executable.Status?.State;
 
-        var urls = GetUrls(executable);
+        var urls = GetUrls(executable, executable.Status?.State);
 
         var environment = GetEnvironmentVariables(executable.Status?.EffectiveEnv, executable.Spec.Env);
 
@@ -183,7 +183,7 @@ private static (ImmutableArray<string> Args, ImmutableArray<int>? ArgsAreSensiti
         return (launchArgsBuilder.ToImmutable(), argsAreSensitiveBuilder.ToImmutable(), anySensitive);
     }
 
-    private ImmutableArray<UrlSnapshot> GetUrls(CustomResource resource)
+    private ImmutableArray<UrlSnapshot> GetUrls(CustomResource resource, string? resourceState)
     {
         var urls = ImmutableArray.CreateBuilder<UrlSnapshot>();
         var appModelResourceName = resource.AppModelResourceName;
@@ -199,21 +199,26 @@ private ImmutableArray<UrlSnapshot> GetUrls(CustomResource resource)
             var name = resource.Metadata.Name;
 
             // Add the endpoint URLs
-            foreach (var service in resourceServices)
+            var serviceEndpoints = new HashSet<(string EndpointName, string ServiceMetadataName)>(resourceServices.Where(s => !string.IsNullOrEmpty(s.EndpointName)).Select(s => (s.EndpointName!, s.Metadata.Name)));
+            foreach (var endpoint in serviceEndpoints)
             {
-                if (endpointUrls.FirstOrDefault(u => string.Equals(service.EndpointName, u.Endpoint?.EndpointName, StringComparisons.EndpointAnnotationName)) is { Endpoint: { } } endpointUrl)
+                var (endpointName, serviceName) = endpoint;
+                var urlsForEndpoint = endpointUrls.Where(u => string.Equals(endpointName, u.Endpoint?.EndpointName, StringComparisons.EndpointAnnotationName)).ToList();
+
+                foreach (var endpointUrl in urlsForEndpoint)
                 {
-                    var activeEndpoint = _resourceState.EndpointsMap.SingleOrDefault(e => e.Value.Spec.ServiceName == service.Metadata.Name && e.Value.Metadata.OwnerReferences?.Any(or => or.Kind == resource.Kind && or.Name == name) == true).Value;
+                    var activeEndpoint = _resourceState.EndpointsMap.SingleOrDefault(e => e.Value.Spec.ServiceName == serviceName && e.Value.Metadata.OwnerReferences?.Any(or => or.Kind == resource.Kind && or.Name == name) == true).Value;
                     var isInactive = activeEndpoint is null;
 
-                    urls.Add(new(Name: endpointUrl.Endpoint.EndpointName, Url: endpointUrl.Url, IsInternal: false) { IsInactive = isInactive, DisplayProperties = new(endpointUrl.DisplayText ?? "", endpointUrl.DisplayOrder ?? 0) });
+                    urls.Add(new(Name: endpointUrl.Endpoint!.EndpointName, Url: endpointUrl.Url, IsInternal: false) { IsInactive = isInactive, DisplayProperties = new(endpointUrl.DisplayText ?? "", endpointUrl.DisplayOrder ?? 0) });
                 }
             }
 
             // Add the non-endpoint URLs
+            var resourceRunning = string.Equals(resourceState, KnownResourceStates.Running, StringComparisons.ResourceState);
             foreach (var url in nonEndpointUrls)
             {
-                urls.Add(new(Name: null, Url: url.Url, IsInternal: false) { IsInactive = false, DisplayProperties = new(url.DisplayText ?? "", url.DisplayOrder ?? 0) });
+                urls.Add(new(Name: null, Url: url.Url, IsInternal: false) { IsInactive = !resourceRunning, DisplayProperties = new(url.DisplayText ?? "", url.DisplayOrder ?? 0) });
             }
         }
 

src/Aspire.Hosting/Orchestrator/ApplicationOrchestrator.cs

@@ -97,12 +97,13 @@ private async Task OnEndpointsAllocated(OnEndpointsAllocatedContext context)
         {
             await lifecycleHook.AfterEndpointsAllocatedAsync(_model, context.CancellationToken).ConfigureAwait(false);
         }
-
-        await ProcessUrls(context.CancellationToken).ConfigureAwait(false);
     }
 
     private async Task OnResourceStarting(OnResourceStartingContext context)
     {
+        // Call the callbacks to configure resource URLs
+        await ProcessUrls(context.Resource, context.CancellationToken).ConfigureAwait(false);
+
         switch (context.ResourceType)
         {
             case KnownResourceTypes.Project:
@@ -152,44 +153,58 @@ private async Task OnResourcesPrepared(OnResourcesPreparedContext _)
         await PublishResourcesWithInitialStateAsync().ConfigureAwait(false);
     }
 
-    private async Task ProcessUrls(CancellationToken cancellationToken)
+    private async Task ProcessUrls(IResource resource, CancellationToken cancellationToken)
     {
-        // Project endpoints to URLS
-        foreach (var resource in _model.Resources.OfType<IResourceWithEndpoints>())
+        if (resource is not IResourceWithEndpoints resourceWithEndpoints)
         {
-            var urls = new List<ResourceUrlAnnotation>();
+            return;
+        }
 
-            if (resource.TryGetEndpoints(out var endpoints))
+        // Project endpoints to URLS
+        var urls = new List<ResourceUrlAnnotation>();
+
+        if (resource.TryGetEndpoints(out var endpoints))
+        {
+            foreach (var endpoint in endpoints)
             {
-                foreach (var endpoint in endpoints)
+                // Create a URL for each endpoint
+                if (endpoint.AllocatedEndpoint is { } allocatedEndpoint)
                 {
-                    // Create a URL for each endpoint
-                    if (endpoint.AllocatedEndpoint is { } allocatedEndpoint)
-                    {
-                        var url = new ResourceUrlAnnotation { Url = allocatedEndpoint.UriString, Endpoint = new EndpointReference(resource, endpoint) };
-                        urls.Add(url);
-                    }
+                    var url = new ResourceUrlAnnotation { Url = allocatedEndpoint.UriString, Endpoint = new EndpointReference(resourceWithEndpoints, endpoint) };
+                    urls.Add(url);
                 }
             }
+        }
 
-            // Run the URL callbacks
-            if (resource.TryGetAnnotationsOfType<ResourceUrlsCallbackAnnotation>(out var callbacks))
+        // Run the URL callbacks
+        if (resource.TryGetAnnotationsOfType<ResourceUrlsCallbackAnnotation>(out var callbacks))
+        {
+            var urlsCallbackContext = new ResourceUrlsCallbackContext(new(DistributedApplicationOperation.Run), resource, urls, cancellationToken)
             {
-                var urlsCallbackContext = new ResourceUrlsCallbackContext(new(DistributedApplicationOperation.Run), resource, urls, cancellationToken)
-                {
-                    Logger = _loggerService.GetLogger(resource.Name)
-                };
-                foreach (var callback in callbacks)
-                {
-                    await callback.Callback(urlsCallbackContext).ConfigureAwait(false);
-                }
+                Logger = _loggerService.GetLogger(resource.Name)
+            };
+            foreach (var callback in callbacks)
+            {
+                await callback.Callback(urlsCallbackContext).ConfigureAwait(false);
             }
+        }
 
-            foreach (var url in urls)
+        // Clear existing URLs
+        if (resource.TryGetUrls(out var existingUrls))
+        {
+            var existing = existingUrls.ToArray();
+            for (var i = existing.Length - 1; i >= 0; i--)
             {
-                resource.Annotations.Add(url);
+                var url = existing[i];
+                resource.Annotations.Remove(url);
             }
         }
+
+        // Add URLs
+        foreach (var url in urls)
+        {
+            resource.Annotations.Add(url);
+        }
     }
 
     private Task ProcessResourcesWithoutLifetime(AfterEndpointsAllocatedEvent @event, CancellationToken cancellationToken)

src/Aspire.Hosting/ResourceBuilderExtensions.cs

@@ -799,7 +799,58 @@ public static IResourceBuilder<T> WithUrl<T>(this IResourceBuilder<T> builder, s
     }
 
     /// <summary>
-    /// Registers a callback to customize the URL displayed for the endpoint with the specified name.
+    /// Adds a URL to be displayed for the resource.
+    /// </summary>
+    /// <typeparam name="T">The resource type.</typeparam>
+    /// <param name="builder">The builder for the resource.</param>
+    /// <param name="url">The interpolated string that produces the URL.</param>
+    /// <param name="displayText">The display text to show when the link is displayed.</param>
+    /// <returns>The <see cref="IResourceBuilder{T}"/>.</returns>
+    /// <remarks>
+    /// Use this method to add a URL to be displayed for the resource.<br/>
+    /// Note that any endpoints on the resource will automatically get a corresponding URL added for them.
+    /// </remarks>
+    public static IResourceBuilder<T> WithUrl<T>(this IResourceBuilder<T> builder, in ReferenceExpression.ExpressionInterpolatedStringHandler url, string? displayText = null)
+        where T : IResource
+    {
+        ArgumentNullException.ThrowIfNull(builder);
+
+        var expression = url.GetExpression();
+
+        return builder.WithUrl(expression, displayText);
+    }
+
+    /// <summary>
+    /// Adds a URL to be displayed for the resource.
+    /// </summary>
+    /// <typeparam name="T">The resource type.</typeparam>
+    /// <param name="builder">The builder for the resource.</param>
+    /// <param name="url">A <see cref="ReferenceExpression"/> that will produce the URL.</param>
+    /// <param name="displayText">The display text to show when the link is displayed.</param>
+    /// <returns>The <see cref="IResourceBuilder{T}"/>.</returns>
+    /// <remarks>
+    /// Use this method to add a URL to be displayed for the resource.<br/>
+    /// Note that any endpoints on the resource will automatically get a corresponding URL added for them.
+    /// </remarks>
+    public static IResourceBuilder<T> WithUrl<T>(this IResourceBuilder<T> builder, ReferenceExpression url, string? displayText = null)
+        where T : IResource
+    {
+        ArgumentNullException.ThrowIfNull(builder);
+        ArgumentNullException.ThrowIfNull(url);
+
+        return builder.WithAnnotation(new ResourceUrlsCallbackAnnotation(async c =>
+        {
+            var endpoint = url.ValueProviders.OfType<EndpointReference>().FirstOrDefault();
+            var urlValue = await url.GetValueAsync(c.CancellationToken).ConfigureAwait(false);
+            if (!string.IsNullOrEmpty(urlValue))
+            {
+                c.Urls.Add(new() { Endpoint = endpoint, Url = urlValue, DisplayText = displayText });
+            }
+        }));
+    }
+
+    /// <summary>
+    /// Registers a callback to update the URL displayed for the endpoint with the specified name.
     /// </summary>
     /// <typeparam name="T">The resource type.</typeparam>
     /// <param name="builder">The builder for the resource.</param>

tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs

@@ -177,26 +177,26 @@ public async Task ExplicitStart_StartExecutable()
         var notStartedResourceEvent = await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.NotStarted).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
         var dependentResourceEvent = await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Waiting).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
 
-        // Inactive URLs and source should be populated on non-started resources.
+        // Source should be populated on non-started resources.
         Assert.Contains("TestProject.ServiceA.csproj", notStartedResourceEvent.Snapshot.Properties.Single(p => p.Name == "project.path").Value?.ToString());
-        Assert.Collection(notStartedResourceEvent.Snapshot.Urls, u =>
-        {
-            Assert.Equal("http://localhost:5156", u.Url);
-            Assert.True(u.IsInactive);
-        });
         Assert.Contains("TestProject.ServiceB.csproj", dependentResourceEvent.Snapshot.Properties.Single(p => p.Name == "project.path").Value?.ToString());
-        Assert.Collection(dependentResourceEvent.Snapshot.Urls, u =>
-        {
-            Assert.Equal("http://localhost:5254", u.Url);
-            Assert.True(u.IsInactive);
-        });
 
         logger.LogInformation("Start explicit start resource.");
         await orchestrator.StartResourceAsync(notStartedResourceEvent.ResourceId, CancellationToken.None).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
-        await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
+        var runningResourceEvent = await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
+        Assert.Collection(runningResourceEvent.Snapshot.Urls, u =>
+        {
+            Assert.Equal("http://localhost:5156", u.Url);
+            Assert.Equal("http", u.Name);
+        });
 
         // Dependent resource should now run.
-        await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
+        var dependentResourceRunningEvent = await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
+        Assert.Collection(dependentResourceRunningEvent.Snapshot.Urls, u =>
+        {
+            Assert.Equal("http://localhost:5254", u.Url);
+            Assert.Equal("http", u.Name);
+        });
 
         logger.LogInformation("Stop resource.");
         await orchestrator.StopResourceAsync(notStartedResourceEvent.ResourceId, CancellationToken.None).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
@@ -239,27 +239,27 @@ public async Task ExplicitStart_StartContainer()
         var notStartedResourceEvent = await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.NotStarted).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
         var dependentResourceEvent = await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Waiting).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
 
-        // Inactive URLs and source should be populated on non-started resources.
+        // Source should be populated on non-started resources.
         Assert.Equal(RedisImageSource, notStartedResourceEvent.Snapshot.Properties.Single(p => p.Name == "container.image").Value?.ToString());
-        Assert.Collection(notStartedResourceEvent.Snapshot.Urls, u =>
+        Assert.Contains("TestProject.ServiceB.csproj", dependentResourceEvent.Snapshot.Properties.Single(p => p.Name == "project.path").Value?.ToString());
+
+        logger.LogInformation("Start explicit start resource.");
+        await orchestrator.StartResourceAsync(notStartedResourceEvent.ResourceId, CancellationToken.None).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
+        var runningResourceEvent = await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
+        Assert.Collection(runningResourceEvent.Snapshot.Urls, u =>
         {
             Assert.Equal("tcp://localhost:6379", u.Url);
             Assert.True(u.IsInactive);
         });
-        Assert.Contains("TestProject.ServiceB.csproj", dependentResourceEvent.Snapshot.Properties.Single(p => p.Name == "project.path").Value?.ToString());
-        Assert.Collection(dependentResourceEvent.Snapshot.Urls, u =>
+
+        // Dependent resource should now run.
+        var dependentRunningResourceEvent = await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
+        Assert.Collection(dependentRunningResourceEvent.Snapshot.Urls, u =>
         {
             Assert.Equal("http://localhost:5254", u.Url);
-            Assert.True(u.IsInactive);
+            Assert.Equal("http", u.Name);
         });
 
-        logger.LogInformation("Start explicit start resource.");
-        await orchestrator.StartResourceAsync(notStartedResourceEvent.ResourceId, CancellationToken.None).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
-        await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
-
-        // Dependent resource should now run.
-        await rns.WaitForResourceAsync(dependentResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Running).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
-
         logger.LogInformation("Stop resource.");
         await orchestrator.StopResourceAsync(notStartedResourceEvent.ResourceId, CancellationToken.None).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);
         await rns.WaitForResourceAsync(notStartedResourceName, e => e.Snapshot.State?.Text == KnownResourceStates.Exited).DefaultTimeout(TestConstants.LongTimeoutTimeSpan);