Moved WebPubSum to generated surface area + extensions #21549
Conversation
| @@ -80,7 +80,7 @@ serviceClient.SendToAll("Hello World!"); | |||
| ```C# Snippet:WebPubSubSendJson | |||
| var serviceClient = new WebPubSubServiceClient(new Uri(endpoint), "some_hub", new AzureKeyCredential(key)); | |||
|
|
|||
| serviceClient.SendToAll( | |||
| serviceClient.SendToAll("application/json", | |||
There was a problem hiding this comment.
Consider either updating https://github.com/Azure/azure-webpubsub/blob/main/docs/references/server-sdks/csharp-server-sdks.md as well or working with them to link to this file so we don't have out of date copies.
| serviceClient.SendToAll( | ||
| RequestContent.Create(stream), | ||
| HttpHeader.Common.OctetStreamContentType.Value); | ||
| serviceClient.SendToAll("application/octet-stream", RequestContent.Create(stream)); |
There was a problem hiding this comment.
Should we just create it from the BinaryData instead of a Stream?
There was a problem hiding this comment.
Service team wanted a stream sample as their users will often have a stream.
| <PropertyGroup> | ||
| <Description>Azure SDK client library for the WebPubSub service</Description> | ||
| <AssemblyTitle>Azure SDK for WebPubSub</AssemblyTitle> | ||
| <Version>1.0.0-beta.2</Version> | ||
| <PackageTags>Azure, WebPubSub, SignalR</PackageTags> | ||
| <TargetFrameworks>$(RequiredTargetFrameworks)</TargetFrameworks> | ||
| <NoWarn>$(NoWarn);419</NoWarn> |
There was a problem hiding this comment.
Nit - consider saying what you're ignoring so folks know when/if it's safe to add back.
| using Azure.Core; | ||
| using Azure.Core.Pipeline; | ||
|
|
||
| #pragma warning disable AZC0007 |
There was a problem hiding this comment.
Consider adding a TODO comment with a bug number to make it easier to clean these up when we improve the analyzers. You can tack it on to Azure/azure-sdk-tools#127 if that's faster.
There was a problem hiding this comment.
I removed this one. But I will do it in other places.
| if (credential == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(credential)); | ||
| } |
There was a problem hiding this comment.
Nit - use Argument.NotNull(credential, nameof(credential)); to match the rest of our code base. You might need an <Compile Include="$(AzureCoreSharedSources)Argument.cs" LinkBase="Shared" /> in your .csproj.
| } | ||
|
|
||
| /// <summary>Broadcast message to all the connected client connections.</summary> | ||
| /// <param name="message"></param> |
There was a problem hiding this comment.
Add a comment saying this is a JSON message? Should we rename the parameter to jsonMessage to make that even more explicit?
| RequestOptions options = default; | ||
| if (cancellationToken != default) | ||
| options = new RequestOptions() { CancellationToken = cancellationToken }; |
There was a problem hiding this comment.
Consider adding a helper so you don't duplicate this code every method.
| /// <param name="excluded">Excluded connection Ids.</param> | ||
| /// <param name="cancellationToken">The cancellation token to use.</param> | ||
| /// <returns>A <see cref="Response"/> if successful.</returns> | ||
| public virtual async Task<Response> SendToAllAsync(string message, IEnumerable<string> excluded = null, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Is this a high level convenience and we're also exposing the low level methods? Or did we hide the low level methods?
There was a problem hiding this comment.
Will do it during docs pass.
| /// <param name="cancellationToken"> The cancellation token to use. </param> | ||
| public virtual async Task<Response<bool>> GroupExistsAsync(string group, CancellationToken cancellationToken = default) | ||
| { | ||
| var options = new RequestOptions() { StatusOption = ResponseStatusOption.NoThrow, CancellationToken = cancellationToken }; |
There was a problem hiding this comment.
Have we GA'ed StatusOption yet? I don't love that name.
There was a problem hiding this comment.
We haven't GA'd it yet - it and RequestOptions are still both in Azure.Core.Experimental
There was a problem hiding this comment.
Yeah, StatusOption is not a great name.
| @@ -91,7 +90,7 @@ public void AddUserToGroup() | |||
| client.AddUserToGroup("some_group", "some_user"); | |||
|
|
|||
| // Avoid sending messages to users who do not exist. | |||
| if (client.UserExists("some_user")) | |||
| if (client.UserExists("some_user", CancellationToken.None).Value) | |||
There was a problem hiding this comment.
Is the CancellationToken required to bind? Do we want convenience wrappers for APIs without any parameters to disambiguate?
There was a problem hiding this comment.
It is necessary, because today we generate RequestOptions as always defaultable. We had discussed making the parameter required in cases where the operation didn't take a body (if it took a body then the protocol method would be a RequestContent parameter and then the compiler could work out that you didn't want that overload) to make it so that if we added a grow-up version the compiler would prefer it.
@KrzysztofCwalina pointed out that it will make these methods hard to call and that for many of them they may never "grow up" and so we'd have a bit of a wart.
There is a tension here, and I think we have to make a call.
The one thing we did discuss is for cases like this (where we decide we want to grow up some method before shipping a GA version) we could generate the protocol versions with a required RequestOptions parameter, so that it didn't lead to ambiguities.
For this specific issue, I'm not sure if we should just teach the code generator about these sort of XyzExists type methods and have a way to denote in the swagger (this operation is a logical existence check, return Response where the bool is checking for some known family of status values.
There was a problem hiding this comment.
I'd prefer the "required RequestOptions when there's no body" by default and only allow it to be relaxed in cases where we've explicitly thought about the scenario enough to make it optional.
I also think an x-ms-returns-successful extension or similar might be worth creating, but I haven't seen it in too many services. This one happens to use it more than most.
| @@ -19,7 +19,7 @@ public static class WebPubSubServiceClientBuilderExtensions | |||
| public static IAzureClientBuilder<WebPubSubServiceClient, WebPubSubServiceClientOptions> AddWebPubSubServiceClient<TBuilder>(this TBuilder builder, string connectionString, string hub) | |||
There was a problem hiding this comment.
We should reorder these parameters as well.
There was a problem hiding this comment.
This has not changed in this PR. Why do you think we should reorder?
There was a problem hiding this comment.
To correspond to the new WebPubSubServiceClient constructor parameter order.
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| // <auto-generated/> |
There was a problem hiding this comment.
I am guessing that this was not actually auto-generated, and we should remove this comment?
| options ??= new WebPubSubServiceClientOptions(); | ||
| _clientDiagnostics = new ClientDiagnostics(options); | ||
| var authPolicy = new WebPubSubAuthenticationPolicy(credential); | ||
| Pipeline = HttpPipelineBuilder.Build(options, new HttpPipelinePolicy[] { authPolicy, new LowLevelCallbackPolicy() }); |
There was a problem hiding this comment.
I think we would want something like this
| Pipeline = HttpPipelineBuilder.Build(options, new HttpPipelinePolicy[] { authPolicy, new LowLevelCallbackPolicy() }); | |
| Pipeline = HttpPipelineBuilder.Build(options, HttpPipelinePolicy[] { new LowLevelCallbackPolicy() }, new HttpPipelinePolicy[] { authPolicy }, new ResponseClassifier()); |
So the low level callback policy is run per call not per retry (we fixed this in the code generator, but it doesn't help with your customization).
| @@ -26,29 +28,4 @@ | |||
| <Compile Include="$(AzureCoreSharedSources)AzureResourceProviderNamespaceAttribute.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" /> | |||
| </ItemGroup> | |||
|
|
|||
| <!-- HACK --> | |||
| <!-- Hack around lack of support for RequestContent in the code generators --> | |||
| <UsingTask TaskName="ReplaceText" TaskFactory="RoslynCodeTaskFactory" AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.Core.dll"> | |||
There was a problem hiding this comment.
This was amazing while it lasted, but I am happy to see it gone :-)
| /// <summary> Initializes a new instance of HealthApiClient. </summary> | ||
| /// <param name="endpoint"> server parameter. </param> | ||
| /// <param name="options"> The options for configuring the client. </param> | ||
| public HealthApiClient(Uri endpoint = null, WebPubSubServiceClientOptions options = null) |
There was a problem hiding this comment.
Similar feedback to something Ted mentioned in the customizations layer, but it feels wrong that we allow null for the endpoint here. The client is unlikely to do anything reasonable. Do you expect null to be a valid value here?
There was a problem hiding this comment.
This client is internal. The service team does not want us to ship it, but it's a part of the current swagger so the generator generates it.
sdk/webpubsub/Azure.Messaging.WebPubSub/src/WebPubSubServiceClientBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
| /// <param name="cancellationToken"> The cancellation token to use. </param> | ||
| public virtual async Task<Response<bool>> GroupExistsAsync(string group, CancellationToken cancellationToken = default) | ||
| { | ||
| var options = new RequestOptions() { StatusOption = ResponseStatusOption.NoThrow, CancellationToken = cancellationToken }; |
There was a problem hiding this comment.
We haven't GA'd it yet - it and RequestOptions are still both in Azure.Core.Experimental
| /// <summary> Check if there are any client connections inside the given group. </summary> | ||
| /// <param name="group"> Target group name, which length should be greater than 0 and less than 1025. </param> | ||
| /// <param name="cancellationToken"> The cancellation token to use. </param> | ||
| public virtual Response<bool> GroupExists(string group, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Fine for now, but longer term would we want to try to teach the code generator to just spit a method that returns Response<bool> here? It feels like we could provide metadata on these operations that would tell the code generator to generate a method that returned Response instead of just Response in these cases. I feel like what the code generator spits out today for any sort of "XyzExists" style operation will immediately need a helper like this to be palatable to customers.
There was a problem hiding this comment.
Yes. I totally agree. @chamons, what do you think?
| @@ -91,7 +90,7 @@ public void AddUserToGroup() | |||
| client.AddUserToGroup("some_group", "some_user"); | |||
|
|
|||
| // Avoid sending messages to users who do not exist. | |||
| if (client.UserExists("some_user")) | |||
| if (client.UserExists("some_user", CancellationToken.None).Value) | |||
There was a problem hiding this comment.
It is necessary, because today we generate RequestOptions as always defaultable. We had discussed making the parameter required in cases where the operation didn't take a body (if it took a body then the protocol method would be a RequestContent parameter and then the compiler could work out that you didn't want that overload) to make it so that if we added a grow-up version the compiler would prefer it.
@KrzysztofCwalina pointed out that it will make these methods hard to call and that for many of them they may never "grow up" and so we'd have a bit of a wart.
There is a tension here, and I think we have to make a call.
The one thing we did discuss is for cases like this (where we decide we want to grow up some method before shipping a GA version) we could generate the protocol versions with a required RequestOptions parameter, so that it didn't lead to ambiguities.
For this specific issue, I'm not sure if we should just teach the code generator about these sort of XyzExists type methods and have a way to denote in the swagger (this operation is a logical existence check, return Response where the bool is checking for some known family of status values.
Azure.Messaging.WebPubSub shipped as a preview a month ago. The preview codebase was an internal generated client wrapped in handcrafted client. This change moves the codebase to generated public API with some handcrafted extensions (aka LLC/Protocol methods).
This change required swagger changes.