Skip to content
This repository has been archived by the owner. It is now read-only.

Renaming Action Parameter causes Binding to Fail #3735

Closed
matthewDDennis opened this issue Dec 10, 2015 · 6 comments

Comments

Projects
None yet
6 participants
@matthewDDennis
Copy link

commented Dec 10, 2015

I have an action on a controller that fails without the [Bind(Prefix="")]. Without it, the keys in the ModelState are description.Name, description.Abstract, and description.Description, not Name, Abstract, and Description. This was working until I change the parameter name and type during some refactoring. Not sure exactly what change cause the issue.

        // POST: ApiDoc/Edit/
        [HttpPost]
        [ValidateAntiForgeryToken]
        public async Task<IActionResult> Edit([Bind(Prefix ="")]ApiGroupDescription description)
        {
            if (ModelState.IsValid)
            {
                var obj = await _service.GetBaseAsync(description.Id);
                if (obj == null)
                    return HttpNotFound();

                if (await TryUpdateModelAsync(obj))
                {
                    await _service.UpdateAsync(obj);
                    return RedirectToAction("Details", new { id = obj.Id });
                }
            }
            return View(description);
        }

@matthewDDennis matthewDDennis changed the title Eidt Post Action requires complex parameter to have [Bind(Prefix="")] Renaming Action Parameter causes Binding to Fail Dec 10, 2015

@rynowak

This comment has been minimized.

Copy link
Member

commented Dec 10, 2015

The cause of this is that the bind is ambiguous because you have a parameter named description with a Description property.

Model binding first looks at the request to see if any data matches the parameter name. In this case you have an entry for Description (which you intended to match the property name), so the system assumes all of the inputs are supposed to look like description.Description, description.Name, etc...

A few things you might want to consider:

  • Using [Bind(...)] in this way is totally fine, it's there so you can configure what model binding does.
  • You could also rename the parameter or property
  • You could also pass a field prefix when generating the form so that the form fields are like description.Name

If you don't like this behavior, you could also write a convention to apply an implicit [Bind(Prefix ="")] to everything in your app. If you need a sample let me know.

@matthewDDennis

This comment has been minimized.

Copy link
Author

commented Dec 10, 2015

I fail to see how this is ambiguous. The parameter is a complex type, so can't be bound to a single form field. This would only get ambiguous if there were multiple parameters with common property names.

I think the case where there is only one parameter is the typical use case, and it should be as robust as possible. In this case, there was not error or warning logged, it just failed to bind, resulting in several wasted hours.

@pranavkm

This comment has been minimized.

Copy link
Member

commented May 9, 2018

The debug log actually does a fairly reasonable job logging what it looked for:

> dbug: Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder[22]
>       Attempting to bind parameter 'description' of type 'MvcSandbox.Controllers.DescriptionModel' ...
> dbug: Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexTypeModelBinder[44]
>       Attempting to bind parameter 'description' of type 'MvcSandbox.Controllers.DescriptionModel' using the name 'description' in request data ...
> dbug: Microsoft.AspNetCore.Mvc.ModelBinding.Binders.SimpleTypeModelBinder[15]
>       Could not find a value in the request with name 'description.Description' for binding property 'MvcSandbox.Controllers.DescriptionModel.Description' of type 'System.String'.
> dbug: Microsoft.AspNetCore.Mvc.ModelBinding.Binders.SimpleTypeModelBinder[14]

Perhaps an analyzer (#7753) would better improve the experience, while not changing the behavior?

@rynowak

This comment has been minimized.

Copy link
Member

commented May 9, 2018

This would be a great case for an analyzer. We know this is confusing and users perceive it as buggy even though it's unfortunately intended to work the way it does.

@mkArtakMSFT

This comment has been minimized.

Copy link
Member

commented May 11, 2018

Closing this issue as we've decided to do the analyzer work #7753

@ygoe

This comment has been minimized.

Copy link

commented Jul 24, 2018

Will the analyzer then only give me a barely visible grey underlining when I happen to have the controller file open? Or will it also make sure I notice the issue when I add a member with a conflicting name to a model class?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.