Skip to content

Commit 6a6e21d

Browse files
halter73MackinnonBuck
halter73
and
MackinnonBuck
authored
Fix stale values from [SupplyParameterFromQuery] (#53443)
Co-authored-by: Mackinnon Buck <[email protected]>
1 parent 7ca0f65 commit 6a6e21d

File tree

7 files changed

+212
-121
lines changed

7 files changed

+212
-121
lines changed

‎src/Components/Components/src/Routing/QueryParameterValueSupplier.cs

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,4 @@ public void ReadParametersFromQuery(ReadOnlyMemory<char> query)
5151

5252
return default;
5353
}
54-
55-
public static bool CanSupplyValueForType(Type targetType)
56-
{
57-
var elementType = targetType.IsArray ? targetType.GetElementType()! : targetType;
58-
return UrlValueConstraint.TryGetByTargetType(elementType, out _);
59-
}
6054
}

‎src/Components/Components/src/Routing/SupplyParameterFromQueryValueProvider.cs

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System.Diagnostics;
5+
using System.Diagnostics.CodeAnalysis;
6+
using Microsoft.AspNetCore.Components.Rendering;
7+
8+
namespace Microsoft.AspNetCore.Components.Routing;
9+
10+
internal sealed class SupplyParameterFromQueryValueProvider(NavigationManager navigationManager) : ICascadingValueSupplier, IDisposable
11+
{
12+
private QueryParameterValueSupplier? _queryParameterValueSupplier;
13+
14+
private HashSet<ComponentState>? _subscribers;
15+
private HashSet<ComponentState>? _pendingSubscribers;
16+
17+
private string? _lastUri;
18+
private bool _isSubscribedToLocationChanges;
19+
private bool _queryChanged;
20+
21+
public bool IsFixed => false;
22+
23+
public bool CanSupplyValue(in CascadingParameterInfo parameterInfo)
24+
=> parameterInfo.Attribute is SupplyParameterFromQueryAttribute;
25+
26+
public object? GetCurrentValue(in CascadingParameterInfo parameterInfo)
27+
{
28+
TryUpdateUri();
29+
30+
var attribute = (SupplyParameterFromQueryAttribute)parameterInfo.Attribute; // Must be a valid cast because we check in CanSupplyValue
31+
var queryParameterName = attribute.Name ?? parameterInfo.PropertyName;
32+
return _queryParameterValueSupplier.GetQueryParameterValue(parameterInfo.PropertyType, queryParameterName);
33+
}
34+
35+
public void Subscribe(ComponentState subscriber, in CascadingParameterInfo parameterInfo)
36+
{
37+
if (_pendingSubscribers?.Count > 0 || (TryUpdateUri() && _isSubscribedToLocationChanges))
38+
{
39+
// Renderer.RenderInExistingBatch triggers Unsubscribe via ProcessDisposalQueueInExistingBatch after subscribing with any new components,
40+
// so this branch should be taken iff there's a pending OnLocationChanged event for the current Uri that we're already subscribed to.
41+
_pendingSubscribers ??= new();
42+
_pendingSubscribers.Add(subscriber);
43+
return;
44+
}
45+
46+
_subscribers ??= new();
47+
_subscribers.Add(subscriber);
48+
SubscribeToLocationChanges();
49+
}
50+
51+
public void Unsubscribe(ComponentState subscriber, in CascadingParameterInfo parameterInfo)
52+
{
53+
// ICascadingValueSupplier is internal, and Subscribe should always precede Unsubscribe.
54+
Debug.Assert(_subscribers is not null);
55+
_subscribers.Remove(subscriber);
56+
_pendingSubscribers?.Remove(subscriber);
57+
58+
if (_subscribers.Count == 0 && _pendingSubscribers is null or { Count: 0 })
59+
{
60+
UnsubscribeFromLocationChanges();
61+
}
62+
}
63+
64+
[MemberNotNull(nameof(_queryParameterValueSupplier))]
65+
private bool TryUpdateUri()
66+
{
67+
_queryParameterValueSupplier ??= new();
68+
69+
// NavigationManager triggers Router.OnLocationChanged which calls GetCurrentValue before this class's OnLocationHandler
70+
// gets a chance to run, so we have to compare strings rather than rely on OnLocationChanged always running before Uri updates.
71+
if (navigationManager.Uri == _lastUri)
72+
{
73+
return false;
74+
}
75+
76+
var query = GetQueryString(navigationManager.Uri);
77+
78+
if (!query.Span.SequenceEqual(GetQueryString(_lastUri).Span))
79+
{
80+
_queryChanged = true;
81+
_queryParameterValueSupplier.ReadParametersFromQuery(query);
82+
}
83+
84+
_lastUri = navigationManager.Uri;
85+
return true;
86+
87+
static ReadOnlyMemory<char> GetQueryString(string? url)
88+
{
89+
var queryStartPos = url?.IndexOf('?') ?? -1;
90+
91+
if (queryStartPos < 0)
92+
{
93+
return default;
94+
}
95+
96+
Debug.Assert(url is not null);
97+
var queryEndPos = url.IndexOf('#', queryStartPos);
98+
return url.AsMemory(queryStartPos..(queryEndPos < 0 ? url.Length : queryEndPos));
99+
}
100+
}
101+
102+
private void SubscribeToLocationChanges()
103+
{
104+
if (_isSubscribedToLocationChanges)
105+
{
106+
return;
107+
}
108+
109+
_isSubscribedToLocationChanges = true;
110+
navigationManager.LocationChanged += OnLocationChanged;
111+
}
112+
113+
private void UnsubscribeFromLocationChanges()
114+
{
115+
if (!_isSubscribedToLocationChanges)
116+
{
117+
return;
118+
}
119+
120+
_isSubscribedToLocationChanges = false;
121+
navigationManager.LocationChanged -= OnLocationChanged;
122+
}
123+
124+
private void OnLocationChanged(object? sender, LocationChangedEventArgs args)
125+
{
126+
Debug.Assert(_subscribers is not null);
127+
128+
TryUpdateUri();
129+
130+
if (_queryChanged)
131+
{
132+
foreach (var subscriber in _subscribers)
133+
{
134+
subscriber.NotifyCascadingValueChanged(ParameterViewLifetime.Unbound);
135+
}
136+
137+
_queryChanged = false;
138+
}
139+
140+
if (_pendingSubscribers is not null)
141+
{
142+
foreach (var subscriber in _pendingSubscribers)
143+
{
144+
_subscribers.Add(subscriber);
145+
}
146+
147+
_pendingSubscribers.Clear();
148+
}
149+
}
150+
151+
public void Dispose() => UnsubscribeFromLocationChanges();
152+
}

‎src/Components/Components/src/SupplyParameterFromQueryProviderServiceCollectionExtensions.cs

Lines changed: 1 addition & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
using Microsoft.AspNetCore.Components.Rendering;
54
using Microsoft.AspNetCore.Components.Routing;
65
using Microsoft.Extensions.DependencyInjection;
76
using Microsoft.Extensions.DependencyInjection.Extensions;
@@ -20,118 +19,7 @@ public static class SupplyParameterFromQueryProviderServiceCollectionExtensions
2019
/// <returns>The <see cref="IServiceCollection"/>.</returns>
2120
public static IServiceCollection AddSupplyValueFromQueryProvider(this IServiceCollection services)
2221
{
23-
services.TryAddEnumerable(ServiceDescriptor.Scoped<ICascadingValueSupplier, SupplyValueFromQueryValueProvider>());
22+
services.TryAddEnumerable(ServiceDescriptor.Scoped<ICascadingValueSupplier, SupplyParameterFromQueryValueProvider>());
2423
return services;
2524
}
26-
27-
internal sealed class SupplyValueFromQueryValueProvider : ICascadingValueSupplier, IDisposable
28-
{
29-
private readonly QueryParameterValueSupplier _queryParameterValueSupplier = new();
30-
private readonly NavigationManager _navigationManager;
31-
private HashSet<ComponentState>? _subscribers;
32-
private bool _isSubscribedToLocationChanges;
33-
private bool _queryParametersMightHaveChanged = true;
34-
35-
public SupplyValueFromQueryValueProvider(NavigationManager navigationManager)
36-
{
37-
_navigationManager = navigationManager;
38-
}
39-
40-
public bool IsFixed => false;
41-
42-
public bool CanSupplyValue(in CascadingParameterInfo parameterInfo)
43-
=> parameterInfo.Attribute is SupplyParameterFromQueryAttribute;
44-
45-
public object? GetCurrentValue(in CascadingParameterInfo parameterInfo)
46-
{
47-
if (_queryParametersMightHaveChanged)
48-
{
49-
_queryParametersMightHaveChanged = false;
50-
UpdateQueryParameters();
51-
}
52-
53-
var attribute = (SupplyParameterFromQueryAttribute)parameterInfo.Attribute; // Must be a valid cast because we check in CanSupplyValue
54-
var queryParameterName = attribute.Name ?? parameterInfo.PropertyName;
55-
return _queryParameterValueSupplier.GetQueryParameterValue(parameterInfo.PropertyType, queryParameterName);
56-
}
57-
58-
public void Subscribe(ComponentState subscriber, in CascadingParameterInfo parameterInfo)
59-
{
60-
SubscribeToLocationChanges();
61-
62-
_subscribers ??= new();
63-
_subscribers.Add(subscriber);
64-
}
65-
66-
public void Unsubscribe(ComponentState subscriber, in CascadingParameterInfo parameterInfo)
67-
{
68-
_subscribers!.Remove(subscriber);
69-
70-
if (_subscribers.Count == 0)
71-
{
72-
UnsubscribeFromLocationChanges();
73-
}
74-
}
75-
76-
private void UpdateQueryParameters()
77-
{
78-
var query = GetQueryString(_navigationManager.Uri);
79-
80-
_queryParameterValueSupplier.ReadParametersFromQuery(query);
81-
82-
static ReadOnlyMemory<char> GetQueryString(string url)
83-
{
84-
var queryStartPos = url.IndexOf('?');
85-
86-
if (queryStartPos < 0)
87-
{
88-
return default;
89-
}
90-
91-
var queryEndPos = url.IndexOf('#', queryStartPos);
92-
return url.AsMemory(queryStartPos..(queryEndPos < 0 ? url.Length : queryEndPos));
93-
}
94-
}
95-
96-
private void SubscribeToLocationChanges()
97-
{
98-
if (_isSubscribedToLocationChanges)
99-
{
100-
return;
101-
}
102-
103-
_isSubscribedToLocationChanges = true;
104-
_queryParametersMightHaveChanged = true;
105-
_navigationManager.LocationChanged += OnLocationChanged;
106-
}
107-
108-
private void UnsubscribeFromLocationChanges()
109-
{
110-
if (!_isSubscribedToLocationChanges)
111-
{
112-
return;
113-
}
114-
115-
_isSubscribedToLocationChanges = false;
116-
_navigationManager.LocationChanged -= OnLocationChanged;
117-
}
118-
119-
private void OnLocationChanged(object? sender, LocationChangedEventArgs args)
120-
{
121-
_queryParametersMightHaveChanged = true;
122-
123-
if (_subscribers is not null)
124-
{
125-
foreach (var subscriber in _subscribers)
126-
{
127-
subscriber.NotifyCascadingValueChanged(ParameterViewLifetime.Unbound);
128-
}
129-
}
130-
}
131-
132-
public void Dispose()
133-
{
134-
UnsubscribeFromLocationChanges();
135-
}
136-
}
13725
}

‎src/Components/Endpoints/test/RazorComponentsServiceCollectionExtensionsTest.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using Microsoft.AspNetCore.Components;
55
using Microsoft.AspNetCore.Components.Forms.Mapping;
6+
using Microsoft.AspNetCore.Components.Routing;
67
using Microsoft.AspNetCore.Hosting;
78
using Microsoft.Extensions.Configuration;
89
using Microsoft.Extensions.FileProviders;
@@ -90,7 +91,7 @@ private Dictionary<Type, Type[]> MultiRegistrationServiceTypes
9091
[typeof(ICascadingValueSupplier)] = new[]
9192
{
9293
typeof(SupplyParameterFromFormValueProvider),
93-
typeof(SupplyParameterFromQueryProviderServiceCollectionExtensions.SupplyValueFromQueryValueProvider),
94+
typeof(SupplyParameterFromQueryValueProvider),
9495
}
9596
};
9697
}

‎src/Components/test/E2ETest/Tests/RoutingTest.cs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1537,7 +1537,7 @@ public void CanNavigateToQueryStringPageWithNoQuery()
15371537
}
15381538

15391539
[Fact]
1540-
public void CanNavigateBetweenPagesWithQueryStrings()
1540+
public void CanNavigateWithinPageWithQueryStrings()
15411541
{
15421542
SetUrlViaPushState("/");
15431543

@@ -1586,6 +1586,30 @@ public void CanNavigateBetweenPagesWithQueryStrings()
15861586
AssertHighlightedLinks("With query parameters (none)", "With query parameters (passing string value)");
15871587
}
15881588

1589+
[Fact]
1590+
public void CanNavigateBetweenDifferentPagesWithQueryStrings()
1591+
{
1592+
SetUrlViaPushState("/");
1593+
1594+
// Navigate between pages with the same querystring parameter "l" for LongValues and GuidValue.
1595+
// https://github.com/dotnet/aspnetcore/issues/52483
1596+
var app = Browser.MountTestComponent<TestRouter>();
1597+
app.FindElement(By.LinkText("With query parameters (none)")).Click();
1598+
app.FindElement(By.LinkText("With IntValue and LongValues")).Click();
1599+
app.FindElement(By.LinkText("Another page with GuidValue")).Click();
1600+
1601+
Browser.Equal("8b7ae9ee-de22-4dd0-8fa1-b31e66abcc79", () => app.FindElement(By.Id("value-QueryGuid")).Text);
1602+
// Verify that OnParametersSet was only called once.
1603+
Browser.Equal("1", () => app.FindElement(By.Id("param-set-count")).Text);
1604+
1605+
app.FindElement(By.LinkText("Another page with LongValues")).Click();
1606+
Assert.Equal("3 values (50, 100, -20)", app.FindElement(By.Id("value-LongValues")).Text);
1607+
1608+
// We can also click back to go the preceding query while retaining the same component instance.
1609+
Browser.Navigate().Back();
1610+
Browser.Equal("8b7ae9ee-de22-4dd0-8fa1-b31e66abcc79", () => app.FindElement(By.Id("value-QueryGuid")).Text);
1611+
}
1612+
15891613
[Fact]
15901614
public void AnchorWithHrefContainingHashSamePage_ScrollsToElementOnTheSamePage()
15911615
{

‎src/Components/test/testassets/BasicTestApp/RouterTest/WithQueryGuidParameter.razor

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
@page "/WithQueryGuidParameter"
2+
3+
<p>GuidValue: <strong id="value-QueryGuid">@GuidValue</strong></p>
4+
<p>StringValue: <strong id="value-StringValue">@StringValue</strong></p>
5+
6+
<p>OnParametersSet count: <strong id="param-set-count">@onParametersSetCount</strong></p>
7+
8+
<p>
9+
Links:
10+
<a href="WithQueryGuidParameter?l=8b7ae9ee-de22-4dd0-8fa1-b31e66abcc79">With GuidValue</a> |
11+
<a href="WithQueryParameters/Abc?l=50&l=100&l=-20">Another page with LongValues</a> |
12+
<a href="WithQueryParameters/Abc?stringvalue=B">Another page with StringValue</a> |
13+
</p>
14+
15+
@code
16+
{
17+
private int onParametersSetCount;
18+
19+
// Use "l" as the query parameter name to test that it doesn't cause problems with
20+
// WithQueryParameters.LongValues which is also called "l".
21+
// https://github.com/dotnet/aspnetcore/issues/52483
22+
[SupplyParameterFromQuery(Name = "l")] public Guid GuidValue { get ; set; }
23+
24+
[SupplyParameterFromQuery] public string StringValue { get ; set; }
25+
26+
protected override void OnParametersSet()
27+
{
28+
onParametersSetCount++;
29+
}
30+
}

‎src/Components/test/testassets/BasicTestApp/RouterTest/WithQueryParameters.razor

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
Links:
1616
<a href="WithQueryParameters/@FirstName?intvalue=123">With IntValue</a> |
1717
<a href="WithQueryParameters/@FirstName?l=50&l=100&l=-20&intvalue=123">With IntValue and LongValues</a> |
18+
<a href="WithQueryGuidParameter?l=8b7ae9ee-de22-4dd0-8fa1-b31e66abcc79">Another page with GuidValue</a> |
19+
<a href="WithQueryGuidParameter?stringvalue=A">Another page with StringValue</a> |
1820
</p>
1921

2022
@code

0 commit comments

Comments
 (0)