huahaofeng
/
aspnetcore
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix #4212 area ambient value page->controller 

This change enhances our ambient value logic to also deal with required
values. In 2.2 we introduced a 'required values' semantic to allow route
values to appear "to the left" of a route pattern for the purpose of
ambient values copying. This is a complicated way of saying "when you
like to a different endpoint then discard the ambient values".

What we didn't consider is that some ambient values are special (like
area). So basically, we'll allow an ambient value to be used if it's
part of the required values - even if we've already decided to discard
the ambient values.

This is a pretty surgical fix and only affected the desired scenario
based on tests.

-----

I also removed an optimization that I think is broken. I put an earlier
optimization in place that attempted to count ambient values as they
were "seen" to try and avoid some extra copying. This copying loop has a
cost even if it no-ops which is what I was trying to prevent.

Unfortunately since we added 'required values' - it's now possible for
an ambient value to be double-counted, which makes this optimization
incorrect.
This commit is contained in:
Ryan Nowak 2019-02-10 11:25:31 -08:00
parent 88ae930fad
commit 5a291d0bc0
3 changed files with 67 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
src/Http/Routing/src/Template/TemplateBinder.cs
View File

@ -118,10 +118,9 @@ namespace Microsoft.AspNetCore.Routing.Template
var slots = new KeyValuePair<string, object>[_slots.Length];
Array.Copy(_slots, 0, slots, 0, slots.Length);
// Keeping track of the number of 'values' and 'ambient values' we've processed can be used to avoid doing
// Keeping track of the number of 'values' we've processed can be used to avoid doing
// some expensive 'merge' operations later.
var valueProcessedCount = 0;
var ambientValueProcessedCount = 0;
// Start by copying all of the values out of the 'values' and into the slots. There's no success
// case where we *don't* use all of the 'values' so there's no reason not to do this up front
@ -168,11 +167,6 @@ namespace Microsoft.AspNetCore.Routing.Template
{
ambientValue = null;
}
else
{
// Track the count of processed ambient values - this allows a fast path later.
ambientValueProcessedCount++;
}
// For now, only check ambient values with required values that don't have a parameter
// Ambient values for parameters are processed below
@ -226,15 +220,10 @@ namespace Microsoft.AspNetCore.Routing.Template
var parameter = parameters[i];
// We are copying **all** ambient values
if (copyAmbientValues)
{
hasAmbientValue = ambientValues != null && ambientValues.TryGetValue(key, out ambientValue);
if (hasAmbientValue)
{
// Track the count of processed ambient values - this allows a fast path later.
ambientValueProcessedCount++;
}
if (hasExplicitValue && hasAmbientValue && !RoutePartsEqual(ambientValue, value))
{
// Stop copying current values when we find one that doesn't match
@ -261,6 +250,29 @@ namespace Microsoft.AspNetCore.Routing.Template
}
}
// This might be an ambient value that matches a required value. We want to use these even if we're
// not bulk-copying ambient values.
//
// This comes up in a case like the following:
// ambient-values: { page = "/DeleteUser", area = "Admin", }
// values: { controller = "Home", action = "Index", }
// pattern: {area}/{controller}/{action}/{id?}
// required-values: { area = "Admin", controller = "Home", action = "Index", page = (string)null, }
//
// OR in plain English... when linking from a page in an area to an action in the same area, it should
// be possible to use the area as an ambient value.
if (!copyAmbientValues && _pattern.RequiredValues.TryGetValue(key, out var requiredValue))
{
hasAmbientValue = ambientValues != null && ambientValues.TryGetValue(key, out ambientValue);
if (hasAmbientValue && RoutePartsEqual(requiredValue, ambientValue))
{
// Treat this an an explicit value to *force it*.
slots[i] = new KeyValuePair<string, object>(key, ambientValue);
hasExplicitValue = true;
value = ambientValue;
}
}
// If the parameter is a match, add it to the list of values we will use for URI generation
if (hasExplicitValue && !ReferenceEquals(value, SentinullValue.Instance))
{
@ -345,15 +357,13 @@ namespace Microsoft.AspNetCore.Routing.Template
// Currently this copy is required because BindValues will mutate the accepted values :(
var combinedValues = new RouteValueDictionary(acceptedValues);
if (ambientValueProcessedCount < (ambientValues?.Count ?? 0))
{
// Add any ambient values that don't match parameters - they need to be visible to constraints
// but they will ignored by link generation.
CopyNonParameterAmbientValues(
ambientValues: ambientValues,
acceptedValues: acceptedValues,
combinedValues: combinedValues);
}
// Add any ambient values that don't match parameters - they need to be visible to constraints
// but they will ignored by link generation.
CopyNonParameterAmbientValues(
ambientValues: ambientValues,
acceptedValues: acceptedValues,
combinedValues: combinedValues);
return new TemplateValuesResult()
{

34
src/Http/Routing/test/UnitTests/Template/TemplateBinderTests.cs
View File

@ -1381,6 +1381,40 @@ namespace Microsoft.AspNetCore.Routing.Template.Tests
Assert.Equal(expected, boundTemplate);
}
// Regression test for aspnet/AspNetCore#4212
//
// An ambient value should be used to satisfy a required value even if if we're discarding
// ambient values.
[Fact]
public void BindValues_LinkingFromPageToAControllerInAreaWithAmbientArea_Success()
{
// Arrange
var expected = "/Admin/LG2/SomeAction";
var template = "{area}/{controller=Home}/{action=Index}/{id?}";
var defaults = new RouteValueDictionary();
var ambientValues = new RouteValueDictionary(new { area = "Admin", page = "/LGAnotherPage", id = "17" });
var explicitValues = new RouteValueDictionary(new { controller = "LG2", action = "SomeAction" });
var binder = new TemplateBinder(
UrlEncoder.Default,
new DefaultObjectPoolProvider().Create(new UriBuilderContextPooledObjectPolicy()),
RoutePatternFactory.Parse(
template,
defaults,
parameterPolicies: null,
requiredValues: new { area = "Admin", action = "SomeAction", controller = "LG2", page = (string)null }),
defaults,
requiredKeys: new string[] { "area", "action", "controller", "page" },
parameterPolicies: null);
// Act
var result = binder.GetValues(ambientValues, explicitValues);
var boundTemplate = binder.BindValues(result.AcceptedValues);
// Assert
Assert.Equal(expected, boundTemplate);
}
[Fact]
public void BindValues_HasUnmatchingAmbientValues_Discard()
{

2
src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesWithBasePathTest.cs
View File

@ -323,7 +323,7 @@ Hello from page";
var expected =
@"<a href=""/Accounts/Manage/RenderPartials"">Link inside area</a>
<a href=""/Products/List/old/20"">Link to external area</a>
<a href="""">Link to area action</a>
<a href=""/Accounts"">Link to area action</a>
<a href=""/Admin"">Link to non-area page</a>";
// Act