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

Base Url should be a path parameter #785

Closed
Tracked by #1049
andrueastman opened this issue Nov 3, 2021 · 4 comments · Fixed by #795
Closed
Tracked by #1049

Base Url should be a path parameter #785

andrueastman opened this issue Nov 3, 2021 · 4 comments · Fixed by #795
Assignees
Labels
Csharp Pull requests that update .net code enhancement New feature or request fixed generator Issues or improvements relater to generation capabilities. Go Java TypeScript Pull requests that update Javascript code

Comments

@andrueastman
Copy link
Member

andrueastman commented Nov 3, 2021

With the latest URL templating changes, it seems the various request builders have the base URL e.g. https://graph.microsoft.com/v1.0/ is "hardcoded" in the URL templates.

Examples
Dotnet - https://github.com/microsoft/kiota-samples/blob/4f23266baf055c9e17bdc5bbef8efad6d7c79175/msgraph-mail/dotnet/Users/UsersRequestBuilder.cs#L31
Typescript - https://github.com/microsoft/kiota-samples/blob/4f23266baf055c9e17bdc5bbef8efad6d7c79175/msgraph-mail/typescript/src/users/usersRequestBuilder.ts#L19

This potentially could cause issues in the event we wish to override the base URL with a national cloud URL such as https://graph.microsoft.us. This wasn't an issue before given that the base URL would be passed forward as the URL would be built allowing the parameter at the GraphClient constructor to dictate this.

This could potentially be resolved if we set the base URL as a path parameter to allow it to be replaced/modified/overriden.

@andrueastman andrueastman added Csharp Pull requests that update .net code Go Java TypeScript Pull requests that update Javascript code labels Nov 3, 2021
@baywet
Copy link
Member

baywet commented Nov 3, 2021

Thanks for bringing this up. I debated myself during the implementation. We could go all the way to saying "each SDK is generated for a specific endpoint, if you want to change endpoints, generate a new SDK" and ship service libraries for national clouds too.
This would make sense as national cloud only provide a subset of the APIs available in the public cloud.
Obviously this would not be practical for ISVs building solutions that span across clouds, or even for local debugging when people test client solutions against a locally running APIs (not necessarily talking about Microsoft graph for that last example).

The alternative would be make the base URL as a parameter as you're suggesting. Couple of notes here:

This parameter need to have a + sign to avoid url encoding (: and /) so it'd look something like that:
{+baseurl}/me/inferenceClassification{?select}

The Api client constructor needs to set the base url, and we need to provide a way for consumers to change that base URL so I'd suggest the following for this https://github.com/microsoft/kiota-samples/blob/4f23266baf055c9e17bdc5bbef8efad6d7c79175/msgraph-mail/dotnet/ApiClient.cs#L25

            _ = requestAdapter ?? throw new ArgumentNullException(nameof(requestAdapter));
            PathParameters = new Dictionary<string, object>();
            UrlTemplate = "{+baseurl}"; // this is changed
            requestAdapter.SetBaseUrl("https://graph.microsoft.com/v1.0"); // this is new
            RequestAdapter = requestAdapter;
            ApiClientBuilder.RegisterDefaultSerializer<JsonSerializationWriterFactory>();
            ApiClientBuilder.RegisterDefaultDeserializer<JsonParseNodeFactory>();

Obviously the request adapter interface would need two more methods Set & Get base URL (dotnet could use auto-properties obviously).
And at runtime, the request adapter implementation would add the parameter value to the request information before resolving the URL and performing the request.

What do you think about all of that? I could take this on as soon as I'm done with Go snippets generation, so probably later this week.

@baywet baywet self-assigned this Nov 3, 2021
@baywet baywet added this to the TypeWriter Replacement milestone Nov 3, 2021
@baywet baywet added generator Issues or improvements relater to generation capabilities. enhancement New feature or request labels Nov 3, 2021
@andrueastman
Copy link
Member Author

Hey @baywet,

I agree with this.

Your description covers how I was thinking about this with the request builder classes getting the baseUrl from one source (in this case the requestAdapter instance) and feeding it to the path parameters collection so that the URL could still be built in a configurable fashion as we could before with minimal changes.

This parameter need to have a + sign to avoid url encoding (: and /) so it'd look something like that:
{+baseurl}/me/inferenceClassification{?select}

I was unsure if you could use the path parameter to even replace the protocol section of the URL but other than that, this makes sense to me.

@darrelmiller
Copy link
Member

I support this approach.

@baywet
Copy link
Member

baywet commented Nov 5, 2021

@andrueastman I've started the work under #795 in case you want to take a look and provide feedback while I work on other languages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code enhancement New feature or request fixed generator Issues or improvements relater to generation capabilities. Go Java TypeScript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants