MudBlazor / MudBlazor

Blazor Component Library based on Material design with an emphasis on ease of use. Mainly written in C# with Javascript kept to a bare minimum it empowers .NET developers to easily debug it if needed.
http://mudblazor.com
MIT License
8.03k stars 1.28k forks source link

Table: Fix KeyNotFoundException with Record types #4938

Closed Mr-Technician closed 2 years ago

Mr-Technician commented 2 years ago

Description

Resolves #4691 Resolves #4904 (duplicate).

Record types would break various table functions as identical records would be treated at the same object by the TableContext Dictionary. This PR resolves this by using a HashSet of records that uses both the T value and the MudTr row to determine equality.

How Has This Been Tested?

All existing tests pass. I could duplicate a handful of the existing tests and replace the class type with record types to specifically test records, but the existing tests cover the replacement of the Dictionary with a HashSet.

Types of changes

Checklist:

codecov[bot] commented 2 years ago

Codecov Report

Merging #4938 (c78c43c) into dev (e9c733a) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              dev    #4938   +/-   ##
=======================================
  Coverage   89.95%   89.95%           
=======================================
  Files         370      371    +1     
  Lines       13300    13300           
=======================================
  Hits        11964    11964           
  Misses       1336     1336           
Impacted Files Coverage Δ
src/MudBlazor/Components/Table/MudTrWithValue.cs 100.00% <100.00%> (ø)
src/MudBlazor/Components/Table/TableContext.cs 98.50% <100.00%> (-0.03%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e9c733a...c78c43c. Read the comment docs.

Mr-Technician commented 2 years ago

@henon I am going to add some specific unit test in a bit. Please let me know if you have any concerns with this change.

Mr-Technician commented 2 years ago

@henon I think a similar change needs to happen to the SelectedItems HashSet as two identical records will be treated as the same item and only stored once. This is trickier because it would require a breaking change to the API (as the SelectedItems are returned as a HashSet on the Table). Additionally, inline editing is based on unique items, meaning if a record is used you end up with multiple rows in edit mode: image

However, I think this is getting out of scope for this PR but it is something to consider.

henon commented 2 years ago

We can also just say records are not supported in MudTable if you want multiple equal value types to be treated as different objects then don't use records.

Mr-Technician commented 2 years ago

That's also true. Would you agree that this PR is still relevant even if the verdict for multi selection is that records are not properly supported?

henon commented 2 years ago

Not only records, all value types should exhibit this problem. But yes, the change in this PR still makes sense. An exception should never be thrown just because you are using value types with MudTable.

henon commented 2 years ago

Thanks!

mphilipp622 commented 2 years ago

I added these changes to my forked repo and am running through the debugger on a unit test I made and I think there's an issue, still. The exception is not thrown, which is good. However, the updated row is not being removed properly. Looking at the Rows property, the changed record never gets purged.

Here is the test case I'm using in MudBlazor.UnitTests/Components/TableTests.cs

/// <summary>
/// Issue #4691
/// Tests disposal of rows after an edit has been committed to server data. Table should successfully dispose
/// without a key not found exception.
/// </summary>
/// <returns></returns>
[Test]
public async Task TableNavigateAfterServerRecordChange()
{
        var comp = Context.RenderComponent<TableServerEditDisposalTest>();
        //Console.WriteLine(comp.Markup);
        comp.FindAll("tr").Count.Should().Be(11); // page size + header row

        // Check that the value in the first row is equal to 'Bob'
        var td = comp.FindAll("td");
        td[0].TextContent.Trim().Should().Be("Bob");

        // Click on the first row
        var trs = comp.FindAll("tr");
        trs[1].Click();

        // Find the textfield and change the value to 'test@test.com'
        comp.Find("#id1").Change("TestChange");

        // Click the commit button
        var commitButton = comp.Find("button");
        commitButton.Click();

        // Value in the first row should be now equal to 'TestChange'
        comp.FindAll("td")[0].TextContent.Trim().Should().Be("TestChange");

        var pagingButtons = comp.FindAll("button");

        // click next page
        try
        {
            // going to next page would trigger the key not found exception.
            pagingButtons[2].Click();
        }
        catch (System.Collections.Generic.KeyNotFoundException)
        {
            Assert.Fail("Key exception thrown");
        }

        comp.FindAll("div.mud-table-pagination-caption")[^1].TextContent.Trim().Should().Be("11-12 of 12");

        td = comp.FindAll("td");
        // after page change, the last two elements of the list should be shown, assuming page size is 10.
        td[0].TextContent.Trim().Should().Be("Alex");

        // go back to first page and make sure data is still modified
        pagingButtons[0].Click();

        comp.FindAll("td")[0].TextContent.Trim().Should().Be("TestChange");
        // If we get here, no error happened and test is successful.
 }

And here's the viewer in MudBlazor.UnitTests.Viewer/TestComponents/Table/TableServerEditDisposalTest.razor

@namespace MudBlazor.UnitTests.TestComponents

<MudTable T="TestData"
          ServerData="@(new Func<TableState, Task<TableData<TestData>>>(ServerReload))"
          ReadOnly="false"
          @bind-SelectedItem="selectedItem"
          RowEditPreview="BackupItem"
          RowEditCancel="ResetItemToOriginalValues"
          OnCommitEditClick="@((e) => CommitChanges())"
          CanCancelEdit="true">
    <HeaderContent>
        <MudTh>Name</MudTh>
    </HeaderContent>
    <RowTemplate>
        <MudTd>@context.Name</MudTd>
    </RowTemplate>
    <RowEditingTemplate>
        <MudTd>
            <MudTextField id="@context.ID" @bind-Value="context.Name"/>
        </MudTd>
    </RowEditingTemplate>
    <PagerContent>
        <MudTablePager />
    </PagerContent>
</MudTable>

@code{

    public record TestData
    {
        public string Name { get; set; }
        public string ID { get; set; }
    }

    private List<TestData> data = new()
    {
        new TestData {Name = "Bob", ID = "id1"},
        new TestData {Name = "Alice", ID = "id2"},
        new TestData {Name = "Joe", ID = "id3"},
        new TestData {Name = "Jame", ID = "id4"},
        new TestData {Name = "Sarah", ID = "id5"},
        new TestData {Name = "Chloe", ID = "id6"},
        new TestData {Name = "Max", ID = "id7"},
        new TestData {Name = "Amber", ID = "id8"},
        new TestData {Name = "Rachel", ID = "id9"},
        new TestData {Name = "Nathan", ID = "id10"},
        new TestData {Name = "Alex", ID = "id11"},
        new TestData {Name = "Steph", ID = "id12"},
    };

    private TestData selectedItemOriginal = null;
    private TestData selectedItem = null;

    private void BackupItem(object item)
    {
        this.selectedItemOriginal = (TestData) item;
        StateHasChanged();
    }

    private void ResetItemToOriginalValues(object item)
    {
        item = selectedItemOriginal ?? new TestData();
        StateHasChanged();
    }

#pragma warning disable CS1998 // async without await
    private async Task<TableData<TestData>> ServerReload(TableState state)
    {
    // var resp = await ParticipantClient.GetFilteredParticipants(this.StudyID, this.searchString, state);
        IEnumerable<TestData> returnDat = data.Skip(state.Page * state.PageSize).Take(state.PageSize);

        return new TableData<TestData>()
        {
            Items = returnDat,
            TotalItems = data.Count
        };
    }

    /// <summary>Commits changes to a participant entry</summary>
    private void CommitChanges()
    {
        if (this.selectedItem == null || this.selectedItemOriginal == null)
        {
            return;
        }

        int index = data.IndexOf(selectedItemOriginal);
        data[index] = this.selectedItem;

        StateHasChanged();
    }
}

The test case mimics editing the first row of a server-side filtered table and then going to the next page. Going to the next page triggers disposals and invokes the TableContext.Remove method. If you put some debugging breakpoints in TableContext.Remove and run the unit test, you should see what I'm talking about.

It's also entirely possible my unit test is just bad and I missed something.

Mr-Technician commented 2 years ago

No I think you're onto something here. The Remove call contains the new state of the record and therefore the existing version stored in the HashSet with the MudTr doesn't match.

Ugh this one is tricky. 😢

Mr-Technician commented 2 years ago

Storing both the T value and the MudTr reference is important for setting Checked, but is there any reason we need to check the value (in this case the record) matches before removing a row from the HashSet? @henon

mphilipp622 commented 2 years ago

I'm away from computer right now, but could we add like a GUID to the MudTrWithValue class and use it to override GetHashCode? The GUID could be instantiated when the record is instantiated. That way, each row has a unique ID we can hash on, so even if the values change, the hashset is still looking at the GUID. I would try this on my fork, but am away from computer.

henon commented 2 years ago

I didn't know the value can change for a row. Now I know why the original code did first check before removing from the dict.

Mr-Technician commented 2 years ago

I'm away from computer right now, but could we add like a GUID to the MudTrWithValue class and use it to override GetHashCode?

I like this idea but I don't think we can pull it off. The Remove method receives the T value and the MudTr reference so it has no knowledge of any Guid that we set. If you can think of a way to map the Remove call to said Guid/hashcode, let me know.

henon commented 2 years ago

@Mr-Technician: maybe we just roll this PR back and prevent the exception?

mphilipp622 commented 2 years ago

How about something like this?

Change to MudTr.cs. Add an Id property:

public Guid Id { get; } = Guid.NewGuid();

Modify MudTrWithValue:

public record MudTrWithValue<T>(T Value, MudTr Row)
{
    public virtual bool Equals(MudTrWithValue<T> other)
    {
        if (ReferenceEquals(null, other))
        {
            return false;
        }

        if (ReferenceEquals(this, other))
        {
            return true;
        }

        return Equals(Row.Id, other.Row.Id);
    }

    public override int GetHashCode()
    {
         return (Row != null ? Row.Id.GetHashCode() : 0);
    }
}

This appears to be working on my test case, I think. I'm not seeing the duplicated record after changing and I'm not getting the key exception.

Ultimately, this feels kind of hacky. The ideal solution would probably be something that more significantly restructures how mud tables are generating and holding onto rows. But a change like that would likely be much more far-reaching and breaking.

Mr-Technician commented 2 years ago

I'm unable to look closely right now but if it's clear and maintainable i'm more inclined to go with what @mphilipp622 came up with than not support records.

mphilipp622 commented 2 years ago

I see this was merged in 6.0.13. Is there any concern with the issue I raised? Should this be-reopened?

Mr-Technician commented 2 years ago

@henon I don't think this PR has caused any issues I am not completely happy with it. I think there needs to be a discussion on the use of record types in tables and if/how that will be supported.

henon commented 2 years ago

@mphilipp622 if there is a problem with this please reproduce it with a minimal TryMB so we can examine it closely. @Mr-Technician I realize that I have not yet entirely understood what the problem with Records and Table is (I thought I did). Maybe a TRY that emphasizes the problem would help in the discussion.

mphilipp622 commented 2 years ago

Forgive my ignorance here, but what is a TryMB?

Yomodo commented 2 years ago

Forgive my ignorance here, but what is a TryMB?

https://try.mudblazor.com/

henon commented 2 years ago

here is an example of an issue with record types in MudTable: #4988

henon commented 2 years ago

@mphilipp622 the whole idea of record types is that they automatically implement equals and hashcode in a way that the type can be used like a value type. If you want to use a record type like a class type (which you do if you use mutable records in an editable table with multiselection) you can of course override equals and hashcode :D. That solves the problem but entirely defeats the purpose of records.

Currently you can either use class, struct, immutable records with MudTable and multiselection. Mutable records are not supported because changing a record changes its identity in a hashset. We could add a Comparer parameter to MudTable like in MudSelect. Then you could have mutable records and your custom comparer would define the identity in the HashSet.

Mr-Technician commented 2 years ago

@henon I think a comparer would be the simplest and clearest solution. Any other solution involves modifying the Table logic significantly.

sharpgraf commented 2 years ago

I have read this topic and brought some solution

What user should to do

  1. Implement IMudTableRecord
  2. Mark record as dirty if user update it.

    What MudBlazor should to do

  3. Call UpdateHashSet right after OnSelectedItemsChanged

Example: https://try.mudblazor.com/snippet/GaQmusYbqqCjHAyZ

p.s. Maybe there are some bugs or pointless things here, I haven't had time to optimize yet, but the concept is working, please let me know what do you think :)

Mr-Technician commented 2 years ago

@sharpgraf I think that's a bit of a workaround that we could handle better with a custom Comparer as @henon described.

The usage would be something like this:

class ElementComparer : IEqualityComparer<Element>
{
    public bool Equals(Element a, Element b) => a?.Number == b?.Number;
    public int GetHashCode(Element x) => HashCode.Combine(x?.Number);
}

And then on the table:

<MudTable Items="@Elements" MultiSelection="true" T="Element" SelectedItemsChanged="OnSelectedItemsChanged" Comparer="Comparer">

Where Comparer is an instance of ElementComparer. I am working on implementing this.

ekjuanrejon commented 2 years ago

this seems to have regressed in version 6.0.15

henon commented 2 years ago

this seems to have regressed in version 6.0.15

@ekjuanrejon can you post a tryMB proving your point?

Mr-Technician commented 2 years ago

@ekjuanrejon Please provide a tryMB as henon suggested and ideally make a new issue here on Github. :)

ekjuanrejon commented 2 years ago

image

Mr-Technician commented 2 years ago

@ekjuanrejon I see the error message but need to see a minimal reproduction of the error (either a repository or ideally a try.mudblazor link) to help you.

ekjuanrejon commented 2 years ago

@Mr-Technician

It's hard to separate it out since i don't have just one page and trying it out.

So user is working on a page that contains records and as soon as the user navigates to another page the error happens. Those it's on the dispose method

Mr-Technician commented 2 years ago

@ekjuanrejon Have you implemented a comparer as per #4998?

ekjuanrejon commented 2 years ago

@Mr-Technician

I implemented the comparer and it now seems to work.

I was looking at record that I have been using and I have not implemented the comparer and it does not throw the error.. weird.

Mr-Technician commented 2 years ago

@ekjuanrejon Not using a comparer won't necessarily break the table with records in all circumstances. Glad to hear it's working, though!

davidbell81 commented 2 years ago

Hi All, I think this change broke my code on upgrade to 6.0.15. Ive created a slimmed down snippet to demonstrate.

https://try.mudblazor.com/snippet/GOwmFuGKluCLxqwm

Try adding a provider number with any values and then clicking to the second mudTab. I get a KeyNotFoundException on disposal. I can confirm that this was working in 6.0.14

Mr-Technician commented 2 years ago

@davidbell81 Hi, your try.mudblazor snippet has several build errors.

davidbell81 commented 2 years ago

Sorry, I didn't save it properly. Try this: https://try.mudblazor.com/snippet/mamQFYGUBdKUqFFM

henon commented 2 years ago

@davidbell81 you incorrectly implemented equality. when you change your model's Equals and GetHashCode to this it works:

        public override bool Equals(object? other)
        {
            return (other as ProviderDTO)?.Id == Id;
        }

        public override int GetHashCode()
        {
            return Id.GetHashCode();
        }
davidbell81 commented 2 years ago

Ok Thanks a lot. This is probably causing issues all through my code. Would've taken me ages to find it. Thanks again