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

Add strong and weak ETag comparisons #695

Merged
merged 1 commit into from
Aug 30, 2016
Merged

Add strong and weak ETag comparisons #695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 36 additions & 1 deletion src/Microsoft.Net.Http.Headers/EntityTagHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,15 @@ public override string ToString()
return _tag;
}

/// <summary>
/// Check against another <see cref="EntityTagHeaderValue"/> for equality.
/// This equality check should not be used to determine if two values match under the RFC specifications (https://tools.ietf.org/html/rfc7232#section-2.3.2).
/// </summary>
/// <param name="obj">The other value to check against for equality.</param>
/// <returns>
/// <c>true</c> if the strength and tag of the two values match,
/// <c>false</c> if the other value is null, is not an <see cref="EntityTagHeaderValue"/>, or if there is a mismatch of strength or tag between the two values.
/// </returns>
public override bool Equals(object obj)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason we were going in a roundabout way of comparing strings previously? Why CompareOrdinal instead of Equals?

Copy link
Member

Choose a reason for hiding this comment

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

If you're going to use string.Equals you still need to pass in Ordinal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is an ordinal comparison.

Copy link
Member

Choose a reason for hiding this comment

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

Always specify for string operations, the defaults are not consistant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting... How and when do they vary?

Copy link
Contributor

Choose a reason for hiding this comment

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

When then stars align ...

Also MSDN recommended use overload without default option.

We recommend that you select an overload that does not use default values, for the following reasons:

  • Some overloads with default parameters (those that search for a Char in the string instance) perform an ordinal comparison, whereas others (those that search for a string in the string instance) are culture-sensitive. It is difficult to remember which method uses which default value, and easy to confuse the overloads.
  • The intent of the code that relies on default values for method calls is not clear. In the following example, which relies on defaults, it is difficult to know whether the developer actually intended an ordinal or a linguistic comparison of two strings, or whether a case difference between protocol and "http" might cause the test for equality to return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fascinating, didn't know the defaults can vary in such ways. I guess it's always better to be on the safe side and just be explicit in what you want to do haha.

Copy link
Contributor

Choose a reason for hiding this comment

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

Being explicit greatly improves readability as it removes confusions.

var other = obj as EntityTagHeaderValue;
Expand All @@ -103,7 +112,7 @@ public override bool Equals(object obj)
}

// Since the tag is a quoted-string we treat it case-sensitive.
return ((_isWeak == other._isWeak) && (string.CompareOrdinal(_tag, other._tag) == 0));
return _isWeak == other._isWeak && string.Equals(_tag, other._tag, StringComparison.Ordinal);
}

public override int GetHashCode()
Expand All @@ -112,6 +121,32 @@ public override int GetHashCode()
return _tag.GetHashCode() ^ _isWeak.GetHashCode();
}

/// <summary>
/// Compares against another <see cref="EntityTagHeaderValue"/> to see if they match under the RFC specifications (https://tools.ietf.org/html/rfc7232#section-2.3.2).
/// </summary>
/// <param name="other">The other <see cref="EntityTagHeaderValue"/> to compare against.</param>
/// <param name="useStrongComparison"><c>true</c> to use a strong comparison, <c>false</c> to use a weak comparison</param>
/// <returns>
/// <c>true</c> if the <see cref="EntityTagHeaderValue"/> match for the given comparison type,
/// <c>false</c> if the other value is null or the comparison failed.
/// </returns>
public bool Compare(EntityTagHeaderValue other, bool useStrongComparison)
{
if (other == null)
{
return false;
}

if (useStrongComparison)
{
return !IsWeak && !other.IsWeak && string.Equals(Tag, other.Tag, StringComparison.Ordinal);
}
else
{
return string.Equals(Tag, other.Tag, StringComparison.Ordinal);
}
}

public static EntityTagHeaderValue Parse(string input)
{
var index = 0;
Expand Down
91 changes: 91 additions & 0 deletions test/Microsoft.Net.Http.Headers.Tests/EntityTagHeaderValueTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,97 @@ public void Equals_UseSameAndDifferentETags_EqualOrNotEqualNoExceptions()
Assert.True(etag1.Equals(etag5), "tag vs. tag..");
}

[Fact]
public void Compare_WithNull_ReturnsFalse()
{
Assert.False(EntityTagHeaderValue.Any.Compare(null, useStrongComparison: true));
Assert.False(EntityTagHeaderValue.Any.Compare(null, useStrongComparison: false));
}

public static TheoryData<EntityTagHeaderValue, EntityTagHeaderValue> NotEquivalentUnderStrongComparison
{
get
{
return new TheoryData<EntityTagHeaderValue, EntityTagHeaderValue>
{
{ new EntityTagHeaderValue("\"tag\""), new EntityTagHeaderValue("\"TAG\"") },
{ new EntityTagHeaderValue("\"tag\"", true), new EntityTagHeaderValue("\"tag\"", true) },
{ new EntityTagHeaderValue("\"tag\""), new EntityTagHeaderValue("\"tag\"", true) },
{ new EntityTagHeaderValue("\"tag\""), new EntityTagHeaderValue("\"tag1\"") },
{ new EntityTagHeaderValue("\"tag\""), EntityTagHeaderValue.Any },
};
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it theory so that all the cases are tested no matter how many of them are failed. In the current form, if the first assertion filed the following assertions won't be executed.


[Theory]
[MemberData(nameof(NotEquivalentUnderStrongComparison))]
public void CompareUsingStrongComparison_NonEquivalentPairs_ReturnFalse(EntityTagHeaderValue left, EntityTagHeaderValue right)
{
Assert.False(left.Compare(right, useStrongComparison: true));
Assert.False(right.Compare(left, useStrongComparison: true));
}

public static TheoryData<EntityTagHeaderValue, EntityTagHeaderValue> EquivalentUnderStrongComparison
{
get
{
return new TheoryData<EntityTagHeaderValue, EntityTagHeaderValue>
{
{ new EntityTagHeaderValue("\"tag\""), new EntityTagHeaderValue("\"tag\"") },
};
}
}

[Theory]
[MemberData(nameof(EquivalentUnderStrongComparison))]
public void CompareUsingStrongComparison_EquivalentPairs_ReturnTrue(EntityTagHeaderValue left, EntityTagHeaderValue right)
{
Assert.True(left.Compare(right, useStrongComparison: true));
Assert.True(right.Compare(left, useStrongComparison: true));
}

public static TheoryData<EntityTagHeaderValue, EntityTagHeaderValue> NotEquivalentUnderWeakComparison
{
get
{
return new TheoryData<EntityTagHeaderValue, EntityTagHeaderValue>
{
{ new EntityTagHeaderValue("\"tag\""), new EntityTagHeaderValue("\"TAG\"") },
{ new EntityTagHeaderValue("\"tag\""), new EntityTagHeaderValue("\"tag1\"") },
{ new EntityTagHeaderValue("\"tag\""), EntityTagHeaderValue.Any },
};
}
}

[Theory]
[MemberData(nameof(NotEquivalentUnderWeakComparison))]
public void CompareUsingWeakComparison_NonEquivalentPairs_ReturnFalse(EntityTagHeaderValue left, EntityTagHeaderValue right)
{
Assert.False(left.Compare(right, useStrongComparison: false));
Assert.False(right.Compare(left, useStrongComparison: false));
}

public static TheoryData<EntityTagHeaderValue, EntityTagHeaderValue> EquivalentUnderWeakComparison
{
get
{
return new TheoryData<EntityTagHeaderValue, EntityTagHeaderValue>
{
{ new EntityTagHeaderValue("\"tag\""), new EntityTagHeaderValue("\"tag\"") },
{ new EntityTagHeaderValue("\"tag\"", true), new EntityTagHeaderValue("\"tag\"", true) },
{ new EntityTagHeaderValue("\"tag\""), new EntityTagHeaderValue("\"tag\"", true) },
};
}
}

[Theory]
[MemberData(nameof(EquivalentUnderWeakComparison))]
public void CompareUsingWeakComparison_EquivalentPairs_ReturnTrue(EntityTagHeaderValue left, EntityTagHeaderValue right)
{
Assert.True(left.Compare(right, useStrongComparison: false));
Assert.True(right.Compare(left, useStrongComparison: false));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above


[Fact]
public void Parse_SetOfValidValueStrings_ParsedCorrectly()
{
Expand Down