Skip to content
This repository was archived by the owner on Jun 8, 2026. It is now read-only.

feat: Implementation for Begin and Rollback clientside statements - #1041

Merged
ankiaga merged 13 commits into
googleapis:mainfrom
ankiaga:client_begin
Dec 4, 2023
Merged

feat: Implementation for Begin and Rollback clientside statements#1041
ankiaga merged 13 commits into
googleapis:mainfrom
ankiaga:client_begin

Conversation

@ankiaga

Copy link
Copy Markdown
Contributor

No description provided.

@ankiaga
ankiaga requested review from a teamNovember 24, 2023 12:23
@product-auto-labelproduct-auto-labelBot added size: xl Pull request size is extra large. api: spanner Issues related to the googleapis/python-spanner API. labels Nov 24, 2023
Comment threadgoogle/cloud/spanner_dbapi/client_side_statement_executor.py
Comment threadgoogle/cloud/spanner_dbapi/connection.py Outdated
Comment threadgoogle/cloud/spanner_dbapi/connection.py Outdated
Comment threadgoogle/cloud/spanner_dbapi/connection.py
Comment threadgoogle/cloud/spanner_dbapi/connection.py

@aseeringaseering left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Some minor comments; nothing that in my opinion needs to be fixed in order to merge this PR.

Comment threadgoogle/cloud/spanner_dbapi/client_side_statement_executor.py
Comment threadgoogle/cloud/spanner_dbapi/connection.py
Comment threadtests/unit/spanner_dbapi/test_connection.py Outdated
Comment threadtests/unit/spanner_dbapi/test_connection.py Outdated
Comment threadtests/system/test_dbapi.py Outdated
conn.commit()
checksum = hashlib.sha256()
checksum.update(pickle.dumps(got_rows[0]))
checksum.update(pickle.dumps(got_rows[1]))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we really use pickle as part of our internal checksumming logic?

I'm always squeamish about any use of pickle because (a) its behavior with extension types is not necessarily defined (it's up to the extension type to implement it, maybe correctly or maybe not) and (b) loading a pickle stream is basically a remote-code-execution waiting to happen. (pickle serializes objects basically by saying "there's an object of this type, please call its constructor and feed it this data." Python constructors are basically just functions, and the last time I checked, pickle doesn't try particularly hard to make sure that the function specified in the pickle data stream is really a constructor; so if you can construct a custom pickle data stream and get code somewhere to deserialize it, you can get the deserializer to call basically any Python function in any library that's installed in the current environment, even if that library hasn't yet been imported.)

I realize that the usage here probably avoids those two specific issues for now. Just ... reflexive "icky dependency!" response 😄

Comment threadtests/system/test_dbapi.py
Comment threadtests/system/test_dbapi.py
Comment threadtests/system/test_dbapi.py
Comment threadtests/system/test_dbapi.py

@olavloiteolavloite left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, module some small nits.

Do we know why the emulator tests are failing?

Comment threadgoogle/cloud/spanner_dbapi/connection.py Outdated
Comment threadgoogle/cloud/spanner_dbapi/connection.py
Comment threadgoogle/cloud/spanner_dbapi/connection.py
Comment threadgoogle/cloud/spanner_dbapi/connection.py
Comment threadgoogle/cloud/spanner_dbapi/connection.py Outdated
@ankiaga
ankiaga merged commit 15623cd into googleapis:mainDec 4, 2023
@release-pleaserelease-pleaseBot mentioned this pull request Dec 4, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: spannerIssues related to the googleapis/python-spanner API.size: xlPull request size is extra large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ankiaga@aseering@olavloite