Priority #1 - Show Stopper Bug - View Rendering engine binds incorrect data to controls in View after DELETE operation

[ghost]

Rewording this bug completely

There is a problem with the way ASP.NET Core MVC (v 3.0) renders collection items in an editor scenario. This is a serious problem. I wish someone would investigate this on priority and provide a fix.

Here is a scenario: The model that is being bound to the view has a collection-style property. In my case, it is a List<T>. The properties in the classes are simple types -- string, DateTime and int.

Here are the two classes:

public class Trip {
    public string Name {get; set;}
    public string Description {get; set;}
    public List<Traveller> Travellers {get; set;}
}

public class Traveller {
    public string Nickname {get; set;}
    public string Name {get; set;}
    public DateTime DateOfBirth {get; set;}
    public int Gender {get; set;}
}

Trip is the class I am binding in the view. I have tried two different ways of binding the view.

Method 1: Custom Editor I created a custom editor. The controls I added were via simple @Html.TextBoxFor().

Method 2: Manual binding I wrote a for loop (using indexing) and bound controls using the same @Html.TextBoxFor() again.

When binding normally, it looks like the binding happens properly. However, you will see the bug when you try to remove data.

In my case, I have a button on the page that binds to a RemoveTraveller action on the controller. In the action, I can see that the required row is being removed correctly. This is the code:

model.Travellers.RemoveAll(t => t.Nickname.Equals(nickname));

where nickname was the nickname being removed (passed in via asp-route-nickname on the remove button on the view).

THIS is where things get interesting.

After the delete, when the collection is being bound, the data that you actually removed is still shown on the UI. If there was only one element in the List, you would need to delete it TWICE to cause it to be removed from the UI.

VERY IMPORTANT If you debug the View's binding code, you can SEE that the data being bound is DIFFERENT to the data that you see after the rendering is complete !!!

@Pilchie - Can I send you more info via e-mail?

[ghost]

One more piece of information:

If I use a for (indexed) loop and bind this way:

@Html.TextBoxFor(m => m.Travellers[index].Nickname)

The bug is triggered. But if I use a foreach (iterator) loop and bind using:

<input id="Travellers_@(index)__Nickname" name="Travellers[@index].Nickname" value="@traveller.Nickname" />

Then it works correctly. So this seems to be something to do with indexed collections?

[Pilchie]

@Pilchie - Can I send you more info via e-mail?

Of course. Also tagging @mkArtakMSFT

[ghost]

@mkArtakMSFT - Let me know what info you need.

[pranavkm]

@sujayvsarma could you share a minimal repro app?

[ghost]

Create a simple ASP.net Core MVC application.. No connections to anything or external packages.

Add one controller and a view. Add the two classes to Models. Bind the view to the Trip class. Hook up add & delete actions in the controller for the Traveller binding.

You should see the bug.

I'll take me 12 hours to send you a repro project. It's 10.30pm here now. About to go to sleep.

[ghost]

I have currently unblocked my team by putting through this workaround. But it is not sustainable.

[ghost]

Attaching the repro application. This is the exact application in which the bug occurs, I have removed portions of the code and system that is out of scope of this issue.

Project: AspNetCore.Issues.13950.zip

How to view the bug: When you currently F5-run it, you will see a trip planner UI. This UI uses the workaround and will not show you the bug. But you can add/remove rows from the Travellers section on the page and see that it works correctly.

Now, open Views > Trips > PlannerView.cshtml.

Comment lines 56 to 112. Uncomment lines 114 to 145.

F5-run. Now add and remove travellers. You will see that the remove part does not work correctly and shows the behaviour detailed above. Notice the difference in how the two blocks of code perform their binding (i.e., between 56-112 and 114-145). I believe the key to the root cause of the problem in somewhere there.

Let me know if you need additional details.

[pranavkm]

@dougbu the problematic case uses EditorFor, so my guess is that removing the item from the model isn't enough since it's still in ModelMetadata. It's basically the same as this: https://github.com/aspnet/Mvc/issues/2509. That said, ModelState.Remove($"List[{ i }]"); works for one instance, what's the way to do this for a complex type with sub properties.

[dougbu]

@sujayvsarma, the answer @pranavkm provided is correct. The view intentionally binds information from the model state to allow round-tripping of values that fail validation. And, the way to prevent that binding is to remove information from both your model and ModelState.

After changing your sample to use a valid namespace, I can't reproduce an issue when using @for or @foreach. Not sure what's happening with @Html.EditorFor(...); it seems to display just a GUID. But, I expect it's possible to make it display elements just deleted from the List<Traveller>. Put another way, the ModelState will still contain information about Travellers[1] after the list is shorted to a single element.

In any case, thank-you for your feedback. We're closing this issue as the questions asked here have been answered.

[ghost]

@dougbu - So this is a "Bug by design" ? Why should developers using the framework have to care whether the data is in the ModelState or not? This is called "Ridiculous Framework Design".

Also, what @pranavkm provided is NOT an answer. It seems to be some thought process. There is no clear instruction or example on how to actually implement this.

Do you seriously expect every single ASP.NET developer that uses model-editortemplate binding to implement such workarounds? If so, then what is the purpose of such a framework beyond usage in highly contrived and simplistic code-samples? Frameworks should address REAL LIFE scenarios not contrived examples.

How can you close such a bug without a proper way forward?

For your urgent attention @Pilchie -- This goes back to my feedback on your feedback systems!

[dougbu]

As I said, the design is intended to handle the normal case of editing after entering invalid data by default (without additional developer thought). Removing data is much less common and requires a small additional step.

[ghost]

@dougbu Are you trying to tell me that "Delete Item" from data-bound lists is not a common scenario in the wild ? If so, you are absolutely wrong.

[dougbu]

No, I'm saying it's less common than editing (or adding) and thus takes an additional gesture

[ghost]

It is not less common. Anywhere there is an "add", there is a "delete" in some way. Even if it is just a soft-delete in the back, it would always be a requirement to remove it from what is shown on the UI. This spans application types from very simple apps to gigantic ERPs.

I fail to understand what kind of a user study gave you the go-ahead to penalize developers that need to do the "remove item" workflow, whether you consider it common or not. To me, as a developer, I put stuff into a Model or remove it from the Model. When I bind that model to data, I expect the engine to show exactly what is in the model. THAT there is another ephermeral thingy called the ModelState (or whatever!) should be transparent to me, it is a vignette of YOUR implementation. Why should I care about ModelState at all as a consumer of that API?

[dougbu]

To clarify: Removing data (an item) from a collection while displaying the entire collection is much less common than other create / update / delete operations.

[dougbu]

Actually, your application is very unusual in that the data exists only in the view. Data is normally stored somewhere farirly permanent. This means the entire collection is almost never bound in delete operations -- only the id is necessary to update the stored model.

[ghost]

@dougbu - I think you are talking about applications that were being written 5 years ago. Nowadays, it is the age of "database-less applications". Store data only when it needs to be stored. There are way too many problems and costs involved with storing data nowadays, so this is the approach that is being increasingly adopted.

The application I pulled that code extract from is a travel-domain application that has zero databases. It works entirely off web services. The UI the controls were part of is an itinerary builder. Why would I store it anywhere? It will be sent off to the booking engine to create the tickets/etc.

Removing data (an item) from a collection while displaying the entire collection is much less common than other create / update / delete operations.

It most certainly is not. There are hundreds of thousands of applications out there that does this. Perhaps they don't use asp.net because of this very bug.