Skip to content

Conversation

@captainsafia
Copy link
Member

@captainsafia captainsafia commented Sep 14, 2021

Background and Motivation

For .NET 6, we added support for OpenAPI extension methods on the DelegateEndpointConventionBuilder used in minimal apps. The DelegateEndpointConventionBuilder is currently implemented as a public sealed class with internal constructors.

While debugging a scenario, we realized that this posed a problem for scenarios where developers wanted to use the extension methods on a builder returned from a 3rd-party API or external source.

// Some library code
public static DelegateEndpointConventionBuilder MapMyFrameworkMethods()
{
...
}

// Some user code that is not possible
app.MapMyFrameworkMethods().ExcludeFromDescription();

Proposed API

namespace Microsoft.AspNetCore.Builder
{
  public class DelegateEndpointConventionBuilder
  {
-    internal DelegateEndpointConventionBuilder(List<IEndpointConventionBuilder> endpointConventionBuilders)
+    public DelegateEndpointConventionBuilder(List<IEndpointConventionBuilder> endpointConventionBuilders)
}

Usage Examples

public static DelegateEndpointConventionBuilder MapToDos(this IEndpointRouteBuilder endpointRouteBuilder)
{
  List<IEndpointConventionBuilder> builders = new();
  builders.Add(endpointRouteBuilder.MapPost("/create", () => { ... });
  builders.Add(endpointRouteBuilder.MapGet("/all", () => { ... });
  return new DelegateEndpointConventionBuilder(builders);
}

app.MapToDos().ExcludeFromDescription();

Alternative Designs

One alternative is to have the extension methods target IEndpointConventionBuilder instead of DelegateEndpointConventionBuilder but this presents some problems because:

  • The compiler will fail to resolve generic types correctly for extension methods that implement more than one generic Produces<TResponse>(TBuilder)
  • We want to keep the high level of specificity for what builders this extension method targets

Risks

  • The DelegateEndpointConventionBuilder was introduced in .NET 6 so this is not a breaking change.
  • Unlike other IEndpointConventionBuilders the DelegateEndpointConventionBuilder has a requirement for this type of extensibility so it's the only one that follows the pattern of public class and public constructor.

@ghost ghost added the area-runtime label Sep 14, 2021
@captainsafia captainsafia marked this pull request as ready for review September 14, 2021 16:23
@captainsafia captainsafia added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Sep 14, 2021
@ghost
Copy link

ghost commented Sep 14, 2021

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

@ghost ghost removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Sep 14, 2021
@captainsafia
Copy link
Member Author

captainsafia commented Sep 14, 2021

@dotnet/aspnet-build Can I get help merging?

The Linux helix run has passed for this but the other legs are stuck: https://helix.dot.net/api/jobs/cfb0e973-8053-41c1-a5be-8e37deee1a86/workitems?api-version=2019-06-17

Edit: Actually looks like all the Helix legs passed successfully

@dougbu
Copy link
Contributor

dougbu commented Sep 14, 2021

Do not merge this as-is

@dougbu
Copy link
Contributor

dougbu commented Sep 14, 2021

See

Detected modification to baseline API files. PublicAPI.Shipped.txt files should only be updated

The Shipped files should never change except immediately after an RTM release e.g. 6.0.0. In between, change PublicAPI.Unshipped.txt files. Add *Removed* lines if necessary to effectively override something that has shipped but carefully.

*REMOVED*Microsoft.AspNetCore.Routing.RouteNameMetadata.RouteName.get -> string!
*REMOVED*Microsoft.AspNetCore.Routing.RouteNameMetadata.RouteNameMetadata(string! routeName) -> void
Microsoft.AspNetCore.Builder.DelegateEndpointConventionBuilder
Microsoft.AspNetCore.Builder.DelegateEndpointConventionBuilder.DelegateEndpointConventionBuilder(System.Collections.Generic.IEnumerable<Microsoft.AspNetCore.Builder.IEndpointConventionBuilder!>! endpointConventionBuilders) -> void
Copy link
Contributor

Choose a reason for hiding this comment

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

Put another way: Unless this was in 5.0.0, the method hasn't shipped.

@dougbu dougbu merged commit db2979b into release/6.0 Sep 14, 2021
@dougbu dougbu deleted the safia/int-to-public branch September 14, 2021 23:43
@ghost ghost added this to the 6.0-rc2 milestone Sep 14, 2021
captainsafia added a commit that referenced this pull request Sep 15, 2021
* Make DelegateEndpointConventionBuilder constructors public

* Address API review feedback

* Only expose a single constructor
captainsafia added a commit that referenced this pull request Sep 15, 2021
…36549)

* Make DelegateEndpointConventionBuilder constructors public (#36492)

* Make DelegateEndpointConventionBuilder constructors public

* Address API review feedback

* Only expose a single constructor

* Update src/Http/Routing/src/Builder/DelegateEndpointConventionBuilder.cs

Co-authored-by: Pranav K <[email protected]>

Co-authored-by: Pranav K <[email protected]>
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants