Skip to content

Conversation

@captainsafia
Copy link
Member

Summary of the changes

  • Adds new OptionalTypeRouteConstraint subclass for RouteConstraint
  • Updates RouteConstraint parsing logic to support optional parameters
  • Updates processing of route string to segments in RouteContext to support more route patterns (like /a//b and /users/)
  • Update Match logic in route entry to support optional parameters in route
  • Updates conditional checks in Match logic to check for new constraints considering changes in RouteContext.Segments
  • Adds E2E test for component with optional parameters
  • Adds unit tests for optional parameters

Addresses #10759

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Mar 10, 2020
@captainsafia
Copy link
Member Author

captainsafia commented Mar 10, 2020

I've left some code comments to explain the changes that require context to understand. While implementing this, I was prioritizing reducing the number of fundamental changes that needed to be made to the routing logic to support this. The consequence of this is that the Match method contains quite a few conditional checks to account for states that ideally would be handled in the underlying data structures within the routing logic, IMO.

I also added test cases to validate this based on my understanding of our test suite and made sure that existing route tests still pass. Happy to add more test coverage to this as needed. @pranavkm had shared a pointer to other routing tests in this repo that can be used as inspiration so I will take a look at that.

@SteveSandersonMS
Copy link
Member

Thanks @captainsafia! This looks superb.

It might be a couple of days until I get to review this properly since I need to complete something else pretty quickly. Hope that's OK. In the meantime if either @pranavkm or @javiercn finds an opportunity to take a look they should feel free :)

@SteveSandersonMS
Copy link
Member

I might be missing the implementation here, but is it also possible add support for optional parameters that don't define a type constraint? For example, /products/{id?}.

@SteveSandersonMS
Copy link
Member

This is looking really good to me. Thanks @captainsafia! People have been wanting this feature for ages.

Hopefully @pranavkm will also get a chance to dig into it and comment on any of the subtleties.

@captainsafia
Copy link
Member Author

I might be missing the implementation here, but is it also possible add support for optional parameters that don't define a type constraint? For example, /products/{id?}.

🤦‍♀ I can add support for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this already works just fine, but I suspect you might have to trim the trailing ? before passing it through. This is incredibly rare, but you could do {id:int:long?} which basically treats the entire segment as optional, not just the long? Would the :int constraint get treated as non-optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

The constraint itself wouldn't be treated as optional, but the segment would be. The result of this is as follows:

  • If the user has a route like /a/{id:int:long?}/literal, then it will through an error because they have provided a non-optional segment after an optional one. It makes sense to me to throw an error in this case but let me know if you think otherwise.
  • When matching it is not required that a match be found for this segment since it contains an optional component.

You'd still get two separate constraints to match with, one optional and one not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Assert.NotNull(context.Handler);
Assert.NotNull(context.Handler);
Assert.Equal(typeof(TestHandler1), context.Handler);

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be useful to verify these scenarios work as expected:

var routeTable = new TestRouteTableBuilder()
                .AddRoute("/users/{id}", typeof(TestHandler1))
                .AddRoute("/users/{id?}", typeof(TestHandler2))
                .Build();

// Scenario 1: 
var context = new RouteContext("/users/");

// Scenario 2: (this should throw)
var context = new RouteContext("/users/2");

Copy link
Member

Choose a reason for hiding this comment

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

Also the reverse of the scenario that @pranavkm mentioned 👍 (reverse the optional and non-optional ones)

Copy link
Member Author

Choose a reason for hiding this comment

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

@pranavkm and I discussed this on teams but I'll post notes on my understanding of how this should function (which I reflected in the tests).

For the situation above, Scenario 1 should actually match with TestHandler2 and Scenario 2 should match with TestHandler1.

IMO, it makes sense that if the routes are otherwise ambiguous but one has an optional parameter and the other does not, then it should favor the non-optional over the optional unless no parameter is provided.

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

This is looking great!

The feedback from @pranavkm and @rynowak is pretty comprehensive, so nothing else from me.

Thanks for digging into all the details here!

@captainsafia
Copy link
Member Author

@pranavkm This is ready for another look.

Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

Looks great

@captainsafia
Copy link
Member Author

Thanks all for the reviews! Merging this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-blazor Includes: Blazor, Razor Components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants