gustavnavar / Grid.Blazor

Grid component with CRUD for Blazor (client-side and server-side) and ASP.NET Core MVC
GNU Lesser General Public License v2.1
698 stars 135 forks source link

Error handling when server call fails after UpdateGrid #192

Open simonp2014 opened 4 years ago

simonp2014 commented 4 years ago

If I call UpdateGrid in my client and my server side implementation throws an exception and returns a 500 HTTP status code I just get a 'No items to display' message in the grid.

How can I add error handling so that I can indicate that the data retrieval actually failed rather than there being no data?

simonp2014 commented 4 years ago

I'm new to Blazor apps but I thought that calling EnsureSuccessStatusCode() after the HttpClient returns a response object would be sufficient to ensure that an exception was raised. However, I can see that in UpdateGrid->GetItemsDTO() there's a catch all exception handler than just uses Console.WriteLine with the exception description.

Would something like this work?

1) GetItemsDTO could call EnsureSuccessStatusCode() after getting the HTTP response object. Then 5xx and 4xx status errors would get converted into exceptions.

2) GetItemsDTO could either not catch exceptions or rethrow them after printing the error message to the console.

3) If the GetItemsDTO throws I can check the Task returned from UpdateGrid. If the IsFaulted property is set I can display a suitable error message instead of the grid display 'No items to display'

simonp2014 commented 4 years ago

After a bit more investigation I found that GetFromJsonAsync() does call EnsureSuccessStatusCode(). The only problem is really that GetItemsDTO() swallows and ignores exceptions.

I added a rethrow like this to the catch block:

    catch (Exception e)
    {
            Console.WriteLine(e.Message);
            throw;
    }

I can now either use the global error handling in my Blazor webassembly client or add specific handling on the grid page.

    @if (_task.IsFaulted)
    {
        <p><em>Error '@_error'</em></p>
    }
    else if (_task.IsCompleted)
    {
        <GridComponent T="Order" Grid="@_grid"></GridComponent>
    }
    else
    {
        <p><em>Loading...</em></p>
    }

The _error value is stored with code like this when calling UpdateGrid:

    _task = client.UpdateGrid();
    try
    {
        await _task;
    }
    catch( Exception e)
    {
        Console.WriteLine(e.Message);
    }
    if (_task.IsFaulted)
    {
        _error = _task.Exception.InnerException.Message;
    }

Could the rethrow be added to the Grid.Blazor source ?

gustavnavar commented 4 years ago

I will try to address this in the next release

gustavnavar commented 4 years ago

GridBlazor 1.8.10 implementes this feature: https://github.com/gustavnavar/Grid.Blazor/blob/master/docs/blazor_client/Events.md#handle-exceptions-from-the-server

simonp2014 commented 4 years ago

Thanks for the update! I've have tried out the feature and have a few items of feedback:

1) Is there any point in allowing the HandleServerErrors(false, false) setting (the default) ? It just swallows exceptions and shows 'no items to display'. Doesn't that just cause confusion because people may think there is no data rather than an error? Would it be better to always throw errors or display errors? i.e. only have the choice to toggle between the two.

2) HandleServerErrors(true, true) is a somewhat redundant setting too because it seems like the thrown exception stops the grid displaying the error.

3) I was planning to use HandleServerErrors( false, true) to have my own error display. However, I've only been able to do that for the initial grid load. If an exception occurs from the search field or one of the pager buttons the exception cannot be caught in client code AFAIK. So in reality this option may be of limited use.

I'm currently looking at having a custom dataservice so that I can handle errors from the server and building a more meaningful message that will be displayed by HandleServerErrors( true, false).

Perhaps in hindsight that best approach would be a hard-coded HandleServerErrors( true, false). If people really didn't want the built in error display they could just use their own custom dataservice that displayed the error elsewhere and returned an empty data set.