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

Generated client code does not replace/set route parameters with 14.0.0-preview010 #4587

Open
vas6ili opened this issue Nov 17, 2023 · 14 comments

Comments

@vas6ili
Copy link

vas6ili commented Nov 17, 2023

For a path template like /odata/QueueRetention({key}) generated client code does not replace {key} url route parameter. Used net80/dotnet-nswag.exe from https://github.com/RicoSuter/NSwag/releases/tag/v14.0.0-preview010 with net8 (which is the reason I'm upgrading to 14)

Generated client code snippet

var urlBuilder_ = new System.Text.StringBuilder();

urlBuilder_.Append("odata");
urlBuilder_.Append('/');
urlBuilder_.Append("QueueRetention({key})");

PrepareRequest(client_, request_, urlBuilder_);

var url_ = urlBuilder_.ToString();
request_.RequestUri = new System.Uri(url_, System.UriKind.RelativeOrAbsolute);

PrepareRequest(client_, request_, url_);

API definition snippet

    "/odata/QueueRetention({key})": {
        "delete": {
            "description": "",
            "operationId": "QueueRetention_Delete",
            "parameters": [
                {
                    "in": "path",
                    "name": "key",
                    "required": true,
                    "schema": {
                        "format": "int64",
                        "type": "integer"
                    },
                    "x-position": 1
                },
                {
                    "in": "header",
                    "name": "api-version",
                    "schema": {
                        "nullable": true,
                        "type": "string"
                    },
                    "x-position": 2
                }
            ],
            "responses": {
                "204": {
                    "description": ""
                }
            },
            "tags": [
                "QueueRetention"
            ]
        }
    }

test-swag.json
NswagClient.zip

Probably related to #4579 @paulomorgado

@paulomorgado
Copy link
Contributor

OOPS! I'll work on that tomorrow.

@RicoSuter, what's the best way to add a unit test for this?

@lahma
Copy link
Collaborator

lahma commented Nov 17, 2023

NJsonSchema uses Verify whici is IMO a nice way to test as snapshots are produced and VCS controlled and you can see differences in pull request when output changes. I think this hasn't been taken into use (yet) on NSwag's side though.

@paulomorgado
Copy link
Contributor

I've added corrections to my draft PR: #4585.

Please, checkout if that solves the issue.

@vas6ili
Copy link
Author

vas6ili commented Nov 17, 2023

It did solve the issue. I'm inspecting a quite large 8 MB generated client from a 2.5 MB swagger file and haven't found any other issue yet.

@RicoSuter
Copy link
Owner

Can you confirm that the latest preview works fine for you?

@vas6ili
Copy link
Author

vas6ili commented Dec 12, 2023

Can confirm with latest RC3 the issue no longer reproduces

@bzwieratinnovadis
Copy link

bzwieratinnovadis commented Jan 4, 2024

@RicoSuter @vas6ili I'm having a similar issue with route parameters with the stable release (v14.0.0) in a .NET 8 project.
I'm using NSwag.MSBuild to generate a CSharp client.

I have the following controller action:

[HttpGet("confirm/{batch}")]
public Task<MessageBoxConfirmBatchModel> ConfirmAsync(Guid batch) => _batchService.ConfirmAsync(batch);

OpenAPI definition:

"/api/message-box-batch/confirm/{batch}": {
  "get": {
    "tags": [
      "MessageBoxBatch"
    ],
    "parameters": [
      {
        "name": "OrganizationReference",
        "in": "header",
        "required": true,
        "schema": {
          "type": "string",
          "format": "uuid"
        }
      },
      {
        "name": "batch",
        "in": "path",
        "required": true,
        "schema": {
          "type": "string",
          "format": "uuid"
        }
      }
    ],
    "responses": {
      "200": {
        "description": "Success",
        "content": {
          "text/plain": {
            "schema": {
              "$ref": "#/components/schemas/MessageBoxConfirmBatchModel"
            }
          },
          "application/json": {
            "schema": {
              "$ref": "#/components/schemas/MessageBoxConfirmBatchModel"
            }
          },
          "text/json": {
            "schema": {
              "$ref": "#/components/schemas/MessageBoxConfirmBatchModel"
            }
          }
        }
      },
      "204": {
        "description": "No Content"
      },
      "400": {
        "description": "Bad Request",
        "content": {
          "application/json": {
            "schema": {
              "$ref": "#/components/schemas/ValidationProblemDetails"
            }
          }
        }
      },
      "500": {
        "description": "Internal Server Error",
        "content": {
          "application/json": {
            "schema": {
              "$ref": "#/components/schemas/ProblemDetails"
            }
          }
        }
      }
    }
  }
}

The resulting client:

var urlBuilder_ = new System.Text.StringBuilder();
urlBuilder_.Append("api/message-box-batch/confirm/{batch}");
urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(batch, System.Globalization.CultureInfo.InvariantCulture)));

The request URI ends up being: api/message-box-batch/confirm/{batch}462995b7-433b-4382-b4d7-5f84599cf732

When the client is called the API returns a 400 Bad Request response due to the malformed URI.

In v13.20.0 the client was generated correctly:

var urlBuilder_ = new System.Text.StringBuilder();
urlBuilder_.Append("api/message-box-batch/confirm/{batch}");
urlBuilder_.Replace("{batch}", System.Uri.EscapeDataString(ConvertToString(batch, System.Globalization.CultureInfo.InvariantCulture)));

Is there some workaround in terms of configuration? Or should I wait for the next minor/patch version?

@paulomorgado
Copy link
Contributor

Thanks for the concrete example, @bzwieratinnovadis.

Which tooling and version are you using?

I just tried that example on NSwagStudio v14.0.0.0 and got this generated code:

var urlBuilder_ = new System.Text.StringBuilder();
if (!string.IsNullOrEmpty(BaseUrl)) urlBuilder_.Append(BaseUrl);
// Operation Path: "api/message-box-batch/confirm/{batch}"
urlBuilder_.Append("api/message-box-batch/confirm/");
urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(batch, System.Globalization.CultureInfo.InvariantCulture)));

@bzwieratinnovadis
Copy link

bzwieratinnovadis commented Jan 5, 2024

Thanks for the concrete example, @bzwieratinnovadis.

Which tooling and version are you using?

I just tried that example on NSwagStudio v14.0.0.0 and got this generated code:

var urlBuilder_ = new System.Text.StringBuilder();
if (!string.IsNullOrEmpty(BaseUrl)) urlBuilder_.Append(BaseUrl);
// Operation Path: "api/message-box-batch/confirm/{batch}"
urlBuilder_.Append("api/message-box-batch/confirm/");
urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(batch, System.Globalization.CultureInfo.InvariantCulture)));

So I've been messing around with NSwagStudio to reproduce the issue and figured out why the code was being generated incorrecly. Apparently there was a template being used that causes the issue because without it everything is fine and path/route parameters are handled correctly.

I closed related #4683

@lfderm
Copy link

lfderm commented Dec 13, 2024

Hi @bzwieratinnovadis, I am still getting this error in NSwag 14.1. Can you elaborate what you mean with "a template being used"? I do nothing special, just generate a C# client from a valid openapi spec. But the generated code is obviously broken, the generated url contains names of the placeholders instead of just the values.

                    if (offset != null)
                    {
                        urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(offset, System.Globalization.CultureInfo.InvariantCulture)));
                    }
                    else
                        if (urlBuilder_.Length > 0) urlBuilder_.Length--;
                    urlBuilder_.Append("{offset}");
                    urlBuilder_.Append('/');
                    if (limit != null)
                    {
                        urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(limit, System.Globalization.CultureInfo.InvariantCulture)));
                    }
                    else
                        if (urlBuilder_.Length > 0) urlBuilder_.Length--;
                    urlBuilder_.Append("{limit}");

URL: .../0{offset}/1{limit}

Is there any fix or workaround available? I am surprised this issue is still open after almost one year.

@paulomorgado
Copy link
Contributor

paulomorgado commented Dec 13, 2024

Hi @bzwieratinnovadis, I am still getting this error in NSwag 14.1. Can you elaborate what you mean with "a template being used"? I do nothing special, just generate a C# client from a valid openapi spec. But the generated code is obviously broken, the generated url contains names of the placeholders instead of just the values.

                    if (offset != null)
                    {
                        urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(offset, System.Globalization.CultureInfo.InvariantCulture)));
                    }
                    else
                        if (urlBuilder_.Length > 0) urlBuilder_.Length--;
                    urlBuilder_.Append("{offset}");
                    urlBuilder_.Append('/');
                    if (limit != null)
                    {
                        urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(limit, System.Globalization.CultureInfo.InvariantCulture)));
                    }
                    else
                        if (urlBuilder_.Length > 0) urlBuilder_.Length--;
                    urlBuilder_.Append("{limit}");

URL: .../0{offset}/1{limit}

Is there any fix or workaround available? I am surprised this issue is still open after almost one year.

@lfderm, can you provide a sample swagger/openapi spec that has this issue, And the full set of options used? You can use NSwag Studio for that and post the nswag file.

@dkone
Copy link

dkone commented Jan 7, 2025

Hi @paulomorgado, i reproduced that when i was manually generating a schema. Adding an OpenApiParameter with a OpenApiParameterKind.Path that has no IsRequired = true will broke code generation.

getOperation.Parameters.Add(new OpenApiParameter
{
    Name = "websitecode",
    Kind = OpenApiParameterKind.Path,
    Description = "code of the website",
    Type = JsonObjectType.String,
    IsRequired = true // Needed to avoid error in code generation
});
getOperation.Parameters.Add(new OpenApiParameter
{
    Name = "id",
    Description = "id of the resource",
    Type = JsonObjectType.Integer,
    Kind = OpenApiParameterKind.Path,
    IsRequired = true // Needed to avoid error in code generation
});
getItem["get"] = getOperation;
doc.Paths.Add("/resources/whatever/{websitecode}/{id}", getItem);

Thank you for all your amazing work.
Regards,

@lfderm
Copy link

lfderm commented Jan 7, 2025

I can confirm that making the route parameter mandatory fixes the issue. Still, this seems like a bug to me. The generated code for an optional parameter (here "offset") does not make sense:

                    if (offset != null)
                    {
                        urlBuilder_.Append(System.Uri.EscapeDataString(ConvertToString(offset, System.Globalization.CultureInfo.InvariantCulture)));
                    }
                    else
                        if (urlBuilder_.Length > 0) urlBuilder_.Length--;
                    urlBuilder_.Append("{offset}");

There is already the if/else clause for the parameter. The last method call is absolutely wrong and should be removed from code generation. Then, I think optional route parameters would work fine.

@paulomorgado
Copy link
Contributor

@dkone, a proper OpenAPI document would go a long way to expedite the fix. Better yet, adding unit tests exposing this.

Although this can be "fixed", "path parameters are always required" (in https://swagger.io/docs/specification/v3_0/describing-parameters/#path-parameters). If you want them optional, you should specify them as query parameters.

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

7 participants