WPS changes based on API reviews, PR reviews, and changes to LLC plans #21968
Conversation
KrzysztofCwalina
commented
Jun 18, 2021
KrzysztofCwalina
commented
- all method groups now have protocol methods (methods taking RequestOptions)
- changed semantics of string messages (they are serialized as plaintext by default)
- added overloads allowing to specify expiresAt, in addition to expiresAfter.
- small renames
KrzysztofCwalina
changed the title
KrzysztofCwalina
changed the title
There was a problem hiding this comment.
Some comments - don't really understand about the use of DateTime instead of DateTimeOffset. Also, a small tip on a way to remove editing the swagger (and hopefully we can remove all of this ceremony when the code generator just generates the correct output shape for HEAD methods that are doing Exists checks.
|
|
||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| [assembly: SuppressMessage("Usage", "AZC0002:DO ensure all service methods, both asynchronous and synchronous, take an optional CancellationToken parameter called cancellationToken.", Justification = "CancellationToken can be passed through RequestOptions")] |
There was a problem hiding this comment.
Opened Azure/azure-sdk-tools#1715 so we don't lose track of fixing the analyzer here.
There was a problem hiding this comment.
Though, to be clear. I think this is triggered by my experiments in not having CT parameters in some helper overloads.
| /// <param name="hmacSha512">SHA512 if true, otherwise SHA256</param> | ||
| /// <returns></returns> | ||
| public static string GenerateAccessToken( | ||
| string audience, | ||
| IEnumerable<Claim> claims, | ||
| AzureKeyCredential key, | ||
| TimeSpan expireAfter = default, | ||
| DateTime expiresAtUtc = default, |
There was a problem hiding this comment.
Why is this not DateTimeOffset ?
There was a problem hiding this comment.
I feel like DT is better for representing UTC times, i.e. is has explicit support for UTC. Possibly I should change it to DTO to comply with guidelines.
There was a problem hiding this comment.
After a discussions, we decided to switch to DTO. But I will do it after the JWT implementation is changed. The current JWT library does not take in DTO.
There was a problem hiding this comment.
We talked about this offline and you are going to move to DTO with the next set of changes.
| @@ -99,7 +99,7 @@ | |||
| "webpubsub" | |||
| ], | |||
| "summary": "Check if the connection with the given connectionId exists.", | |||
| "operationId": "WebPubSubService_ConnectionExists", | |||
| "operationId": "WebPubSubService_ConnectionExistsImpl", | |||
There was a problem hiding this comment.
We should be able to do this via a transform in the auto-rest MD (so you don't have to make this change in the swagger itself, so we can keep it in sync with the authoritative version.)
Maybe something like this in the autorest.md?
Make WebPubSubPermission a regular enum
- from: swagger-document
where: $.paths
transform: >
$["/api/hubs/{hub}/connections/{connectionId}"].head.operationId = "WebPubSubService_ConnectionExistsImpl";
$["/api/hubs/{hub}/groups/{group}"].head.operationId = "WebPubSubService_GroupExistsImpl";
$["/api/hubs/{hub}/users/{userId}"].head.operationId = "WebPubSubService_UserExistsImpl";There was a problem hiding this comment.
Yeah, this is temporary. Having said that, this swagger is already forked (PR a week ago). It's forked to generate some methods as internal (and possibly some other small changes). We should work on getting all these customizations out and get back to the original swagger.
| /// <returns>A <see cref="Response"/> if successful.</returns> | ||
| public virtual async Task<Response> SendToAllAsync(string message, IEnumerable<string> excluded = null, CancellationToken cancellationToken = default) | ||
| public virtual async Task<Response> SendToAllAsync(string content, ContentType contentType = default) |
There was a problem hiding this comment.
I still miss having cancellation tokens on these high level methods, FWIW.
There was a problem hiding this comment.
Let's chat more about it. What I am going after is a very common BCL pattern where only the most powerful overload takes CTs. See https://docs.microsoft.com/en-us/dotnet/api/system.io.stream.writeasync?view=net-5.0 as an example.
I am not sure why we want to add CTs to all overloads. I think we went too far with the support for CTs. They prevent us from having simple overloads, from using param arrays, etc.
Add a note to Event Grid readme about review (Azure#21968) * Add a note to Event Grid readme about review * Update specification/eventgrid/data-plane/readme.md Co-authored-by: Laurent Mazuel <[email protected]> Co-authored-by: Laurent Mazuel <[email protected]>
