Fix: Path Parameter Not In Path Issue

[Scullyon]

Summary

Fixing issue whereby the generated swagger includes path parameters that don't actually appear in the relative path / route. The new behaviour is to omit any path parameters (present in the action method signature) that do not appear in the relative path / route.

Reason for Change

Importing swagger into some other systems with a required path parameter that does not appear in the path causes validation issues.

Details

Given a controller that has two routes:

[ApiController]
[Route("[controller]")]
public class WeatherForecastController : ControllerBase
{
  
    /// <summary>
    /// Returns weather forecasts for the next 5 days.
    /// </summary>
    /// <response code="200">Success / resource found.</response>
    [HttpGet]
    [Route("{message}")]
    [Route("{message}/{secondMessage}")]
    [ProducesResponseType(typeof(string), (int) HttpStatusCode.OK)]
    public IActionResult GetMore([FromRoute] string message, [FromRoute] string secondMessage = "")
    {
        if (string.IsNullOrWhiteSpace(message))
        {
            return Ok("Nothing to see here");
        }

        return Ok($"{message} {secondMessage ?? "no second message"}");
    }
}

The following swagger was being generated:

{
  "openapi": "3.0.1",
  "info": {
    "title": "Weather Forecast Test API",
    "description": "The best default template API you'll ever see...maybe.",
    "termsOfService": "https://my-domain.com/terms_of_service",
    "contact": {
      "name": "Alice Bobinson",
      "url": "https://my-domain.com/contact"
    },
    "license": {
      "name": "The License",
      "url": "https://my-domain.com/license"
    },
    "version": "v1"
  },
  "paths": {
    "/WeatherForecast/{message}": {
      "get": {
        "tags": [
          "WeatherForecast"
        ],
        "summary": "Returns weather forecasts for the next 5 days.",
        "parameters": [
          {
            "name": "message",
            "in": "path",
            "required": true,
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "secondMessage",
            "in": "path",
            "required": true,
            "schema": {
              "type": "string",
              "default": ""
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Success / resource found.",
            "content": {
              "text/plain": {
                "schema": {
                  "type": "string"
                }
              },
              "application/json": {
                "schema": {
                  "type": "string"
                }
              },
              "text/json": {
                "schema": {
                  "type": "string"
                }
              }
            }
          }
        }
      }
    },
    "/WeatherForecast/{message}/{secondMessage}": {
      "get": {
        "tags": [
          "WeatherForecast"
        ],
        "summary": "Returns weather forecasts for the next 5 days.",
        "parameters": [
          {
            "name": "message",
            "in": "path",
            "required": true,
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "secondMessage",
            "in": "path",
            "required": true,
            "schema": {
              "type": "string",
              "default": ""
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Success / resource found.",
            "content": {
              "text/plain": {
                "schema": {
                  "type": "string"
                }
              },
              "application/json": {
                "schema": {
                  "type": "string"
                }
              },
              "text/json": {
                "schema": {
                  "type": "string"
                }
              }
            }
          }
        }
      }
    }
  },
  "components": { },
  "tags": [
    {
      "name": "WeatherForecast",
      "description": "The default weather forecast service example included in the WebAPI template."
    }
  ]
}

Note that in the above JSON for the first route with the path /WeatherForecast/{message}, the secondMessage parameter is listed as a required path parameter, despite not actually being in the path.

The new output is:

{
  "openapi": "3.0.1",
  "info": {
    "title": "Weather Forecast Test API",
    "description": "The best default template API you'll ever see...maybe.",
    "termsOfService": "https://my-domain.com/terms_of_service",
    "contact": {
      "name": "Alice Bobinson",
      "url": "https://my-domain.com/contact"
    },
    "license": {
      "name": "The License",
      "url": "https://my-domain.com/license"
    },
    "version": "v1"
  },
  "paths": {
    "/WeatherForecast/{message}": {
      "get": {
        "tags": [
          "WeatherForecast"
        ],
        "summary": "Returns weather forecasts for the next 5 days.",
        "parameters": [
          {
            "name": "message",
            "in": "path",
            "required": true,
            "schema": {
              "type": "string"
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Success / resource found.",
            "content": {
              "text/plain": {
                "schema": {
                  "type": "string"
                }
              },
              "application/json": {
                "schema": {
                  "type": "string"
                }
              },
              "text/json": {
                "schema": {
                  "type": "string"
                }
              }
            }
          }
        }
      }
    },
    "/WeatherForecast/{message}/{secondMessage}": {
      "get": {
        "tags": [
          "WeatherForecast"
        ],
        "summary": "Returns weather forecasts for the next 5 days.",
        "parameters": [
          {
            "name": "message",
            "in": "path",
            "required": true,
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "secondMessage",
            "in": "path",
            "required": true,
            "schema": {
              "type": "string",
              "default": ""
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Success / resource found.",
            "content": {
              "text/plain": {
                "schema": {
                  "type": "string"
                }
              },
              "application/json": {
                "schema": {
                  "type": "string"
                }
              },
              "text/json": {
                "schema": {
                  "type": "string"
                }
              }
            }
          }
        }
      }
    }
  },
  "components": { },
  "tags": [
    {
      "name": "WeatherForecast",
      "description": "The default weather forecast service example included in the WebAPI template."
    }
  ]
}

[geckotron]

Is the issue not with the defined routes?

There is most likely only one route specification required for the provided example controller action, which should clearly indicate that the secondMessage route parameter is optional by using the optional parameter notation which is absent from the provided example.

Optional parameter documentation https://docs.microsoft.com/en-us/aspnet/web-api/overview/web-api-routing-and-actions/attribute-routing-in-web-api-2#optional-uri-parameters-and-default-values

[Scullyon]

Thanks for your comment @geckotron. Optional route parameters are not allowed by the OpenAPI spec and so the only 'OpenAPI' way I can see to get around this is to have multiple routes per action.

[martincostello]

Thanks for contributing - if you'd like to continue with this pull request, please rebase against the default branch to pick up our new CI.

[Scullyon]

Thanks @martincostello. I don't have a great deal of spare time at the moment, so my question would be: is anyone actually interested in the changes in this branch and merging them, if I did get time to update it to the latest? Cheers.

[martincostello]

I'd hope a rebase would be 5 minutes of time to do.

Once rebased (so it builds against our new CI and could be merged) I'll take a look at it.