Add client invocation for RestHublifetimeManager #2206
Conversation
src/Microsoft.Azure.SignalR.Management/RestHubLifetimeManager.cs
Outdated
Show resolved
Hide resolved
|
Don't forget to add e2e tests in |
|
@Y-Sindo E2E should wait for next release after the RT fixed is deployed right? After discussion will add it |
src/Microsoft.Azure.SignalR.Management/RestHubLifetimeManager.cs
Outdated
Show resolved
Hide resolved
| args, | ||
| async response => | ||
| { | ||
| responseContent = await response.Content.ReadAsStringAsync(cancellationToken); |
There was a problem hiding this comment.
Use DeserializeAsync here directly
There was a problem hiding this comment.
And RestClient already contains an ObjectSerializer, you can reuse that one.
There was a problem hiding this comment.
Updated by exposing the ObjectSerializer in the RestClient and using that.
| throw new HubException(errorContent ?? "Unknown error in response"); | ||
| } | ||
|
|
||
| return wrapper != null && wrapper.Result != null ? wrapper.Result : throw new HubException("Result not found in response"); |
There was a problem hiding this comment.
Is a null invocation result invalid?
| // Ensure we have a response | ||
| if (!isSuccess) | ||
| { | ||
| throw new HubException(errorContent ?? "Unknown error in response"); |
There was a problem hiding this comment.
Let's throw an AzureSignalRException to keep consistent with other functions.
| #nullable enable | ||
| sealed class InvocationResponse<T> | ||
| { | ||
| [JsonPropertyName("result")] |
There was a problem hiding this comment.
It may not work if the object serializer is backed by Newtonsoft.Json.
| { | ||
| await using var contentStream = await response.Content.ReadAsStreamAsync(cancellationToken); | ||
|
|
||
| var deserialized = await _restClient.ObjectSerializer.DeserializeAsync( |
There was a problem hiding this comment.
The ObjectSerializer should be used to deserialize the value of `InvocationResponse.Result“ only. How to deserialize the key "result" should be controlled by ourselves.
|
|
||
| public ObjectSerializer ObjectSerializer => _payloadContentBuilder switch | ||
| { | ||
| JsonPayloadContentBuilder jsonBuilder => jsonBuilder.ObjectSerializer, |
There was a problem hiding this comment.
This doesn't work if the PayloadContentBuilder is not a JsonPayloadContentBuider. I'd suggest injecting IOptionsMonior<ServiceManagerOptions> into RestClient, using the ServiceManagerOptions.ObjectSerializer or providing a default one when ServiceManagerOptions.ObjectSerializer=null
2 participants
Summary of the changes (Less than 80 chars)