spanner: Refine Transaction::ReadWriteOptions with IsolationLevel

This commit improves the API design by integrating `IsolationLevel` directly into `Transaction::ReadWriteOptions`.

Previously, `IsolationLevel` was passed as a separate argument to some `Transaction` constructors, even though it is semantically relevant only to read-write transactions. This could lead to ambiguity or misuse.

By making `IsolationLevel` a member of `ReadWriteOptions`, the API more accurately reflects the underlying Spanner transaction semantics, enforcing that isolation levels are configured solely within the context of read-write transactions. This enhances type safety and improves the clarity of the client library's API.

This change addresses reviewer feedback regarding the logical grouping of `IsolationLevel` within the `ReadWriteOptions` message.#

Author: prathmeshcc

google/cloud/spanner/CMakeLists.txt

@@ -173,7 +173,6 @@ add_library(
     internal/tuple_utils.h
     interval.cc
     interval.h
-    isolation_level.h
     json.h
     keys.cc
     keys.h

google/cloud/spanner/google_cloud_cpp_spanner.bzl

@@ -94,7 +94,6 @@ google_cloud_cpp_spanner_hdrs = [
     "internal/transaction_impl.h",
     "internal/tuple_utils.h",
     "interval.h",
-    "isolation_level.h",
     "json.h",
     "keys.h",
     "lock_hint.h",

google/cloud/spanner/integration_tests/client_integration_test.cc

@@ -16,7 +16,6 @@
 #include "google/cloud/spanner/client.h"
 #include "google/cloud/spanner/database.h"
 #include "google/cloud/spanner/mutations.h"
-#include "google/cloud/spanner/isolation_level.h"
 #include "google/cloud/spanner/options.h"
 #include "google/cloud/spanner/testing/database_integration_test.h"
 #include "google/cloud/credentials.h"
@@ -241,6 +240,9 @@ TEST_F(ClientIntegrationTest, MultipleInserts) {
                                    RowType(4, "test-fname-4", "test-lname-4")));
 }
 
+// A smoke test to verify that the TransactionIsolationLevelOption is
+// correctly processed and accepted by the Spanner backend. It does not
+// test the behavioral semantics of the isolation level itself.
 /// @test Verify that TransactionIsolationLevel works as expected.
 TEST_F(ClientIntegrationTest, TransactionIsolationLevel) {
   auto& client = *client_;
@@ -251,15 +253,13 @@ TEST_F(ClientIntegrationTest, TransactionIsolationLevel) {
         for (auto const& row : rows) {
           if (!row) return row.status();
         }
-        // std::cout << "Transaction is active." << std::endl;
         return Mutations{};
       },
-      Options{}.set<TransactionIsolationLevelOption>(
-          IsolationLevel::kRepeatableRead));
+      Options{}.set<spanner::TransactionIsolationLevelOption>(
+          Transaction::IsolationLevel::kRepeatableRead));
   EXPECT_THAT(commit, StatusIs(StatusCode::kOk));
 }
 
-
 /// @test Verify that Client::Rollback works as expected.
 TEST_F(ClientIntegrationTest, TransactionRollback) {
   ASSERT_NO_FATAL_FAILURE(InsertTwoSingers());
@@ -1419,6 +1419,9 @@ TEST_F(PgClientIntegrationTest, FineGrainedAccessControl) {
   ASSERT_STATUS_OK(metadata);
 }
 
+// A smoke test to verify that the TransactionIsolationLevelOption is
+// correctly processed and accepted by the Spanner backend. It does not
+// test the behavioral semantics of the isolation level itself.
 /// @test Verify that TransactionIsolationLevel works as expected.
 TEST_F(PgClientIntegrationTest, TransactionIsolationLevel) {
   auto& client = *client_;
@@ -1430,8 +1433,8 @@ TEST_F(PgClientIntegrationTest, TransactionIsolationLevel) {
         }
         return Mutations{};
       },
-      Options{}.set<TransactionIsolationLevelOption>(
-          IsolationLevel::kRepeatableRead));
+      Options{}.set<spanner::TransactionIsolationLevelOption>(
+          Transaction::IsolationLevel::kRepeatableRead));
   EXPECT_THAT(commit, StatusIs(StatusCode::kOk));
 }
 

google/cloud/spanner/isolation_level.h

@@ -41,12 +41,12 @@
 #include "google/cloud/spanner/backoff_policy.h"
 #include "google/cloud/spanner/directed_read_replicas.h"
 #include "google/cloud/spanner/internal/session.h"
-#include "google/cloud/spanner/isolation_level.h"
 #include "google/cloud/spanner/lock_hint.h"
 #include "google/cloud/spanner/order_by.h"
 #include "google/cloud/spanner/polling_policy.h"
 #include "google/cloud/spanner/request_priority.h"
 #include "google/cloud/spanner/retry_policy.h"
+#include "google/cloud/spanner/transaction.h"
 #include "google/cloud/spanner/version.h"
 #include "google/cloud/options.h"
 #include "absl/types/variant.h"
@@ -417,23 +417,22 @@ struct ExcludeTransactionFromChangeStreamsOption {
 };
 
 /**
- * Option for `google::cloud::Options` to return additional statistics
- * about the committed transaction in a `spanner::CommitResult`.
+ * Option for `google::cloud::Options` to set the transaction isolation level.
  *
  * @ingroup google-cloud-spanner-options
  */
-struct CommitReturnStatsOption {
-  using Type = bool;
+struct TransactionIsolationLevelOption {
+  using Type = spanner::Transaction::IsolationLevel;
 };
 
 /**
- * Option for `google::cloud::Options` to set the isolation level for
- * read-write transactions.
+ * Option for `google::cloud::Options` to return additional statistics
+ * about the committed transaction in a `spanner::CommitResult`.
  *
  * @ingroup google-cloud-spanner-options
  */
-struct TransactionIsolationLevelOption {
-  using Type = IsolationLevel;
+struct CommitReturnStatsOption {
+  using Type = bool;
 };
 
 /**

google/cloud/spanner/options.h

@@ -54,6 +54,22 @@ ProtoReadLockMode(
   }
 }
 
+google::spanner::v1::TransactionOptions_IsolationLevel ProtoIsolationLevel(
+    absl::optional<Transaction::IsolationLevel> const& isolation_level) {
+  if (!isolation_level) {
+    return google::spanner::v1::TransactionOptions::ISOLATION_LEVEL_UNSPECIFIED;
+  }
+  switch (*isolation_level) {
+    case Transaction::IsolationLevel::kSerializable:
+      return google::spanner::v1::TransactionOptions::SERIALIZABLE;
+    case Transaction::IsolationLevel::kRepeatableRead:
+      return google::spanner::v1::TransactionOptions::REPEATABLE_READ;
+    default:
+      return google::spanner::v1::TransactionOptions::
+          ISOLATION_LEVEL_UNSPECIFIED;
+  }
+}
+
 google::spanner::v1::TransactionOptions MakeOpts(
     google::spanner::v1::TransactionOptions_ReadOnly ro_opts) {
   google::spanner::v1::TransactionOptions opts;
@@ -63,21 +79,21 @@ google::spanner::v1::TransactionOptions MakeOpts(
 
 google::spanner::v1::TransactionOptions MakeOpts(
     google::spanner::v1::TransactionOptions_ReadWrite rw_opts,
-    IsolationLevel isolation_level) {
+    absl::optional<Transaction::IsolationLevel> isolation_level) {
   google::spanner::v1::TransactionOptions opts;
   *opts.mutable_read_write() = std::move(rw_opts);
   auto const& current = internal::CurrentOptions();
   if (current.get<ExcludeTransactionFromChangeStreamsOption>()) {
     opts.set_exclude_txn_from_change_streams(true);
   }
-  if (isolation_level == IsolationLevel::kUnspecified) {
-    isolation_level = current.get<TransactionIsolationLevelOption>();
-  }
-  if (isolation_level != IsolationLevel::kUnspecified) {
+  if (isolation_level &&
+      *isolation_level != Transaction::IsolationLevel::kUnspecified) {
+    opts.set_isolation_level(ProtoIsolationLevel(isolation_level));
+  } else if (current.has<TransactionIsolationLevelOption>()) {
     opts.set_isolation_level(
-        static_cast<google::spanner::v1::TransactionOptions::IsolationLevel>(
-            isolation_level));
+        ProtoIsolationLevel(current.get<TransactionIsolationLevelOption>()));
   }
+
   return opts;
 }
 
@@ -112,6 +128,13 @@ Transaction::ReadWriteOptions& Transaction::ReadWriteOptions::WithTag(
   return *this;
 }
 
+Transaction::ReadWriteOptions&
+Transaction::ReadWriteOptions::WithIsolationLevel(
+    IsolationLevel isolation_level) {
+  isolation_level_ = isolation_level;
+  return *this;
+}
+
 Transaction::SingleUseOptions::SingleUseOptions(ReadOnlyOptions opts) {
   ro_opts_ = std::move(opts.ro_opts_);
 }
@@ -146,6 +169,17 @@ Transaction::Transaction(ReadWriteOptions opts) {
       std::move(opts.tag_).value_or(std::string()));
 }
 
+Transaction::Transaction(ReadWriteOptions opts,
+                         IsolationLevel isolation_level) {
+  google::spanner::v1::TransactionSelector selector;
+  *selector.mutable_begin() =
+      MakeOpts(std::move(opts.rw_opts_), isolation_level);
+  auto const route_to_leader = true;  // read-write
+  impl_ = std::make_shared<spanner_internal::TransactionImpl>(
+      std::move(selector), route_to_leader,
+      std::move(opts.tag_).value_or(std::string()));
+}
+
 Transaction::Transaction(Transaction const& txn, ReadWriteOptions opts) {
   google::spanner::v1::TransactionSelector selector;
   *selector.mutable_begin() =

google/cloud/spanner/transaction.cc

@@ -18,7 +18,6 @@
 #include "google/cloud/spanner/internal/transaction_impl.h"
 #include "google/cloud/spanner/timestamp.h"
 #include "google/cloud/spanner/version.h"
-#include "google/cloud/spanner/isolation_level.h"
 #include "absl/types/optional.h"
 #include <google/spanner/v1/transaction.pb.h>
 #include <chrono>
@@ -58,6 +57,12 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
  */
 class Transaction {
  public:
+  enum class IsolationLevel {
+    kUnspecified,
+    kSerializable,
+    kRepeatableRead,
+  };
+
   /**
    * Options for ReadOnly transactions.
    */
@@ -101,13 +106,13 @@ class Transaction {
 
     explicit ReadWriteOptions(ReadLockMode read_lock_mode);
 
+    explicit ReadWriteOptions(IsolationLevel isolation_level)
+        : isolation_level_(isolation_level) {}
+
     // A tag used for collecting statistics about the transaction.
     ReadWriteOptions& WithTag(absl::optional<std::string> tag);
 
-    ReadWriteOptions& WithIsolationLevel(IsolationLevel isolation_level) {
-      isolation_level_ = isolation_level;
-      return *this;
-    }
+    ReadWriteOptions& WithIsolationLevel(IsolationLevel isolation_level);
 
    private:
     friend Transaction;
@@ -157,6 +162,8 @@ class Transaction {
   /// @copydoc Transaction(ReadOnlyOptions)
   explicit Transaction(ReadWriteOptions opts);
   /// @copydoc Transaction(ReadOnlyOptions)
+  explicit Transaction(ReadWriteOptions opts, IsolationLevel isolation_level);
+  /// @copydoc Transaction(ReadOnlyOptions)
   Transaction(Transaction const& txn, ReadWriteOptions opts);
   ///@}
 

google/cloud/spanner/transaction.h

@@ -173,7 +173,7 @@ TEST(Transaction, MultiplexedPreviousTransactionId) {
 
 TEST(Transaction, IsolationLevel) {
   auto opts = Transaction::ReadWriteOptions().WithIsolationLevel(
-      IsolationLevel::kRepeatableRead);
+      Transaction::IsolationLevel::kRepeatableRead);
   Transaction txn = MakeReadWriteTransaction(opts);
   spanner_internal::Visit(
       txn, [](spanner_internal::SessionHolder&,
@@ -183,29 +183,24 @@ TEST(Transaction, IsolationLevel) {
         EXPECT_TRUE(s->begin().has_read_write());
         EXPECT_EQ(s->begin().isolation_level(),
                   google::spanner::v1::TransactionOptions::REPEATABLE_READ);
-
-        std::cout << "Isolation Level: "
-                  << s->begin().isolation_level() << std::endl;
         return 0;
       });
 }
 
 TEST(Transaction, IsolationLevelPrecedence) {
   internal::OptionsSpan span(Options{}.set<TransactionIsolationLevelOption>(
-      IsolationLevel::kSerializable));
+      Transaction::IsolationLevel::kSerializable));
 
   // Case 1: Per-call overrides client default
   auto opts = Transaction::ReadWriteOptions().WithIsolationLevel(
-      IsolationLevel::kRepeatableRead);
+      Transaction::IsolationLevel::kRepeatableRead);
   Transaction txn = MakeReadWriteTransaction(opts);
   spanner_internal::Visit(
       txn, [](spanner_internal::SessionHolder&,
               StatusOr<google::spanner::v1::TransactionSelector>& s,
               spanner_internal::TransactionContext const&) {
         EXPECT_EQ(s->begin().isolation_level(),
                   google::spanner::v1::TransactionOptions::REPEATABLE_READ);
-        std::cout << "Isolation Level: "
-                  << s->begin().isolation_level() << std::endl;
         return 0;
       });
 
@@ -218,8 +213,6 @@ TEST(Transaction, IsolationLevelPrecedence) {
                       spanner_internal::TransactionContext const&) {
         EXPECT_EQ(s->begin().isolation_level(),
                   google::spanner::v1::TransactionOptions::SERIALIZABLE);
-        std::cout << "Isolation Level: "
-                  << s->begin().isolation_level() << std::endl;
         return 0;
       });
 }
@@ -265,6 +258,22 @@ TEST(Transaction, ReadWriteOptionsWithReadLockMode) {
       google::spanner::v1::TransactionOptions_ReadWrite::OPTIMISTIC);
 }
 
+TEST(Transaction, ReadWriteOptionsWithIsolationLevel) {
+  auto opts = Transaction::ReadWriteOptions(
+      Transaction::IsolationLevel::kRepeatableRead);
+  Transaction txn(opts, Transaction::IsolationLevel::kRepeatableRead);
+  spanner_internal::Visit(
+      txn, [](spanner_internal::SessionHolder&,
+              StatusOr<google::spanner::v1::TransactionSelector>& s,
+              spanner_internal::TransactionContext const&) {
+        EXPECT_TRUE(s->has_begin());
+        EXPECT_TRUE(s->begin().has_read_write());
+        EXPECT_EQ(s->begin().isolation_level(),
+                  google::spanner::v1::TransactionOptions::REPEATABLE_READ);
+        return 0;
+      });
+}
+
 }  // namespace
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace spanner