Skip to content

Clear unused routing params. Fixes #7419 #12407

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 4 commits into from
Jul 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,36 +34,32 @@ else
</tbody>
</table>
<p>
<a href="fetchdata/@StartDate.AddDays(-5).ToString("yyyy-MM-dd")" class="btn btn-secondary float-left">
<a href="fetchdata/@startDate.AddDays(-5).ToString("yyyy-MM-dd")" class="btn btn-secondary float-left">
◀ Previous
</a>
<a href="fetchdata/@StartDate.AddDays(5).ToString("yyyy-MM-dd")" class="btn btn-secondary float-right">
<a href="fetchdata/@startDate.AddDays(5).ToString("yyyy-MM-dd")" class="btn btn-secondary float-right">
Next ▶
</a>
</p>
}

@code {
[Parameter] public DateTime StartDate { get; set; }
[Parameter] public DateTime? StartDate { get; set; }

WeatherForecast[] forecasts;

public override Task SetParametersAsync(ParameterCollection parameters)
{
StartDate = DateTime.Now;
return base.SetParametersAsync(parameters);
}
DateTime startDate;

protected override async Task OnParametersSetAsync()
{
startDate = StartDate.GetValueOrDefault(DateTime.Now);
forecasts = await Http.GetJsonAsync<WeatherForecast[]>(
$"sample-data/weather.json?date={StartDate.ToString("yyyy-MM-dd")}");
$"sample-data/weather.json?date={startDate.ToString("yyyy-MM-dd")}");

// Because StandaloneApp doesn't really have a server endpoint to get dynamic data from,
// fake the DateFormatted values here. This would not apply in a real app.
for (var i = 0; i < forecasts.Length; i++)
{
forecasts[i].DateFormatted = StartDate.AddDays(i).ToShortDateString();
forecasts[i].DateFormatted = startDate.AddDays(i).ToShortDateString();
}
}

Expand Down
11 changes: 10 additions & 1 deletion src/Components/Components/src/Reflection/MemberAssignment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,16 @@ public PropertySetter(MethodInfo setMethod)
}

public void SetValue(object target, object value)
=> _setterDelegate((TTarget)target, (TValue)value);
{
if (value == null)
{
_setterDelegate((TTarget)target, default);
}
else
{
_setterDelegate((TTarget)target, (TValue)value);
}
}
}
}
}
17 changes: 16 additions & 1 deletion src/Components/Components/src/Routing/RouteEntry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,17 @@ namespace Microsoft.AspNetCore.Components.Routing
{
internal class RouteEntry
{
public RouteEntry(RouteTemplate template, Type handler)
public RouteEntry(RouteTemplate template, Type handler, string[] unusedRouteParameterNames)
{
Template = template;
UnusedRouteParameterNames = unusedRouteParameterNames;
Handler = handler;
}

public RouteTemplate Template { get; }

public string[] UnusedRouteParameterNames { get; }

public Type Handler { get; }

internal void Match(RouteContext context)
Expand Down Expand Up @@ -45,6 +48,18 @@ internal void Match(RouteContext context)
}
}

// In addition to extracting parameter values from the URL, each route entry
// also knows which other parameters should be supplied with null values. These
// are parameters supplied by other route entries matching the same handler.
if (UnusedRouteParameterNames.Length > 0)
Copy link
Member

@rynowak rynowak Jul 21, 2019

Choose a reason for hiding this comment

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

You should double check that this handles cases with optional parameters (if Blazor supports them) like {a}/{b?}. Depending on the implementation it's possible for you to get b omitted, or b => null in the dictionary and with this change seems like we need b => null.

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s a good thought, but Blazor routes don’t yet support optional parameters.

Copy link
Member

Choose a reason for hiding this comment

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

OK - worth a comment somewhere then? It's really likely we'll try to converge on features at some point. If we have an issue tracking that then we could add the note there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, commented here: #10759 (comment)

{
parameters ??= new Dictionary<string, object>(StringComparer.Ordinal);
for (var i = 0; i < UnusedRouteParameterNames.Length; i++)
{
parameters[UnusedRouteParameterNames[i]] = null;
}
}

context.Parameters = parameters;
context.Handler = Handler;
}
Expand Down
39 changes: 33 additions & 6 deletions src/Components/Components/src/Routing/RouteTableFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,26 +34,53 @@ public static RouteTable Create(Assembly appAssembly)

internal static RouteTable Create(IEnumerable<Type> componentTypes)
{
var routes = new List<RouteEntry>();
foreach (var type in componentTypes)
var templatesByHandler = new Dictionary<Type, string[]>();
foreach (var componentType in componentTypes)
{
// We're deliberately using inherit = false here.
//
// RouteAttribute is defined as non-inherited, because inheriting a route attribute always causes an
// ambiguity. You end up with two components (base class and derived class) with the same route.
var routeAttributes = type.GetCustomAttributes<RouteAttribute>(inherit: false);
var routeAttributes = componentType.GetCustomAttributes<RouteAttribute>(inherit: false);

var templates = routeAttributes.Select(t => t.Template).ToArray();
templatesByHandler.Add(componentType, templates);
}
return Create(templatesByHandler);
}

foreach (var routeAttribute in routeAttributes)
internal static RouteTable Create(Dictionary<Type, string[]> templatesByHandler)
{
var routes = new List<RouteEntry>();
foreach (var keyValuePair in templatesByHandler)
{
var parsedTemplates = keyValuePair.Value.Select(v => TemplateParser.ParseTemplate(v)).ToArray();
var allRouteParameterNames = parsedTemplates
.SelectMany(GetParameterNames)
.Distinct(StringComparer.OrdinalIgnoreCase)
.ToArray();

foreach (var parsedTemplate in parsedTemplates)
{
var template = TemplateParser.ParseTemplate(routeAttribute.Template);
var entry = new RouteEntry(template, type);
var unusedRouteParameterNames = allRouteParameterNames
.Except(GetParameterNames(parsedTemplate), StringComparer.OrdinalIgnoreCase)
.ToArray();
var entry = new RouteEntry(parsedTemplate, keyValuePair.Key, unusedRouteParameterNames);
routes.Add(entry);
}
}

return new RouteTable(routes.OrderBy(id => id, RoutePrecedence).ToArray());
}

private static string[] GetParameterNames(RouteTemplate routeTemplate)
{
return routeTemplate.Segments
.Where(s => s.IsParameter)
.Select(s => s.Value)
.ToArray();
}

/// <summary>
/// Route precedence algorithm.
/// We collect all the routes and sort them from most specific to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,24 @@ public void DeclaredParameterClashesWithInheritedParameter_Throws()
ex.Message);
}

[Fact]
public void SupplyingNullWritesDefaultForType()
{
// Arrange
var parameterCollection = new ParameterCollectionBuilder
{
{ nameof(HasInstanceProperties.IntProp), null },
{ nameof(HasInstanceProperties.StringProp), null },
}.Build();
var target = new HasInstanceProperties { IntProp = 123, StringProp = "Hello" };

// Act
parameterCollection.SetParameterProperties(target);

// Assert
Assert.Equal(0, target.IntProp);
Assert.Null(target.StringProp);
}

class HasInstanceProperties
{
Expand Down
67 changes: 60 additions & 7 deletions src/Components/Components/test/Routing/RouteTableFactoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,9 @@ public void PrefersLiteralTemplateOverTemplateWithParameters()
{
// Arrange
var routeTable = new TestRouteTableBuilder()
.AddRoute("/an/awesome/path")
.AddRoute("/{some}/awesome/{route}/").Build();
.AddRoute("/an/awesome/path", typeof(TestHandler1))
.AddRoute("/{some}/awesome/{route}/", typeof(TestHandler2))
.Build();
var context = new RouteContext("/an/awesome/path");

// Act
Expand Down Expand Up @@ -346,9 +347,58 @@ public void DetectsAmbiguousRoutes(string left, string right)
Assert.Equal(expectedMessage, exception.Message);
}

[Fact]
public void SuppliesNullForUnusedHandlerParameters()
{
// Arrange
var routeTable = new TestRouteTableBuilder()
.AddRoute("/", typeof(TestHandler1))
.AddRoute("/products/{param1:int}", typeof(TestHandler1))
.AddRoute("/products/{param2}/{PaRam1}", typeof(TestHandler1))
.AddRoute("/{unrelated}", typeof(TestHandler2))
.Build();
var context = new RouteContext("/products/456");

// Act
routeTable.Route(context);

// Assert
Assert.Collection(routeTable.Routes,
route =>
{
Assert.Same(typeof(TestHandler1), route.Handler);
Assert.Equal("/", route.Template.TemplateText);
Assert.Equal(new[] { "param1", "param2" }, route.UnusedRouteParameterNames);
},
route =>
{
Assert.Same(typeof(TestHandler2), route.Handler);
Assert.Equal("{unrelated}", route.Template.TemplateText);
Assert.Equal(Array.Empty<string>(), route.UnusedRouteParameterNames);
},
route =>
{
Assert.Same(typeof(TestHandler1), route.Handler);
Assert.Equal("products/{param1:int}", route.Template.TemplateText);
Assert.Equal(new[] { "param2" }, route.UnusedRouteParameterNames);
},
route =>
{
Assert.Same(typeof(TestHandler1), route.Handler);
Assert.Equal("products/{param2}/{PaRam1}", route.Template.TemplateText);
Assert.Equal(Array.Empty<string>(), route.UnusedRouteParameterNames);
});
Assert.Same(typeof(TestHandler1), context.Handler);
Assert.Equal(new Dictionary<string, object>
{
{ "param1", 456 },
{ "param2", null },
}, context.Parameters);
}

private class TestRouteTableBuilder
{
IList<(string, Type)> _routeTemplates = new List<(string, Type)>();
IList<(string Template, Type Handler)> _routeTemplates = new List<(string, Type)>();
Type _handler = typeof(object);

public TestRouteTableBuilder AddRoute(string template, Type handler = null)
Expand All @@ -361,10 +411,10 @@ public RouteTable Build()
{
try
{
return new RouteTable(_routeTemplates
.Select(rt => new RouteEntry(TemplateParser.ParseTemplate(rt.Item1), rt.Item2))
.OrderBy(id => id, RouteTableFactory.RoutePrecedence)
.ToArray());
var templatesByHandler = _routeTemplates
.GroupBy(rt => rt.Handler)
.ToDictionary(group => group.Key, group => group.Select(g => g.Template).ToArray());
return RouteTableFactory.Create(templatesByHandler);
}
catch (InvalidOperationException ex) when (ex.InnerException is InvalidOperationException)
{
Expand All @@ -373,5 +423,8 @@ public RouteTable Build()
}
}
}

class TestHandler1 { }
class TestHandler2 { }
}
}
4 changes: 0 additions & 4 deletions src/Components/test/E2ETest/Tests/RoutingTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,6 @@ public void CanFollowLinkToPageWithParameters()
AssertHighlightedLinks("With parameters", "With more parameters");

// Can remove parameters while remaining on same page
// WARNING: This only works because the WithParameters component overrides SetParametersAsync
// and explicitly resets its parameters to default when each new set of parameters arrives.
// Without that, the page would retain the old value.
// See https://github.com/aspnet/AspNetCore/issues/6864 where we reverted the logic to auto-reset.
app.FindElement(By.LinkText("With parameters")).Click();
Browser.Equal("Your full name is Abc .", () => app.FindElement(By.Id("test-info")).Text);
AssertHighlightedLinks("With parameters");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,4 @@
[Parameter] public string FirstName { get; set; }

[Parameter] public string LastName { get ; set; }

public override Task SetParametersAsync(ParameterCollection parameters)
{
// Manually reset parameters to defaults so we don't retain any from an earlier URL
FirstName = default;
LastName = default;
return base.SetParametersAsync(parameters);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,28 +34,24 @@ else
</tbody>
</table>
<p>
<a href="fetchdata/@StartDate.AddDays(-5).ToString("yyyy-MM-dd")" class="btn btn-secondary float-left">
<a href="fetchdata/@startDate.AddDays(-5).ToString("yyyy-MM-dd")" class="btn btn-secondary float-left">
◀ Previous
</a>
<a href="fetchdata/@StartDate.AddDays(5).ToString("yyyy-MM-dd")" class="btn btn-secondary float-right">
<a href="fetchdata/@startDate.AddDays(5).ToString("yyyy-MM-dd")" class="btn btn-secondary float-right">
Next ▶
</a>
</p>
}

@code {
[Parameter] public DateTime StartDate { get; set; }
[Parameter] public DateTime? StartDate { get; set; }

WeatherForecast[] forecasts;

public override Task SetParametersAsync(ParameterCollection parameters)
{
StartDate = DateTime.Now;
return base.SetParametersAsync(parameters);
}
DateTime startDate;

protected override async Task OnParametersSetAsync()
{
forecasts = await ForecastService.GetForecastAsync(StartDate);
startDate = StartDate.GetValueOrDefault(DateTime.Now);
forecasts = await ForecastService.GetForecastAsync(startDate);
}
}