Repository navigation
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates SpannerTransaction to return null for its DbConnection property once the transaction is disposed, and adds a unit test to verify this behavior. The review feedback correctly points out that according to the ADO.NET specification, the connection should also return null after the transaction has been committed or rolled back, not just upon disposal. It is recommended to update the implementation to handle these additional states and expand the test coverage accordingly.
|
|
||
| /// <inheritdoc /> | ||
| protected override DbConnection DbConnection => SpannerConnection; | ||
| protected override DbConnection DbConnection => _disposed == 0 ? SpannerConnection : null; |
There was a problem hiding this comment.
According to the ADO.NET specification, DbTransaction.Connection should return null when the transaction is no longer valid, which includes after it has been committed or rolled back, not just when it is disposed. If _disposed is only set during disposal, DbConnection will still return the connection after Commit() or Rollback() until Dispose() is called. Please check if there is an existing state or completion flag that can be used to return null after commit or rollback, and add corresponding tests for these scenarios.
There was a problem hiding this comment.
Addressed: transaction completion is tracked so Connection returns null after commit, rollback, or disposal, with regression coverage.
| public void Connection_IsNullAfterDispose() | ||
| { | ||
| SpannerClient spannerClientMock = SpannerClientHelpers.CreateMockClient(Logger.DefaultLogger); | ||
| spannerClientMock.SetupCreateSessionAsync(); | ||
| SpannerConnection connection = SpannerCommandTests.BuildSpannerConnection(spannerClientMock); | ||
| SpannerTransaction transaction = connection.BeginTransaction(); | ||
|
|
||
| Assert.Same(connection, transaction.Connection); | ||
|
|
||
| transaction.Dispose(); | ||
|
|
||
| Assert.Null(transaction.Connection); | ||
| } |
There was a problem hiding this comment.
To ensure full compliance with the ADO.NET specification, consider adding test cases to verify that Connection also returns null after the transaction is committed or rolled back, as the transaction is no longer valid after these operations.
There was a problem hiding this comment.
Added regression coverage for Connection returning null after commit and rollback.
2ff2cd5 to
5d579d2
Compare
alaricd
changed the title
5d579d2 to
e445907
Compare
e445907 to
70a31f4
Compare
alaricd
changed the title
Summary
Return null from the inherited ADO.NET
DbTransaction.Connectionproperty after the Spanner transaction has been disposed or otherwise invalidated, while preserving the provider-specificSpannerConnectionreference. Add a regression test for disposal.Resolves #16051.
Microsoft contract
Microsoft documents that
IDbTransaction.Connectionreturns null when the transaction is no longer valid: https://learn.microsoft.com/dotnet/api/system.data.common.dbtransaction.system-data-idbtransaction-connectionCompatibility
The inherited ADO.NET
Connectionsurface will change from the original connection to null after transaction invalidation. The provider-specificSpannerConnectionproperty remains available. Consumers usingDbTransaction.Connectionafter completion will observe the documented invalid-state value.Validation
Focused provider test:
dotnet test apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/Google.Cloud.Spanner.Data.Tests.csproj --no-restore -f net8.0 -p:NoWarn=CS8981%3BxUnit2004%3BxUnit2013 --filter FullyQualifiedName~Connection_IsNullAfterDispose— 1 passed, 0 failed.Discovery
Found using pengdows.AdoNetApiTest.conformance.