Skip to content

Use reference equality to compare model instances in EditContext #18172

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 8, 2020
Merged

Use reference equality to compare model instances in EditContext #18172

merged 2 commits into from
Jan 8, 2020

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Jan 7, 2020

Fixes #18069

@pranavkm pranavkm requested a review from rynowak January 7, 2020 19:43
@github-actions github-actions bot added the area-blazor Includes: Blazor, Razor Components label Jan 7, 2020
@pranavkm pranavkm force-pushed the prkrishn/reference-equality branch from de32762 to dc94234 Compare January 7, 2020 19:48
pranavkm
pranavkm commented Jan 7, 2020
View reviewed changes
src/Components/Forms/src/FieldIdentifier.cs
@@ -35,7 +38,7 @@ public static FieldIdentifier Create<TField>(Expression<Func<TField>> accessor)
/// <param name="fieldName">The name of the editable field.</param>
public FieldIdentifier(object model, string fieldName)
{
if (model == null)
if (model is null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case a user type has a poorly implemented equality operator.

src/Components/Forms/src/FieldIdentifier.cs
{
// Ww want to compare Model instances by reference RuntimeHelpers.GetHashCode returns identical hashes for equal object references which is what we want.
var modelHash = RuntimeHelpers.GetHashCode(Model);
var fieldHash = StringComparer.Ordinal.GetHashCode(FieldName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to use the same Comparer used by Equals

SteveSandersonMS
SteveSandersonMS reviewed Jan 8, 2020
View reviewed changes
SteveSandersonMS
SteveSandersonMS approved these changes Jan 8, 2020
View reviewed changes
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Looks great!

@pranavkm pranavkm merged commit 66f4e91 into master Jan 8, 2020
@pranavkm pranavkm deleted the prkrishn/reference-equality branch January 8, 2020 20:38
@pranavkm pranavkm added this to the 5.0.0-preview1 milestone Jan 8, 2020
@rynowak
Copy link
Member

rynowak commented Jan 9, 2020

:shipit:

pranavkm added a commit that referenced this pull request Jan 28, 2020
@pranavkm
Use reference equality to compare model instances in EditContext (#18172
a109301 
)

* Use reference equality to compare model instances in EditContext

Fixes #18069
mkArtakMSFT pushed a commit that referenced this pull request Feb 14, 2020
@pranavkm
Use reference equality to compare model instances in EditContext (#18172
d7f98bd 
) (#18649)

* Use reference equality to compare model instances in EditContext

Fixes #18069
@aaronhudon
Copy link

@pranavkm when will this bugfix be available ?

@pranavkm
Copy link
Contributor Author

@aaronhudon the change was ported to the 3.1.3 release as part of #18649. 3.1.3 is scheduled to be released mid-March.

@aaronhudon
Copy link

@pranavkm I have installed the 3.1.3 runtime, and SDK. Unfortunately I'm not seeing a change in behavior (e.g. the bug isn't fixed). Are there any additional project settings that need to be applied? I'm assuming no.

@pranavkm
Copy link
Contributor Author

@aaronhudon are you building a Blazor Server project?

@aaronhudon
Copy link

@aaronhudon are you building a Blazor Server project?

Yes

@aaronhudon
Copy link

@pranavkm Any updates on this?

@SteveSandersonMS
Copy link
Member

This is fixed in master and so will ship in the 5.0 releases (e.g., https://devblogs.microsoft.com/aspnet/asp-net-core-updates-in-net-5-preview-1/)

@aaronhudon
Copy link

This is fixed in master and so will ship in the 5.0 releases (e.g., https://devblogs.microsoft.com/aspnet/asp-net-core-updates-in-net-5-preview-1/)

So it did not make it in to 3.1.103 ?

@pranavkm
Copy link
Contributor Author

@SteveSandersonMS we ported the fix to 3.1.3. @aaronhudon are you certain you have Microsoft.AspNetCore.App 3.1.3 installed on your box? If you're still seeing the issue, could you file a new issue so we can track it?

@aaronhudon
Copy link

This is what dotnet --list-runtimes is showing me. The behavior still has not changed when using protocol buffers as models. I can dig up my original repro and submit it to you again.

PM> dotnet --list-runtimes
Microsoft.AspNetCore.All 2.1.16 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.App 2.1.16 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 2.1.16 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 3.1.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

@aaronhudon
Copy link

@pranavkm !#%^%^ My project file had <RuntimeIdentifier>win-x64</RuntimeIdentifier> which was forcing the runtime to 3.1.

Removed, and the fix is fantastic. Thanks again!

@pranavkm
Copy link
Contributor Author

@aaronhudon you might have an older SDK. The version of the .NET runtime for self contained deployment is determined by the version of the SDK. Using 3.1.103 should get you the right version, but you want to make sure you're not using a different preview.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

@rynowak rynowak Awaiting requested review from rynowak

Assignees
No one assigned
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Milestone
5.0.0-preview1
Development

Successfully merging this pull request may close these issues.

Blazor: EditContext fieldstate tracking falls apart when model implements GetHashCode
4 participants
@pranavkm @rynowak @aaronhudon @SteveSandersonMS