Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional parameters don't work #122

Closed
mkatrikh opened this issue May 3, 2017 · 5 comments
Closed

Optional parameters don't work #122

mkatrikh opened this issue May 3, 2017 · 5 comments
Assignees

Comments

@mkatrikh
Copy link

mkatrikh commented May 3, 2017

There was controller in asp.net core 1.1 with action method with optional parameter:

[Route("api")]
 public abstract class BaseController {
        // omitted
        [HttpPost("[controller]/{id?}")]
        public virtual async Task<IActionResult> Post([FromBody]TDto dto, int? id, string[] include)
        {
              // omitted
         }
}

It worked. then versioning was added. and this pattern stops work:

[Route("api/v{version:apiVersion}")]
public abstract class BaseController {

with error

Microsoft.AspNetCore.Mvc.Versioninig.ApiVersionActionSelector[5] Multiple candidate actions were found, but none matched the requested service API version '1.0'. Candidate actions:

If I add constraint to route pattern for method ("[controller]/{id:int?}") it strats work again but only with supplied id value. The only solution I found was adding new action method with url template "[controller]".
Is it error or I missed something?

@commonsensesoftware commonsensesoftware self-assigned this May 3, 2017
@commonsensesoftware
Copy link
Collaborator

Can you provide a skeleton of one of your controllers and the request URLs that you expect to work? Sharing any additional API versioning configuration that you've specified would also be help (unless it's the default).

@mkatrikh
Copy link
Author

mkatrikh commented May 3, 2017

API versioning configuration:

services.AddApiVersioning(
                o =>
                {
                    o.DefaultApiVersion = new ApiVersion(1, 0);
                } 
);

base controller class:

    [Route("api/v{version:apiVersion}")]
    public abstract class BaseController<TEntity, TDto, TShortDto> : ControllerBase where TEntity : BasePoco
    {
        protected IRepository<TEntity> repository { get; set; }
        protected IMapper mapper { get; set; }
        protected IContextResolver contextResolver { get; set; }


        public BaseController(IRepository<TEntity> repository, IMapper mapper, IContextResolver contextResolver)
        {
            this.repository = repository;
            this.mapper = mapper;
            this.contextResolver = contextResolver;
        }

        [HttpGet("[controller]")]
        public virtual async Task<IActionResult> GetCollection(BasePocoParameters entityParameters, CollectionParameters collectionParameters, string[] include)
        {
                //...
        }

        [HttpGet("[controller]/{id:int}")]
        public virtual async Task<IActionResult> Get(int id, string[] include)
        {
          //...
        }

        [HttpPost("[controller]/{id:int?}")]
        public virtual async Task<IActionResult> Post([FromBody]TDto dto, int? id, string[] include)
        {
             //...
        }

        [HttpDelete("[controller]/{id:int}")]
        public virtual async Task<IActionResult> Delete(int id) 
        {
              //...
         }
         //...
    }

controller derived from basic class:

    public class PeopleController : BaseController<Person, PersonDto, PersonShortDto>
    {
        
        public PeopleController(IRepository<Person> repository, IMapper mapper, IContextResolver contextResolver) : base(repository, mapper, contextResolver)
        {
        }

        [HttpGet("[controller]")]
        public async Task<IActionResult> GetCollection(PersonParametersDto entityParametersDto, CollectionParametersDto collectionParametersDto, string[] include)
        {
            var entityParameters = mapper.Map<PersonParameters>(entityParametersDto);
            var collectionParameters = mapper.Map<CollectionParameters>(collectionParametersDto);
            return await base.GetCollection(entityParameters, collectionParameters, include);
        }
    }

after POST request to /api/v1.0/people with new person info in request body I want method Post([FromBody]TDto dto, int? id, string[] include) handle this request

@commonsensesoftware
Copy link
Collaborator

OK ... this is an issue, but not a new one. The problem isn't specifically about optional parameters. You can see another variation in #106.

In short, two things cause this behavior:

  • Overlapping routes with different route constraints in the parameters (e.g. [controller]/{id:int} and [controller]/{id:int?})
  • The IActionSelector is called multiple times, which is unexpected

I don't know how or why - yet, but IActionSelector.SelectBestCandidate is called multiple times by ASP.NET in your scenario and others like it. This is unexpected. The API versioning IActionSelector short-circuits the pipeline and returns 400. Since it will be called 2+ times, this process is happening too early.

I'm in the process of tracking down the issue and engaging with the ASP.NET team. This behavior is not documented anywhere. In the meantime, you can work around the problem by making your route templates be the same when they overlap for a single controller. In other words, make them both:

[controller]/{id:int}

OR

[controller]/{id:int?}

I tried both variations and they work. This is not a solution and is far from ideal, but it should unblock you in the interim. This likely means you'll have to have your own custom validation in one of the paths that you should get for free using the route constraints.

commonsensesoftware added a commit that referenced this issue May 10, 2017
Refactors the ApiVersionActionSelector and fixes re-entry related issues. Related to #106, #122, and possibly #123.
@commonsensesoftware
Copy link
Collaborator

The 1.1 RC has now been published that resolves this issue. Note that there is breaking, behavioral change for ASP.NET Core to support it. :( You'll now need to call app.UseMvc().UseApiVersioning(). This is called out in the release notes.

Let me know if you run into anything, but I think 1.1 is ready.

@mkatrikh
Copy link
Author

it works. thank you

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

No branches or pull requests

2 participants