dotnet / AspNetCore.Docs

Documentation for ASP.NET Core
https://docs.microsoft.com/aspnet/core
Creative Commons Attribution 4.0 International
12.64k stars 25.29k forks source link

Possible error in route parameter name "authorId" #10341

Open stansp2004 opened 5 years ago

stansp2004 commented 5 years ago

I didn't understand why the example in "Custom model binder sample" section employed "authorId" route template parameter whereas action method's signature awaits parameter named "author":

[HttpGet("get/{authorId}")]
public IActionResult Get(Author author)
{
    if (author == null)
    {
        return NotFound();
    }

    return Ok(author);
}

I downloaded source files from the link provided in the abstract, ran them with the Aid of Visual Studio 2017 v.15.9 and noticed that this endpoint didn't work as the bindingContext.ModelName was equal to "". When I renamed the route variable from "authorId" to "author" the endpoint started to work. This is how action method looks after renaming:

[HttpGet("get/{author}")]
public IActionResult Get(Author author)
{
    return Ok(author);
}

Could you possibly update this article if there is an error with route parameter naming?


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

makunhappy commented 5 years ago

@dotnet-bot Same issue to @stansp2004 when I use WebAPI on Ver2.1 .

khtang commented 5 years ago

I think to make the authorId param work according to its intend ("...by fetching the entity from a data source using Entity Framework Core and an authorId..."), you need to assign a "default authorId" in the AuthorEntityBinder class by adding something like below, after the modelName = bindingContext.ModelName assignment:

if (String.IsNullOrEmpty(modelName)) { modelName = "authorId"; }

With this default name in place, the {authorId} in route binds correctly.

Rick-Anderson commented 4 years ago

Moved to #16319

serpent5 commented 4 years ago

@Rick-Anderson Would it be ok to ask @pranavkm to have a quick look at this? I've looked into it and can see a couple of ways to fix it, but both seem smelly so it'd be great if one of the experts could look at it. I can't see it taking very long...

Rick-Anderson commented 4 years ago

@pranavkm please review.

mkArtakMSFT commented 4 years ago

I believe the default guidance should be to name the router parameter as the model action parameter - author in this case. @khtang 's suggestion would also work, but it is not really straight forward and it's a bit arbitrary - as in that case you're completely customizing the matching rule.

@pranavkm can you please confirm that the router parameter in the example here should indeed be author instead of authorId?

Rick-Anderson commented 4 years ago

@pranavkm can you please confirm that the router parameter in the example here should indeed be author instead of authorId?

rwlnd commented 4 years ago

I just ran into something similar after playing around a bit with .NET 5 and I think that the entire example of the custom model binder is quite confusing. The example relies heavily on bindingContext.ModelName but if you are trying to parse the actual request body, this is of no value if I understand correctly.

Although changing the parameter in the route to author instead of authorId might solve the issue, it is far from the convention you should use when writing clean api routes imho. I know this is not supposed to be a real life example but still..

I think it might be better to add an additional example on how to actually parse data from a request body, which is something I was trying to do but turned out not to be as trivial as it thought it would be.

Let me know if this makes any sense. Willing to help improve the documentation based on your feedback.

rafikiassumani-msft commented 2 years ago

@brunolins16, Can you review this and see if we need to fix the docs?

brunolins16 commented 2 years ago

I had a long discussion about this doc, specifically the polymorphic binder sample, and indeed the docs are confusing and not clear about binding from body.

https://github.com/dotnet/AspNetCore.Docs/issues/26626

Related to usage of author or authorId the first example looks wrong should be author, in the route, while the second example explicit talks about a scenario where we want to keep the parameter name and a different route parameter name.

I can send an PR with a quick fix while we decided to improve the doc here.