Fix #18539 - add Blazor catch-all route parameter #24038

zbecknell · 2020-07-16T23:57:50Z

Summary of the changes

Adds a catch-all parameter to the router using https://docs.microsoft.com/en-us/aspnet/core/fundamentals/routing?view=aspnetcore-3.1#route-template-reference as inspiration.

Addresses #18539.

Hopefully this is a useful start, @captainsafia!

javiercn

Overall looks great!

Maybe it would be good to add a couple of E2E tests to validate the encoding bit here

This commit can be used as a reference on what to change to add an E2E test for it

javiercn

Thanks for adding the E2E tests!

This looks great!

I'll merge once the build passes. Thanks again @zbecknell for the contribution!

zbecknell · 2020-07-17T13:24:09Z

Hopefully those will pass @javiercn, I get this message at the end of running tests on my Mac:

Selenium tests are not supported for OS 'Unix' and architecture 'x64'.

javiercn · 2020-07-17T13:26:36Z

@zbecknell Yep, we haven't gotten around to enable selenium cross-plat. The setup is quite painful to do, so we mostly run on Windows/chromium

javiercn · 2020-07-17T14:44:17Z

src/Components/test/E2ETest/Tests/RoutingTest.cs

+        [Fact]
+        public void CanArriveAtPageWithCatchAllParameterWithoutEscapedSlashes()
+        {
+            SetUrlViaPushState("/WithCatchAllParameterWithoutEscapedSlashes/life/the/universe/and/everything = 42");


I think this here might need to be in escaped syntax so that we can see the difference. I think that's why the tests are failing. On this one you should see the unescaped value, on the one above you should see the same value.

Ah, ok, I did notice another call to SetUrlViaPushState had encoded values. I'll get that changed.

zbecknell · 2020-07-17T14:47:52Z

Ok, pretty easy to see what's happening. I guess my first question should've been: do we expect the full value to be encoded? I didn't catch any tests that show this yet, but I may've missed them.

Current behavior:

{*catchAllWithEncodedSlashes} <- encodes slashes but not anything else
{**catchAllWithoutEncodedSlashes} <- doesn't encode anything

What is our desired behavior?

javiercn · 2020-07-17T14:58:36Z

Ok, pretty easy to see what's happening. I guess my first question should've been: do we expect the full value to be encoded? I didn't catch any tests that show this yet, but I may've missed them.

Current behavior:
{*catchAllWithEncodedSlashes} <- encodes slashes but not anything else
{**catchAllWithoutEncodedSlashes} <- doesn't encode anything
What is our desired behavior?

It's not about encoding, it's about decoding. If the url has encoded segments on it, using ** allows you to capture the URL without decoding, which allows you to "round-trip". If you use * the segments get decoded, which means that if you use that for a link, the new link will not be escaped (doesn't roundtrip). Does that make sense?

javiercn · 2020-07-17T15:02:11Z

src/Components/Components/src/Routing/RouteEntry.cs

+                else
+                {
+                    catchAllValue = string.Join('/', context.Segments[Range.StartAt(Template.Segments.Length - 1)]);
+                }


I realized now that I overlooked this. We should be using Uri.UnescapeDataString here. It is likely that the previous behavior for * was to not do anything, which should have not been the case, as that's the behavior ** has on ASP.NET Core routing.

I think I need to think about this a bit more, and chat with some other folks on the team, since this is a small breaking change, and I just want to make sure we are ok with it before we take it.

I think previously * would be completely rejected in a template, right? So for Blazor routing we aren't introducing a breaking change, to my knowledge. I do understand the problem now, I seem to have had it backward in my head, so I can at least start by updating to unescape these segments while you double-check with the team.

That's true, I had a brainfreeze :). So no breaking change, we should just make sure the behavior matches the one in ASP.NET Core routing, which is * decodes the url, and ** does not

Actually, * and ** I believe are used for link generation, which might not apply here. Give me a second to confirm, it's been a while since I've looked at this specifically.

Doh, ** is used for link generation... so I've really mangled my interpretation of what needs to happen here. In this case, I can just decode the entire catch-all value when it's incoming. And I don't really need to look for ** but just * presumably.

It looks like I can simplify this by not worrying about **. Also the RouteContext already decodes the values when adding the context segments so I believe that is taken care of as well.

That makes sense. Lets just do * for now. We can always add ** back if we have to

Final question: do we want to throw if a template starts with more than one *?

Yes, that way we make sure we can add it in the future without breaking anyone

Thanks! That's what I thought, just wanted to confirm.

zbecknell · 2020-07-17T16:30:51Z

Cleaned up now. Fingers crossed for E2E tests.

captainsafia

Thank you, @zbecknell! This looks great.

sfmskywalker · 2020-08-30T17:20:46Z

I'd like to take advantage of this new feature, and assumed that it would be built as part of the nightly build feed at https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet5/nuget/v3/index.json. However, when I try I get the following exception: InvalidOperationException: Invalid template 'categories/{*slug}'. The character '*' in parameter segment '{*slug}' is not allowed.

I referenced Microsoft.AspNetCore.Components* version 5.0.0-rc.1.20429.8

captainsafia · 2020-08-31T20:10:46Z

@sfmskywalker Can you open an issue with a reproduction?

zbecknell · 2020-09-01T13:30:25Z

I'm able to get this working when updating to the latest nightly rc1 of both the SDK and NuGet packages @sfmskywalker.

(Ignore that Preview8Repro title, just reused an existing project.)

zbecknell requested review from SteveSandersonMS and a team as code owners July 16, 2020 23:57

ghost added the area-blazor Includes: Blazor, Razor Components label Jul 16, 2020

mkArtakMSFT added the community-contribution Indicates that the PR has been added by a community member label Jul 17, 2020

javiercn reviewed Jul 17, 2020

View reviewed changes

Fix dotnet#18539 - add Blazor catch-all route parameter

83c8b82

zbecknell force-pushed the master branch from acd7524 to 83c8b82 Compare July 17, 2020 12:51

Add E2E tests for catch-all parameter

9d4f47e

javiercn approved these changes Jul 17, 2020

View reviewed changes

javiercn reviewed Jul 17, 2020

View reviewed changes

Adjust E2E test for catch-all params

13c2625

javiercn reviewed Jul 17, 2020

View reviewed changes

Remove ** scenarios for catch-all params

2944abf

Fix typo causing test failure

a26b5b4

captainsafia approved these changes Jul 18, 2020

View reviewed changes

captainsafia added this to the 5.0.0-rc1 milestone Jul 18, 2020

captainsafia merged commit 0889a62 into dotnet:master Jul 18, 2020

captainsafia mentioned this pull request Aug 24, 2020

Validate catch-all routing support in Blazor #25199

Closed

guardrex mentioned this pull request Aug 25, 2020

Blazor catch-all parameters dotnet/AspNetCore.Docs#19640

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #18539 - add Blazor catch-all route parameter #24038

Fix #18539 - add Blazor catch-all route parameter #24038

zbecknell commented Jul 16, 2020

javiercn left a comment •

edited

Loading

javiercn left a comment

zbecknell commented Jul 17, 2020

javiercn commented Jul 17, 2020

javiercn Jul 17, 2020

zbecknell Jul 17, 2020

zbecknell commented Jul 17, 2020

javiercn commented Jul 17, 2020

javiercn Jul 17, 2020

zbecknell Jul 17, 2020

javiercn Jul 17, 2020

javiercn Jul 17, 2020

zbecknell Jul 17, 2020

zbecknell Jul 17, 2020

javiercn Jul 17, 2020

zbecknell Jul 17, 2020

javiercn Jul 17, 2020

zbecknell Jul 17, 2020

zbecknell commented Jul 17, 2020

captainsafia left a comment

sfmskywalker commented Aug 30, 2020 •

edited

Loading

captainsafia commented Aug 31, 2020

zbecknell commented Sep 1, 2020

Fix #18539 - add Blazor catch-all route parameter #24038

Fix #18539 - add Blazor catch-all route parameter #24038

Conversation

zbecknell commented Jul 16, 2020

javiercn left a comment • edited Loading

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

zbecknell commented Jul 17, 2020

javiercn commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbecknell commented Jul 17, 2020

javiercn commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbecknell commented Jul 17, 2020

captainsafia left a comment

Choose a reason for hiding this comment

sfmskywalker commented Aug 30, 2020 • edited Loading

captainsafia commented Aug 31, 2020

zbecknell commented Sep 1, 2020

javiercn left a comment •

edited

Loading

sfmskywalker commented Aug 30, 2020 •

edited

Loading