feat: Add credential type parameter to CredentialFactory #3064
Conversation
This change introduces new overloads to the that allow specifying the credential type as a string parameter. This provides a way to create credentials from JSON data without having to specify the type statically.
Summary of ChangesHello @robertvoinescu-work, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant enhancements to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new overloads to CredentialFactory that allow specifying the credential type as a string, which is a useful enhancement. The implementation is clean, with good refactoring to reduce code duplication, particularly in error handling for JSON deserialization. The accompanying tests are thorough, covering both success and failure cases for the new functionality. I have one high-severity comment regarding a missing null check that could lead to an unhandled exception.
| internal static GoogleCredential FromJsonParameters(JsonCredentialParameters credentialParameters, string credentialType) | ||
| { | ||
| if (credentialParameters.Type != credentialType) | ||
| { | ||
| throw new InvalidOperationException($"Json data has type = '{credentialParameters.Type}', but type = '{credentialType}' was expected."); | ||
| } |
There was a problem hiding this comment.
This method does not handle the case where credentialParameters is null. If a null value is passed (which can happen with empty or "null" JSON input), an unhandled NullReferenceException will be thrown when accessing credentialParameters.Type.
To prevent this, you should add a null check at the beginning of the method. Using credentialParameters.ThrowIfNull(nameof(credentialParameters)) would be consistent with other methods in this class and provide a more informative ArgumentNullException.
internal static GoogleCredential FromJsonParameters(JsonCredentialParameters credentialParameters, string credentialType)
{
credentialParameters.ThrowIfNull(nameof(credentialParameters));
if (credentialParameters.Type != credentialType)
{
throw new InvalidOperationException($"Json data has type = '{credentialParameters.Type}', but type = '{credentialType}' was expected.");
}
| @@ -15,6 +15,7 @@ limitations under the License. | |||
| */ | |||
|
|
|||
| using System; | |||
| using System.Diagnostics; | |||
| /// Thrown if the <paramref name="credentialType"/> is unrecognized, | ||
| /// or if the credential data is incompatible with the requested type. | ||
| /// </exception> | ||
| internal static GoogleCredential FromJsonParameters(JsonCredentialParameters credentialParameters, string credentialType) |
There was a problem hiding this comment.
This can be private.
| { | ||
| return deserializationFunc(); | ||
| } | ||
| catch (Exception e) when (e is Newtonsoft.Json.JsonException || e is IOException) |
There was a problem hiding this comment.
We were not filtering exceptions earlier. And we were catching them for everything. That last part was wrong, even with respect to the methods that we deprecated which were only catching the exceptions for the deserialization itself. We need to add tests to make certain that we don't mask other errors with this one.
Also, I'm not a fan of these methods, if I'm honest. I think they make the code harder to read than if we had the explicit try catch everywhere we needed it (and just wrapping the actual deserialization). I think if we are worried about reusing the exception message everywhere, we can put that in a constant.
And if these methods are staying aroung, they shouldn't be called Handle..., they should be called Wrap....
| { | ||
| if (credentialParameters.Type != credentialType) | ||
| { | ||
| throw new InvalidOperationException($"Json data has type = '{credentialParameters.Type}', but type = '{credentialType}' was expected."); |
There was a problem hiding this comment.
This exception message should be the same as the one we use on the generic version of the methods.
| /// <param name="credentialType">The type of credential to be loaded. Valid strings can be found in <see cref="JsonCredentialParameters"/>.</param> | ||
| /// <param name="cancellationToken">The cancellation token to cancel the operation.</param> | ||
| /// <returns>A task that will be completed with the created credential.</returns> | ||
| public static async Task<GoogleCredential> FromStreamAsync(Stream stream, string credentialType, CancellationToken cancellationToken) => |
There was a problem hiding this comment.
I'm very tempted to keep these private. We only need the FromFile ones for Spanner et al.
| /// <param name="json">The string that contains the JSON credential data.</param> | ||
| /// <param name="credentialType">The type of credential to be loaded.</param> | ||
| /// <returns>The created credential.</returns> | ||
| public static GoogleCredential FromJson(string json, string credentialType) => |
There was a problem hiding this comment.
We don't need these at the moment. We should remove them and just add them if there's a user need.
2 participants
This change introduces new overloads to the that allow specifying the credential type as a string parameter. This provides a way to create credentials from JSON data without having to specify the type statically.