Skip to content

Revert feature that resets unspecified component parameters to default. Fixes #6864 #6931

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 1 commit into from
Jan 23, 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 @@ -48,14 +48,14 @@ else

WeatherForecast[] forecasts;

protected override async Task OnParametersSetAsync()
public override Task SetParametersAsync(ParameterCollection parameters)
{
// If no value was given in the URL for StartDate, apply a default
if (StartDate == default)
{
StartDate = DateTime.Now;
}
StartDate = DateTime.Now;
return base.SetParametersAsync(parameters);
}

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<Description>Components feature for ASP.NET Core.</Description>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<IsShippingPackage>true</IsShippingPackage>
</PropertyGroup>

Expand Down
53 changes: 6 additions & 47 deletions src/Components/Components/src/ParameterCollectionExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ private readonly static ConcurrentDictionary<Type, WritersForType> _cachedWriter
/// </summary>
/// <param name="parameterCollection">The <see cref="ParameterCollection"/>.</param>
/// <param name="target">An object that has a public writable property matching each parameter's name and type.</param>
public unsafe static void SetParameterProperties(
public static void SetParameterProperties(
in this ParameterCollection parameterCollection,
object target)
{
Expand All @@ -41,17 +41,6 @@ public unsafe static void SetParameterProperties(
_cachedWritersByType[targetType] = writers;
}

// We only want to iterate through the parameterCollection once, and by the end of it,
// need to have tracked which of the parameter properties haven't yet been written.
// To avoid allocating any list/dictionary to track that, here we stackalloc an array
// of flags and set them based on the indices of the writers we use.
var numWriters = writers.WritersByIndex.Count;
var numUsedWriters = 0;

// TODO: Once we're able to move to netstandard2.1, this can be changed to be
// a Span<bool> and then the enclosing method no longer needs to be 'unsafe'
bool* usageFlags = stackalloc bool[numWriters];

foreach (var parameter in parameterCollection)
{
var parameterName = parameter.Name;
Expand All @@ -62,9 +51,7 @@ public unsafe static void SetParameterProperties(

try
{
writerWithIndex.Writer.SetValue(target, parameter.Value);
usageFlags[writerWithIndex.Index] = true;
numUsedWriters++;
writerWithIndex.SetValue(target, parameter.Value);
}
catch (Exception ex)
{
Expand All @@ -73,23 +60,6 @@ public unsafe static void SetParameterProperties(
$"type '{target.GetType().FullName}'. The error was: {ex.Message}", ex);
}
}

// Now we can determine whether any writers have not been used, and if there are
// some unused ones, find them.
for (var index = 0; numUsedWriters < numWriters; index++)
{
if (index >= numWriters)
{
// This should not be possible
throw new InvalidOperationException("Ran out of writers before marking them all as used.");
}

if (!usageFlags[index])
{
writers.WritersByIndex[index].SetDefaultValue(target);
numUsedWriters++;
}
}
}

internal static IEnumerable<PropertyInfo> GetCandidateBindableProperties(Type targetType)
Expand Down Expand Up @@ -125,12 +95,11 @@ private static void ThrowForUnknownIncomingParameterName(Type targetType, string

class WritersForType
{
public Dictionary<string, (int Index, IPropertySetter Writer)> WritersByName { get; }
public List<IPropertySetter> WritersByIndex { get; }
public Dictionary<string, IPropertySetter> WritersByName { get; }

public WritersForType(Type targetType)
{
var propertySettersByName = new Dictionary<string, IPropertySetter>(StringComparer.OrdinalIgnoreCase);
WritersByName = new Dictionary<string, IPropertySetter>(StringComparer.OrdinalIgnoreCase);
foreach (var propertyInfo in GetCandidateBindableProperties(targetType))
{
var shouldCreateWriter = propertyInfo.IsDefined(typeof(ParameterAttribute))
Expand All @@ -143,24 +112,14 @@ public WritersForType(Type targetType)
var propertySetter = MemberAssignment.CreatePropertySetter(targetType, propertyInfo);

var propertyName = propertyInfo.Name;
if (propertySettersByName.ContainsKey(propertyName))
if (WritersByName.ContainsKey(propertyName))
{
throw new InvalidOperationException(
$"The type '{targetType.FullName}' declares more than one parameter matching the " +
$"name '{propertyName.ToLowerInvariant()}'. Parameter names are case-insensitive and must be unique.");
}

propertySettersByName.Add(propertyName, propertySetter);
}

// Now we know all the entries, construct the resulting list/dictionary
// with well-defined indices
WritersByIndex = new List<IPropertySetter>();
WritersByName = new Dictionary<string, (int, IPropertySetter)>(StringComparer.OrdinalIgnoreCase);
foreach (var pair in propertySettersByName)
{
WritersByName.Add(pair.Key, (WritersByIndex.Count, pair.Value));
WritersByIndex.Add(pair.Value);
WritersByName.Add(propertyName, propertySetter);
}
}
}
Expand Down
2 changes: 0 additions & 2 deletions src/Components/Components/src/Reflection/IPropertySetter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,5 @@ namespace Microsoft.AspNetCore.Components.Reflection
internal interface IPropertySetter
{
void SetValue(object target, object value);

void SetDefaultValue(object target);
}
}
4 changes: 0 additions & 4 deletions src/Components/Components/src/Reflection/MemberAssignment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,10 @@ public PropertySetter(MethodInfo setMethod)
{
_setterDelegate = (Action<TTarget, TValue>)Delegate.CreateDelegate(
typeof(Action<TTarget, TValue>), setMethod);
var propertyType = typeof(TValue);
}

public void SetValue(object target, object value)
=> _setterDelegate((TTarget)target, (TValue)value);

public void SetDefaultValue(object target)
=> _setterDelegate((TTarget)target, default);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public void IncomingParameterMatchesInheritedDeclaredParameter_SetsValue()
}

[Fact]
public void NoIncomingParameterMatchesDeclaredParameter_SetValuesDefault()
public void NoIncomingParameterMatchesDeclaredParameter_LeavesValueUnchanged()
{
// Arrange
var existingObjectValue = new object();
Expand All @@ -90,9 +90,9 @@ public void NoIncomingParameterMatchesDeclaredParameter_SetValuesDefault()
parameterCollection.SetParameterProperties(target);

// Assert
Assert.Equal(0, target.IntProp);
Assert.Null(target.StringProp);
Assert.Null(target.ObjectPropCurrentValue);
Assert.Equal(456, target.IntProp);
Assert.Equal("Existing value", target.StringProp);
Assert.Same(existingObjectValue, target.ObjectPropCurrentValue);
}

[Fact]
Expand Down
4 changes: 4 additions & 0 deletions src/Components/test/E2ETest/Tests/RoutingTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ 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();
WaitAssert.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 @@ -5,9 +5,15 @@

@functions
{
[Parameter]
string FirstName { get; set; }
[Parameter] string FirstName { get; set; }

[Parameter]
string LastName { get ; set; }
[Parameter] 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 @@ -48,14 +48,14 @@ else

WeatherForecast[] forecasts;

protected override async Task OnParametersSetAsync()
public override Task SetParametersAsync(ParameterCollection parameters)
{
// If no value was given in the URL for StartDate, apply a default
if (StartDate == default)
{
StartDate = DateTime.Now;
}
StartDate = DateTime.Now;
return base.SetParametersAsync(parameters);
}

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