dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.42k stars 10k forks source link

Router incorrectly calls SetParametersAsync multiple times #44781

Open Liero opened 2 years ago

Liero commented 2 years ago

Is there an existing issue for this?

Describe the bug

WHEN I'm on /index?queryparam=abc and I click a link to /index/999 which is handled by the same component,

THEN SetParametersAsync is called twice:

  1. With QueryParam=null
  2. With Id=999, QueryParam="abc"
      OnNavigateAsync to 'index?queryparam=abc'
      SetParametersAsync Id:0, QueryParam:abc

      OnNavigateAsync to 'index/999'
      SetParametersAsync Id:0, QueryParam:
      SetParametersAsync Id:999, QueryParam:

This is causing a lot of issue when trying to load data based on parameters.

@page "/index"
@page "/index/{id:int}"
@inject ILogger<Index> _logger

<a href="/index?queryparam=abc">queryparam=def</a>
<a href="/index/999">/999</a>

@code {
    [Parameter] public int Id { get; set; }
    [Parameter, SupplyParameterFromQuery(Name = "queryparam")] public string? QueryParamProperty { get; set; }

    public override async Task SetParametersAsync(ParameterView parameters)
    {
        await base.SetParametersAsync(parameters);
        _logger.LogInformation($"SetParametersAsync Id:{Id}, QueryParam:{QueryParamProperty}");
    }
}

This only happens under specific circumstances:

  1. App must have authorization enabled
  2. there is OnNavigateAsync handler on the Router
  3. there is CascadingAuthenticationState on top of the Router (default in blazor template)
@inject IConfiguration Config
<CascadingAuthenticationState>
<Router AppAssembly="@typeof(Program).Assembly" OnNavigateAsync="OnNavigateAsyncHandler">
    <Found Context="routeData">
        <AuthorizeRouteView RouteData="@routeData" DefaultLayout="@typeof(MainLayout)">
        </AuthorizeRouteView>
        <FocusOnNavigate RouteData="@routeData" Selector="[data-action=search]" />
    </Found>
    <NotFound>
    </NotFound>

</Router>
</CascadingAuthenticationState>
@code {
    [Inject] NavigationManager NavigationManager { get; set; } = default!;
    [Inject] AuthenticationStateProvider AuthenticationStateProvider { get; set; } = default!;
    [Inject] ILogger<App> Logger { get; set; } = default!;

    protected override async Task OnInitializedAsync()
    {
        ClaimsPrincipal user = (await AuthenticationStateProvider.GetAuthenticationStateAsync()).User;
        await Task.Delay(100);

        await base.OnInitializedAsync();
    }

    void OnNavigateAsyncHandler(NavigationContext context)
    {
        Logger.LogInformation("OnNavigateAsync to '{Path}'", context.Path);
    }
}

Expected Behavior

SetParametersAsync should be called only once per single navigation event

Steps To Reproduce

  1. Download reproducible demo: https://1drv.ms/u/s!An7wnk66a6RRlrI7ETRR9LsXHi2cmg?e=FsLOC2
  2. Start using Visual Studio
  3. Register new user (it uses lndividualAccounts authorization)
  4. Login
  5. Navigate to /index?queryparam=abc
  6. Click the /999 link
  7. Check the console/debug logs

Exceptions (if any)

No response

.NET Version

6.0.400

Anything else?

.NET SDK (reflecting any global.json): Version: 6.0.400 Commit: 7771abd614

Runtime Environment: OS Name: Windows OS Version: 10.0.22000 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\6.0.400\

global.json file: Not found

Host: Version: 6.0.8 Architecture: x64 Commit: 55fb7ef977

ghost commented 1 year ago

Thanks for contacting us. We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

TanayParikh commented 1 year ago

@Liero, can you please provide the minimal reproduction application as a publicly hosted GitHub repository?

Liero commented 1 year ago

I've added my test project to github: https://github.com/Liero/Bug_Router_Querystring_SetParameters

Please, investigate it.

If I'm right, this is already causing a lot of nasty bugs to many users that have no idea, because it is hard to spot. It wasn't easy to track it down.

danm-de commented 1 year ago

Hello, we have the same issue in our in-house application (Blazor server-side). The error occurs when we define a <CascadingValue> element with a complex type (custom class) as value. If a child component has a [CascadingParameter] property, the SetParametersAsync method will be called twice for each parameter change:

This is actually a big problem if you bind properties (marked with [Parameter] or [Parameter, SupplyParameterFromQuery]) to other components. Imagine the following:

  1. The user changes a datagrid's page from 2 to 3. A data-bind property called Page is updated to 3. A PageChanged event handler loads new data from a remote source and triggers the NavigationManager to update the browser URI from http://myapp/persons?page=2 to http://myapp/persons?page=3 (Deep linking is still a desired feature - even at SPAs).
  2. The query parameter named page requires Blazor to update the property [Parameter,SupplyParameterFromQuery(Name="page")] Page { get; set; }
  3. SetParametersAsync is called twice. The first time with the previous value Page=2, the second time with actual value Page=3.
  4. Data-binding (property Page) triggers the datagrid to switch back from page 3 to page 2, ....

The problem does not occur when using <CascadingValue ... IsFixed="True"> . Unfortunately, this is not applicable in every situation.

I've made a sample project: https://github.com/danm-de/BlazorSetParametersAsyncBug

In the following picture you can see that the method SetParametersAsync called twice: SetParameterAsync-Called-Twice

axylophon commented 1 year ago

We have the same problem as @Liero.

The OnParameterSetAsync method is called right before navigating away even to another page under these circumstances:

  1. there is an OnNavigateAsync handler on the Router
  2. there is a CascadingAuthenticationState on top of the Router (default in blazor template)
  3. the current page has at least one query parameter set via SupplyParameterFromQuery
Liero commented 1 year ago

@danm-de , @axylophon : upvote the original issue

afuersch commented 1 year ago

Having the same problem as @Liero, @danm-de or @axylophon E.g. when having query parameters, which are set via SupplyParameterFromQuery attribute the OnParametersSetAsync is called when navigating away to another page.

Is there any info about investigation or planning of a fix?

ghost commented 1 year ago

To learn more about what this message means, what to expect next, and how this issue will be handled you can read our Triage Process document. We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. Because it's not immediately obvious what is causing this behavior, we would like to keep this around to collect more feedback, which can later help us determine how to handle this. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact work.

Tharizdun commented 11 months ago

I have an issue when navigating from page that has property with [SupplyParameterFromQuery] attribute to another page that also has property with [SupplyParameterFromQuery] attribute - it causes the OnParametersSetAsync method to be called twice, any advice?

ghost commented 10 months ago

To learn more about what this message means, what to expect next, and how this issue will be handled you can read our Triage Process document. We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. Because it's not immediately obvious what is causing this behavior, we would like to keep this around to collect more feedback, which can later help us determine how to handle this. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact work.

CharlieJKendall commented 9 months ago

I've come across this in a personal project recently when fiddling around with Blazor Wasm, and it's stumped me a bit. I'm not hugely familiar with the ASP.NET Core Razor component lifecycle, so this could very well be an intended functionality and this is just me making assumptions!

I've got a minimal repro pushed up in a repository on my GitHub here 👉 https://github.com/CharlieJKendall/supply-parameter-from-query-bug

Summary of the repro

Navigating from a page that has a parameter decorated with [SupplyParameterFromQuery] to another page that also has a parameter decorated with [SupplyParameterFromQuery] causes the OnParametersSet() event to fire twice for the page that you are navigating to - the first time with the parameter value is null, and the second time with the parameter populated with the expected value.

Verbose client log output shows that there are in fact two distinct renders happening for the BlazorApp.Pages.Page component which explains why we're seeing a second call to OnParametersSet(), however I haven't managed to get to the bottom of why this is happening and why the parameter value has not been bound at this point...

invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.RenderTree.Renderer[3]
      Rendering component 11 of type Microsoft.AspNetCore.Components.Sections.SectionOutlet+SectionOutletContentRenderer
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Routing.Tree.TreeRouter[1]
      Request successfully matched the route with name '(null)' and template '/page'
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.Routing.Router[2]
      Navigating to component BlazorApp.Pages.Page in response to path '/page' with base URI 'https://localhost:7129/'
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.RenderTree.Renderer[3]
      Rendering component 1 of type Microsoft.AspNetCore.Components.Routing.Router
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.RenderTree.Renderer[3]
      Rendering component 2 of type Microsoft.AspNetCore.Components.RouteView
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.RenderTree.Renderer[3]
      Rendering component 3 of type Microsoft.AspNetCore.Components.Routing.FocusOnNavigate
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.RenderTree.Renderer[3]
      Rendering component 4 of type Microsoft.AspNetCore.Components.LayoutView
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.RenderTree.Renderer[3]
      Rendering component 5 of type BlazorApp.Layout.MainLayout
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.RenderTree.Renderer[1]
      Initializing component 14 (BlazorApp.Pages.Page) as child of 5 (BlazorApp.Layout.MainLayout)
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.RenderTree.Renderer[4]
      Disposing component 6 of type BlazorApp.Pages.Home
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.RenderTree.Renderer[3]
      Rendering component 14 of type BlazorApp.Pages.Page
invoke-js.ts:176 dbug: Microsoft.AspNetCore.Components.RenderTree.Renderer[3]
      Rendering component 14 of type BlazorApp.Pages.Page
axylophon commented 9 months ago

.

I've come across this in a personal project recently when fiddling around with Blazor Wasm, and it's stumped me a bit. I'm not hugely familiar with the ASP.NET Core Razor component lifecycle, so this could very well be an intended functionality and this is just me making assumptions!

I've got a minimal repro pushed up in a repository on my GitHub here 👉 https://github.com/CharlieJKendall/supply-parameter-from-query-bug

Summary of the repro

Navigating from a page that has a parameter decorated with [SupplyParameterFromQuery] to another page that also has a parameter decorated with [SupplyParameterFromQuery] causes the OnParametersSet() event to fire twice for the page that you are navigating to - the first time with the parameter value is null, and the second time with the parameter populated with the expected value.

Verbose client log output shows that there are in fact two distinct renders happening for the BlazorApp.Pages.Page component which explains why we're seeing a second call to OnParametersSet(), however I haven't managed to get to the bottom of why this is happening and why the parameter value has not been bound at this point...

There are then two types of issues:

  1. You describe that the Parameter for the target page is first null and then has the correct value.

  2. The other problem is, that when navigating away the OnParametersSet of the source page is called again.

CharlieJKendall commented 9 months ago

Following on from the above, it would be great to get some clarity from the team around whether this is intentional or not. Trouble occurs when we make an HTTP request from within the OnParametersSetAsync() method using the query string parameter - for example, for pagination purposes.

The request fired on the first render returns the result set for page 1, and the request fired for the second render returns the result set for the expected page. If the continuation for the first request is executed after the continuation for the second request, then the incorrect data is displayed to the user.

Query parameters are typically optional, so null is usually considered a valid value and an HTTP request to our backend should still be made in this case. I cannot see an obvious (or rather, not unacceptable) way to either avoid the duplicate request being made or to sensibly select which result set we want to prioritise and display in the case that a race occurs.

All thoughts and input welcome! 😌

joarath commented 8 months ago

This obviously isn't a proper solution, but I've had some success with a work around.

I had the exact situation @CharlieJKendall described with incorrect data being displayed to the user.

So far, the correct parameter state is always second. The following code at least ensures the proper parameter state is the last thing to process resulting in the proper data displayed to the user. The drawback is that I'm wasting time processing an incorrect parameter state. Plus, I have to wait for it to finish before I can process the correct parameter state.

I look forward to a proper solution. Until then I'll take the performance sacrifice to ensure correct data.

@code {
     private SemaphoreSlim Semaphore = new SemaphoreSlim(1, 1);

    protected override async Task OnParametersSetAsync()
    {
        await Semaphore.WaitAsync();
        try
        {
               //do async stuff
        }
        finally
        {
            Semaphore.Release();
        }
    }
}
Liero commented 7 months ago

I look forward to a proper solution. Until then I'll take the performance sacrifice to ensure correct data.

Unfortunately there is no proper solution. This is a bug a there might be a workaround at best. I've looked at the SupplyParameterFromQueryAttribute source code and I'm afraid a fix would be a breaking change.

My recommendation is: Don't use [SupplyParameterFromQueryAttribute]!

Following should work, but does not prevent from calling OnParametersSetAsync twice:

protected override async Task OnParametersSetAsync()
{
     await Task.Yield();
     //parameter supplier from query attribute should be updated here
}

if you are familiar with RX (see my blog post:

MyComponent()
{
  _parametersSet.Throttle(TimeSpan.FromMilliseconds(1))
      .Select(_ => (SomeParameter, YourQueryStringParamter)
      .DistinctUntilChanged()
      .ObserveOn(SynchronizationContext.Current!)
      .Subscribe(async _ => 
      {
          // do your async stuff here when one or both properties changes
          StateHasChanged()
      }
}

 //put this into base class
private readonly Subject<Unit> _parametersSet = new ();
protected Observable<Unit> ParametersSet => _parametersSet.AsObservable()
public override async Task SetParametersAsync(ParameterView parameters)
{

    await base.SetParametersAsync(parameters);
    _parametersSet.OnNext(Unit.Default);
}
bingmapsts commented 7 months ago

You can try the following solution for: the Parameter for the target page is first null and then has the correct value.

[SupplyParameterFromQuery]
public string ListType { get; set; }

protected override async Task OnParametersSetAsync()
{
    if (string.IsNullOrWhiteSpace(ListType))
    {
        string[] uriParts = NavigationManager.Uri.Split("?");
        string queryString = uriParts.Length > 1 ? uriParts[1] : string.Empty;
        string queryListType = HttpUtility.ParseQueryString(queryString).Get(nameof(ListType));

        if (!string.IsNullOrWhiteSpace(queryListType)) return;
    }

You can try the following solution for: The first time with the previous value Page=2, the second time with actual value Page=3

[SupplyParameterFromQuery]
public string ListType { get; set; }

protected override async Task OnParametersSetAsync()
{
    string[] uriParts = NavigationManager.Uri.Split("?");
    string queryString = uriParts.Length > 1 ? uriParts[1] : string.Empty;
    string queryListType = HttpUtility.ParseQueryString(queryString).Get(nameof(ListType));

    if (!string.Equals(ListType, queryListType, StringComparison.OrdinalIgnoreCase))
    {
        return;
    }
lnaie commented 7 months ago

I'll add my problem to this pile as well, as it seems to be a possible side effect of the design of the Router based hierarchy where the downstream components are loading their params with a CascadingParameterAttributeBase derivative like the SupplyParameterFromQuery attribute.

I have an example route /projects/1/files/?sk=2 , where:

When I navigate out of Files (/files) component to go up in the Project Details (/projects/1), by clicking for instance on a breadcrumb link, then the OnParametersSetAsync in the Files component is called and there is no way to tell who triggered it. This is obviously a big problem when API data is loaded in OnParametersSetAsync, making unnecessary HTTP calls.

I need a solution for this common routing use case. It's not breaking, but it's a network and compute waste.

Eventually, if handling the state change internally is not practical, I could work with some sort of a property on the Router OR maybe a source in the OnParametersSetAsync(object source) as in the good old days :D, that will allow me to manually manage the 'state change', if I want to, in the downstream components of a route.

Thanks, Lucian

terrajobst commented 7 months ago

I can repro this as well. For now, I'm back to using NavigationManager.LocationChanged + manual query string processing.

shoaibpervaiz commented 1 month ago

I am facing this issue but my reproduction steps are slightly different. I am writing a tab control with steps very similar to described here. https://blazor-university.com/templating-components-with-renderfragements/creating-a-tabcontrol/

I am passing a @text variable to Tab's Text parameter (in the markup), so I am expecting the 'Text' of the tab to change when the value of @text changes

However, when @text changes, and I call StateHasChanged on the tab control component, Blazor fires two SetParametersAsync events. Below is the sequence.

On application load: @text = "Test1" component renders correctly - no issue here

@text - changed to "Test2" SetPrametersAsync called with old value of "Test1" Component is re-rendered with value of "Test1" SetPrametersAsync called with new value of "Test2" No re-render happens - "Test1" still shows in the UI

@text - changed to "Test3" SetPrametersAsync called with old value of "Test2" Component re-rendered with value of "Test2" SetPrametersAsync called with new value of "Test3" No re-render happens - "Test2" is now shown in the UI when it should be "Test3"

Any idea what could be going wrong?

Thanks very much