Swashbuckle.aspnetcore: Query string parameters missing

[smarttouchtechnology]

I created a new ASP.NET 5 project using Swashbuckle 6.0.0-rc1-final and configured with the following controller:

[Route("api/[controller]")]
public class SecurityController : Controller
{
[HttpPost]
[Route("Login")]
public void Login(string userName, string password)
{
}
}

Swagger renders the method correctly and provides inputs for the parameters but the Request URL sent from the UI doesnt include the parms. This same controller works correctly with a WebAPI 2.2 project.

[domaindrivendev]

Technically, there's nothing in your code that's explicitly calling those parameters out as query string parameters. In fact (and I've just tested this), they could also be sent in the body as form data:

POST /api/security/login HTTP/1.1
Host: localhost:5054
Cache-Control: no-cache
Content-Type: application/x-www-form-urlencoded

username=foob&password=bar

This opens up your API to being used in a way you didn't actually intend - probably harmless in this case but still something I would advise against. In addition, this ambiguity will confuse self-documenting tools like Swashbuckle because they can't be certain which way is the "correct" way. So, I would recommend being more explicit with your parameter bindings:

[HttpPost]
[Route("Login")]
public void Login([FromQuery]string userName, [FromQuery]string password)

[FreakyJ]

I recently had the same problem and solved it with an OperationFilter:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Swashbuckle.SwaggerGen.Generator;

namespace MediaPortal.Plugins.AspNetServer
{
    public class HandleModelbinding : IOperationFilter
  {
    public void Apply(Operation operation, OperationFilterContext context)
    {
      if (operation.Parameters == null) return;

      foreach (IParameter param in operation.Parameters)
      {
        if (param.In == "modelbinding")
          param.In = "query";
      }
    }
  }
}

domaindrivendev is probably right, that one should be more precise, but I think it is a lot of extra code just for making the query string working.

[mjrousos]

Since ASP.NET web APIs support parameters that can be passed in multiple ways, I think it would make sense to have Swashbuckle handle that scenario and do the modelbinding->query mapping automatically.

A lot of people starting out with ASP.NET Core may not specify explicit parameter-in values for their parameters and will be confused if Swashbuckle-generated swagger is invalid for those APIs.

[ericwj]

That "modelbinding" value is not a valid option in Swagger 2.0.

| Field Name | Type | Description |
| --- | :-: | --- |
| in | string | Required. The location of the parameter. Possible values are "query", "header", "path", "formData" or "body". |

Is the idea that you are championing it to be included in the 2.0 spec or a future version?

[mjrousos]

I think the best option would be to automatically apply a filter that @FreakyJ is using. Ideally, API authors would be clearer about how parameters should be passed. But if they aren't, I think it would make sense for Swashbuckle to just pick a way (@FreakyJ uses query) so that the generated swagger is valid.

[ericwj]

I agree on the ideal world - adding the flexibility to the Swagger spec leads to a host of issues, but convention over configuration is the best option I think. Ahoy actually knows in is allowed to be query where it sais modelbinding now, so it could just pick that as a default to be at least valid, unless there is a filter to override that behaviour.

Would the parameter need to be secured, the author has to specify an option that'd be encrypted over SSL after seeing it being padded to the URL...

Putting some invalid string in the swagger is fairly bad...I had AutoRest crash over it.

[domaindrivendev]

A general question: If you only want to document "one way" of passing an API parameter why would you want the API to support alternatives?

An API is a contract and like any contract, ambiguity introduces risk. If you want a consumer to use an operation in a specific way then just be explicit and type the extra 10 or so characters. If you don't do this, you're increasing the surface area of your API meaning more ways for consumers to become coupled and hence making your implementation more difficult to change. You're also leaving future developers of your API left to wonder what exactly was the original intended interaction for these operations.

But alas, if the overwhelming opinion is to continue supporting this practice of ambiguous API design, then I'm left with no choice but to introduce these defaults you're all asking for :)

[ericwj]

I like model binding. Just not in Swagger 2.0 ;)

Conceptually, perhaps it'd be worth to move (or rather, mimic) the Swagger behaviour to (or near) compile time by means of a Roslyn analyzer? Just so there would be warnings upon build for this kind of issues.

That'd be far better than generating invalid Swagger or picking a default that doesn't reflect the API. I mean, model binding still works whatever the Swagger schema sais right? And you're quite right that has its own problems in terms of spec vs use and maintainability etc - the prime violator perhaps being the original developer of the API...

The tricky part might be to faithfully warn/break the build considering the actual Swashbuckle configuration in effect at the time of build, short of actually running the fully configured generator.

[domaindrivendev]

It's worth noting that the next release (already overdue) will raise a runtime error instead of returning invalid Swagger. Compile time would be ideal but might take considerable effort. I think the runtime error is acceptable. Being part of the product, I would hope that some form of testing (automated or otherwise), even a quick smoke test, is carried out on the generated docs.

[ericwj]

So you decided to break model binding. Swaggers job is to generate schema, not impose your opinion of good design, imho

It'd be much more useful to make the schema valid and warn lets say in the Swagger UI, perhaps even only if IHostingEnvironment.IsDevelopment()

Unless you also designed an easy way for the developer to make that error go away, have modelbinding work and generate valid Swagger.

And I mean really, really easy, so no requirements of implementing a filter or so. Lets say by e.g. setting a single property on the options in one of the add/use calls in Startup services.AddSwagger(options => options.DefaultParameterLocation = LocationEnum.FromQuery); or something like that. (Off topic: whats a Gen?)

[jlehew]

"in":"modelbinding" in the swagger.json file generally means the parameter attribute is missing. I generally search the swagger.json file for modelbinding to find the signatures I need to fix.

Here's an example of what the problem looks like in swagger.json

    "parameters": [
      {
        "name": "folderid",
        "in": "modelbinding",

In C# for example, had to add [FromBody], folderid was required and would compile without a [FromRoute] attribute.

    [HttpGet("{folderid}")]
    public IActionResult GET( Int64 folderid, [FromBody] APICall callvalue)

Also look for an optional parameter specified in the Http Attribute with a question mark:

    [HttpGet("{folderid?}")]
    public IActionResult GET( [FromRoute] Int64 folderid)

Optional parameters require the [FromRoute] attribute. When I added this attribute the swagger.json updated to:

    "parameters": [
      {
        "name": "folderid",
        "in": "path",

And AutoRest was able to code generate the client successfully.

Since this is an issue with developer code and it is not possible for a tool to determine developer intentions, there isn't much that can be done except to add a better error message IMO. If the tool defaults to Query or Route, it may not line up with how the code needs to work causing downstream issues instead of pointing to the source of the problem.

John

[domaindrivendev]

@jlehew - I completely agree that a tool of this nature should not attempt to guess the developers intentions and that a more informative error message is the right path forward.

These are added with 9bf2cb4e430eaa5cc515d56b75f8ea9aee294291 and will be available in with the 6.0.0 release

[ericwj]

Well... It _does_ guess. And it produces invalid swagger as a result. "in": "modelbinding" is just plain non-sense - that's my argument.

How would you feel if your favorite web server software framework would start spitting out random HTML tags due to some issue with your code that you didn't detect? How would that render in the browser? That's what is happening here.

If I make an error of the same kind with any other tool or technology, if my project at all builds, the site just crashes at startup. Or I get HTTP 500's.. That'd be much easier to test as well.

E.g. if my database connection string is wrong, or I reference a file that doesn't exist.

[domaindrivendev]

Good man ... as per my last comment, this will be handled by informative error messages and these are already in master, pending release.

@ericwj - thanks for your understanding & support to this OS project which I provide to the community with many personal hours and zero financial gain

[ericwj]

Ah yes, thanks - I should have said I will be looking forward to the 6.0.0 release

Your time and effort is appreciated! Ahoy has certainly been a great addition for .NET Core projects

[domaindrivendev]

Available with latest pre-release. Note the package rename to "Swashbuckle.AspNetCore.1.0.0-rc1"