[Blazor] Fixes issues with route precedence

[javiercn]

Description

In 5.0 we introduced two features on Blazor routing that enable users to write routing templates that match paths with variable length segments. These two features are optional parameters {parameter?} and catch all parameters {*catchall}.

Our routing system ordered the routes based on precedence and the (now false) assumption that route templates would only match paths with an equal number of segments.

The implementation that we have worked for naïve scenarios but breaks on more real world scenarios. The change here includes fixes to the way we order the routes in the route table to match the expectations as well as fixes on the route matching algorithm to ensure we match routes with variable number of segments correctly.

Customer Impact

This was reported by customers on #27250

The impact is that a route with {*catchall} will prevent more specific routes like /page/{parameter} from being accessible.

There are no workarounds since precedence is a fundamental behavior of the routing system.

Regression?

No, these Blazor features were initially added in 5.0.

Risk

Low. These two features were just introduced in 5.0 and their usage is not as prevalent as in asp.net core routing. That said, it's important to fix them as otherwise we run the risk of diverting in behavior from asp.net core routing and Blazor routing, which is not something we want to do.

We have functional tests covering the area and we've added a significant amount of unit tests to validate the changes.

[ghost]

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

[SteveSandersonMS]

To build more confidence that the algorithmic changes don't have any unanticipated breaking effect on existing apps, I had a go at running the old tests against the new implementation. Six of the old tests fail with the new implementation, all of which may be intentional and for good reasons, but it's worth spelling out the exact details given this is going in a patch.

Here are the old tests that fail with the new algorithm:

• Parse_DoesNotThrowIfOptionalConstraint
  • The implementation now does throw if you pass a value ending with ? to RouteConstraint.Parse. However the template parser no longer does this, as it strips off the ? before calling RouteConstraint.Parse.
  • Conclusion: this is fine because the test represented an internal implementation detail that no longer applies
• InvalidTemplate_WithRepeatedParameter
  • The error message generated by the older code contains a typo which has been fixed in the newer code, so the unit test was also updated correspondingly
  • Conclusion: this is fine, because fixing a typo in an exception message is not considered to be a breaking change
• CanMatchSegmentWithMultipleConstraints
  • Now fails with message Unsupported constraint 'double?' in route '/{value:double?:int?}/'
  • The syntax before didn't make sense. It was getting the developer to duplicate the "optional" flag on each constraint, leading to nonsensical possibilities like some of the constraints say it's optional, and others don't.
  • The new syntax is {value:double:int?} (i.e., a single trailing ? to indicate the segment is optional, independently of its constraints), which makes more sense
  • Conclusion: this is a breaking change, but does make things better
• PrefersOptionalParamsOverNonOptionalParams and PrefersOptionalParamsOverNonOptionalParamsReverseOrder
  • Now fails saying that /users/{id} and /users/{id?} are ambiguous
  • The previous logic was that it prioritized non-optional params over optional ones (even though the name of the unit test says the exact opposite). The new logic is that this is considered an error, which is consistent with the server-side routing rules.
  • Conclusion: this is a breaking change, but does make things better
• SuppliesNullForUnusedHandlerParameters
  • My interpretation is that this fails because RouteTable used to sort by number of segments. Previously /{unrelated} would go before /products/{id} simply because /{unrelated} has fewer segments, even though /products/{id} could be regarded as more specific. In practice this never mattered because a one-segment template wouldn't match a two-segment URL. The new algorithm prefers specificity regardless of the number of segments and hence puts /products/{id} before /{unrelated}.
  • Conclusion: I think this change is fine because it doesn't seem like it can affect routing results

[captainsafia]

@SteveSandersonMS Great summary, Steve! Thanks for taking the time to assemble that. I think the change in how we handle optional and non-optional constraints for the same parameter might end up being the most impactful breaking change for users. Since this is being put into a patch release, what's our approach for handling the breaking changes here?

[SteveSandersonMS]

@captainsafia I think we'll need to make this change opt-in (and on by default in the project template for new apps).

[SteveSandersonMS]

@javiercn I'm assuming I'm still finishing this off (and am working on it now), but if you preferred to take it back, please let me know! Just don't want us both to be working on it separately at the same time.

[javiercn]

LGTM!

(I can't sign-off because I created the PR)

[javiercn]

/AzurePipelines run AspNetCore-ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mkArtakMSFT]

Signing off on behalf of @javiercn 👍

[SteveSandersonMS]

@mkArtakMSFT Looks like this is ready to merge.

Our test coverage for the project templates is very weak at the moment (seems that all the Blazor template tests are currently quarantined and failing), so I hope we have CTI coverage and will get preview builds of the 5.0.1 SDK to test manually before it ships.