Skip to content

fix(Spanner.Data): Specify credential type when loading from file. #15565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robertvoinescu-work wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(Spanner.Data): Specify credential type when loading from file. #15565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions ...le.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerClientCreationOptionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,22 @@ public async Task CredentialFileRelative()
Assert.NotNull(await options.CreateSpannerClientAsync(new Spanner.V1.SpannerSettings()));
}

[Fact]
public async Task CredentialFile_WithWrongCredentialType_Fails()
{
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()));
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. When asserting that an exception is thrown in a test, also assert on the exception message to ensure the correct exception is being propagated.

}

[Fact]
public async Task CredentialFile_WithCorrectCredentialType_Succeeds()
{
var builder = new SpannerConnectionStringBuilder("CredentialFile=SpannerEF-8dfc036f6000.json;CredentialType=service_account");
var options = new SpannerClientCreationOptions(builder);
Assert.NotNull(await options.CreateSpannerClientAsync(new Spanner.V1.SpannerSettings()));
}

[Fact]
public async Task CredentialFileP12Error()
{
Expand Down
28 changes: 28 additions & 0 deletions ...Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerConnectionStringBuilderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,5 +373,33 @@ public void IsolationLevelIsConvertedToEnum()

Assert.Equal(System.Data.IsolationLevel.RepeatableRead, builder.IsolationLevel);
}

[Fact]
public void CredentialType_Default()
{
var builder = new SpannerConnectionStringBuilder();
Assert.Equal(JsonCredentialParameters.ServiceAccountCredentialType, builder.CredentialType);
}

[Fact]
public void CredentialType_Explicit()
{
var builder = new SpannerConnectionStringBuilder("CredentialType=authorized_user");
Assert.Equal("authorized_user", builder.CredentialType);
builder.CredentialType = "service_account";
Assert.Equal("service_account", builder.CredentialType);
// DbConnectionStringBuilder lower-cases keywords, annoyingly.
Assert.Equal("credentialtype=service_account", builder.ToString());
}

[Fact]
public void CredentialType_NullSet_RestoresDefault()
{
var builder = new SpannerConnectionStringBuilder();
builder.CredentialType = "authorized_user";
Assert.Equal("authorized_user", builder.CredentialType);
builder.CredentialType = null;
Assert.Equal(JsonCredentialParameters.ServiceAccountCredentialType, builder.CredentialType);
}
}
}
43 changes: 22 additions & 21 deletions apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerClientCreationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

using Google.Api.Gax.Grpc.Gcp;
using Google.Apis.Auth.OAuth2;
using Google.Cloud.Spanner.Admin.Database.V1;
using Google.Cloud.Spanner.V1;
using System;
Expand All @@ -32,16 +33,17 @@ internal sealed class SpannerClientCreationOptions : IEquatable<SpannerClientCre

internal bool UsesEmulator { get; }

private readonly string _credentialFile;
private readonly string _credentialType;
private readonly Lazy<GoogleCredential> _effectiveGoogleCredential;

internal SpannerClientCreationOptions(SpannerConnectionStringBuilder builder)
{
var clientBuilder = new SpannerClientBuilder
{
EmulatorDetection = builder.EmulatorDetection,
EnvironmentVariableProvider = builder.EnvironmentVariableProvider,
Endpoint = builder.ContainsKey(nameof(builder.Host)) || builder.ContainsKey(nameof(builder.Port)) ? builder.EndPoint : null,
#pragma warning disable CS0618 // Temporarily disable warnings for obsolete methods. See b/453009677 for more details.
CredentialsPath = builder.CredentialFile == "" ? null: builder.CredentialFile,
#pragma warning restore CS0618
ChannelCredentials = builder.CredentialOverride,
GoogleCredential = builder.GoogleCredential,
Copy link
Copy Markdown
Contributor

@amanda-tarafa amanda-tarafa Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you call here GetEffectiveGoogleCredential instead of later on? Also to avoid creating a new credential every time.

Copy link
Copy Markdown
Contributor Author

@robertvoinescu-work robertvoinescu-work Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

AffinityChannelPoolConfiguration = new ChannelPoolConfig
Expand All @@ -59,6 +61,11 @@ internal SpannerClientCreationOptions(SpannerConnectionStringBuilder builder)

UsesEmulator = emulatorBuilder is not null;
ClientBuilder = emulatorBuilder ?? clientBuilder;
_credentialFile = builder.CredentialFile;
_credentialType = builder.CredentialType;
// Use Lazy to avoid throwing in the constructor if the credential file is missing.
_effectiveGoogleCredential = new Lazy<GoogleCredential>(() =>
ClientBuilder.GoogleCredential ?? (string.IsNullOrEmpty(_credentialFile) ? null : CredentialFactory.FromFile(_credentialFile, _credentialType)));
}

internal Task<SpannerClient> CreateSpannerClientAsync(SpannerSettings settings) =>
Expand All @@ -68,11 +75,8 @@ internal Task<SpannerClient> CreateSpannerClientAsync(SpannerSettings settings)
// Note we don't copy emulator detection properties because we already took care
// of emulator detection on the constructor.
Endpoint = ClientBuilder.Endpoint,
#pragma warning disable CS0618 // Temporarily disable warnings for obsolete methods. See b/453009677 for more details.
CredentialsPath = ClientBuilder.CredentialsPath,
#pragma warning disable CS0618
ChannelCredentials = ClientBuilder.ChannelCredentials,
GoogleCredential = ClientBuilder.GoogleCredential,
GoogleCredential = _effectiveGoogleCredential.Value,
AffinityChannelPoolConfiguration = ClientBuilder.AffinityChannelPoolConfiguration,
LeaderRoutingEnabled = ClientBuilder.LeaderRoutingEnabled,
DirectedReadOptions = ClientBuilder.DirectedReadOptions,
Expand All @@ -88,11 +92,8 @@ internal DatabaseAdminClientBuilder CreateDatabaseAdminClientBuilder()
// Note we don't copy emulator detection properties because we already took care
// of emulator detection on the constructor.
Endpoint = ClientBuilder.Endpoint,
#pragma warning disable CS0618 // Temporarily disable warnings for obsolete methods. See b/453009677 for more details.
CredentialsPath = ClientBuilder.CredentialsPath,
#pragma warning restore CS0618
ChannelCredentials = ClientBuilder.ChannelCredentials,
GoogleCredential = ClientBuilder.GoogleCredential,
GoogleCredential = _effectiveGoogleCredential.Value,
// If we ever have settings of our own, we need to merge those with these.
Settings = CreateDatabaseAdminSettings(),
UniverseDomain = ClientBuilder.UniverseDomain,
Expand All @@ -113,11 +114,10 @@ other is not null &&
UsesEmulator == other.UsesEmulator &&
// TODO: Consider overriding ClientBuilderBase and SpannerClientBuilder Equals, etc.
Equals(ClientBuilder.Endpoint, other.ClientBuilder.Endpoint) &&
#pragma warning disable CS0618 // Temporarily disable warnings for obsolete methods. See b/453009677 for more details.
Equals(ClientBuilder.CredentialsPath, other.ClientBuilder.CredentialsPath) &&
#pragma warning restore CS0618
Equals(ClientBuilder.ChannelCredentials, other.ClientBuilder.ChannelCredentials) &&
Equals(ClientBuilder.GoogleCredential, other.ClientBuilder.GoogleCredential) &&
_credentialFile == other._credentialFile &&
_credentialType == other._credentialType &&
Equals(ClientBuilder.AffinityChannelPoolConfiguration, other.ClientBuilder.AffinityChannelPoolConfiguration) &&
ClientBuilder.LeaderRoutingEnabled == other.ClientBuilder.LeaderRoutingEnabled &&
Equals(ClientBuilder.DirectedReadOptions, other.ClientBuilder.DirectedReadOptions) &&
Expand All @@ -129,11 +129,10 @@ public override int GetHashCode()
{
int hash = 31;
hash = hash * 23 + (ClientBuilder.Endpoint?.GetHashCode() ?? 0);
#pragma warning disable CS0618 // Temporarily disable warnings for obsolete methods. See b/453009677 for more details.
hash = hash * 23 + (ClientBuilder.CredentialsPath?.GetHashCode() ?? 0);
#pragma warning restore CS0618
hash = hash * 23 + (ClientBuilder.ChannelCredentials?.GetHashCode() ?? 0);
hash = hash * 23 + (ClientBuilder.GoogleCredential?.GetHashCode() ?? 0);
hash = hash * 23 + (_credentialFile?.GetHashCode() ?? 0);
hash = hash * 23 + (_credentialType?.GetHashCode() ?? 0);
hash = hash * 23 + UsesEmulator.GetHashCode();
hash = hash * 23 + (ClientBuilder.AffinityChannelPoolConfiguration?.GetHashCode() ?? 0);
hash = hash * 23 + (ClientBuilder.LeaderRoutingEnabled.GetHashCode());
Expand All @@ -150,12 +149,14 @@ public override int GetHashCode()
public override string ToString()
{
var builder = new StringBuilder($"EndPoint: {ClientBuilder.Endpoint ?? "Default"}");
#pragma warning disable CS0618 // Temporarily disable warnings for obsolete methods. See b/453009677 for more details.
if (!string.IsNullOrEmpty(ClientBuilder.CredentialsPath))
if (!string.IsNullOrEmpty(_credentialFile))
{
builder.Append($"; CredentialsFile: {_credentialFile}");
}
if (!string.IsNullOrEmpty(_credentialType))
{
builder.Append($"; CredentialsFile: {ClientBuilder.CredentialsPath}");
builder.Append($"; CredentialType: {_credentialType}");
}
Comment thread
robertvoinescu-work marked this conversation as resolved.
#pragma warning restore CS0618
if (ClientBuilder.ChannelCredentials is not null)
{
builder.Append($"; CredentialsOverride: True");
Expand Down
12 changes: 12 additions & 0 deletions apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerConnectionStringBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public sealed class SpannerConnectionStringBuilder : DbConnectionStringBuilder
internal const int DefaultMaxConcurrentStreamsLowWatermark = 20;

private const string CredentialFileKeyword = "CredentialFile";
private const string CredentialTypeKeyword = "CredentialType";
private const string DataSourceKeyword = "Data Source";
private const string UseClrDefaultForNullKeyword = "UseClrDefaultForNull";
private const string EnableGetSchemaTableKeyword = "EnableGetSchemaTable";
Expand Down Expand Up @@ -94,6 +95,17 @@ public string CredentialFile
set => this[CredentialFileKeyword] = value;
}

/// <summary>
/// The expected type of the credential provided via <see cref="CredentialFile"/>.
/// Accepted values can be found in <see cref="JsonCredentialParameters"/>.
/// Defaults to <see cref="JsonCredentialParameters.ServiceAccountCredentialType"/>.
Copy link
Copy Markdown
Contributor

@amanda-tarafa amanda-tarafa Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a note also saying that the rest of accepted values are the ones defined in JsonCredentialParameters

Copy link
Copy Markdown
Contributor Author

@robertvoinescu-work robertvoinescu-work Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

/// </summary>
public string CredentialType
{
get => GetValueOrDefault(CredentialTypeKeyword, JsonCredentialParameters.ServiceAccountCredentialType);
set => this[CredentialTypeKeyword] = value;
}

/// <summary>
/// Option to change between the default handling of null database values (return <see cref="DBNull.Value">DBNull.Value</see>) or
/// the non-standard handling (return the default value for whatever type is requested).
Expand Down
9 changes: 9 additions & 0 deletions apis/Google.Cloud.Spanner.Data/docs/connection_string.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,15 @@ Example:

- CredentialFile=/path/to/credentials.json

## CredentialType

The expected type of the credential provided via `CredentialFile`.
Defaults to `service_account`.

Example:

- CredentialType=authorized_user

## EnableGetSchemaTable

When set to true (and when targeting .NET 4.5 or .NET Standard 2.0;
Expand Down
Loading