fix(Spanner.Data): Specify credential type when loading from file.#15565
fix(Spanner.Data): Specify credential type when loading from file.#15565robertvoinescu-work wants to merge 1 commit intogoogleapis:mainfrom
Conversation
![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 the CredentialType property to the SpannerConnectionStringBuilder and SpannerClientCreationOptions, allowing users to specify the expected credential type when using a credential file. The changes include updates to client creation logic, equality checks, and documentation. Feedback suggests improving test assertions by verifying exception messages and refining the ToString output to nest the credential type within the credential file check for better clarity.
| { | ||
| var builder = new SpannerConnectionStringBuilder("CredentialFile=SpannerEF-8dfc036f6000.json;CredentialType=authorized_user"); | ||
| var options = new SpannerClientCreationOptions(builder); | ||
| await Assert.ThrowsAsync<InvalidOperationException>(() => options.CreateSpannerClientAsync(new Spanner.V1.SpannerSettings())); |
There was a problem hiding this comment.
According to the general rules, when asserting that an exception is thrown in a test, the exception message should also be asserted to ensure the correct exception is being propagated.
var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => options.CreateSpannerClientAsync(new Spanner.V1.SpannerSettings()));
Assert.Contains("credential type", exception.Message);References
- When asserting that an exception is thrown in a test, also assert on the exception message to ensure the correct exception is being propagated.
amanda-tarafa
added
the
do not merge
|
(Marking as do not merge as we need to wait for Spanner's major version to have been at least configured and we can't do that just yet). |
|
|
||
| /// <summary> | ||
| /// The expected type of the credential provided via <see cref="CredentialFile"/>. | ||
| /// Defaults to <see cref="JsonCredentialParameters.ServiceAccountCredentialType"/>. |
There was a problem hiding this comment.
Leave a note also saying that the rest of accepted values are the ones defined in JsonCredentialParameters
| CredentialsPath = builder.CredentialFile == "" ? null: builder.CredentialFile, | ||
| #pragma warning restore CS0618 | ||
| ChannelCredentials = builder.CredentialOverride, | ||
| GoogleCredential = builder.GoogleCredential, |
There was a problem hiding this comment.
Should you call here GetEffectiveGoogleCredential instead of later on? Also to avoid creating a new credential every time.
There was a problem hiding this comment.
Yeah good point, I was keeping the original google credential override around for equality considerations, but we can just add a reference field for that. Removed.
| return builder.ToString(); | ||
| } | ||
|
|
||
| private GoogleCredential GetEffectiveGoogleCredential() => |
There was a problem hiding this comment.
Can't this go into SpannerConnectionStringBuilder? You can still keep the path and the type for ToString?
Or is it to avoid both places knowing how the effective credential is obtained?
There was a problem hiding this comment.
Yes you are correct, just to keep it in one place. I chose here because it's the lowest layer in Spanner.Data that we can add it.
robertvoinescu-work
force-pushed
the
spannerData/windyEagle
branch
from
dc8db54 to
3649f08
Compare
2 participants
b/453009677