From e91e94da53fb4974d3ede2b7d35866aed330cfc2 Mon Sep 17 00:00:00 2001 From: Mackinnon Buck Date: Wed, 17 Jan 2024 13:25:06 -0800 Subject: [PATCH] [Blazor] Allow `null` parameter values to be supplied to interactive components via enhanced page update (#53317) (#53342) # Allow `null` parameter values to be supplied to interactive components via enhanced page update Backport of https://github.com/dotnet/aspnetcore/pull/53317 Fixes an issue where a `NullRefrenceException` would be thrown if an enhanced page update supplied a `null` parameter to an interactive root component. ## Description In .NET 8, SSR'd components can supply updated parameters to existing interactive root components. If one of those updated parameters is `null`, an exception currently gets thrown from within the framework. This causes the circuit to crash when using Server interactivity, and it would causes an error to be logged in the browser console when using WebAssembly interactivity. This PR fixes the problem by treating `null` as a valid value for a serialized parameter that gets supplied to an interactive root component. Fixes #52434 ## Customer Impact Without this fix, customers may encounter the unfriendly exception and have a hard time figuring out the underlying cause. We have not yet seen customer reports of the issue, but it's possible that customers have this bug in their apps without knowing it, especially since it only occurs when supplying updated parameters to existing components (not when supplying the initial set of parameters). One workaround would be to use a different value than `null` to specify an empty parameter value, but this may not be possible in cases where the parameter gets supplied by the framework (e.g., via route value), or if the interactive root component's implementation is not under the developer's control. ## Regression? - [ ] Yes - [X] No Only applicable to new scenarios in .NET 8. ## Risk - [ ] High - [ ] Medium - [X] Low The fix is straightforward and well-tested. ## Verification - [x] Manual (required) - [x] Automated ## Packaging changes reviewed? - [ ] Yes - [ ] No - [X] N/A --- .../test/WebRootComponentParametersTest.cs | 55 ++++++++++++++++--- .../Shared/src/WebRootComponentParameters.cs | 14 ++++- .../EnhancedNavigationTest.cs | 32 +++++++++++ ...eRenderingComponentWithNullParameter.razor | 24 ++++++++ .../Shared/EnhancedNavLayout.razor | 2 + .../ComponentAcceptingNullParameter.razor | 52 ++++++++++++++++++ 6 files changed, 167 insertions(+), 12 deletions(-) create mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/EnhancedNav/PageRenderingComponentWithNullParameter.razor create mode 100644 src/Components/test/testassets/TestContentPackage/ComponentAcceptingNullParameter.razor diff --git a/src/Components/Endpoints/test/WebRootComponentParametersTest.cs b/src/Components/Endpoints/test/WebRootComponentParametersTest.cs index 624a8d609b41..e18c6e556c8c 100644 --- a/src/Components/Endpoints/test/WebRootComponentParametersTest.cs +++ b/src/Components/Endpoints/test/WebRootComponentParametersTest.cs @@ -100,36 +100,73 @@ public void WebRootComponentParameters_DefinitelyEquals_ReturnsTrue_ForEmptySetO } [Fact] - public void WebRootComponentParameters_DefinitelyEquals_Throws_WhenComparingNonJsonElementParameterToJsonElement() + public void WebRootComponentParameters_DefinitelyEquals_ReturnsFalse_WhenComparingNonJsonElementParameterToJsonElement() { // Arrange var parameters1 = CreateParametersWithNonJsonElements(new() { ["First"] = 123 }); var parameters2 = CreateParameters(new() { ["First"] = 456 }); - // Act/assert - Assert.Throws(() => parameters1.DefinitelyEquals(parameters2)); + // Act + var result = parameters1.DefinitelyEquals(parameters2); + + // Assert + Assert.False(result); } [Fact] - public void WebRootComponentParameters_DefinitelyEquals_Throws_WhenComparingJsonElementParameterToNonJsonElement() + public void WebRootComponentParameters_DefinitelyEquals_ReturnsFalse_WhenComparingJsonElementParameterToNonJsonElement() { // Arrange var parameters1 = CreateParameters(new() { ["First"] = 123 }); var parameters2 = CreateParametersWithNonJsonElements(new() { ["First"] = 456 }); - // Act/assert - Assert.Throws(() => parameters1.DefinitelyEquals(parameters2)); + // Act + var result = parameters1.DefinitelyEquals(parameters2); + + // Assert + Assert.False(result); + } + + [Fact] + public void WebRootComponentParameters_DefinitelyEquals_ReturnsTrue_WhenComparingEqualNonJsonElementParameters() + { + // Arrange + var parameters1 = CreateParametersWithNonJsonElements(new() { ["First"] = 123 }); + var parameters2 = CreateParametersWithNonJsonElements(new() { ["First"] = 123 }); + + // Act + var result = parameters1.DefinitelyEquals(parameters2); + + // Assert + Assert.True(result); } [Fact] - public void WebRootComponentParameters_DefinitelyEquals_Throws_WhenComparingNonJsonElementParameters() + public void WebRootComponentParameters_DefinitelyEquals_ReturnsFalse_WhenComparingInequalNonJsonElementParameters() { // Arrange var parameters1 = CreateParametersWithNonJsonElements(new() { ["First"] = 123 }); var parameters2 = CreateParametersWithNonJsonElements(new() { ["First"] = 456 }); - // Act/assert - Assert.Throws(() => parameters1.DefinitelyEquals(parameters2)); + // Act + var result = parameters1.DefinitelyEquals(parameters2); + + // Assert + Assert.False(result); + } + + [Fact] + public void WebRootComponentParameters_DefinitelyEquals_ReturnsTrue_WhenComparingNullParameters() + { + // Arrange + var parameters1 = CreateParametersWithNonJsonElements(new() { ["First"] = null }); + var parameters2 = CreateParametersWithNonJsonElements(new() { ["First"] = null }); + + // Act + var result = parameters1.DefinitelyEquals(parameters2); + + // Assert + Assert.True(result); } private static WebRootComponentParameters CreateParameters(Dictionary parameters) diff --git a/src/Components/Shared/src/WebRootComponentParameters.cs b/src/Components/Shared/src/WebRootComponentParameters.cs index 9e93ce924b37..8f07e8fb524e 100644 --- a/src/Components/Shared/src/WebRootComponentParameters.cs +++ b/src/Components/Shared/src/WebRootComponentParameters.cs @@ -45,9 +45,17 @@ public bool DefinitelyEquals(in WebRootComponentParameters other) return false; } - var value = ((JsonElement)_serializedParameterValues[i]).GetRawText(); - var otherValue = ((JsonElement)other._serializedParameterValues[i]).GetRawText(); - if (!string.Equals(value, otherValue, StringComparison.Ordinal)) + // We expect each serialized parameter value to be either a 'JsonElement' or 'null'. + var value = _serializedParameterValues[i]; + var otherValue = other._serializedParameterValues[i]; + if (value is JsonElement jsonValue && otherValue is JsonElement otherJsonValue) + { + if (!string.Equals(jsonValue.GetRawText(), otherJsonValue.GetRawText(), StringComparison.Ordinal)) + { + return false; + } + } + else if (!Equals(value, otherValue)) { return false; } diff --git a/src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTest.cs b/src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTest.cs index f7d9e031ce95..7ed178425cfc 100644 --- a/src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTest.cs +++ b/src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTest.cs @@ -554,6 +554,38 @@ public void LocationChangingEventGetsInvokedOnEnhancedNavigationOnlyForRuntimeTh Browser.Equal("0", () => Browser.Exists(By.Id($"location-changing-count-{anotherRuntime}")).Text); } + [Theory] + [InlineData("server")] + [InlineData("wasm")] + public void CanReceiveNullParameterValueOnEnhancedNavigation(string renderMode) + { + // See: https://github.com/dotnet/aspnetcore/issues/52434 + Navigate($"{ServerPathBase}/nav"); + Browser.Equal("Hello", () => Browser.Exists(By.TagName("h1")).Text); + + Browser.Exists(By.TagName("nav")).FindElement(By.LinkText($"Null component parameter ({renderMode})")).Click(); + Browser.Equal("Page rendering component with null parameter", () => Browser.Exists(By.TagName("h1")).Text); + Browser.Equal("0", () => Browser.Exists(By.Id("current-count")).Text); + + Browser.Exists(By.Id("button-increment")).Click(); + Browser.Equal("0", () => Browser.Exists(By.Id("location-changed-count")).Text); + Browser.Equal("1", () => Browser.Exists(By.Id("current-count")).Text); + + // This refresh causes the interactive component to receive a 'null' parameter value + Browser.Exists(By.Id("button-refresh")).Click(); + Browser.Equal("1", () => Browser.Exists(By.Id("location-changed-count")).Text); + Browser.Equal("1", () => Browser.Exists(By.Id("current-count")).Text); + + // Increment the count again to ensure that interactivity still works + Browser.Exists(By.Id("button-increment")).Click(); + Browser.Equal("2", () => Browser.Exists(By.Id("current-count")).Text); + + // Even if the interactive runtime continues to function (as the WebAssembly runtime might), + // fail the test if any errors were logged to the browser console + var logs = Browser.GetBrowserLogs(LogLevel.Warning); + Assert.DoesNotContain(logs, log => log.Message.Contains("Error")); + } + private void AssertEnhancedUpdateCountEquals(long count) => Browser.Equal(count, () => ((IJavaScriptExecutor)Browser).ExecuteScript("return window.enhancedPageUpdateCount;")); diff --git a/src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/EnhancedNav/PageRenderingComponentWithNullParameter.razor b/src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/EnhancedNav/PageRenderingComponentWithNullParameter.razor new file mode 100644 index 000000000000..5603f0a2a3a3 --- /dev/null +++ b/src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/EnhancedNav/PageRenderingComponentWithNullParameter.razor @@ -0,0 +1,24 @@ +@page "/nav/null-parameter/{mode}" +@using TestContentPackage + +@* https://github.com/dotnet/aspnetcore/issues/52434 *@ + +

Page rendering component with null parameter

+ +@if (Mode == "server") +{ + +} +else if (Mode == "wasm") +{ + +} +else +{ +

Expected a render mode of 'server' or 'wasm', but got '@Mode'.

+} + +@code { + [Parameter] + public string? Mode { get; set; } +} diff --git a/src/Components/test/testassets/Components.TestServer/RazorComponents/Shared/EnhancedNavLayout.razor b/src/Components/test/testassets/Components.TestServer/RazorComponents/Shared/EnhancedNavLayout.razor index 5362f567049e..d6c0928d658f 100644 --- a/src/Components/test/testassets/Components.TestServer/RazorComponents/Shared/EnhancedNavLayout.razor +++ b/src/Components/test/testassets/Components.TestServer/RazorComponents/Shared/EnhancedNavLayout.razor @@ -23,6 +23,8 @@ LocationChanged/LocationChanging event (server) LocationChanged/LocationChanging event (wasm) LocationChanged/LocationChanging event (server-and-wasm) + Null component parameter (server) + Null component parameter (wasm)
diff --git a/src/Components/test/testassets/TestContentPackage/ComponentAcceptingNullParameter.razor b/src/Components/test/testassets/TestContentPackage/ComponentAcceptingNullParameter.razor new file mode 100644 index 000000000000..404f82c5fa3e --- /dev/null +++ b/src/Components/test/testassets/TestContentPackage/ComponentAcceptingNullParameter.razor @@ -0,0 +1,52 @@ +@implements IDisposable +@inject NavigationManager NavigationManager +@using Microsoft.AspNetCore.Components.Routing + +

Value: @(Value ?? "(null)")

+ +@if (_interactive) +{ + + +

Location changed count: @_locationChangedCount

+} + +@code { + private bool _interactive; + private int _count; + private int _locationChangedCount; + + [Parameter] + public string Value { get; set; } + + protected override void OnAfterRender(bool firstRender) + { + if (firstRender) + { + NavigationManager.LocationChanged += OnLocationChanged; + _interactive = true; + StateHasChanged(); + } + } + + private void OnLocationChanged(object sender, LocationChangedEventArgs e) + { + _locationChangedCount++; + StateHasChanged(); + } + + private void Increment() + { + _count++; + } + + private void Refresh() + { + NavigationManager.Refresh(); + } + + public void Dispose() + { + NavigationManager.LocationChanged -= OnLocationChanged; + } +}