Skip to content
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.

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

Router incorrectly calls SetParametersAsync multiple times #44781

Open
1 task done
Liero opened this issue Oct 28, 2022 · 23 comments
Open
1 task done

Router incorrectly calls SetParametersAsync multiple times #44781

Liero opened this issue Oct 28, 2022 · 23 comments
Labels
area-blazor Includes: Blazor, Razor Components feature-routing investigate Pillar: Technical Debt Priority:1 Work that is critical for the release, but we could probably ship without triaged

Comments

@Liero
Copy link

Liero commented Oct 28, 2022

Is there an existing issue for this?

  • I have searched the existing issues

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

@Liero Liero changed the title Router calls incorrectly calls SetParametersAsync multiple times with foreach route parameter Router calls incorrectly calls SetParametersAsync multiple times Oct 28, 2022
@mkArtakMSFT mkArtakMSFT added area-blazor Includes: Blazor, Razor Components feature-routing investigate labels Oct 28, 2022
@mkArtakMSFT mkArtakMSFT added this to the .NET 8 Planning milestone Oct 31, 2022
@ghost
Copy link

ghost commented Oct 31, 2022

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
Copy link
Contributor

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

@Liero Liero changed the title Router calls incorrectly calls SetParametersAsync multiple times Router incorrectly calls SetParametersAsync multiple times Nov 2, 2022
@Liero
Copy link
Author

Liero commented Nov 2, 2022

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
Copy link

danm-de commented Jun 22, 2023

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:

  • The first time with the previous (old) values.
  • The second time with the actual values.

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
Copy link

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
Copy link
Author

Liero commented Aug 8, 2023

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

@afuersch
Copy link

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
Copy link

ghost commented Oct 6, 2023

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
Copy link

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
Copy link

ghost commented Dec 13, 2023

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
Copy link

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
Copy link

.

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
Copy link

CharlieJKendall commented Jan 5, 2024

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 often 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.

@joarath
Copy link

joarath commented Mar 1, 2024

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
Copy link
Author

Liero commented Mar 6, 2024

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
Copy link

bingmapsts commented Mar 16, 2024

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
Copy link

lnaie commented Mar 20, 2024

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:

  • Files component takes care of the /files/?sk=2 fragment
  • ProjectDetails component takes care of the /projects/1 fragment
  • Projects component takes care of the /project fragment
  • Skip param is handled with [Parameter, SupplyParameterFromQuery(Name = "sk")] in Files component
  • API data is loaded in OnParametersSetAsync() in all components
  • all the pages are rendered in pure WASM more with @rendermode @(new InteractiveWebAssemblyRenderMode(prerender: false)) but the skeleton is a SSR project.

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
Copy link
Contributor

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

@shoaibpervaiz
Copy link

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

@axylophon
Copy link

As this is still an issue with OnNavigateAsync we removed it from our project and use the LocationChanged event of the NavigationManager instead.

@lnaie
Copy link

lnaie commented Nov 13, 2024

Great guys, instead of fixing infra problems you're focusing on AI bullshit.

@MajMcCloud
Copy link

MajMcCloud commented Dec 9, 2024

Good evening everyone,

Im working with Blazor since 2021 (.Net 6) and had this issue (if it even is one) all the time.
For that I just want to provide a solution im using in all projects lately. BaseClass.cs

So what it is it catches the SetParametersAsync in a base class and when initiated all necessary parameters it runs a seperate "OnLoaded" and "OnLoadedAsync" method to prevent multiple calls.

For parameters just add them to the list via the "AddParameterToCheck" method.

I'll do this mostly within the OnInitialized method with a call like:

AddParameterToCheck(nameof(ProductId))

Cheers
Florian Zevedei

@javiercn
Copy link
Member

#20637

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components feature-routing investigate Pillar: Technical Debt Priority:1 Work that is critical for the release, but we could probably ship without triaged
Projects
None yet
Development

No branches or pull requests