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

The method 'Post' on path '/api/Products' is registered multiple times. #664

Closed
couellet opened this issue Mar 9, 2017 · 27 comments
Closed

Comments

@couellet
Copy link

couellet commented Mar 9, 2017

Looks similar to a previous issue but it happens with the 9.10 version. My API is built with ASP.NET Web API 5.2.3

I keep getting the following exception:

The method 'Post' on path '/Products' is registered multiple times.

Here's my controller signature:

[SwaggerOperation("GetProducts")]
[HttpGet, Route("products")]
public async Task<IHttpActionResult> GetProducts([FromUri] ProductPagedResult payload)

[SwaggerOperation("GetProductByUserDefinedId")]
[HttpGet, Route("products/{userDefinedId}")]
public IHttpActionResult GetProduct(string userDefinedId)

[SwaggerOperation("GetProductByUniqueId")]
[HttpGet, Route("products/{id:guid}")]
public IHttpActionResult GetProductByUniqueId(Guid id)

[SwaggerOperation("DeleteProductByUserDefinedId")]
[HttpDelete, Route("products/{userDefinedId}")]
public async Task<IHttpActionResult> Delete(string userDefinedId)

[SwaggerOperation("DeleteProductByUniqueId")]
[HttpDelete, Route("products/{id:guid}")]
public async Task<IHttpActionResult> DeleteByUniqueId

[SwaggerOperation("PutProductByUserDefinedId")]
[HttpPut, Route("products/{userDefinedId}")]
public async Task<IHttpActionResult> Put(string userDefinedId, [FromBody] Product data)

[SwaggerOperation("PutProductByUniqueId")]
[HttpPut, Route("products/{id:guid}")]
public async Task<IHttpActionResult> Put(Guid id, [FromBody] Product data)

[SwaggerOperation("PostProducts")]
[HttpPost, Route("products")]
public async Task<IHttpActionResult> FetchAll([FromBody] AddProductPayload data)

And finally, here's my code that returns the Swagger JSON document:

var settings = new WebApiToSwaggerGeneratorSettings
{
    DefaultUrlTemplate = "{controller}/{id}",
    AddMissingPathParameters = false,
};

var generator = new WebApiToSwaggerGenerator(settings);
var document = await generator.GenerateForControllerAsync<ProductsController>();
var swaggerSpecification = document.ToJson();

var response = new HttpResponseMessage(HttpStatusCode.OK)
    {
        Content = new StringContent(swaggerSpecification, Encoding.UTF8, "application/json")
    };

return response;
@RicoSuter RicoSuter added this to the vNext milestone Mar 9, 2017
RicoSuter added a commit that referenced this issue Mar 16, 2017
@RicoSuter
Copy link
Owner

Works for me: 532cd25

Can you tell me how to change the unit test to reproduce your problem?

@couellet
Copy link
Author

Thanks, I'll try to make the unit test fail ;)

@RicoSuter
Copy link
Owner

RicoSuter commented Mar 20, 2017

Can you test with the latest CI artifacts?

I hope this pull request fixes your problem: #683

@Cephei

@RicoSuter
Copy link
Owner

Can you confirm that this is still a problem with v10.4?

@brianhyder
Copy link

I downloaded v10.6 today and tried it. I ran into the same issue, for what it is worth. However, in full discloser this is the first time I have used this project.

@RicoSuter
Copy link
Owner

Have you tried "api/{controller}/{action}/{id}" as template?

@brianhyder
Copy link

I was running against a different set of endpoints than what was used in the description above. The controller in question declared the same route pattern but distinguished between them via explicit HTTP verbs declared as attributes.

After running into the issue and then reading the above I downgraded to v9.12 and everything worked as desired.

@shaihan
Copy link

shaihan commented May 24, 2017

The issue is reproduced in version 10.6. 10.4 version seems to work fine

@RicoSuter
Copy link
Owner

Maybe a regression of #738

@RicoSuter
Copy link
Owner

Is this still an open issue with v11.1? If yes, we have to investigate further...

@ceastwood
Copy link

ceastwood commented Jul 10, 2017

@RSuter I was just looking through some of the references and I believe the op referenced issue might be present in 11.3.0 but let me know if it is just me.

Asp.Net Core v1.1.2
NSwag.AspNetCore v11.3.0

snip from controller

 [HttpGet]
        public Task<IEnumerable<SftpUser>>  GetAll()
        {
            return _service.GetAll();
        }

        [HttpGet(Name="GetByClientId")]
        public Task<IEnumerable<SftpUser>> GetByClientId([RequiredFromQuery]int clientId)
        {
            return _service.GetByClientId(clientId);
        }

        [HttpGet("{uid}",Name = "GetById")]
        public Task<SftpUser> GetById(int uid)
        {
            return _service.Get(uid);
        }

swagger.json returns the following exception

System.InvalidOperationException: The method 'Get' on path '/v1/Sftp' is registered multiple times (check the DefaultUrlTemplate setting [default for Web API: 'api/{controller}/{id}'; for MVC projects: '{controller}/{action}/{id?}']).

Note: I went and applied [SwaggerOperation("xyz")] to the separate HttpGet methods but no change in the result.

@ceastwood
Copy link

ceastwood commented Jul 10, 2017

@RSuter I have installed a few versions of NSwag.AspNetCore down to v10.0.0 and the issue was still present. For now I have changed the controller as follows and everything generates nicely.

[HttpGet]
        public Task<IEnumerable<SftpUser>>  GetAll()
        {
            return _service.GetAll();
        }

        [HttpGet("ByClientId",Name="GetByClientId")]
        public Task<IEnumerable<SftpUser>> GetByClientId([RequiredFromQuery]int clientId)
        {
            return _service.GetByClientId(clientId);
        }

        [HttpGet("{uid}",Name = "GetById")]
        public Task<SftpUser> GetById(int uid)
        {
            return _service.Get(uid);
        }

Looks like the swagger document is aware of using the Controller method name though:
snip from swagger.json

"/v1/Sftp": {
"get": {
"tags": [
"Sftp"
],
"operationId": "Sftp_GetAll",
"parameters": [],
"responses": {
"200": {
"description": "",
"schema": {
"type": "array",
"items": {
"$ref": "#/definitions/SftpUser"
}
},
"x-nullable": true
}
},
"security": [
{
"oauth2": []
}
]
},

@RicoSuter
Copy link
Owner

The problem is that

Task<IEnumerable<SftpUser>>  GetAll()

and

public Task<IEnumerable<SftpUser>> GetByClientId([RequiredFromQuery]int clientId)

have the same route and HTTP method (GET). The SwaggerOperation name is just used for the OperationId, it does not change the route or HTTP method... The question is now: Can you actually call both operations with HTTP requests? What are the actual routes processed by Web API? If this works, then the NSwag handling is wrong - but I need to know how Web API is handling this...

@ceastwood
Copy link

ceastwood commented Jul 11, 2017

@RSuter

In this example, the following routes work just fine (webapi) but swagger fails to generate--

[HttpGet]
        public Task<IEnumerable<SftpUser>>  GetAll()
        {
            return _service.GetAll();
        }

        [HttpGet]
        public Task<IEnumerable<SftpUser>> GetByClientId([RequiredFromQuery]int clientId)
        {
            return _service.GetByClientId(clientId);
        }

        [HttpGet("{uid}",Name = "GetById")]
        public Task<SftpUser> GetById(int uid)
        {
            return _service.Get(uid);
        }

(respectively, and I realize my pathing should be SftpUser ;D)
/v1/Sftp (GetAll)
/v1/Sftp?clientId=20 (GetByClientId)
/v1/Sftp/13 (GetById)

My implementation of RequredFromQueryAttribute and RequiredFromQueryActionConstraint follows, without it, I would normally get the Microsoft.AspNetCore.Mvc.Internal.AmbiguousActionException for multiple matching routes.

public class RequiredFromQueryAttribute: FromQueryAttribute, IParameterModelConvention
    {
        public void Apply(ParameterModel parameter)
        {
            if (parameter.Action.Selectors != null && parameter.Action.Selectors.Any())
            {
                parameter.Action.Selectors.Last().ActionConstraints.Add(new RequiredFromQueryActionConstraint(parameter.BindingInfo?.BinderModelName ?? parameter.ParameterName));
            }
        }
    }
public class RequiredFromQueryActionConstraint : IActionConstraint
  {
      private readonly string _parameter;

      public RequiredFromQueryActionConstraint(string parameter)
      {
          _parameter = parameter;
      }

      public int Order => 768;

      public bool Accept(ActionConstraintContext context)
      {
          if (!context.RouteContext.HttpContext.Request.Query.ContainsKey(_parameter))
          {
              return false;
          }

          return true;
      }
  }

@RicoSuter
Copy link
Owner

I think the problem is that this cannot be described with swagger, because there can only be one operation per route and

/v1/Sftp (GetAll)
/v1/Sftp?clientId=20 (GetByClientId)

Have the same route

@whoiskevinrich
Copy link

whoiskevinrich commented Aug 10, 2017

I had this issue while using a dotnet core project targeting net462. The controller in question had a single get method. I think the issue might have something to do with .net core sharing a single controller between MVC and WebAPI. I had to filter the assembly types specifically down to the folder in which I kept my ApiControllers. I was able to overcome the issue by using the following snippet:

\* in startup.cs Configure method *\
var assembly = Assembly.GetAssembly(typeof(Startup));

\* where path is path to api controllers... *\
var path = ".Controllers.Api.";
var assemblyTypes = assembly.GetTypes()
                .Where(x => x.FullName != null && x.FullName.StartsWith($"{assembly.GetName().Name}.Controllers.Api."));

app.UseSwaggerUi(assemblyTypes, settings => 
{
    settings.IsAspNetCore = true;
    settings.DefaultPropertyNameHandling = PropertyNameHandling.CamelCase;
};

I'm swamped the rest of the evening, but I'm happy to provide a project sample that exhibits this issue.

@nkm8
Copy link

nkm8 commented Nov 1, 2017

@RSuter This issue is tagged as done - is a fix ready for the next release?

This is a fairly common occurrence in a large WebApi project - C# even allows you to have the same function name with overloaded parameters.

@RicoSuter
Copy link
Owner

The problem is that this cannot be described with swagger. The only solution would be to merge these operation into one swagger operation. But i think this is not worth the effort. You should design your api for swagger...

@nkm8
Copy link

nkm8 commented Nov 2, 2017

@RSuter The API I am trying to generate Swagger docs for is over 4,000 lines of code, so it isn't feasible to completely re-architect the way it was built.

Is there any way to make the endpoints unique based on a hash, function name, parameters, or something else? I believe that structuring APIs this way is fairly common and is well within the best practices for defining endpoints.

I am also confused why using something like [SwaggerOperation("GetProductByUserDefinedId")] doesn't work. If we use annotations to uniquely identify the endpoints, shouldn't that be sufficient for Swagger?

@RicoSuter
Copy link
Owner

SwaggerOperation only sets the operationId in the spec, but the paths are still the same.

The problem is the spec: You can only have one operation per path and HTTP method (and the query params are not part of the path). How would you describe your API with Swagger then?

@nkm8
Copy link

nkm8 commented Nov 2, 2017

API endpoints are like functions and I would argue that the query params are part of the path. You need to know the function parameters to call a function; you also need to know the query parameters to call an API endpoint.

I am new to Swagger, so forgive my lack of knowledge, but if the purpose of a tool is to document API endpoints, then one would assume that the tool supports expressing API documentation in every way an API endpoint can be defined.

@RicoSuter
Copy link
Owner

I'm have completely the same opinion as you, and yes, Swagger/Open API has some (serious) limitations and you cannot describe every possible API...

https://swagger.io/specification/#pathsObject

As I said, it would be possible to merge API operations with the same path into one operation - but in this case you would loose information (you only have one operation in the description).

@RicoSuter
Copy link
Owner

@couellet

From https://swagger.io/specification/#pathsObject

Templated paths with the same hierarchy but different templated names MUST NOT exist as they are identical. In case of ambiguous matching, it's up to the tooling to decide which one to use.

violated for example by:

[SwaggerOperation("PutProductByUserDefinedId")]
[HttpPut, Route("products/{userDefinedId}")]
public async Task<IHttpActionResult> Put(string userDefinedId, [FromBody] Product data)

[SwaggerOperation("PutProductByUniqueId")]
[HttpPut, Route("products/{id:guid}")]
public async Task<IHttpActionResult> Put(Guid id, [FromBody] Product data)

@nkm8
Copy link

nkm8 commented Nov 2, 2017

Interesting - I had no idea that the Swagger spec was so limited/strict. I am curious what the reasoning is for this restriction - it is clearly functionally possible since it is supported by tools like C# WebApi etc.

Thank you for the information - it leaves me stuck, but it is good to know that you need to build an Api within the rules of Swagger if you want to use it.

@stijnherreman
Copy link
Contributor

@nkm8 See domaindrivendev/Swashbuckle.WebApi#142 (comment) for some background information about the query parameters not being part of the path:

[...]
In it's essence, the constraint really just says the following paths have to be described as one operation.

GET api/products
GET api/products?producType={productType}

Behind the scenes these may be implemented as separate C# or separate Java methods, but in the context of describing a REST API, would you describe them separately? Swagger 2.0 says "No ... it's one operation with an optional productType parameter".
[...]

@nkm8
Copy link

nkm8 commented Jan 24, 2018

@stijnherreman That is some awesome insight; thank you for the link. It is nice to see that these considerations were brought up when the spec was created, but unfortunate that they weren't directly addressed.

The example of different return types (list vs. single item) is the type of occurrence I see most often in our API. Our list of controllers is ~100, with several endpoints for each controller, so it isn't possible to re-structure the API design.

I will definitely follow the swagger spec for future endpoints to allow us to leverage swagger documentation for all new code.

@neildodson
Copy link

FYI you may also get this kind of message if you're using API versioning with multiple versions of the same action, and you haven't narrowed the swagger document contents down to a single version (set your version in the ApiGroupNames property when configuring the generator settings).

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

9 participants