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

Client generated code, from OpenAPI, could not bind route parameter of a Minimal Endpoint when route pattern's parameter and delegate's parameter case does not match #41422

Closed
1 task done
brunolins16 opened this issue Apr 28, 2022 · 1 comment · Fixed by #41701
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels bug This issue describes a behavior which is not expected - a bug. feature-minimal-actions Controller-like actions for endpoint routing feature-openapi
Milestone

Comments

@brunolins16
Copy link
Member

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

When I have a minimal endpoint route pattern that includes a route parameter that does not match the delegate parameter name case, eg.:

app.MapGet("/todo/{id}", (int Id)  => Results.Ok());

The route system will be able to bind the parameter value correctly, since it is case insensitive and all incoming request will be processed correctly.

if (string.Equals(parameter.Name, name, StringComparison.OrdinalIgnoreCase))

However, when the OpenAPI metadata is enabled, the following metadata is generated, where the parameter name matches the delegate parameter name case instead of the route parameter.

"paths": {
    "/todo/{id}": {
      "get": {
        "tags": [
          "WebApplication4"
        ],
        "parameters": [
          {
            "name": "Id",
            "in": "path",
            "required": true,
            "schema": {
              "type": "integer",
              "format": "int32"
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Success"
          }
        }
      }
    }
  },

This behavior might seem irrelevant and undetectable, however, when the Open API metadata is used in a client generation tool, eg. VS Connected Services tool or even Swagger UI, this error will cause the client to no provide the route parameter value correctly, causing the following issue because the request url will be something like http://server:port/todo/{id} since the generated code is not able to replace the {id} with the provided value.

Microsoft.AspNetCore.Http.BadHttpRequestException: Failed to bind parameter "int Id" from "{id}".
   at lambda_method2(Closure, Object, HttpContext)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext)
--- End of stack trace from previous location ---
   at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext)
   at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

The following block of code is an example of the client generated code where the request url is build and fail to replace the placeholder with the appropriated value.

var urlBuilder_ = new System.Text.StringBuilder();
urlBuilder_.Append(BaseUrl != null ? BaseUrl.TrimEnd('/') : "").Append("/todo/{id}");
urlBuilder_.Replace("{Id}", System.Uri.EscapeDataString(ConvertToString(id, System.Globalization.CultureInfo.InvariantCulture)));

This error could be easily fixed by changing the delegate's parameter or route pattern's parameter case, however, as mentioned the error is undetectable and only the client generation will be affected. Another important point is that this error will be more likely to happen when #40712 is available

Expected Behavior

Since the routing system is case insensitive already, I would expect that the following endpoints produce the same metadata:

app.MapGet("/todo/{id}", (int Id)  => Results.Ok());
app.MapGet("/todo/{id}", (int id)  => Results.Ok());
"paths": {
    "/todo/{id}": {
      "get": {
        "tags": [
          "WebApplication4"
        ],
        "parameters": [
          {
            "name": "id",
            "in": "path",
            "required": true,
            "schema": {
              "type": "integer",
              "format": "int32"
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Success"
          }
        }
      }
    }
  },

If the correct metadata generation is not possible, we should at least have the tool available in Visual Studio able to generate a client capable of binding the route parameter value correctly.

Steps To Reproduce

Server

Program.cs

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddEndpointsApiExplorer();
builder.Services.AddSwaggerGen();

var app = builder.Build();

app.MapGet("/todo/{id}", (int Id)  => Results.Ok());

app.Run();

Client

While running the server application, collect the metadata json file and try to create the generated client with the following steps:

  1. Add a service reference from OpenAPI
    image

  2. Specify the required information for the reference
    image

  3. Add the following code to your app

using var httpClient = new HttpClient();
var todoService = new ConsoleApp2.Endpoints.TodoService("[your app url]", httpClient);
await todoService.TodoAsync(10);

Exceptions (if any)

Microsoft.AspNetCore.Http.BadHttpRequestException: Failed to bind parameter "int Id" from "{id}".
   at lambda_method2(Closure, Object, HttpContext)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext)
--- End of stack trace from previous location ---
   at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext)
   at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

.NET Version

7.0.100-preview.4.22218.7

Anything else?

N/A

@brunolins16 brunolins16 added feature-openapi feature-minimal-actions Controller-like actions for endpoint routing area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Apr 28, 2022
@halter73 halter73 mentioned this issue Apr 28, 2022
4 tasks
@captainsafia
Copy link
Member

Triage: We do have access to the RoutePattern in both the ApiExplorer and the new M.A.OpenApi package so we can determine the actual parameter name as defined in the route instead of the mismatched parameter name.

In general, we propose adding a new rule here that favors using the name in the template instead of the parameter name for request parameters where applicable.

@captainsafia captainsafia added the bug This issue describes a behavior which is not expected - a bug. label May 3, 2022
@captainsafia captainsafia self-assigned this May 3, 2022
@captainsafia captainsafia added this to the 7.0-preview5 milestone May 3, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 17, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels bug This issue describes a behavior which is not expected - a bug. feature-minimal-actions Controller-like actions for endpoint routing feature-openapi
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants