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

Support trailing slashes in OpenAPI paths #1580

Closed
mderriey opened this issue Mar 6, 2024 · 7 comments · Fixed by #1584 or #1835
Closed

Support trailing slashes in OpenAPI paths #1580

mderriey opened this issue Mar 6, 2024 · 7 comments · Fixed by #1584 or #1835
Assignees
Labels
help wanted priority:p2 Medium. Generally has a work-around and a smaller sub-set of customers is affected. SLA <=30 days type:bug A broken experience
Milestone

Comments

@mderriey
Copy link
Contributor

mderriey commented Mar 6, 2024

Coming from microsoft/kiota#4291

Is your feature request related to a problem? Please describe.
We're working with an API which endpoints end with trailing slashes, for example /api/v1/app/{app_id}/msg/.
See the OpenAPI document at https://github.com/svix/svix-webhooks/blob/8d32e47e0484f5d0839bce364d8700d2c7457937/openapi.json#L7779.

We're using Kiota to generate a .NET client for this API, and the generated client doesn't contain the trailing slashes (see the issue at the top for more details).

While we agree it's not common to have trailing slashes, apparently it's supported according to the relevant RFCs.

Describe the solution you'd like
OpenAPI.NET should respect the OpenAPI path when building out the OpenApiUrlTreeNode tree.

Describe alternatives you've considered
N/A.

Additional context
I've forked the repo and pushed a branch which shows how the trailing slash is removed, see https://github.com/microsoft/OpenAPI.NET/compare/vnext...mderriey:OpenAPI.NET:mderriey/support-trailing-slashes?expand=1.

I haven't opened a PR yet because it might be too much noise given it's not actionable as-is.

@baywet baywet added type:bug A broken experience help wanted priority:p2 Medium. Generally has a work-around and a smaller sub-set of customers is affected. SLA <=30 days labels Mar 6, 2024
@baywet baywet added this to the Backlog milestone Mar 6, 2024
@baywet
Copy link
Member

baywet commented Mar 6, 2024

Hi @mderriey,
Thanks for creating this issue and for starting the work for a PR.
We'll be happy to review your PR once it's ready! Make sure to add plenty of test cases!
We want to be sure extraneous nodes don't appear magically or nodes don't disappear because of that change.

@mderriey
Copy link
Contributor Author

mderriey commented Mar 6, 2024

Hey @baywet,

To be clear, the "work" I started is really just a showcase of how the trailing slash gets eaten away when building a URL tree node.

If you'd like me to continue that work, I'm going to need some guidance as to which approach we should take.

If we find the path ends with a slash, maybe we exclude the last slash from the split, so the last segment we process has the trailing slash?

Open to suggestions here.

@baywet
Copy link
Member

baywet commented Mar 6, 2024

@MaggieKimani1 can you help guide the contributions here please? (currently traveling for the next 10 days, it'll probably be faster this way)

@darrelmiller
Copy link
Member

@mderriey Thank you for raising this and it is awesome that you have made an attempt at a PR. My first reaction to this problem is one of mild terror. You are absolutely correct that we should support this scenario. However, we need to make sure that trailing slashes do not create child nodes. We effectively want to special case this so that the trailing slash is part of the prior path segment identifier. We may need to add a new property to the tree node to indicate that it is a leaf node with a trailing slash. I'll try and spend some time to look at your PR and provide feedback.

@sevein
Copy link

sevein commented Jul 23, 2024

FWIW, @typespec/http has recently added support for including the trailing slash (microsoft/typespec#3350).

@jlarmstrongiv
Copy link
Contributor

Supporting trailing slashes would allow me to write clients for APIs like https://docs.bunny.net/reference/get_-storagezonename-path-

@mderriey
Copy link
Contributor Author

If someone wants to pick up the work on #1584, please go ahead, I don't have the capacity anymore.

Apologies for leaving things unfinished.

The changes requested in the latest comment (#1584 (comment)) should be easy to implement, I think most of them are suggestions already.

Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted priority:p2 Medium. Generally has a work-around and a smaller sub-set of customers is affected. SLA <=30 days type:bug A broken experience
Projects
None yet
5 participants