RicoSuter / NSwag

The Swagger/OpenAPI toolchain for .NET, ASP.NET Core and TypeScript.
http://NSwag.org
MIT License
6.78k stars 1.29k forks source link

Reporting a possible issue for c# API client gen #3339

Open lnaie opened 3 years ago

lnaie commented 3 years ago

Hello, I've just generated a c# client for my API and there seem to be a problem with the rest delete endpoint. The endpoint is: "/api/channels/{referenceId}" The code generated is:

        public async System.Threading.Tasks.Task DeleteChannelAsync(string referenceId, string user_Agent, string x_Api_Version = null, string x_Origin_Country_Code = null, string x_Forwarded_For = null, string accept_Charset = null, string accept_Encoding = null, string cache_Control = null, string if_None_Match = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken))
        {
            var urlBuilder_ = new System.Text.StringBuilder();
            urlBuilder_.Append(BaseUrl != null ? BaseUrl.TrimEnd('/') : "").Append("/api/channels/{referenceId}");
            urlBuilder_.Replace("{ReferenceId}", System.Uri.EscapeDataString(ConvertToString(referenceId, System.Globalization.CultureInfo.InvariantCulture)));

            var client_ = _httpClient;
            var disposeClient_ = false;
            try
            {
                 ...

I did use these settings + the defaults to create the c# api client, with nswagstudio v13.10.7.0:

Screenshot 2021-03-04 at 10 43 07

The problem seems to be at line:

urlBuilder_.Replace("{ReferenceId}", System.Uri.EscapeDataString(ConvertToString(referenceId, System.Globalization.CultureInfo.InvariantCulture)));

where it should be:

urlBuilder_.Replace("{referenceId}", System.Uri.EscapeDataString(ConvertToString(referenceId, System.Globalization.CultureInfo.InvariantCulture)));

It looks to me that the code line with Replace() should be case insensitive to be more fault tolerant.

I hope this helps, Lucian

jeremyVignelles commented 3 years ago

Can you send us the OpenAPI specification for your API ?

lnaie commented 3 years ago

I can provide the endpoint fragment generated with Swashbuckle.AspNetCore:

    "/api/channels/{referenceId}": {
      "delete": {
        "tags": [
          "Channel and Program Data"
        ],
        "summary": "Removes a channel.",
        "operationId": "DeleteChannel",
        "parameters": [
          {
            "name": "ReferenceId",
            "in": "path",
            "description": "Channel reference id.",
            "required": true,
            "schema": {
              "type": "string",
              "description": "Channel reference id.",
              "nullable": true
            }
          },
          {
            "name": "X-Api-Version",
            "in": "header",
            "description": "API version",
            "style": "simple",
            "schema": {
              "type": "string"
            },
            "example": "1.0"
          },
          {
            "name": "User-Agent",
            "in": "header",
            "description": "The User-Agent should be filled with the following information: Client Name, Client Version, OS Name, OS Version",
            "required": true,
            "style": "simple",
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "X-Origin-Country-Code",
            "in": "header",
            "description": "Geo IP country code",
            "style": "simple",
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "X-Forwarded-For",
            "in": "header",
            "description": "Geo IP address",
            "style": "simple",
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "Accept-Charset",
            "in": "header",
            "description": "Other values than UTF-8 are not supported at this time.",
            "style": "simple",
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "Accept-Encoding",
            "in": "header",
            "description": "At the moment only gzip is supported.",
            "style": "simple",
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "Cache-Control",
            "in": "header",
            "description": "Depending on the resource we might ignore these properties.",
            "style": "simple",
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "If-None-Match",
            "in": "header",
            "description": "Used for cache check when ETag is used.",
            "style": "simple",
            "schema": {
              "type": "string"
            }
          }
        ],
        "responses": {
          "204": {
            "description": "Success"
          },
          "400": {
            "description": "Bad Request",
            "content": {
              "text/plain": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              },
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              },
              "text/json": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              }
            }
          },
          "409": {
            "description": "Conflict",
            "content": {
              "text/plain": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              },
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              },
              "text/json": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              }
            }
          },
          "401": {
            "description": "Unauthorized",
            "content": {
              "text/plain": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              },
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              },
              "text/json": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              }
            }
          },
          "500": {
            "description": "Server Error",
            "content": {
              "text/plain": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              },
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              },
              "text/json": {
                "schema": {
                  "$ref": "#/components/schemas/IErrorResult"
                }
              }
            }
          },
          "503": {
            "description": "Server Error"
          }
        }
      }
    },
jeremyVignelles commented 3 years ago

Looks like a Swashbuckle issue:

"/api/channels/{referenceId}": {

But in the parameters section: "name": "ReferenceId",

lnaie commented 3 years ago

That is true, and it's the default Swashbuckle behaviour. Otherwise special annotations are needed to preserve params' casing. But the current NSwag implementation is making it also very rigid, while if a case insensitive replace will be performed then won't be any problems. It should not matter if the parameter is or not case sensitive. I see it as a plus for NSwag.

RicoSuter commented 3 years ago

It should not matter if the parameter is or not case sensitive

I have the expectation that this is actually an invalid OpenAPI spec.

From https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#fixed-fields-10

If in is "path", the name field MUST correspond to the associated path segment from the path field in the Paths Object. See Path Templating for further information.

I'd say it does not match...

while if a case insensitive replace will be performed

We could do that yes, but i wonder if it is really the right approach to be that resilient and also support invalid specs?