-
Notifications
You must be signed in to change notification settings - Fork 17
Introduce CustomHosts option
#45
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
Conversation
MIchaelMainer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we apply retry-count header so we should add the retry handler to this.
As a global comment, lets add a change table at the bottom of these changed spec pages with date and a description of the change so that we can track the changes to spec over time without having to look at the commit history. This way, an SDK owner can understand at a glance whether there are new changes to a spec.
GraphClientFactory.md
Outdated
| - Enable the developer to select a supported sovereign cloud using an enumerated list. Selecting the sovereign cloud should ensure that the AuthenticationProvider uses the appropriate Authentication Endpoint. | ||
| - Enable a developer to configure a HTTP proxy that will be used for outgoing requests. | ||
|
|
||
| - Enable a developer provide custom URLs or endpoints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable a developer provide custom URLs or endpoints.
Enable a developer to provide custom URLs or endpoints.
I think we should consider using the RequestContext mechanism to pass through the CustomUrls option through the middleware, in addition to an on individual request option where we can disable applying our headers.
Should this only be applicable to our default middleware or should the mechanism be accessible to custom middleware? Ideally custom middleware should be able to take advantage of this.
Do you have an example of a workload failing with an unexpected header? I can only think of OneDrive on redirection to a download/upload URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example of a workload failing with an unexpected header - I got this error with Outlook large file upload too.
andrueastman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can, I think it would be great to add a code sample to demonstrate how the CustomURLs options will be used to be able to assess if this will affect the developer experience of the SDK user in any way.
baywet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor things but besides those, LGTM
GraphClientFactory.md
Outdated
| - Enable a developer to provide custom hosts. | ||
| - `CustomHosts` option should be set on client creation. | ||
| - `CustomHosts` option should be made available in the `Context` so that it is available to the middleware during request processing. | ||
| - These URLs are different from the Graph service endpoints on the national clouds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - These URLs are different from the Graph service endpoints on the national clouds. | |
| - These hostnames are different from the Graph service endpoints on the national clouds. |
GraphClientFactory.md
Outdated
| - If the request URL is a Graph service endpoint or a custom host provided by the developer, then append request headers or modify the request content. | ||
| - Else the middleware should delete request headers added by that middleware. | ||
| - Example of an workload error - [LargeFileUploadTask upload to OneDrive caused CORS error due `SDKVersion` telemetry header](https://github.com/microsoftgraph/msgraph-sdk-javascript/issues/265) | ||
| - Provide capabilities to modify or update the `CustomURLs` option after the client creation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Provide capabilities to modify or update the `CustomURLs` option after the client creation. | |
| - Provide capabilities to modify or update the `CustomHosts` option after the client creation. |
Certain workloads error out when an unexpected header is present in the request.
Example -
In case of
LargeFileUploadTaskin JS library, the upload is as follows:The middleware appends request headers which are meant for the Graph API.
Outlook, an error was caused because ofAuthorizationheader.sdkversiontelemetry headerThe solution for this is to add a check if the request URL is a Graph endpoint and not send headers to
UPLOAD_URL.Adding the headers only for Graph endpoints ignores the case where the developer might have their own test endpoints.
Example -
MGT uses a proxy sandbox environment
or
microsoftgraph/microsoft-graph-toolkit#1053
I had a conversation with @darrelmiller about handling this case and he proposed that we allow developers to provide custom endpoints and update our conditions to modify request headers.