Skip to content
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.

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

fix(spanner/spannertest): support queries in ExecuteSql #3640

Merged
merged 9 commits into from Feb 4, 2021

Conversation

olavloite
Copy link
Contributor

Normal queries from the Spanner client use the ExecuteStreamingSql method,
while DML statements use ExecuteSql. This distinction was also built into
spannertest where ExecuteSql would only support DML statements and required
a transaction to be specified. The session pool however uses ExecuteSql to
execute a simple SELECT 1 query without specifying any transaction. This
would cause a nil pointer dereference.

This PR introduces support for queries in the ExecuteSql method. The current
logic assumes that the statement is a query if the transaction is a single-
use read-only transaction.

Fixes #3639

Normal queries from the Spanner client use the ExecuteStreamingSql method,
while DML statements use ExecuteSql. This distinction was also built into
spannertest where ExecuteSql would only support DML statements and required
a transaction to be specified. The session pool however uses ExecuteSql to
execute a simple `SELECT 1` query without specifying any transaction. This
would cause a nil pointer dereference.

This PR introduces support for queries in the ExecuteSql method. The current
logic assumes that the statement is a query if the transaction is a single-
use read-only transaction.

Fixes #3639
@olavloite olavloite requested review from skuruppu and a team as code owners February 1, 2021 08:33
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 1, 2021
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Feb 1, 2021
spanner/spannertest/inmem.go Outdated Show resolved Hide resolved
spanner/spannertest/inmem.go Outdated Show resolved Hide resolved
spanner/spannertest/integration_test.go Show resolved Hide resolved
spanner/spannertest/integration_test.go Show resolved Hide resolved
@olavloite olavloite added the automerge Merge the pull request once unit tests and other checks pass. label Feb 2, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 2, 2021
@olavloite
Copy link
Contributor Author

@skuruppu Apparently your review is also required in this case to allow merging.

@olavloite olavloite added the automerge Merge the pull request once unit tests and other checks pass. label Feb 4, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 8eede84 into master Feb 4, 2021
@gcf-merge-on-green gcf-merge-on-green bot deleted the issue-3639 branch February 4, 2021 08:04
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner/spannertest: ping causes nil pointer dereference error
4 participants