request ends in 400 when model binder success value is null

Is this a Bug or Feature request?:

I would say it's a bug

Steps to reproduce (preferably a link to a GitHub repo with a repro project):

https://github.com/BenjaminAbt/AspNetCoreModelBinderDemo

Description of the problem:

I want to implement a custom model binder as shown in the official documentation; I'm pretty sure the doc sample wont work.
https://docs.microsoft.com/en-us/aspnet/core/mvc/advanced/custom-model-binding?view=aspnetcore-2.1#custom-model-binder-sample

I have a poco, a binder and a binder provider (see sample).
On an action I want to inject the model, loaded from a "database".

This works quite nice, if the id (example id = 1) was found. The action is called and the object is being returned.
If not (exmaple id = 2) and the object is null, the middleware responds with bad request and the action is not called -> I cannot return 404.

Expectation:

I expect if no poco was found the poco parameter is null and I can handle that in the action.

Thanks for contacting us, @BenjaminAbt.
@pranavkm, can you please look into this? Thanks!

I think I found the issue, but still think it's a bug.

Problem:

[HttpGet("{id}", Name = "GetAuthorById")]
public async Task<ActionResult<AuthorApiViewModel>> Get(Author author)

author is set if route id was found. Action is called.
If id was not found and author should be null, action is NOT called and middleware returns 400.

Solution:

Name the route parameter like the action parameter

[HttpGet("{author}", Name = "GetAuthorById")]
public async Task<ActionResult<AuthorApiViewModel>> Get(Author author)

I renamed id to author and action is also called when action parameter author is null.

bad side: without the ModelBinder Attribute you still cannot access the parameter name in the binder.

Thanks for contacting us, @BenjaminAbt.
@dougbu, can you please look into this? Thanks!

@BenjaminAbt I believe your comments are similar to your confusion about [ApiController] and how that affects ModelState.IsValid checks in dotnet/AspNetCore.Docs#9175. Your questions were answered there and dotnet/AspNetCore.Docs#9377 may improve things for other readers.

In particular,

I'm pretty sure the doc sample wont work.

I'm positive it works in many cases. But, it should be improved to use ModelName and not BinderModelName. I've asked @scottaddie to file the appropriate issue in the Docs repo.

the middleware responds with bad request and the action is not called

This is by design and occurs because your controller has an associated [ApiController]. If you prefer different handling,

Remove that attribute and handle !ModelState.IsValid in your action
Set ApiBehaviorOptions.SuppressModelStateInvalidFilter to false and handle !ModelState.IsValid in your action
Override ApiBehaviorOptions.InvalidModelStateResponseFactory with your preferred behaviour

without the ModelBinderAttribute you still cannot access the parameter name in the binder

This is not true. ModelBindingContext.OriginalModelName always contains that parameter name. And, ModelName uses the parameter name as the prefix (or whole value when at the top level) as long as the prefix matches information found in the form, query string, or route values. (ModelName is the empty string in a few cases, especially if no data with the parameter name is found in the request and no binding attribute provided an explicit name.)

Clarifying

always contains that parameter name

Model binding attributes may override this if the user specifies a Name.

@dougbu I agree with you that the doc should be improved here.
There is no hint that ApiController overrides some stuff.
You only know this when you take a deep look into the ApiController.

But what for me is still not "clear" - and I dont know if I should all it bug - is the confusing behavior when parameter in URL and method dont match.

bindingContext.Result = ModelBindingResult.Success(object)

works if param in url and method match
works if param in url and method dont match

bindingContext.Result = ModelBindingResult.Success(null)

works if param in url and method match
fails with bad request when param in url and method dont match

I know why this happens. But I dont know if this should be "fixed" in MVC; should be covered by Roslyn (custom analyzer) or should just be a hint in the documentation, that the params should match.

@BenjaminAbt MVC client-side validation, model binding and server-side validation work as they're intended to work (with a few small corner cases we continue to address).

The documentation for [ApiController] already mentions such things as automatic 400 responses -- see https://docs.microsoft.com/en-ca/aspnet/core/web-api/?view=aspnetcore-2.1#automatic-http-400-responses And, there too, we continue to address confusions and corner cases.

What is left unreported that you consider a bug?

If you say the reported behavior of bindingContext.Result = ModelBindingResult.Success(null) is like it should be... then we could close this issue..

There's nothing wrong with the Success(...) method accepting null because, for many models, null is valid.

@dougbu.. did you read what I've written???

There is a difference of the behavior between Success(null) and Success(object).
I know that Success() can handle null........ that already shows my sample.

I guess nobody reads what my sample shows ?!

@BenjaminAbt I've responded to your points, read your sample, and provided guidance.

There's no difference in behaviour between Success(null) and Success(object) -- both indicate success for the particular bound property. Your sample doesn't show anything different.

I don't have time to review all of the issues in your model binder. But, I suspect you've got entries in ModelState that are never validated. Use the debugger and see what's going wrong when the names and values are inconsistent.

Backing up, your scenario has no need of a custom model binder. Bind the identifier and do the lookup in the action, returning NotFound() when appropriate.

@dougbu I understand that you have not much time. But I don't understand that you don't want to read the problem I'm describing here properly.

If you say that this is not an appropriate scenario (and I do not agree with you here), why does Microsoft show this exact scenario in the documentation?
https://docs.microsoft.com/en-us/aspnet/core/mvc/advanced/custom-model-binding?view=aspnetcore-2.1#custom-model-binder-sample

Are most useful for eliminating repetitive code and cross-cutting concerns from action methods.

This bug report describes exactly the fact that your example in your documentation cannot work like this.
Because there is a difference between Success(null) and Success(object)

I have tested this several times, shown here with a sufficient example and it is absolutely comprehensible.
cc @mkArtakMSFT

The difference you're seeing is between adding ModelState entries with a name unknown to the rest of the system (id) and the correct name. Again, use ModelName, make sure the names align, and use a debugger if that's insufficient.

Nothing about the success or failure of your scenario is attributable to the model binding system or supposed differences between Success(null) and Success(object). Success(...) doesn't have any special cases and is just

Mvc/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelBindingResult.cs

Line 30 in a6199bb

return new ModelBindingResult( model, isModelSet: true);