Skip to content

Conversation

@bart-vmware
Copy link
Contributor

@bart-vmware bart-vmware commented Oct 22, 2025

Closes #dotnet/aspire#4224.

Microsoft Reviewers: Open in CodeFlow

@bart-vmware bart-vmware force-pushed the service-discovery-apis branch from bf2d2f6 to 8ce3522 Compare October 22, 2025 12:25
@bart-vmware bart-vmware marked this pull request as ready for review October 22, 2025 14:44
@Copilot Copilot AI review requested due to automatic review settings October 22, 2025 14:44
Copy link
Contributor

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 exposes building blocks for external service discovery implementations by making previously internal APIs public. The changes enable external developers to implement custom service discovery providers while maintaining consistency with internal patterns.

Key changes:

  • Made UriEndPoint class public with proper constructor and null validation
  • Exposed ApplyAllowedSchemes as a public instance method on ServiceDiscoveryOptions
  • Added ServiceEndpoint.TryParse static method to provide a standard way to parse endpoint strings

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
ServiceDiscoveryOptions.cs Changed ApplyAllowedSchemes from internal static to public instance method with XML documentation
ConfigurationServiceEndpointProvider.cs Updated to use new public ApplyAllowedSchemes method and consolidated endpoint parsing logic
UriEndPoint.cs Changed from internal sealed to public class with public constructor and null validation
ServiceEndpoint.cs Added public TryParse method to centralize endpoint string parsing logic

Comment on lines +68 to +70
#pragma warning disable CS8602
if (value.IndexOf("://", StringComparison.Ordinal) < 0 && Uri.TryCreate($"fakescheme://{value}", default, out var uri))
#pragma warning restore CS8602
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

The pragma warning disable for CS8602 is unnecessary here. The preceding check !string.IsNullOrWhiteSpace(value) on line 66 ensures that value is not null at this point, so the null-forgiving operator or null check is not needed. Remove the pragma directives.

Suggested change
#pragma warning disable CS8602
if (value.IndexOf("://", StringComparison.Ordinal) < 0 && Uri.TryCreate($"fakescheme://{value}", default, out var uri))
#pragma warning restore CS8602
if (value.IndexOf("://", StringComparison.Ordinal) < 0 && Uri.TryCreate($"fakescheme://{value}", default, out var uri))

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't true. The build fails without it. That's because string.IsNullOrWhiteSpace is not annotated for nullability in netstandard and netfx.

_includeAllSchemes = serviceDiscoveryOptions.Value.AllowAllSchemes && query.IncludedSchemes.Count == 0;
_schemes = ServiceDiscoveryOptions.ApplyAllowedSchemes(query.IncludedSchemes, serviceDiscoveryOptions.Value.AllowedSchemes, serviceDiscoveryOptions.Value.AllowAllSchemes);
var allowedSchemes = serviceDiscoveryOptions.Value.ApplyAllowedSchemes(query.IncludedSchemes);
_schemes = allowedSchemes as string[] ?? allowedSchemes.ToArray();
Copy link

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

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

This code performs a type check and potentially creates a new array on every instantiation. Since ApplyAllowedSchemes now returns IReadOnlyList<string> and can return different concrete types (ReadOnlyCollection<string> or List<T>.AsReadOnly()), consider storing allowedSchemes directly as IReadOnlyList<string> if _schemes field type can be changed, or call .ToArray() unconditionally to avoid the type check overhead.

Suggested change
_schemes = allowedSchemes as string[] ?? allowedSchemes.ToArray();
_schemes = allowedSchemes.ToArray();

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The updated code employs the same optimization to avoid allocating when the runtime type is already an array. The logic has just moved.

@bart-vmware
Copy link
Contributor Author

@ReubenBond, can you please review this PR?

@bart-vmware bart-vmware force-pushed the service-discovery-apis branch from a5b1712 to 2fe85ad Compare October 23, 2025 08:52
@bart-vmware
Copy link
Contributor Author

@ReubenBond Thanks for the review and approval.
@davidfowl I've rebased on the latest main after approval. Do we need another pair of eyes, or is this ready to merge now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants