fix(spanner): wrap errors in inline begin transaction to avoid masking

[harshachinta]

When inline begin transaction was added, the error messages were getting masked with

spanner: code = "Internal", desc = "failed inline begin transaction"

This PR fixes this by wrapping the error to unmask original errors.

[rahul2393]

@harshachinta Thanks for picking this up, please add the test case to mock the invalid SQL statement and check if the invalid SQL error message is contained in the final error.

[ajorgensen]

@harshachinta I'd be happy to take over this changeset and help push it through if thats ok with you. We run into this fairly frequently so this change would be greatly beneficial.

[harshachinta]

@ajorgensen Feel free to work on top of this. I guess the changes in this PR is working as expected except there was some test failures. I don't exactly recall the pending stuff in this PR. Will try taking a look at this next week.

[ajorgensen]

@harshachinta I found one more spot that looks to be missing a bit of extra information that is available.

EDIT: Never mind I figured it out... required a bit of url hacking on the pull-request screen to make a PR against your fork but I think this should work: harshachinta#5

Github doesn't seem to want to allow me to make a PR against your fork of the repo for some reason but the change is here: https://github.com/ajorgensen/google-cloud-go/tree/wrap-inline-begin-transaction-error

or if its easier here is the patch:

diff --git a/spanner/transaction.go b/spanner/transaction.go
index dab7ceb76a..6fedf82165 100644
--- a/spanner/transaction.go
+++ b/spanner/transaction.go
@@ -1216,14 +1216,22 @@ func (t *ReadWriteTransaction) batchUpdateWithOptions(ctx context.Context, stmts
 		}
 		counts = append(counts, count)
 	}
+
+	if resp.Status != nil && resp.Status.Code != 0 {
+		err = spannerErrorf(codes.Code(uint32(resp.Status.Code)), resp.Status.Message)
+		if hasInlineBeginTransaction && !haveTransactionID {
+			t.setTransactionID(nil)
+			return counts, toSpannerErrorDuringInlineBegin(err)
+		}
+
+		return counts, err
+	}
+
 	if hasInlineBeginTransaction && !haveTransactionID {
 		// retry with explicit BeginTransaction
 		t.setTransactionID(nil)
 		return counts, errInlineBeginTransactionFailed()
 	}
-	if resp.Status != nil && resp.Status.Code != 0 {
-		return counts, spannerErrorf(codes.Code(uint32(resp.Status.Code)), resp.Status.Message)
-	}
 	return counts, nil
 }

here is a fix for the failing client_test as well:

diff --git a/spanner/session.go b/spanner/session.go
index 547ed464bc..27c309ad68 100644
--- a/spanner/session.go
+++ b/spanner/session.go
@@ -1656,7 +1656,7 @@ func isFailedInlineBeginTransaction(err error) bool {
 	if err == nil {
 		return false
 	}
-	return strings.Contains(err.Error(), errInlineBeginTransactionFailed().Error())
+	return strings.Contains(err.Error(), inlineBeginTransactionFailedMsg)
 }
 
 // isClientClosing returns true if the given error is a

[codyoss]

@rahul2393 do we still want to accecpt this PR?

[richardartoul]

This P.R would have saved me a LOT of time. Could we get this merged please? Its a great improvement

[codyoss]

@rahul2393 please see my comment above