Add CancellationToken support for GoogleJsonWebSignature #2672
Conversation
|
This is a (binary) breaking change - old compiled code won't find the new method signatures. Overloads can be added, but will need to be added with care to avoid creating ambiguous calls. |
There was a problem hiding this comment.
@gunsh As @jskeet mentioned, this is a binary breaking change so we cannot accept this PR as is. You can add overloads to both public methods, that receive the cancellation token, or you can maybe add a cancellation token on ValidationSettings. That will cover the public method that receives the ValidationSettings and I think we could leave at that, adding a note to the ValidateAsync(string jwt, IClock clock, bool forceGoogleCertRefresh) existing overload with something like "If you want to use a cancellation token you can use the overload that accepts ValidationSettings. ValidationSettings accepts a cancellation token".
I'd prefer this last option over adding overloads.
amanda-tarafa
added
the
needs more info
|
@amanda-tarafa I have submitted a different set of changes that will hopefully not affect binary compatibility. Please note that two methods were affected (just like the first time). Both issues were solved with an overload. I understand this was not your preference but having |
| /// <param name="cancellationToken">Cancellation token</param> | ||
| /// <returns>The JWT payload, if the JWT is valid. Throws an <see cref="InvalidJwtException"/> otherwise.</returns> | ||
| /// <exception cref="InvalidJwtException">Thrown when passed a JWT that is not a valid JWT signed by Google.</exception> | ||
| public static Task<Payload> ValidateWithCancellationAsync(string jwt, IClock clock = null, bool forceGoogleCertRefresh = false, CancellationToken cancellationToken = default) => |
There was a problem hiding this comment.
The problem with this naming pattern is that it can explode very quickly. Next option we have to add is then ValidateWithCancellationAndOptionAsync, etc.
So, let's not add this method and only add the overload ValidateAsync(string, ValidationSettings, CancellationToken) then ValidateAsync(string, IClock, bool) can stay for backward compatibility as a very specific shortcut (to not having to create settings).
| // internal for testing | ||
| internal static async Task<Payload> ValidateInternalAsync(string jwt, ValidationSettings validationSettings) | ||
| internal static async Task<Payload> ValidateInternalAsync(string jwt, ValidationSettings validationSettings, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Better to be explicit in our own internal code.
| internal static async Task<Payload> ValidateInternalAsync(string jwt, ValidationSettings validationSettings, CancellationToken cancellationToken = default) | |
| internal static async Task<Payload> ValidateInternalAsync(string jwt, ValidationSettings validationSettings, CancellationToken cancellationToken) |
There was a problem hiding this comment.
After removing the default value I had to create a version of ValidateInternalAsync without a token in order for the tests to build. An alternative would be to alter all tests.
There was a problem hiding this comment.
Yes please, alter all the tests. Thanks!
Add ValidateInternalAsync in order to avoid CancellationToken with a default value.
amanda-tarafa
removed
the
needs more info
GoogleJsonWebSignaturefeatures async methods but offer noCancellationSupport. Async methods are now updated to expose aCancellationToken.