Support trailing slash in URL paths

[chelkyl]

Fixes #4291

Nearly all language-specific writers support parameterized paths that have a trailing slash. The TypeScript writer does correctly write the request builder files but because it writes to index.ts rather than to a filename based on the request builder like the other writers, there is a conflict when a code namespace has more than one code file.
When the spec has paths like /{foo} and /{foo}/, kiota writes the first request builder correctly then overwrites it with the second request builder.

The first commit addresses this in the TypeScript refiner by detecting when the code namespace has more than one code file and merging them all into one.
However, this creates a new problem because the request metadata generator only selects the first UriTemplate. With the code files merged, there are multiple UriTemplate elements so one of the request metadata constants would have the wrong UriTemplate.

The second commit addresses this by taking advantage of how the UriTemplate property is unused in a CodeConstantKind.RequestsMetadata code constant and sets it to match the expected name of the correct UriTemplate. The request metadata generator can then use this now-populated property to find the correct UriTemplate.
Unfortunately, this creates a hidden coupling with how UriTemplates are named but it appears to be unavoidable.

Like the first issue, non-parameterized paths that have a trailing slash also result in conflicts. However, the conflict is earlier in the process so it affects all languages.
When the spec has paths like /foo and /foo/, kiota resolves both nodes (segments foo and foo\\ respectively) to the same name which causes a lot of problems later, resulting in broken generated code.

The third commit addresses this by changing how slashes are cleaned up so that the foo segment still results in foo but the foo\\ segment results in fooSlash. This also changes the naming of code elements like foo\\RequestBuilder from fooRequestBuilder to fooSlashRequestBuilder.
I'm not completely happy with this third commit because the trailing slash will now leak into the code element names and files. However, with my shallow understanding of the project, I'm not seeing a better way without messier changes.

[baywet]

Thanks for the contribution! And for the great description here.

I do think understanding the problem better will help us design a less intrusive solution.

Can we start by identifying patterns where the trailing slash in the URI template is problematic?

e.g. if we only have /foo/bar/ (instead of /foo/bar) in the description/selected operations, this is not and issue, we can simply project the trailing slash in the uri template, using the same conventions for the request builders and whatnot.

if we have both /foo/bar/ and /foo/{id} what happens? what about /foo/ and /foo? Any other pattern we should be careful about?

Some extremely informative information on the topic

[baywet]

why are we using the class name as uri template here?

[chelkyl]

Mentioned it briefly in the PR description but part of the fix in the TypeScript refiner is merging multiple CodeFiles into one CodeFile.
The consequence is that a CodeFile now has multiple UriTemplates in it.
When the TypeScript writer finally gets to writing the request builder's RequestsMetadata constant, it previously only selects the first UriTemplate even if it is the wrong one.
This commit sets UriTemplate in the RequestMetadata CodeConstant so that the TypeScript writer can use it to look up the correct UriTemplate.

[baywet]

from reading the unit tests this effectively inserts an additional suffix in the request builder's name.
this solution looks disruptive to me, but I'll follow up in the main comment.

[chelkyl]

I completely agree and look forward to whatever alternative we come up with!

[chelkyl]

Thanks for the feedback!

if we have both /foo/bar/ and /foo/{id} what happens? what about /foo/ and /foo?

For a minimal spec containing /foo/bar/ and /foo/{id}, the main¹ branch of kiota generates the following because bar/ and {id} are not collapsed:

├── apiSdk.ts           # main entry point contains `get foo()`
├── foo
│   ├── bar
│   │   └── index.ts    # UriTemplate is "{+baseurl}/foo/bar/"
│   ├── item
│   │   └── index.ts    # UriTemplate is "{+baseurl}/foo/{id}"
│   └── index.ts        # UriTemplate is "{+baseurl}/foo"
│                       # RequestBuilder contains `byId(id: string)` and `get bar()` and the standard
│                       # NavigationMetadata contains `byId` and `bar`
├── kiota-lock.json
└── models
    └── index.ts

For a minimal spec containing /foo/ and /foo, the main¹ branch of kiota generates

├── apiSdk.ts       # main entry point contains `get foo()`
├── foo
│   └── index.ts    # only has 1 UriTemplate "{+baseurl}/foo/"
│                   # RequestBuilder contains the usual `get(requestConfiguration)` and `toGetRequestInformation(requestConfiguration)`
│                   # RequestsMetadata contains 2 entries for `get` with the exact same contents
├── kiota-lock.json
└── models
    └── index.ts

/foo/index.ts

export const FooRequestBuilderRequestsMetadata: RequestsMetadata = {
    get: {
        uriTemplate: FooRequestBuilderUriTemplate,
        responseBodyContentType: "text/plain;q=0.9",
        adapterMethodName: "sendPrimitive",
        responseBodyFactory:  "string",
    },
    get: {
        uriTemplate: FooRequestBuilderUriTemplate,
        responseBodyContentType: "text/plain;q=0.9",
        adapterMethodName: "sendPrimitive",
        responseBodyFactory:  "string",
    },
};

Any other pattern we should be careful about?

The patterns I specifically focused on for this PR are in this sample test file: tests/Kiota.Builder.Tests/OpenApiSampleFiles/TrailingSlashSampleYml.cs

However, as you brought up, there are more!

Can we start by identifying patterns where the trailing slash in the URI template is problematic?

I have started this and it turns out that there are some edge cases I had not considered. I'll post another comment once I have a report for you, likely in the form of a GitHub repo so we can browse and discuss differences and desired state between the generated results.

I've put this PR back into a draft state for now.

Footnotes

1. main specifically being 6c4376e ↩ ↩²

[baywet]

Thanks for the detailed answer here! I think this and your next answer will help flesh things out to an improved solution.

[baywet]

@chelkyl gentle nudge on the topic :)

[chelkyl]

Thanks for the poke! Please see the report here.

Highlights:

Test URI Paths	TypeScript kiota main Results	Python kiota main Results
File: 6.yaml /foo /foo/	🚫 has only 1 uri template has /foo/ but missing /foo weirdly, some comments reference /foo/ and others /foo RequestsMetadata has duplicate "get" property	🚫 almost completely correct has /foo/ but missing /foo
File: 7.yaml /foo/ /foo	🚫 almost same as 6 only difference is comments have reversed which route they refer to	🚫 almost completely correct has /foo but missing /foo/ however, help doc references /foo/
File: 8.yaml /foo/ /foo/bar	🚫 missing /foo/	🚫 missing /foo/
File: 9.yaml /foo/ /foo/bar/	🚫 same as 8 missing /foo/	🚫 same as 8 missing /foo/
File: 14.yaml /{path} /{path}/	🚫 missing /{path}/	✅ /{path} and /{path}/ are handled separately
File: 15.yaml /{path}/ /{path}	🚫 missing /{path}	✅ same as 14
File: 17.yaml /{path}/ /{path}/{id}	🚫 missing /{path}/ NavigationMetadata uses a RequestsMetadata value that was not imported	✅

[baywet]

Thank you for this really detailed report!

I think one thing is being worrisome is the difference between languages.

The flakiness (collision based on ordering), I'd have expected BEFORE the "Slash" convention in the naming was added.

I think the first goal should be to get rid of flaky results since then generated code for each language depends on this step. Looking at the DOM before refiners are applied would probably help abstract for language changes.
Another approach would be to add additional languages to that table to see if any specific language is an outlier.

Let us know if you have any additional comments or questions. And thanks for the hard work here!