Skip to content

Fixes: #4597 Parse URI path with an endpoint #9728

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

Merged
merged 3 commits into from
May 3, 2019
Merged

Conversation

rynowak
Copy link
Member

@rynowak rynowak commented Apr 24, 2019

Adds functionality to LinkParser to parse a URI path given a way to
find an endpoint. This is the replacement for various machinications
using the global route collection and RouteData.Routers in earlier
versions.

For now I'm just adding a way to do this using Endpoint Name since it's
a pretty low level feature. Endpoint Name is also very direct, so it
feels good for something like this.

Example usage:

[HttpPut]
[Route("/Products/{id}", Name = "ProductsRoute")]
public async Task<IActionResult> AddRelatedProduct(int projectId, string path)
{
   var values = parser.ParsePathByEndpointName("ProductsRoute", path);
   var relatedId = (string)values["id"];

   // Do something with this information
}

@rynowak rynowak requested a review from JamesNK April 24, 2019 22:53
@rynowak
Copy link
Member Author

rynowak commented Apr 24, 2019

@JamesNK looking for your COOL high level feedback on this before adding tests.

@rynowak
Copy link
Member Author

rynowak commented Apr 24, 2019

This is also pretty simple and straightforward compared to what we have today 👍

@rynowak
Copy link
Member Author

rynowak commented Apr 24, 2019

There's a few more decisions here to be made like - does this apply default values? Does this run constraints? I'm planning to reach out to some of the folks that asked for this feature and poll them.

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 24, 2019
@rynowak
Copy link
Member Author

rynowak commented Apr 25, 2019

@oferns @mgolois - I'm looking at adding an API to address your feedback here: #4597

Could you take a look at this and let me know what you think?

Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

Seems alright.

It does feel a little odd to have matching on link generator. Consider whether a new type makes sense. LinkParser?

public override RouteValueDictionary ParsePathByAddress<TAddress>(TAddress address, PathString path)
{
var endpoints = GetEndpoints(address);
if (endpoints.Count == 0)
Copy link

Choose a reason for hiding this comment

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

How/where is GetEndpoints implemented? This was my issue, getting access to the routes/endpoints that used to be in RouteData.Routes

Copy link
Member Author

Choose a reason for hiding this comment

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

Get endpoints is implemented on the base class.

If you want a list of all of the endpoints in the app, you do do that by grabbing EndpointDataSource from DI. If you want the endpoint that matched the current request, use this. https://github.com/aspnet/AspNetCore/blob/425c196cba530b161b120a57af8f1dd513b96f67/src/Http/Http.Abstractions/src/Routing/EndpointHttpContextExtensions.cs

@rynowak rynowak force-pushed the rynowak/parse-url-path branch 2 times, most recently from 3c2c6a2 to 39622b1 Compare May 1, 2019 21:55
@rynowak rynowak marked this pull request as ready for review May 1, 2019 21:55
@rynowak
Copy link
Member Author

rynowak commented May 1, 2019

@JamesNK - ready for review. I decided to go with a separate class.

@rynowak rynowak force-pushed the rynowak/parse-url-path branch from 39622b1 to 61306cf Compare May 2, 2019 15:28
@rynowak
Copy link
Member Author

rynowak commented May 2, 2019

@JamesNK updated

Ryan Nowak and others added 2 commits May 2, 2019 14:22
Adds functionality to LinkGenerator to parse a URI path given a way to
find an endpoint. This is the replacement for various machinications
using the global route collection and `RouteData.Routers` in earlier
versions.

For now I'm just adding a way to do this using Endpoint Name since it's
a pretty low level feature. Endpoint Name is also very direct, so it
feels good for something like this.

I added this to LinkGenerator because I think it feels like the right
thing do, despite the naming conflict. I don't really want to create a
new top-level service for this.
@rynowak rynowak force-pushed the rynowak/parse-url-path branch from 61306cf to b55ec1c Compare May 3, 2019 02:16
@rynowak
Copy link
Member Author

rynowak commented May 3, 2019

@JamesNK updated

@rynowak rynowak merged commit 127bc7d into master May 3, 2019
@rynowak rynowak deleted the rynowak/parse-url-path branch May 3, 2019 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants