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 slash in URL paths #5651

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

chelkyl
Copy link

@chelkyl chelkyl commented Oct 21, 2024

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.

@chelkyl chelkyl force-pushed the chelkyl/support-trailing-slash-in-paths branch from 47fea19 to 4373e58 Compare October 22, 2024 01:11
@chelkyl chelkyl marked this pull request as ready for review October 22, 2024 01:49
@chelkyl chelkyl requested a review from a team as a code owner October 22, 2024 01:49
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -95,6 +95,7 @@ public CodeDocumentation Documentation
Name = $"{codeClass.Name.ToFirstCharacterLowerCase()}{RequestsMetadataSuffix}",
Kind = CodeConstantKind.RequestsMetadata,
OriginalCodeElement = codeClass,
UriTemplate = $"{codeClass.Name.ToFirstCharacterLowerCase()}{UriTemplateSuffix}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -266,6 +266,11 @@ private static string NormalizeSymbolsBeforeCleanup(string original)
result = result.Replace("+", "_plus_", StringComparison.OrdinalIgnoreCase);
}

if (result.Contains('\\', StringComparison.OrdinalIgnoreCase))
{
result = result.Replace(@"\", "Slash", StringComparison.OrdinalIgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@chelkyl chelkyl marked this pull request as draft October 24, 2024 04:44
@chelkyl
Copy link
Author

chelkyl commented Oct 24, 2024

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 main1 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 main1 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 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trailing slash in URL is dropped during generation
2 participants