Blazor should throw when there are duplicate @key values in components

[Andrzej-W]

Describe the bug

Currently it is possible to create components with duplicate @key values. It is even documented:

If clashing @key values are requested within the same parent, the @key values won't be honored.

Expected behavior

Blazor should throw exception, because it is potentially serious bug with catastrophic consequences.

Additional context

People who use @key expect that it works. If it doesn't some serious bugs are possible, usually related to unexpected lifecycle methods calls.

[SteveSandersonMS]

potentially serious bug with catastrophic consequences

Can you clarify what catastrophic consequences you anticipate?

If clashing @key values are requested within the same parent, the @key values won't be honored.

This means that we may produce a suboptimal diff (e.g., re-rendering more descendants than you'd expect), but it's rare that it would result in an observable difference to the end user.

The design intent was to handle key clashes gracefully, because there may be cases where developers find them hard to avoid, or worse, don't encounter them during development but do in production. In the latter case, producing a suboptimal diff is much better than outright crashing the application.

[mkArtakMSFT]

Thank you for your feedback.
We're closing this issue as the behavior discussed is by design.

/cc @rynowak, in case you have any thoughts, please comment.

[Andrzej-W]

The design intent was to handle key clashes gracefully, because there may be cases where developers find them hard to avoid, or worse, don't encounter them during development but do in production. In the latter case, producing a suboptimal diff is much better than outright crashing the application.

Sorry, but I don't agree. We can have plenty of bugs in our programs like division by zero or access to null reference and in my opinion it is good that we see an exception in all these cases. In ASP.NET Core source we can find hundreds of places where you are checking for null at the beginning of a method and throw exception. You don't silently ignore the problem.

I perceive @key attribute as an unique key in a database. It has to be unique or you will get an error. In the doc we can read:

Generally, it makes sense to supply one of the following kinds of value for @key:

• Model object instances (for example, a Person instance as in the earlier example). This ensures preservation based on object reference equality.
• Unique identifiers (for example, primary key values of type int, string, or Guid).

These suggestions are perfect. People who cannot guarantee uniqueness should not use @key attribute. Otherwise they will have hard to find bugs.

Can you clarify what catastrophic consequences you anticipate?
This means that we may produce a suboptimal diff (e.g., re-rendering more descendants than you'd expect), but it's rare that it would result in an observable difference to the end user.

Observable (or worse hidden) difference can result from the fact that components can have some internal state. This internal state can be set initially when the object is created and then eventually changed as a result of some external events, for example OnClick(). When @key attribute contains duplicate value you can intermix this internal state of one component with current values of parameters of other component. Below is a simple example (small modification of Counter component).

@page "/counter"

<h1>Dish: @DishName</h1>
<p>Ordered quantity: @currentCount</p>
<button class="btn btn-primary" @onclick="@IncrementCount">Order more</button>
@if (currentCount > 1)
{
    <button class="btn btn-primary" @onclick="@DecrementCount">Order less</button>
}

@code
{
    // internal state initialized when object is created
    int currentCount = 1;
    [Parameter] protected string DishName { get; set; }

    void IncrementCount()
    {
        currentCount++;
    }
    void DecrementCount()
    {
        currentCount--;
    }
}

And here is a modified index.razor

@page "/"
<div>
    @for (int i = 0; i < NumberOfDishes; i++)
    {
        if (!hiddenDishes[i])
        {
            <Counter DishName="@dishes[i]" />
        }
    }
</div>
<div>
    <br />
    @for (int i = 0; i < NumberOfDishes; i++)
    {
        int x = i;
        <button class="btn btn-primary" @onclick="@(() => Hide(x))">@(hiddenDishes[x] ? "Add" : "Remove") @dishes[x]</button>
    }
</div>

@code
{
    const int NumberOfDishes = 3;
    bool[] hiddenDishes = new bool[NumberOfDishes];
    string[] dishes = { "Pizza", "Hamburger", "Chips" };
    void Hide(int counterNumber)
    {
        hiddenDishes[counterNumber] = !hiddenDishes[counterNumber];
    }
}

Take into account that source code is written this way to demonstrate the problem and it is not a recommended design pattern for anybody. Here is a screenshot after a few Order more clicks.

Now after Remove pizza click

As you can see I originally ordered 3 hamburgers and now I have only 2. The same problem is with chips.

After Add pizza click

I have 2 pizzas (should be 1) and I have 1 chips only (originally was 4).

I intentionally don't use @key attribute to demonstrate the effect of duplicate keys. In real application developer uses @key attribute. All tests created by developer use unique key values. Everything works perfectly. Now let's assume that in production sometimes (for example once a month) there will be duplication. People will complain their orders have wrong quantities. It will be almost impossible to find this bug in real, much more complicated application, especially if wrong data is hidden in internal data structure and not displayed on the screen. In my opinion that can have catastrophic consequences. It is possible that you write an application for a hospital and you will kill some people because some of them will get wrong drugs.

Each time you encounter a duplicate key value you should throw an exception. It is much better than silently ignore a bug.

I vote to change the behaviour now (before 3.0 release) because it is a breaking change.

[Andrzej-W]

@mkArtakMSFT please reopen because it is in my opinion serious bug. I have just added a new comment with clarification - read and decide.
/cc @SteveSandersonMS

[mkArtakMSFT]

Reopening to discuss this in our next triage.

[Andrzej-W]

I have yet another example. It will not kill people but it can kill performance of your application.

You can have some expensive calculations or database calls in your component each time value of the parameter changes. When you have duplicate key values wrong components are created/destroyed and all other components unintentionally receive new parameter values.

This bug (or unfortunate design) is similar to original design of Entity Framework Core where some LINQ queries where not executed on the server but on the client. This led to unacceptable drops in performance at unpredictable moments. Fortunately it is now fixed in 3.0 and every time LINQ query cannot be correctly transformed to server only SQL query exception will be thrown.

I have also found a few older related issues: #8954 #7048

[SteveSandersonMS]

Thanks for this suggestion, @Andrzej-W!

We were convinced by your reasoning, and implemented this change in #12187.