Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Collections are not correctly model-bound #2129

Closed
ghost opened this issue Mar 5, 2015 · 24 comments
Closed

Collections are not correctly model-bound #2129

ghost opened this issue Mar 5, 2015 · 24 comments
Assignees
Milestone

Comments

@ghost
Copy link

ghost commented Mar 5, 2015

I retrieve an list of items with the action below

    public IActionResult Edit(int id)
    {
        ObservationRepository _obsRepo = new ObservationRepository(_db);
        List<Observation> allMsg = null;
        allMsg = _obsRepo.getObservations(id);
        ViewBag.ModeEdit = true;
        return PartialView("_Observations", allMsg);
    }

The model's view is defined as @model List<Mediagral.Core.Models.Observation>

there an example of input generated for a property with an attribut name that contain an indexed array without name.

@dougbu
Copy link
Member

dougbu commented Mar 5, 2015

@branciat am I correct the partial view named _Observations.cshtml contains something like the following?

@using System.Collections
@model List
...
@Html.EditorFor(m => m)

If so you may get better results using @model List<Observation> instead. In any case please provide the view and detail the unexpected results.

@danroth27
Copy link
Member

@branciat Could you please share exactly what your cshtml view looks like and also the Mediagral.Core.Models.Observation type so that we can try to reproduce the issue? Thanks!

@ghost
Copy link
Author

ghost commented Mar 6, 2015

The partial view

@model List<Mediagral.Core.Models.Observation>

<div class="portlet box green">
    <div class="portlet-title">
        <div class="caption">
            <i class="fa fa-edit"></i>Observations
        </div>
        <div class="tools">
            <a class="btn red" href="@Url.RouteUrl("CoreApi", new { area = "Core", controller = "Observation", action = "Create", id = Model.First().AddressableId })" data-ajax="true" data-ajax-mode="after"
               data-ajax-update="#obs_table tbody" style="height:30px">
                <i class="fa fa-plus"></i>
            </a>
        </div>
    </div>
    <div class="portlet-body form">
        @if (ViewBag.ModeEdit)
        {
            Html.BeginForm(FormMethod.Post, new { data_ajax = "true", data_ajax_mode = "replace", data_ajax_update = "#AjaxContent" });
        }
                <div class="form-body">
                    <div class="table-scrollable no-footer">
                        <table aria-describedby="obs_info" role="grid" class="table table-condensed table-hover table-bordered no-footer" id="obs_table">
                            <thead>
                                <tr role="row">
                                    <th>Contenu</th>
                                    <th>Date</th>
                                    <th>Créé par</th>
                                    <th>Actions</th>
                                </tr>
                            </thead>
                            <tbody>
                                @for (int i = 0; i < Model.Count; i++) { 
                                    <tr class="@((i % 2) == 0 ? "even" : "odd" )" role="row">
                                        @if (ViewBag.ModeEdit)
                                        {
                                            <td>
                                                @Html.AntiForgeryToken()
                                                @Html.EditorFor(m => m[i].Content, new { HtmlAttributes = new { @class = "form-control" } })
                                            </td>
                                            <td>
                                                @Html.HiddenFor(m => m[i].AddressableId)
                                                @Html.EditorFor(m => m[i].Created, new { HtmlAttributes = new { @class = "form-control" } })
                                            </td>
                                            <td>
                                                @Html.HiddenFor(m => m[i].Id)
                                                @Html.EditorFor(m => m[i].CreatedBy.UserName, new { HtmlAttributes = new { @class = "form-control" } })
                                            </td>
                                        }
                                        else
                                        {
                                            <td>@Html.DisplayFor(m => m[i].Content)</td>
                                            <td>@Html.DisplayFor(m => m[i].Created)</td>
                                            <td>@Html.DisplayFor(m => m[i].CreatedBy.UserName)</td>
                                        }
                                        <td>
                                            <a class="btn default btn-sm purple" href="@Url.RouteUrl("CoreApi", new { area = "Core", controller = "Observation", action = "Delete", id = Model[i].Id })" data-ajax="true">
                                                <i class="fa fa-trash-o"></i> Supprimer
                                            </a>
                                        </td>
                                    </tr>
                                }
                            </tbody>
                        </table>
                    </div>
                </div>
                <div class="form-actions">
                    <div class="row">
                        <div class="col-md-6">
                            <div class="row">
                                <div class="col-md-offset-3 col-md-9">
                                    @if (ViewBag.ModeEdit)
                                    {
                                        <button type="submit" class="btn green" data-ajax="true" data-ajax-mode="replace" data-ajax-update="#AjaxContent">
                                            <i class="fa fa-pencil"></i> Valider
                                        </button>
                                        <button type="reset" class="btn yellow" data-ajax="true" data-ajax-mode="replace" data-ajax-update="#AjaxContent">
                                            <i class="fa fa-pencil"></i> Annuler
                                        </button>
                                    }
                                    else
                                    {
                                        <a href="@Url.RouteUrl("CoreApi", new { area = "Core", controller = "Observation", action = "Edit", id = Model.First().AddressableId })" class="btn green"
                                           data-ajax="true" data-ajax-mode="replace" data-ajax-update="#AjaxContent">
                                            <i class="fa fa-pencil"></i> Editer
                                        </a>
                                    }
                                </div>
                            </div>
                        </div>
                    </div>
                </div>
        @if(ViewBag.ModeEdit)
        {
            Html.EndForm();
        }
    </div>
</div>

The entity

using System;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;
using Mediagral.User.Models;

namespace Mediagral.Core.Models
{
    public partial class Observation
    {
        public int Id { get; set; }

        public string CreatedById { get; set; }

        public int AddressableId { get; set; }

        [Required, DataType(DataType.Text)]
        public string Content { get; set; }

        [Column(TypeName = "datetime2")]
        public DateTime Created { get; set; }

        public virtual MediagralUser CreatedBy { get; set; }
        public Addressable Addressable { get; set; }
    }
}

@dougbu
Copy link
Member

dougbu commented Mar 6, 2015

@branciat which part of the generated HTML is not what you expect? anything similarly unexpected in the HTML generated when !ViewBag.ModeEdit?

@danroth27
Copy link
Member

@dougbu Presumably the name attribute is empty - we should try to reproduce this. Could you please try it out? Thanks!

@ghost
Copy link
Author

ghost commented Mar 6, 2015

@dougbu
I'm at home, i live in france. I try the negation after update my config is the same at work.
i'm missing the database migration scripts for complete installation. I will see monday, back to work.

@ghost
Copy link
Author

ghost commented Mar 9, 2015

@dougbu
This don't change anything. I've still the attribute name empty

<input class="form-control text-box single-line" data-val="true" data-val-required="Le champ Content est requis." id="z0__Content" name="[0].Content" value="Le lundi 5 septembre 2011 13:34:55" type="text">

@dougbu
Copy link
Member

dougbu commented Mar 9, 2015

@branciat did you edit the HTML slightly? everything looks as I'd expect except it should be a self-closing tag e.g. <input ... />.

and what in particular is empty?

@ghost
Copy link
Author

ghost commented Mar 9, 2015

Yes i forgot the tag </input> in example, no override editor template, i have the same problem with other partial view and other entity, always a collection as model.
I use visual studio 2015 ctp6 and asp mvc master beta 3.

@dougbu
Copy link
Member

dougbu commented Mar 9, 2015

the name attribute has value "[0].Content" in the sample you provided. what is the problem?

@ghost
Copy link
Author

ghost commented Mar 9, 2015

Yes it's the problem for each view with a collection as model.
The problem is to action level, the [FromForm] don't hydrate the entity when the name is empty.

[HttpPost]
        public IActionResult Edit([FromForm]List<Observation> id)
        {
            ObservationRepository _obsRepo = new ObservationRepository(_db);
            if (id != null && ModelState.IsValid)
            {
                foreach (Observation obs in id)
                {
                    _obsRepo.UpdateObservation(obs);
                }
                return Edit(id[0].AddressableId);
            }
            else
            {
                return PartialView("_Common", id);
            }
        }

I tried with Observation[], the parameter is always null.

@ghost
Copy link
Author

ghost commented Mar 9, 2015

I add "id" in the name of the html generated with the web development toolbox of firefox, no problem, the FromForm hydrate the entity.

@danroth27 danroth27 added this to the 6.0.0-rc1 milestone Mar 9, 2015
@rynowak
Copy link
Member

rynowak commented Mar 12, 2015

I add "id" in the name of the html generated with the web development toolbox of firefox, no problem, the FromForm hydrate the entity.

Does the submission work correctly if you remove [FromForm]? What if you do [FromForm(Name = "")] (explicitly empty name).

To explain: traditional model binding does a pass where it tries first using the parameter name as the binding prefix - id[0].AddressableId, id[0].Content, etc.... If no result is found then it tries again with the the empty string as prefix - [0].AddressableId, [0].Content, etc.... When using an attribute like [FromForm(...)] it looks like it's requiring the parameter name as-prefix unless told otherwise.

/cc @harshgMSFT

@dougbu
Copy link
Member

dougbu commented Mar 12, 2015

good timing @rynowak I just reproduced this issue locally. it occurs with or without [FromForm] on the parameter. also tried MVC 5 (just the no attribute case) and see things working fine there. so this is a regression from that version.

I observe the data is submitted correctly and has the expected keys in the HttpRequest. however the List<Observation> parameter is consistently null.

[FromForm(Name = "")] is a rather counter-intuitive workaround but it works.

@harshgMSFT
Copy link
Contributor

Is this related to #1865?

@dougbu
Copy link
Member

dougbu commented Mar 12, 2015

@harshgMSFT no, the collections are at different levels. also #1865 reproduces with MVC 5 and this bug does not.

@harshgMSFT
Copy link
Contributor

I see, the cause of concern seems to be similar for both bugs, i think this is because in https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/GenericModelBinder.cs#L29 we return a value even if there was no value bound. Which is why it does not fallback to empty prefix. We should fix this asap.

@dougbu
Copy link
Member

dougbu commented Mar 12, 2015

might be why the bug's assigned to someone (me in this case). I'm debugging further...

@ghost
Copy link
Author

ghost commented Mar 16, 2015

I'm interested by the workaround [FromForm(Name = "Model")], but appear not implemented in the beta3. Only dev branch ?

@rynowak
Copy link
Member

rynowak commented Mar 16, 2015

@branciat is [FromForm] missing from beta3 or just the Name =?.

If the former, subclass FromFormAttribute and implement IModelNameProvider. If the latter, just ModelBinderAttribute - you won't be able to restrict model binding to form-data, but this will allow you to set a name.

dougbu added a commit that referenced this issue Mar 16, 2015
- #2129
- do not propagate results with `!IsModelSet`, allowing empty prefix fallback
- adjust ComplexModelDtoModelBinder to at least fake-bind all properties
 - default values not consistently picked up otherwise
@dougbu
Copy link
Member

dougbu commented Mar 17, 2015

@branciat we're working on a fix for this. it should be in our dev feed later this week.

for Beta3, the workaround is to add [Bind(Prefix ="")] to your action parameter.

dougbu added a commit that referenced this issue Mar 18, 2015
- #2129
- do not propagate results with `!IsModelSet`, allowing empty prefix fallback
- adjust ComplexModelDtoModelBinder to at least fake-bind all properties
 - default values not consistently picked up otherwise
@dougbu dougbu changed the title EditorFor generate input name empty for the collections Collections are not correctly model-bound Mar 18, 2015
@dougbu
Copy link
Member

dougbu commented Mar 18, 2015

updating the subject because reported issue is about problems model binding collections. HTML generation is fine (at least in this respect 😈)

dougbu added a commit that referenced this issue Mar 19, 2015
- #2129
- do not propagate results with `!IsModelSet`, allowing empty prefix fallback
- adjust ComplexModelDtoModelBinder to at least fake-bind all properties
 - default values not consistently picked up otherwise
dougbu added a commit that referenced this issue Mar 20, 2015
- #2129
- do not propagate results with `!IsModelSet`, allowing empty prefix fallback
- adjust ComplexModelDtoModelBinder to at least fake-bind all properties
 - default values not consistently picked up otherwise
dougbu added a commit that referenced this issue Mar 20, 2015
- remaining `MutableObjectModelBinder` tests
- functional test of #2129 scenario
dougbu added a commit that referenced this issue Mar 20, 2015
- #2129
- do not propagate results with `!IsModelSet`, allowing empty prefix fallback
- adjust `ComplexModelDtoModelBinder` to at least fake-bind all properties
 - default values not consistently picked up otherwise

nit: correct 2 test names in `KeyValuePairModelBinderTest`
dougbu added a commit that referenced this issue Mar 20, 2015
…e match

- #1865
- change `MutableObjectModelBinder` to ignore exact match in value providers
 - had an incorrect assumption: don't want exact model name to match since
   this binder supports only complex objects
 - also ignored `BinderModelName`, value provider filtering, et cetera
- reduces over-binding e.g. `[Required]` validation within missing properties

also add more tests of #2129 scenarios
@dougbu
Copy link
Member

dougbu commented Mar 20, 2015

commit 94e326f

@ghost
Copy link
Author

ghost commented May 27, 2015

Perfect ! Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants