From d7f98bd562041645c212be1992c9f27aaba60dbd Mon Sep 17 00:00:00 2001 From: Pranav K Date: Fri, 14 Feb 2020 08:34:44 -0800 Subject: [PATCH] Use reference equality to compare model instances in EditContext (#18172) (#18649) * Use reference equality to compare model instances in EditContext Fixes https://github.com/aspnet/AspNetCore/issues/18069 --- src/Components/Forms/src/FieldIdentifier.cs | 16 +++++-- src/Components/Forms/test/EditContextTest.cs | 30 +++++++++++++ .../Forms/test/FieldIdentifierTest.cs | 42 +++++++++++++++++++ 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/Components/Forms/src/FieldIdentifier.cs b/src/Components/Forms/src/FieldIdentifier.cs index 1c2c923d38..41fe513a43 100644 --- a/src/Components/Forms/src/FieldIdentifier.cs +++ b/src/Components/Forms/src/FieldIdentifier.cs @@ -3,6 +3,7 @@ using System; using System.Linq.Expressions; +using System.Runtime.CompilerServices; namespace Microsoft.AspNetCore.Components.Forms { @@ -35,7 +36,7 @@ namespace Microsoft.AspNetCore.Components.Forms /// The name of the editable field. public FieldIdentifier(object model, string fieldName) { - if (model == null) + if (model is null) { throw new ArgumentNullException(nameof(model)); } @@ -64,7 +65,16 @@ namespace Microsoft.AspNetCore.Components.Forms /// public override int GetHashCode() - => (Model, FieldName).GetHashCode(); + { + // We want to compare Model instances by reference. RuntimeHelpers.GetHashCode returns identical hashes for equal object references (ignoring any `Equals`/`GetHashCode` overrides) which is what we want. + var modelHash = RuntimeHelpers.GetHashCode(Model); + var fieldHash = StringComparer.Ordinal.GetHashCode(FieldName); + return ( + modelHash, + fieldHash + ) + .GetHashCode(); + } /// public override bool Equals(object obj) @@ -75,7 +85,7 @@ namespace Microsoft.AspNetCore.Components.Forms public bool Equals(FieldIdentifier otherIdentifier) { return - otherIdentifier.Model == Model && + ReferenceEquals(otherIdentifier.Model, Model) && string.Equals(otherIdentifier.FieldName, FieldName, StringComparison.Ordinal); } diff --git a/src/Components/Forms/test/EditContextTest.cs b/src/Components/Forms/test/EditContextTest.cs index 5c8e7af36e..e26bda2c7f 100644 --- a/src/Components/Forms/test/EditContextTest.cs +++ b/src/Components/Forms/test/EditContextTest.cs @@ -236,5 +236,35 @@ namespace Microsoft.AspNetCore.Components.Forms Assert.False(isValid); Assert.Equal(new[] { "Some message" }, editContext.GetValidationMessages()); } + + [Fact] + public void LookingUpModel_ThatOverridesGetHashCodeAndEquals_Works() + { + // Test for https://github.com/aspnet/AspNetCore/issues/18069 + // Arrange + var model = new EquatableModel(); + var editContext = new EditContext(model); + + editContext.NotifyFieldChanged(editContext.Field(nameof(EquatableModel.Property))); + + model.Property = "new value"; + + Assert.True(editContext.IsModified(editContext.Field(nameof(EquatableModel.Property)))); + } + + class EquatableModel : IEquatable + { + public string Property { get; set; } = ""; + + public bool Equals(EquatableModel other) + { + return string.Equals(Property, other?.Property, StringComparison.Ordinal); + } + + public override int GetHashCode() + { + return StringComparer.Ordinal.GetHashCode(Property); + } + } } } diff --git a/src/Components/Forms/test/FieldIdentifierTest.cs b/src/Components/Forms/test/FieldIdentifierTest.cs index f19751a45d..6192017b59 100644 --- a/src/Components/Forms/test/FieldIdentifierTest.cs +++ b/src/Components/Forms/test/FieldIdentifierTest.cs @@ -76,6 +76,19 @@ namespace Microsoft.AspNetCore.Components.Forms Assert.False(fieldIdentifier1.Equals(fieldIdentifier2)); } + [Fact] + public void FieldIdentifier_ForModelWithoutField_ProduceSameHashCodesAndEquality() + { + // Arrange + var model = new object(); + var fieldIdentifier1 = new FieldIdentifier(model, fieldName: string.Empty); + var fieldIdentifier2 = new FieldIdentifier(model, fieldName: string.Empty); + + // Act/Assert + Assert.Equal(fieldIdentifier1.GetHashCode(), fieldIdentifier2.GetHashCode()); + Assert.True(fieldIdentifier1.Equals(fieldIdentifier2)); + } + [Fact] public void SameContentsProduceSameHashCodesAndEquality() { @@ -89,6 +102,20 @@ namespace Microsoft.AspNetCore.Components.Forms Assert.True(fieldIdentifier1.Equals(fieldIdentifier2)); } + [Fact] + public void SameContents_WithOverridenEqualsAndGetHashCode_ProduceSameHashCodesAndEquality() + { + // Arrange + var model = new EquatableModel(); + var fieldIdentifier1 = new FieldIdentifier(model, nameof(EquatableModel.Property)); + model.Property = "changed value"; // To show it makes no difference if the overridden `GetHashCode` result changes + var fieldIdentifier2 = new FieldIdentifier(model, nameof(EquatableModel.Property)); + + // Act/Assert + Assert.Equal(fieldIdentifier1.GetHashCode(), fieldIdentifier2.GetHashCode()); + Assert.True(fieldIdentifier1.Equals(fieldIdentifier2)); + } + [Fact] public void FieldNamesAreCaseSensitive() { @@ -194,5 +221,20 @@ namespace Microsoft.AspNetCore.Components.Forms { public TestModel Child { get; set; } } + + class EquatableModel : IEquatable + { + public string Property { get; set; } = ""; + + public bool Equals(EquatableModel other) + { + return string.Equals(Property, other?.Property, StringComparison.Ordinal); + } + + public override int GetHashCode() + { + return StringComparer.Ordinal.GetHashCode(Property); + } + } } }