-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Implement Response abstraction for Azure #46530
Conversation
For now, we can't consume NuGet package from MGC due to microsoft/typespec#4507. |
eng/packages/http-client-csharp/generator/Azure.Generator/src/Azure.Generator.csproj
Outdated
Show resolved
Hide resolved
Will create a new PR to upgrade MGC version to reduce the changes in this PR. |
It would be super helpful for these to see an example of what the generated code looks like. |
...ient-csharp/generator/TestProjects/Local/Basic-TypeSpec/src/Generated/BasicTypeSpecClient.cs
Show resolved
Hide resolved
.../generator/TestProjects/Local/Basic-TypeSpec/src/Generated/BasicTypeSpecClient.RestClient.cs
Outdated
Show resolved
Hide resolved
.../generator/TestProjects/Local/Basic-TypeSpec/src/Generated/BasicTypeSpecClient.RestClient.cs
Show resolved
Hide resolved
...ages/http-client-csharp/generator/TestProjects/Local/Basic-TypeSpec/src/BasicTypeSpec.csproj
Outdated
Show resolved
Hide resolved
...ages/http-client-csharp/generator/TestProjects/Local/Basic-TypeSpec/src/BasicTypeSpec.csproj
Show resolved
Hide resolved
@@ -50,11 +52,11 @@ public BasicTypeSpecClient(Uri endpoint, ApiKeyCredential keyCredential, BasicTy | |||
|
|||
_endpoint = endpoint; | |||
_keyCredential = keyCredential; | |||
Pipeline = ClientPipeline.Create(options, Array.Empty<PipelinePolicy>(), new PipelinePolicy[] { ApiKeyAuthenticationPolicy.CreateHeaderApiKeyPolicy(_keyCredential, AuthorizationHeader) }, Array.Empty<PipelinePolicy>()); | |||
Pipeline = HttpPipelineBuilder.Build(options, new HttpPipelinePolicy[] { }); |
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.
Should this have an authentication policy?
PipelineRequest request = message.Request; | ||
request.Method = "GET"; | ||
Request request = message.Request; | ||
request.Method = new RequestMethod("GET"); |
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.
RequestMethod.Get will prevent making an allocation here.
request.Headers.Set("Accept", "application/json"); | ||
message.Apply(options); | ||
request.Uri = uri; | ||
request.Headers.Add("head-parameter", headParameter); |
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.
Why Add and not Set?
PipelineRequest request = message.Request; | ||
request.Method = "GET"; | ||
Request request = message.Request; | ||
request.Method = new RequestMethod("GET"); | ||
ClientUriBuilder uri = new ClientUriBuilder(); |
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.
Does Azure.Core have ClientUriBuilder? I believe Azure clients can set the URI in the request directly.
|
||
namespace BasicTypeSpec | ||
{ | ||
/// <summary> Client options for <see cref="BasicTypeSpecClient"/>. </summary> | ||
public partial class BasicTypeSpecClientOptions : ClientPipelineOptions | ||
public partial class BasicTypeSpecClientOptions : ClientOptions |
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 will be more different going forward ... just a heads up that more will need to differ here.
{ | ||
await pipeline.SendAsync(message).ConfigureAwait(false); | ||
await pipeline.SendAsync(message, default).ConfigureAwait(false); |
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.
This should pass the CancellationToken through to pipeline.Send from RequestContext.
} | ||
|
||
PipelineResponse response = message.BufferResponse ? message.Response : message.ExtractResponse(); | ||
Response response = message.BufferResponse ? message.Response : ExtractResponseContent(message); |
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.
We should sync on ExtractResponseContent ... this may not be the right pattern for Azure clients.
|
||
namespace BasicTypeSpec | ||
{ | ||
internal partial class ErrorResult<T> : ClientResult<T> |
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.
We should remove this from unbranded clients. SCM-based clients should never throw from a property.
@annelo-msft Thanks a lot for reviewing the generated SDK for Azure Plugin! |
Sounds great. Many thanks, @live1206! |
Resolves #45705