fix: Loading value is not set in DataGrid when using Items parameter

[TekMechanic]

🐛 Bug Report

After changes made in PR #2998, when using the Items parameter as a data feed and not ItemsProvider, the Loading boolean is never set and is always null. This results in the Items IQueryable being enumerated whenever OnParametersSetAsync is called.

💻 Repro or Code Sample

@using System.Linq.Expressions
@using Microsoft.FluentUI.AspNetCore.Components

<FluentDataGrid
    Style="@_style"
    Items="@_queryable">
    <PropertyColumn Title="ID" Property="@(c => c)" />
</FluentDataGrid>

<FluentButton OnClick="OnButtonClick">Update Parameters</FluentButton>

<p>Number of queryable executions: <strong>@_numExecutions</strong></p>

@code {
    IQueryable<string>? _queryable;
    int? _numExecutions;
    private string _style = "";

    protected override Task OnInitializedAsync()
    {
        _queryable = new LoggingQueryable(["foo", "bar"], () => _numExecutions += 1);

        // Display the number of executions just for information. This is completely separate from the grid.
        _numExecutions = 1;
        return Task.CompletedTask;
    }

    private class LoggingQueryable(List<string> list, Action fetchCounter) : EnumerableQuery<string>(list), IQueryProvider
    {
        private readonly IQueryable<string> _list = list.AsQueryable();

        object? IQueryProvider.Execute(Expression expression)
        {
            fetchCounter();
            return (_list as IQueryProvider)!.Execute(expression);
        }

        TElement IQueryProvider.Execute<TElement>(Expression expression)
        {
            fetchCounter();
            return (_list as IQueryProvider)!.Execute<TElement>(expression);
        }
    }

    private void OnButtonClick()
    {
        _style += "";
    }

}

🤔 Expected Behavior

The queryable should not be enumerated unless there is a proper change.

😯 Current Behavior

The queryable is executed anytime a parameter is updated on the data grid.

💁 Possible Solution

The ResolveItemsRequestAsync method needs to set Loading appropriately when the Items parameter is used, similar to how it sets it for when ItemsProvider is used.

🔦 Context

🌍 Your Environment

Fluent UI 4.11.3, Chrome

[vnbaaij]

Getting errors with your reproduction code:

Also, your code is not using the Loading parameter anywhere so not really sure what you are trying to do, or not trying to do.

This results in the Items IQueryable being enumerated whenever OnParametersSetAsync is called.

That is by design. Please see #1514 (comment)

[TekMechanic]

Apologies, I forgot to include the needed using statements in my reproduction code:

@using System.Linq.Expressions
@using Microsoft.FluentUI.AspNetCore.Components

I also created a repo that has the reproduction projects, you can find it here: https://github.com/TekMechanic/DataGridIssue-3261 The relevant code can be found in the Home.razor files.

My code is not using the Loading parameter, but I believe it is part of the issue because of this change:

PR 3064 Conversation

File Change: src/Core/Components/DataGrid/FluentDataGrid.razor.cs

If this is by design, was this design decision made between v4.10.4 and v4.11.3? This issue does not occur with the former version. I included two test projects with the two versions in the repo I mentioned above. Here is a GIF of the difference:

Also, the comment(s) in the code seem to suggest otherwise. mustRefreshData in this case will always be true since Loading will always be null, unlike for the ItemsProvider path which does update the Loading property.

fluentui-blazor/src/Core/Components/DataGrid/FluentDataGrid.razor.cs

Lines 409 to 424 in 4408f0a

	// Perform a re-query only if the data source or something else has changed
	var dataSourceHasChanged = !Equals(Items, _lastAssignedItems) || !Equals(ItemsProvider, _lastAssignedItemsProvider);
	if (dataSourceHasChanged)
	{
	_scope?.Dispose();
	_scope = ScopeFactory.CreateAsyncScope();
	_lastAssignedItemsProvider = ItemsProvider;
	_lastAssignedItems = Items;
	_asyncQueryExecutor = AsyncQueryExecutorSupplier.GetAsyncQueryExecutor(_scope.Value.ServiceProvider, Items);
	}
	var paginationStateHasChanged =
	Pagination?.ItemsPerPage != _lastRefreshedPaginationState?.ItemsPerPage
	|| Pagination?.CurrentPageIndex != _lastRefreshedPaginationState?.CurrentPageIndex;
	var mustRefreshData = dataSourceHasChanged || paginationStateHasChanged || Loading is null;

fluentui-blazor/src/Core/Components/DataGrid/FluentDataGrid.razor.cs

Lines 781 to 802 in 4408f0a

	if (ItemsProvider is not null)
	{
	var gipr = await ItemsProvider(request);
	if (gipr.Items is not null && Loading is null)
	{
	Loading = false;
	StateHasChanged();
	}
	return gipr;
	}
	else if (Items is not null)
	{
	var totalItemCount = _asyncQueryExecutor is null ? Items.Count() : await _asyncQueryExecutor.CountAsync(Items, request.CancellationToken);
	_internalGridContext.TotalItemCount = totalItemCount;
	var result = request.ApplySorting(Items).Skip(request.StartIndex);
	if (request.Count.HasValue)
	{
	result = result.Take(request.Count.Value);
	}
	var resultArray = _asyncQueryExecutor is null ? [.. result] : await _asyncQueryExecutor.ToArrayAsync(result, request.CancellationToken);
	return GridItemsProviderResult.From(resultArray, totalItemCount);
	}

[vnbaaij]

There have been a lot of changes to the DataGrid between 4.10 and 4.11. In fact, that was the most important change. We changed from using web components to HTML table elements and discussed that at various places at length.

Also, the comment(s) in the code seem to suggest otherwise. mustRefreshData in this case will always be true since Loading will always be null, unlike for the ItemsProvider path which does update the Loading property.

Yes, Items and ItemsProvider are treated differently (always have). We operate under the assumption that when you use Items the data is there immediately and you do not want to display LoadingContent. That is different for ItemsProvider which can have the LoadingContent shown until the providers first return.

I don't understand what exactly is the issue you are trying to prove or point out with this code. Can you explain what is happening and how that is an issue according to you.

Ok, I think I understand now. I think we need to change line 424
https://github.com/microsoft/fluentui-blazor/blob/4408f0a82e76135eb2e6438b322188328c12342f/src/Core/Components/DataGrid/FluentDataGrid.razor.cs?plain=1#L424C1-L424C106
to be

var mustRefreshData = dataSourceHasChanged || paginationStateHasChanged || EffectiveLoadingValue;

That way you'll get the same behavior as with 4.10.

I'll test that change further and if it all checks out, we will change it for next patch release

[TekMechanic]

I believe that should solve it, thanks!

For more context, I was trying to describe the root problem for another issue that this is causing in our application without getting too down in the weeds of the specifics of our application. However, I think it would be benefical for future reference to try and explain in the most concise way I can:

In our case, the IQueryable being used is backed by an EF Core DbContext connected to MSSQL. There are actions the user can take in the parent component (that are not related to the data feed and should not warrant a re-fetch of the data) that ultimately cause OnParametersSetAsync to be called on the DataGrid. This results in over-fetching and in our case, over fetching in rapid succession. While this is also inefficient, our primary problem is that the cancellation token logic in the DataGrid is canceling the SqlCommand to the database resulting in a Microsoft.Data.SqlClient.SqlException "Operation cancelled by user" to be throw that isn't handled. This is attempting to be handled in this block of code, but does not catch this case and could be improved:

fluentui-blazor/src/Core/Components/DataGrid/FluentDataGrid.razor.cs

Lines 804 to 807 in 47b8a3d

	catch (OperationCanceledException oce) when (oce.CancellationToken == request.CancellationToken)
	{
	// No-op; we canceled the operation, so it's fine to suppress this exception.
	}