Skip to content

Set EndpointName and DisplayName given method name in endpoints #35439

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Aug 19, 2021
Merged

Set EndpointName and DisplayName given method name in endpoints #35439

merged 8 commits into from
Aug 19, 2021

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Aug 18, 2021 •
edited
Loading

Description

Adds support for automatically setting the name of an endpoint given the name of the method used as a delegate. For example:

app.MapGet("/hello", HelloWorld")

will produce HelloWorld as an endpoint name.

The method name is also incorporated into the display name to make it easier for users to debug issues with routes.

Old display name: /hello HTTP: GET
New display name: HTTP: GET /hello => HelloWorld

Customer Impact

The changes in this PR make it easier for users to interact with the OpenAPI improvements that were shipped in RC1. They also make it easier for users to debug issues related to duplicate route names by improving the display name used to represent routes.

Screen Shot 2021-08-17 at 9 22 19 PM

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Changes here are isolated to minimal APIs and are part of new changes that were introduced in rc1.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Fixes #35435 and #34540.

@captainsafia captainsafia force-pushed the safia/mapi-endpoint-name branch from 6eff9d2 to 870b2df Compare August 18, 2021 04:08
@captainsafia captainsafia changed the title Set EndpointName given method name in minimal APIs Set EndpointName and DisplayName given method name in minimal APIs Aug 18, 2021
@captainsafia captainsafia changed the title Set EndpointName and DisplayName given method name in minimal APIs Set EndpointName and DisplayName given method name in endpoints Aug 18, 2021
@captainsafia captainsafia changed the base branch from main to release/6.0-rc1 August 18, 2021 15:33
@captainsafia captainsafia force-pushed the safia/mapi-endpoint-name branch from 870b2df to 8fb48c3 Compare August 18, 2021 15:33
@captainsafia captainsafia added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Aug 18, 2021
@captainsafia captainsafia marked this pull request as ready for review August 18, 2021 16:08
BrennanConroy
BrennanConroy approved these changes Aug 18, 2021
View reviewed changes
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Nice 👍

tmat
tmat reviewed Aug 18, 2021
View reviewed changes
src/Shared/RoslynUtils/TypeHelper.cs Outdated
/// <summary>
/// Checks to see if a given type is compiler generated.
/// <remarks>
/// The compiler doesn't always annotate every time it generates with the
Copy link
Member

Choose a reason for hiding this comment

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

The compiler doesn't always annotate every time it generates with the

If you also check the attribute on containing types you shouldn't need to check for the generated name pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Check the member itself first, then containing type, then its containing type, etc.

cston
cston reviewed Aug 18, 2021
View reviewed changes
src/Http/Routing/src/Builder/MinimalActionEndpointRouteBuilderExtensions.cs Outdated
@@ -184,6 +187,21 @@ public static class MinimalActionEndpointRouteBuilderExtensions
// Add MethodInfo as metadata to assist with OpenAPI generation for the endpoint.
builder.Metadata.Add(action.Method);

// Methods defined in a top-level program are generated as statics so the delegate
// target will be null. Inline lambdas are compiler generated properties so they can
Copy link
Member

Choose a reason for hiding this comment

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

properties

Should this be "compiler generated methods"?

davidfowl
davidfowl approved these changes Aug 18, 2021
View reviewed changes
Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Just nits but this looks good

@captainsafia captainsafia added the ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. label Aug 18, 2021
@captainsafia
Copy link
Member Author

@davidfowl Since you're filling in as M2 while Kevin is out, want to apply the Servicing-Approved label if you're good with merging.

cc: @wtgodbe Need your help merging once it's all clear.

@BrennanConroy
Copy link
Member

Don't forget to forward port to main, it wont be automatic.

@davidfowl davidfowl added the Servicing-approved Shiproom has approved the issue label Aug 19, 2021
halter73
halter73 requested changes Aug 19, 2021
View reviewed changes
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I think we need to check if the method is a local function before excluding it for being compiler generated.

tmat
tmat approved these changes Aug 19, 2021
View reviewed changes
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@BrennanConroy BrennanConroy merged commit 8ab6265 into release/6.0-rc1 Aug 19, 2021
@BrennanConroy BrennanConroy deleted the safia/mapi-endpoint-name branch August 19, 2021 16:25
@ghost ghost added this to the 6.0-rc1 milestone Aug 19, 2021
captainsafia added a commit that referenced this pull request Aug 19, 2021
@captainsafia
Set EndpointName and DisplayName given method name in endpoints (#35439)
1ed966d
captainsafia added a commit that referenced this pull request Aug 20, 2021
@captainsafia
Set EndpointName and DisplayName given method name in endpoints (#35439…
d2421ad 
…) (#35527)
@captainsafia captainsafia mentioned this pull request Sep 14, 2021
Merged
@captainsafia captainsafia mentioned this pull request Oct 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@campersau campersau campersau left review comments

@cston cston cston left review comments

@tmat tmat tmat approved these changes

@halter73 halter73 halter73 approved these changes

@davidfowl davidfowl davidfowl approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@javiercn javiercn Awaiting requested review from javiercn

@Pilchie Pilchie Awaiting requested review from Pilchie

@pranavkm pranavkm Awaiting requested review from pranavkm

Assignees
No one assigned
Labels
ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Servicing-approved Shiproom has approved the issue
Projects
None yet
Milestone
6.0-rc1
Development

Successfully merging this pull request may close these issues.

Improve reporting for duplicate route names in minimal APIs
7 participants
@captainsafia @BrennanConroy @tmat @halter73 @davidfowl @campersau @cston