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

Trailing slash in URL is dropped during generation #4291

Open
mderriey opened this issue Mar 5, 2024 · 11 comments · May be fixed by #5651
Open

Trailing slash in URL is dropped during generation #4291

mderriey opened this issue Mar 5, 2024 · 11 comments · May be fixed by #5651
Labels
help wanted Issue caused by core project dependency modules or library type:bug A broken experience WIP
Milestone

Comments

@mderriey
Copy link

mderriey commented Mar 5, 2024

Hi there

Problem

We're working with an API which endpoints contain trailing slashes, for example {+baseUrl}/api/v1/app/{app_id}/msg/.

During the client generation, the trailing slash is dropped, and the generated client doesn't work as expected as all requests return a 404.

Repro

For reference, here's a link to the OpenAPI document used here: https://github.com/svix/svix-webhooks/blob/8d32e47e0484f5d0839bce364d8700d2c7457937/openapi.json#L7779

  1. Generate the .NET client
    kiota generate `
      --openapi https://raw.githubusercontent.com/svix/svix-webhooks/main/openapi.json `
      --output .\TrailingSlashDroppedIssue4291 `
      --language CSharp `
      --class-name SvixClient `
      --namespace-name SvixApiClient `
      --exclude-backward-compatible true `
      --serializer Microsoft.Kiota.Serialization.Json.JsonSerializationWriterFactory `
      --deserializer Microsoft.Kiota.Serialization.Json.JsonParseNodeFactory `
      --structured-mime-types application/json `
      --include-path '/api/v1/app/{app_id}/msg/#POST'
    
  2. See the missing trailing slashes in the constructor definitions in the TrailingSlashDroppedIssue4291\Api\V1\App\Item\Msg\MsgRequestBuilder.cs file
    public class MsgRequestBuilder : BaseRequestBuilder {
        public MsgRequestBuilder(Dictionary<string, object> pathParameters, IRequestAdapter requestAdapter) : base(requestAdapter, "{+baseurl}/api/v1/app/{app_id}/msg{?with_content*}", pathParameters) {
        }
        public MsgRequestBuilder(string rawUrl, IRequestAdapter requestAdapter) : base(requestAdapter, "{+baseurl}/api/v1/app/{app_id}/msg{?with_content*}", rawUrl) {
        }
    }

Expected result

Kiota shouldn't manipulate paths defined in the OpenAPI document, or should provide an option to opt out of this behavior.

@baywet
Copy link
Member

baywet commented Mar 5, 2024

Hi @mderriey,
Thanks for using kiota and for reaching out.
Let me start by saying that while requiring trailing slashes is supported according to RFCs 9110 and 3986, it's a bit unusual from a REST API design perspective, see this stack overflow post. This is not an excuse, just adding some perspective here.

Kiota generates the URI templates here and additional unit tests can be added here

The challenge is that kiota relies on OpenAPI.net to parse the description and build the tree of the API. And that process kind of relies on trimming the trailing slashes to work as you can see here and there. In fact it's removing the trailing slash

From kiota's perspective, the trailing slash doesn't exist, and adding it systematically would be wrong in the same sense it's wrong to remove it.

What needs to happen it:

  1. OpenAPI.net needs to updated to keep the trailing slash without derailing the control logic
  2. Once the patch is released and pulled in kiota, the slash should appear "magically"

Would you be willing to work on a pull request for openapi.net?

@baywet baywet added type:bug A broken experience help wanted Issue caused by core project dependency modules or library dependencies Pull requests that update a dependency file blocked This work can't be done until an external dependent work is done. labels Mar 5, 2024
@baywet baywet added this to the Backlog milestone Mar 5, 2024
@mderriey

This comment was marked as outdated.

@mderriey
Copy link
Author

mderriey commented Mar 6, 2024

My bad, please ignore my last message, after writing a test in the Kiota codebase, I can see now how it comes from OpenApiUrlTreeNode, which you originally pointed to. And I also realised what I thought was the leading slash is actually the trailing slash as it works segment by segment.

Let me open an issue in the OpenAPI.NET repo, thanks again.

@jlarmstrongiv
Copy link

We just fixed trailing slashes in the OpenAPI.NET repo via microsoft/OpenAPI.NET#1835, which was released in 1.6.22.

Can we update the OpenAPI.NET dependency to see if it adds trailing slash support in Kiota?

@jlarmstrongiv
Copy link

It looks like the OpenAPI.NET dependency was already updated by dependabot https://github.com/microsoft/kiota/blob/main/src/Kiota.Builder/Kiota.Builder.csproj#L47 once the latest preview includes it, I’ll try it out.

@baywet baywet modified the milestones: Backlog, Kiota v1.19, Kiota v1.20 Oct 2, 2024
@baywet
Copy link
Member

baywet commented Oct 2, 2024

Thanks for the follow up @jlarmstrongiv !
Since we have a minor release this week, we won't have a preview release.
You should be able to try it with 1.19, and if more changes are required, we'll be more than happy to review any incoming PR for 1.20

@baywet baywet removed dependencies Pull requests that update a dependency file blocked This work can't be done until an external dependent work is done. labels Oct 2, 2024
@sevein
Copy link
Contributor

sevein commented Oct 17, 2024

I've just tried kiota v1.19.1, and I can confirm that the issue is resolved for me. I had been using an appendTrailingSlashHandler middleware as a workaround, and I've now tested that my client works as expected with the new client code generated by Kiota v1.19.1, without the need for that middleware.

@baywet
Copy link
Member

baywet commented Oct 17, 2024

Thanks for confirming! Closing.

@baywet baywet closed this as completed Oct 17, 2024
@jlarmstrongiv
Copy link

Could we re-open this issue? Trailing slashes are still broken in TypeScript

@sevein
Copy link
Contributor

sevein commented Oct 18, 2024

Oh no! I only tested with a Go client.

@baywet baywet reopened this Oct 18, 2024
@baywet
Copy link
Member

baywet commented Oct 18, 2024

@jlarmstrongiv thanks for letting us know! Do you have more context to share around this issue?

@chelkyl chelkyl linked a pull request Oct 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issue caused by core project dependency modules or library type:bug A broken experience WIP
Projects
Status: In Progress 🚧
Development

Successfully merging a pull request may close this issue.

4 participants