Skip to content

fix(spanner): transaction_tag should be set on BeginTransactionRequest #13463

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

Merged
gcf-merge-on-green merged 1 commit into from
Dec 10, 2025
Merged

fix(spanner): transaction_tag should be set on BeginTransactionRequest #13463

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
fix(spanner): transaction_tag should be set on BeginTransactionRequest 
When using multiplexed sessions, the transaction_tag should also be set on the
BeginTransactionRequest.
  • Loading branch information
@olavloite
olavloite committed Dec 10, 2025
commit 509bdbdcba44c9af62fa6c858dde08a1f28eaba0
20 changes: 20 additions & 0 deletions spanner/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6319,6 +6319,26 @@ func TestClient_ReadWriteTransactionWithTag_AbortedOnce(t *testing.T) {
if g != w {
t.Fatalf("mutations count mismatch\nGot: %v\nWant: %v", g, w)
}
// Verify that the BeginTransaction requests also contain the transaction tag.
// This is required when using multiplexed sessions.
for _, index := range []int{1, 3, 5} {
beginReq, ok := requests[index].(*sppb.BeginTransactionRequest)
if !ok {
t.Fatalf("%d is not a BeginTransactionRequest", index)
}
if beginReq.RequestOptions == nil {
t.Fatalf("%d has no RequestOptions", index)
}
var want string
if index < 5 {
want = "test-tag1"
} else {
want = "test-tag2"
}
if g, w := beginReq.RequestOptions.TransactionTag, want; g != w {
t.Fatalf("%d: transaction tag mismatch\n Got: %v\nWant: %v", index, g, w)
}
}
}

func TestClient_ReadWriteTransactionWithTag_SessionNotFound(t *testing.T) {
Expand Down
11 changes: 8 additions & 3 deletions spanner/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1730,8 +1730,7 @@ func beginTransaction(ctx context.Context, opts transactionBeginOptions) (transa
if opts.multiplexEnabled {
readWriteOptions.MultiplexedSessionPreviousTransactionId = opts.previousTx
}

res, err := opts.client.BeginTransaction(ctx, &sppb.BeginTransactionRequest{
request := &sppb.BeginTransactionRequest{
Session: opts.sessionID,
Options: &sppb.TransactionOptions{
Mode: &sppb.TransactionOptions_ReadWrite_{
Expand All @@ -1741,7 +1740,13 @@ func beginTransaction(ctx context.Context, opts transactionBeginOptions) (transa
IsolationLevel: opts.txOptions.IsolationLevel,
},
MutationKey: opts.mutation,
})
}
// When using multiplexed sessions, the BeginTransaction request must include the transaction tag (if any).
if opts.multiplexEnabled && opts.txOptions.TransactionTag != "" {
request.RequestOptions = &sppb.RequestOptions{TransactionTag: opts.txOptions.TransactionTag}
}

res, err := opts.client.BeginTransaction(ctx, request)
if err != nil {
return nil, nil, err
}
Expand Down
12 changes: 10 additions & 2 deletions spanner/transaction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,19 +206,27 @@ func TestApply_RetryOnAbort(t *testing.T) {
Insert("Accounts", []string{"AccountId"}, []interface{}{int64(1)}),
}

if _, e := client.Apply(ctx, ms); e != nil {
if _, e := client.Apply(ctx, ms, TransactionTag("my_tag")); e != nil {
t.Fatalf("ReadWriteTransaction retry on abort, got %v, want nil.", e)
}

if _, err := shouldHaveReceived(server.TestSpanner, []interface{}{
if reqs, err := shouldHaveReceived(server.TestSpanner, []interface{}{
&sppb.BatchCreateSessionsRequest{},
&sppb.BeginTransactionRequest{},
&sppb.CommitRequest{}, // First commit fails.
&sppb.BeginTransactionRequest{},
&sppb.CommitRequest{}, // Second commit succeeds.
}); err != nil {
t.Fatal(err)
} else {
if g, w := reqs[1].(*sppb.BeginTransactionRequest).RequestOptions.TransactionTag, "my_tag"; g != w {
t.Fatalf("transaction tag mismatch\n Got: %v\nWant: %v", g, w)
}
if g, w := reqs[3].(*sppb.BeginTransactionRequest).RequestOptions.TransactionTag, "my_tag"; g != w {
t.Fatalf("transaction tag mismatch\n Got: %v\nWant: %v", g, w)
}
Comment on lines +222 to +227
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 10, 2025

Choose a reason for hiding this comment

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

high

These checks could cause a panic if RequestOptions is nil. It's safer to verify that RequestOptions is not nil before accessing TransactionTag, similar to the approach taken in spanner/client_test.go. Additionally, refactoring the repeated checks for different request indices into a loop would improve the test's readability and maintainability.

		for _, i := range []int{1, 3} {
			beginReq, ok := reqs[i].(*sppb.BeginTransactionRequest)
			if !ok {
				t.Fatalf("request at index %d is not a BeginTransactionRequest", i)
			}
			if beginReq.RequestOptions == nil {
				t.Fatalf("request at index %d has no RequestOptions", i)
			}
			if g, w := beginReq.RequestOptions.TransactionTag, "my_tag"; g != w {
				t.Fatalf("transaction tag mismatch for request %d\n Got: %v\nWant: %v", i, g, w)
			}
		}

}

}

// Tests that SessionNotFound errors are retried.
Expand Down
Loading