Skip to content
This repository was archived by the owner on Nov 27, 2018. It is now read-only.

Add Remove(string key, out object value) overload to RouteValueDictionary #858

Merged
merged 2 commits into from
Oct 17, 2018
Merged

Add Remove(string key, out object value) overload to RouteValueDictionary #858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b17852a
Add Remove(string key, out object value) overload to RouteValueDictio…
drieseng Oct 14, 2018
4f7e82b
Consistently use _count field instead of Count property in Remove ove…
drieseng Oct 17, 2018
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
46 changes: 45 additions & 1 deletion src/Microsoft.AspNetCore.Routing.Abstractions/RouteValueDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,9 @@ public bool Remove(string key)
return false;
}

EnsureCapacity(Count);
// Ensure property storage is converted to array storage as we'll be
// applying the lookup and removal on the array
EnsureCapacity(_count);

var index = FindIndex(key);
if (index >= 0)
Expand All @@ -404,6 +406,48 @@ public bool Remove(string key)
return false;
}

/// <summary>
/// Attempts to remove and return the value that has the specified key from the <see cref="RouteValueDictionary"/>.
/// </summary>
/// <param name="key">The key of the element to remove and return.</param>
/// <param name="value">When this method returns, contains the object removed from the <see cref="RouteValueDictionary"/>, or <c>null</c> if key does not exist.</param>
/// <returns>
/// <c>true</c> if the object was removed successfully; otherwise, <c>false</c>.
/// </returns>
public bool Remove(string key, out object value)
{
if (key == null)
{
ThrowArgumentNullExceptionForKey();
}

if (_count == 0)
{
value = default;
return false;
}

// Ensure property storage is converted to array storage as we'll be
// applying the lookup and removal on the array
EnsureCapacity(_count);

var index = FindIndex(key);
if (index >= 0)
{
_count--;
var array = _arrayStorage;
value = array[index].Value;
Array.Copy(array, index + 1, array, index, _count - index);
array[_count] = default;

return true;
}

value = default;
return false;
}


/// <summary>
/// Attempts to the add the provided <paramref name="key"/> and <paramref name="value"/> to the dictionary.
/// </summary>
Expand Down
6 changes: 1 addition & 5 deletions src/Microsoft.AspNetCore.Routing/Template/TemplateBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,11 +471,7 @@ private bool TryBindValuesCore(UriBuildingContext context, RouteValueDictionary
else if (part is RoutePatternParameterPart parameterPart)
{
// If it's a parameter, get its value
var hasValue = acceptedValues.TryGetValue(parameterPart.Name, out var value);
if (hasValue)
{
acceptedValues.Remove(parameterPart.Name);
}
acceptedValues.Remove(parameterPart.Name, out var value);

var isSameAsDefault = false;
if (_defaults != null &&
Expand Down
229 changes: 229 additions & 0 deletions test/Microsoft.AspNetCore.Mvc.Routing.Abstractions.Tests/RouteValueDictionaryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1393,6 +1393,235 @@ public void Remove_ListStorage_True_CaseInsensitive()
Assert.IsType<KeyValuePair<string, object>[]>(dict._arrayStorage);
}


[Fact]
public void Remove_KeyAndOutValue_EmptyStorage()
{
// Arrange
var dict = new RouteValueDictionary();

// Act
var result = dict.Remove("key", out var removedValue);

// Assert
Assert.False(result);
Assert.Null(removedValue);
}

[Fact]
public void Remove_KeyAndOutValue_EmptyStringIsAllowed()
{
// Arrange
var dict = new RouteValueDictionary();

// Act
var result = dict.Remove("", out var removedValue);

// Assert
Assert.False(result);
Assert.Null(removedValue);
}

[Fact]
public void Remove_KeyAndOutValue_PropertyStorage_Empty()
{
// Arrange
var dict = new RouteValueDictionary(new { });

// Act
var result = dict.Remove("other", out var removedValue);

// Assert
Assert.False(result);
Assert.Null(removedValue);
Assert.Empty(dict);
Assert.NotNull(dict._propertyStorage);
}

[Fact]
public void Remove_KeyAndOutValue_PropertyStorage_False()
{
// Arrange
var dict = new RouteValueDictionary(new { key = "value" });

// Act
var result = dict.Remove("other", out var removedValue);

// Assert
Assert.False(result);
Assert.Null(removedValue);
Assert.Collection(dict, kvp => { Assert.Equal("key", kvp.Key); Assert.Equal("value", kvp.Value); });
Assert.IsType<KeyValuePair<string, object>[]>(dict._arrayStorage);
}

[Fact]
public void Remove_KeyAndOutValue_PropertyStorage_True()
{
// Arrange
object value = "value";
var dict = new RouteValueDictionary(new { key = value });

// Act
var result = dict.Remove("key", out var removedValue);

// Assert
Assert.True(result);
Assert.Same(value, removedValue);
Assert.Empty(dict);
Assert.IsType<KeyValuePair<string, object>[]>(dict._arrayStorage);
}

[Fact]
public void Remove_KeyAndOutValue_PropertyStorage_True_CaseInsensitive()
{
// Arrange
object value = "value";
var dict = new RouteValueDictionary(new { key = value });

// Act
var result = dict.Remove("kEy", out var removedValue);

// Assert
Assert.True(result);
Assert.Same(value, removedValue);
Assert.Empty(dict);
Assert.IsType<KeyValuePair<string, object>[]>(dict._arrayStorage);
}

[Fact]
public void Remove_KeyAndOutValue_ListStorage_False()
{
// Arrange
var dict = new RouteValueDictionary()
{
{ "key", "value" },
};

// Act
var result = dict.Remove("other", out var removedValue);

// Assert
Assert.False(result);
Assert.Null(removedValue);
Assert.Collection(dict, kvp => { Assert.Equal("key", kvp.Key); Assert.Equal("value", kvp.Value); });
Assert.IsType<KeyValuePair<string, object>[]>(dict._arrayStorage);
}

[Fact]
public void Remove_KeyAndOutValue_ListStorage_True()
{
// Arrange
object value = "value";
var dict = new RouteValueDictionary()
{
{ "key", value }
};

// Act
var result = dict.Remove("key", out var removedValue);

// Assert
Assert.True(result);
Assert.Same(value, removedValue);
Assert.Empty(dict);
Assert.IsType<KeyValuePair<string, object>[]>(dict._arrayStorage);
}

[Fact]
public void Remove_KeyAndOutValue_ListStorage_True_CaseInsensitive()
{
// Arrange
object value = "value";
var dict = new RouteValueDictionary()
{
{ "key", value }
};

// Act
var result = dict.Remove("kEy", out var removedValue);

// Assert
Assert.True(result);
Assert.Same(value, removedValue);
Assert.Empty(dict);
Assert.IsType<KeyValuePair<string, object>[]>(dict._arrayStorage);
}

Copy link
Member

@rynowak rynowak Oct 17, 2018

Choose a reason for hiding this comment

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

For array storage I'd also like to see tests for the case where the value removed is the first/middle/last

Copy link
Contributor Author

@drieseng drieseng Oct 17, 2018

Choose a reason for hiding this comment

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

Done

[Fact]
public void Remove_KeyAndOutValue_ListStorage_KeyExists_First()
{
// Arrange
object value = "value";
var dict = new RouteValueDictionary()
{
{ "key", value },
{ "other", 5 },
{ "dotnet", "rocks" }
};

// Act
var result = dict.Remove("key", out var removedValue);

// Assert
Assert.True(result);
Assert.Same(value, removedValue);
Assert.Equal(2, dict.Count);
Assert.False(dict.ContainsKey("key"));
Assert.True(dict.ContainsKey("other"));
Assert.True(dict.ContainsKey("dotnet"));
Assert.IsType<KeyValuePair<string, object>[]>(dict._arrayStorage);
}

[Fact]
public void Remove_KeyAndOutValue_ListStorage_KeyExists_Middle()
{
// Arrange
object value = "value";
var dict = new RouteValueDictionary()
{
{ "other", 5 },
{ "key", value },
{ "dotnet", "rocks" }
};

// Act
var result = dict.Remove("key", out var removedValue);

// Assert
Assert.True(result);
Assert.Same(value, removedValue);
Assert.Equal(2, dict.Count);
Assert.False(dict.ContainsKey("key"));
Assert.True(dict.ContainsKey("other"));
Assert.True(dict.ContainsKey("dotnet"));
Assert.IsType<KeyValuePair<string, object>[]>(dict._arrayStorage);
}

[Fact]
public void Remove_KeyAndOutValue_ListStorage_KeyExists_Last()
{
// Arrange
object value = "value";
var dict = new RouteValueDictionary()
{
{ "other", 5 },
{ "dotnet", "rocks" },
{ "key", value }
};

// Act
var result = dict.Remove("key", out var removedValue);

// Assert
Assert.True(result);
Assert.Same(value, removedValue);
Assert.Equal(2, dict.Count);
Assert.False(dict.ContainsKey("key"));
Assert.True(dict.ContainsKey("other"));
Assert.True(dict.ContainsKey("dotnet"));
Assert.IsType<KeyValuePair<string, object>[]>(dict._arrayStorage);
}

[Fact]
public void TryAdd_EmptyStringIsAllowed()
{
Expand Down