Skip to content
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.

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

Fix multiple routes of same template in attribute-based routing #2250

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tchowice
Copy link

@tchowice tchowice commented Oct 26, 2024

Changes

ASP.NET instrumentation fails to extract route for attribute-based routing, if multiple HTTP methods use the same route template--a common scenario for RESTful APIs. For example:

public class WidgetsController : ApiController
{
    [Route("api/widgets/{id}")]
    public string Get(int id) {...}

    [Route("api/widgets/{id}")]
    public void Put(int id, [FromBody]string value) {...}

    [Route("api/widgets/{id}")]
    public void Delete(int id) {...}
}

In this case, the MS_Subroutes array will have multiple elements, each corresponding to a different HTTP method.

The original code only extracts the route template if there's only one element in the array. The correct thing to do is to just extract the template from the first subroute, since all subroutes have the same template.

It's not possible for MS_Subroutes array to have different templates in its elements. WebAPI would fail to route and throw an exception, if you declare multiple actions for the same method/template

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@tchowice tchowice requested a review from a team as a code owner October 26, 2024 01:58
Copy link

linux-foundation-easycla bot commented Oct 26, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added the comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet label Oct 26, 2024
@Kielek
Copy link
Contributor

Kielek commented Oct 28, 2024

Please sign EasyCLA. It is hard requirement to accept any contribution to this repository.

@tchowice
Copy link
Author

Please sign EasyCLA. It is hard requirement to accept any contribution to this repository.

Thanks. Working with my company to figure this out.

@CodeBlanch CodeBlanch marked this pull request as draft November 1, 2024 16:27
@tchowice
Copy link
Author

tchowice commented Nov 6, 2024

Please sign EasyCLA. It is hard requirement to accept any contribution to this repository.

Thanks. Working with my company to figure this out.

@tchowice tchowice closed this Nov 6, 2024
@tchowice
Copy link
Author

tchowice commented Nov 6, 2024

Please sign EasyCLA. It is hard requirement to accept any contribution to this repository.

Okay, I finally got my company to sign the CLA. Please proceed with reviewing!

@Kielek Kielek reopened this Nov 6, 2024
@Kielek Kielek marked this pull request as ready for review November 6, 2024 17:19
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 14, 2024
@Kielek Kielek removed the Stale label Nov 14, 2024
Copy link
Contributor

@ysolomchenko ysolomchenko left a comment

Choose a reason for hiding this comment

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

Add an entry to the Changelog

Copy link
Contributor

@ysolomchenko ysolomchenko left a comment

Choose a reason for hiding this comment

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

Update the PR title to start with [Instrumentation.AspNet]

Comment on lines +45 to +111
routeData = new RouteData();
var multiMethodSubroutes = new[]
{
new
{
Route = new
{
RouteTemplate = routeTemplate,
DataTokens = new Dictionary<string, object>
{
["actions"] = new[]
{
new
{
SupportedHttpMethods = new[]
{
HttpMethod.Get,
},
},
},
},
},
},
new
{
Route = new
{
RouteTemplate = routeTemplate,
DataTokens = new Dictionary<string, object>
{
["actions"] = new[]
{
new
{
SupportedHttpMethods = new[]
{
HttpMethod.Put,
},
},
},
},
},
},
new
{
Route = new
{
RouteTemplate = routeTemplate,
DataTokens = new Dictionary<string, object>
{
["actions"] = new[]
{
new
{
SupportedHttpMethods = new[]
{
HttpMethod.Delete,
},
},
},
},
},
},
};
routeData.Values.Add(
"MS_SubRoutes",
multiMethodSubroutes);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can simplify to:

    routeData = new RouteData();
    var httpMethods = new[] { HttpMethod.Get, HttpMethod.Put, HttpMethod.Delete };

    var multiMethodSubroutes = httpMethods.Select(method => new
    {
        Route = new
        {
            RouteTemplate = routeTemplate,
            DataTokens = new Dictionary<string, object>
            {
                ["actions"] = new[]
                {
                    new
                    {
                        SupportedHttpMethods = new[] { method }
                    }
                }
            }
        }
    }).ToArray();

    routeData.Values.Add("MS_SubRoutes", multiMethodSubroutes);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants