feat(Spanner): Add support for Protobuf Enums by efevans · Pull Request #15699 · googleapis/google-cloud-dotnet

efevans · 2026-06-22T22:52:00Z

b/522532048

gemini-code-assist

Code Review

This pull request introduces support for Spanner Enum types by adding a ProtobufEnumName property, a constructor, and helper methods to SpannerDbType, as well as supporting conversion of protobuf values to CLR enum types. The review feedback suggests correcting the string representation of the enum type from PROTO to ENUM, handling nullable enum types correctly during conversion, and fixing a typo in a parameter name.

robertvoinescu-work · 2026-06-25T17:20:39Z

Looking good, I think writing the integration tests should help clear up how spanner is actually handling the enums. You've got the logic to handle enums coming from Spanner almost there, but we will also need to support the other direction of enum types flowing to spanner from the client.

efevans · 2026-06-25T19:15:11Z

@robertvoinescu-work answered most of our comments but there's some discussion still worth having. I'll be going over with Amanda the issues I'm seeing with FQNs later today and should be able to respond again with more afterwards.

robertvoinescu-work · 2026-07-20T18:23:59Z

+/// <summary>
+/// Convenience methods for working with Protobuf enums
+/// </summary>
+public static class ProtobufEnumExtensions


I lean towards limiting our changes to the public surface as much as possible so I'll suggest marking this internal for our own use cases/tests. Open to discussion - also Amanda might have some input here, too.

The hope for this was to be a convenience for users writing DDL when knowing the FQN of a Protobuf enum might be more obscure knowledge (even more than protos already are), and the path to getting it programmatically isn't as seamless as getting the FQN for Protobuf messages. But I'm fine either way, this probably also doesn't have much discoverability without demonstrating the use in public facing samples

efevans

Addressed feedback

efevans · 2026-07-20T20:07:44Z

+/// <summary>
+/// Convenience methods for working with Protobuf enums
+/// </summary>
+public static class ProtobufEnumExtensions


The hope for this was to be a convenience for users writing DDL when knowing the FQN of a Protobuf enum might be more obscure knowledge (even more than protos already are), and the path to getting it programmatically isn't as seamless as getting the FQN for Protobuf messages. But I'm fine either way, this probably also doesn't have much discoverability without demonstrating the use in public facing samples

robertvoinescu-work

Mostly looks good to me, my only hang up is the extension method @amanda-tarafa please let us know your take on including this.

efevans · 2026-07-27T23:36:16Z

Mostly looks good to me, my only hang up is the extension method @amanda-tarafa please let us know your take on including this.

@robertvoinescu-work narrowed it down from public to internal

amanda-tarafa

There's some fundamental feature creep here that we don't want, and some improvements. I didn't review all of the tests but I'm pretty sure they'll need the same fixes as the ones I did review once we stop supporting conversions of non-
Protobuf enums and conversions to other than Protobuf enums (like string and numeric) .

amanda-tarafa · 2026-07-31T18:02:08Z

                    }
                    throw new ArgumentException($"Interval parameters must be of type {typeof(Interval).FullName} or string");
+                case TypeCode.Enum:
+                    if (value is string enumStr)


Is this string value meant to be the integer value or the name associated with the Enum value? Or would spanner accept both?

For instance:

enum Season { Spring=0, Summer=1 , Autumn=2, Winter=3 }

Can enumStr be 0 and Spring indistinctly, or just one of those? If it can be Spring, what about spring?

Spanner is accepting a stringified integrer constant, so "0" would work, but "Spring" and other case variations will fail

amanda-tarafa · 2026-07-31T18:12:25Z

+                    Value.KindOneofCase.NumberValue => System.Enum.ToObject(targetClrType, Convert.ToInt64(wireValue.NumberValue, InvariantCulture)),
+                    Value.KindOneofCase.StringValue => System.Enum.Parse(targetClrType, wireValue.StringValue),


In which cases does Spanner returns a NumberValue or a StringValue? I would expect they always return one or the other so I don't understand this. Also here you need to check that the type code is Enum, otherwise you are allowing the random conversion of string and numerical values to enums and we shouldn't do that, users can always do that if they want to.

Spanner returns values from a Protobuf enum field in the StringValue field as the stringified integer constant.

Spanner returns NumberValue in the case of reading from a numeric column like Int64, and was included here for the case of supporting more conversions. I've removed most of the LoC here and replaced it with an additional check in the if to look for a TypeCode of Enum and only parsing the StringValue field.

amanda-tarafa · 2026-07-31T18:14:18Z

                }
            }
+
+            if (targetClrType.IsEnum)


nit: This block seems randomly placed here, add it after the Protobuf logic.

Updated this block to be right after the Protobuf logic

amanda-tarafa · 2026-07-31T18:41:33Z

                    + $" {nameof(SpannerDbType.Timestamp)}, {nameof(SpannerDbType.Date)}, {nameof(SpannerDbType.String)},"
                    + $" {nameof(SpannerDbType.Bytes)}, {nameof(SpannerDbType.Json)}, {nameof(SpannerDbType.PgJsonb)}, {nameof(SpannerDbType.Numeric)},"
-                    + $" {nameof(SpannerDbType.PgNumeric)}, {nameof(SpannerDbType.PgOid)}), {nameof(SpannerDbType.Interval)}, {nameof(SpannerDbType.Uuid)}");
+                    + $" {nameof(SpannerDbType.PgNumeric)}, {nameof(SpannerDbType.PgOid)}), {nameof(SpannerDbType.Interval)}, {nameof(SpannerDbType.Uuid)},"


At this point I'd prefer to simplify this error message to something like "{nameof(SpannerDbType)} set to {nameof(SpannerDbType.Unspecified)} for {Value}"

Updated this whole block with your suggestion

amanda-tarafa · 2026-07-31T18:45:57Z

+        /// <summary>
+        /// Protobuf message
+        /// </summary>
+        internal const string ProtobufMessage = "Protobuf Message";
+
+        internal const string ProtobufEnum = "Protobuf Enum";
+


Just a reminder to get rid of these after the error message is simplified.

amanda-tarafa · 2026-07-31T22:24:56Z

+            yield return new object[] { Color.Red, SpannerDbType.Float64, "1" };
+            yield return new object[] { Pet.Types.Species.Dog, SpannerDbType.Float64, "2" };


Same, we don't want this.

amanda-tarafa · 2026-07-31T22:25:14Z

+            yield return new object[] { Color.Red, SpannerDbType.Int64, Quote("1") };
+            yield return new object[] { Pet.Types.Species.Dog, SpannerDbType.Int64, Quote("2") };


amanda-tarafa · 2026-07-31T22:26:04Z

+            yield return new object[] { Color.Red, SpannerDbType.String, Quote(Color.Red.ToString()) };
+            yield return new object[] { Pet.Types.Species.Dog, SpannerDbType.String, Quote(Pet.Types.Species.Dog.ToString()) };
+


Also definitely not.

amanda-tarafa · 2026-07-31T22:29:45Z

            yield return new object[] { "passthrubadbytes", SpannerDbType.Bytes, Quote("passthrubadbytes") };

+            // Protobuf Enum
+            yield return new object[] { Color.Red, SpannerDbType.FromClrType(typeof(Pet.Types.Species)), Quote(((long) Color.Red).ToString()) };


If this test pass, there's something very wrong here, you are using the Species enum to convert colors back and forth. (Precisely why we don't want random conversions that we cannot validate).

What is the wire format of a Protobuf enum value expected and sent by Spanner. That's the only thing we need to support back and forth.

After this is fixed, I want to see this test still in, but as a failing test, so that we make certain we validate the convertions.

This was succeeding because the checks in the conversions back and forth between Protobuf Value and CLR type for Protobuf enums was only doing simple checks on if the CLR type was an enum. It lacked checks if the Full Name of the value (either Value Protobuf or enum value, depending on direction of conversion) was similar to the Full name of either the target CLR type and/or specified SpannerDbType.

I've updated the conversion logic for both ways to include a check that the Full Name matches, essentially that we are doing a conversion from the matching Protobuf enum types on both sides of the conversion.

To validate this, I've added a couple of separate Facts below, ToProtobufType_DifferentEnumTypeAndValue_ThrowsException and ConvertToClrType_DifferentEnumTypeAndValue_ThrowsException to ensure that these cases are properly failing as you requested.

amanda-tarafa · 2026-07-31T22:30:06Z

            };
            yield return new object[]
+            {
+                new List<DayOfWeek>(GetEnumsForArray()), SpannerDbType.ArrayOf(SpannerDbType.String),


No. And if this was supported before as for any other object type that we could call ToString() on that's fine, but then we add a test confirming that non-protobuf enums cannot be converted to their Enum type, only to strings.

Removed along with the other tests with non-Protobuf Enums

efevans · 2026-08-07T17:02:48Z

Do not merge until response PRs have been squashed

efevans

@amanda-tarafa Updated the design like we discussed offline. The two-way conversions between Protobuf enum values and Protobuf enum types (columns) is limited to only the case when the Protobuf enum value is of the Protobuf enum type (e.g. no conversions from Color.Red to Species). There's a couple of new tests in SpannerDbTypeTests.ValueConversions that check for a proper failure in the case of mismatched Protobuf enums: ToProtobufType_DifferentEnumTypeAndValue_ThrowsException and ConvertToClrType_DifferentEnumTypeAndValue_ThrowsException.

All of the new change to your comments are in a separate commit in case you'd like to see just the diff'd changes, we will squash down to the single conventional commit before merging.

efevans · 2026-08-05T17:28:11Z

                    }
                    throw new ArgumentException($"Interval parameters must be of type {typeof(Interval).FullName} or string");
+                case TypeCode.Enum:
+                    if (value is string enumStr)


Spanner is accepting a stringified integrer constant, so "0" would work, but "Spring" and other case variations will fail

efevans · 2026-08-05T17:53:21Z

+                    Value.KindOneofCase.NumberValue => System.Enum.ToObject(targetClrType, Convert.ToInt64(wireValue.NumberValue, InvariantCulture)),
+                    Value.KindOneofCase.StringValue => System.Enum.Parse(targetClrType, wireValue.StringValue),


Spanner returns values from a Protobuf enum field in the StringValue field as the stringified integer constant.

Spanner returns NumberValue in the case of reading from a numeric column like Int64, and was included here for the case of supporting more conversions. I've removed most of the LoC here and replaced it with an additional check in the if to look for a TypeCode of Enum and only parsing the StringValue field.

efevans · 2026-08-05T17:57:07Z

                }
            }
+
+            if (targetClrType.IsEnum)


Updated this block to be right after the Protobuf logic

efevans · 2026-08-05T18:00:02Z

                    + $" {nameof(SpannerDbType.Timestamp)}, {nameof(SpannerDbType.Date)}, {nameof(SpannerDbType.String)},"
                    + $" {nameof(SpannerDbType.Bytes)}, {nameof(SpannerDbType.Json)}, {nameof(SpannerDbType.PgJsonb)}, {nameof(SpannerDbType.Numeric)},"
-                    + $" {nameof(SpannerDbType.PgNumeric)}, {nameof(SpannerDbType.PgOid)}), {nameof(SpannerDbType.Interval)}, {nameof(SpannerDbType.Uuid)}");
+                    + $" {nameof(SpannerDbType.PgNumeric)}, {nameof(SpannerDbType.PgOid)}), {nameof(SpannerDbType.Interval)}, {nameof(SpannerDbType.Uuid)},"


Updated this whole block with your suggestion

efevans · 2026-08-05T18:17:10Z

+        /// <summary>
+        /// Protobuf message
+        /// </summary>
+        internal const string ProtobufMessage = "Protobuf Message";
+
+        internal const string ProtobufEnum = "Protobuf Enum";
+


efevans · 2026-08-06T20:35:21Z

+            yield return new object[] { Color.Red, SpannerDbType.Float64, "1" };
+            yield return new object[] { Pet.Types.Species.Dog, SpannerDbType.Float64, "2" };


efevans · 2026-08-06T20:35:25Z

+            yield return new object[] { Color.Red, SpannerDbType.Int64, Quote("1") };
+            yield return new object[] { Pet.Types.Species.Dog, SpannerDbType.Int64, Quote("2") };


efevans · 2026-08-06T20:35:29Z

+            yield return new object[] { Color.Red, SpannerDbType.String, Quote(Color.Red.ToString()) };
+            yield return new object[] { Pet.Types.Species.Dog, SpannerDbType.String, Quote(Pet.Types.Species.Dog.ToString()) };
+


efevans · 2026-08-06T20:37:10Z

            };
            yield return new object[]
+            {
+                new List<DayOfWeek>(GetEnumsForArray()), SpannerDbType.ArrayOf(SpannerDbType.String),


Removed along with the other tests with non-Protobuf Enums

efevans · 2026-08-06T21:20:02Z

            yield return new object[] { "passthrubadbytes", SpannerDbType.Bytes, Quote("passthrubadbytes") };

+            // Protobuf Enum
+            yield return new object[] { Color.Red, SpannerDbType.FromClrType(typeof(Pet.Types.Species)), Quote(((long) Color.Red).ToString()) };


This was succeeding because the checks in the conversions back and forth between Protobuf Value and CLR type for Protobuf enums was only doing simple checks on if the CLR type was an enum. It lacked checks if the Full Name of the value (either Value Protobuf or enum value, depending on direction of conversion) was similar to the Full name of either the target CLR type and/or specified SpannerDbType.

I've updated the conversion logic for both ways to include a check that the Full Name matches, essentially that we are doing a conversion from the matching Protobuf enum types on both sides of the conversion.

To validate this, I've added a couple of separate Facts below, ToProtobufType_DifferentEnumTypeAndValue_ThrowsException and ConvertToClrType_DifferentEnumTypeAndValue_ThrowsException to ensure that these cases are properly failing as you requested.

product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Jun 22, 2026

gemini-code-assist Bot reviewed Jun 22, 2026

View reviewed changes

robertvoinescu-work reviewed Jun 25, 2026

View reviewed changes

efevans force-pushed the spanner/protobuf-enums branch 3 times, most recently from ba4527f to ab7bd7c Compare July 10, 2026 22:30

efevans force-pushed the spanner/protobuf-enums branch 5 times, most recently from 7a1eca7 to 783c20e Compare July 15, 2026 18:22

efevans commented Jul 15, 2026

View reviewed changes

Comment thread apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDbType.cs

Comment thread apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDbType.cs Outdated

efevans marked this pull request as ready for review July 17, 2026 19:12

efevans requested a review from a team as a code owner July 17, 2026 19:12

robertvoinescu-work reviewed Jul 20, 2026

View reviewed changes

efevans commented Jul 20, 2026

View reviewed changes

efevans force-pushed the spanner/protobuf-enums branch 2 times, most recently from 04e2d21 to d8d2b5e Compare July 20, 2026 22:00

robertvoinescu-work self-requested a review July 24, 2026 22:39

efevans requested a review from a team July 27, 2026 17:06

robertvoinescu-work reviewed Jul 27, 2026

View reviewed changes

robertvoinescu-work requested a review from a team July 27, 2026 20:51

efevans force-pushed the spanner/protobuf-enums branch from d8d2b5e to 71114e5 Compare July 27, 2026 23:29

feat(Spanner): Add support for Protobuf Enums

186c437

efevans force-pushed the spanner/protobuf-enums branch from 71114e5 to 186c437 Compare July 27, 2026 23:43

amanda-tarafa requested changes Jul 31, 2026

View reviewed changes

Address PR comments

c90a361

Useless commit to try re-triggering GH checks

856fd27

efevans added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 7, 2026

Remove remaining non-enum to enum type conversion tests

6a288e8

efevans commented Aug 7, 2026

View reviewed changes

efevans requested a review from amanda-tarafa August 7, 2026 17:28

		Value.KindOneofCase.NumberValue => System.Enum.ToObject(targetClrType, Convert.ToInt64(wireValue.NumberValue, InvariantCulture)),
		Value.KindOneofCase.StringValue => System.Enum.Parse(targetClrType, wireValue.StringValue),

		yield return new object[] { Color.Red, SpannerDbType.Float64, "1" };
		yield return new object[] { Pet.Types.Species.Dog, SpannerDbType.Float64, "2" };

Uh oh!

Conversation

efevans commented Jun 22, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertvoinescu-work commented Jun 25, 2026

Uh oh!

efevans commented Jun 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efevans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertvoinescu-work left a comment

Choose a reason for hiding this comment

Uh oh!

efevans commented Jul 27, 2026

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!