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

feat: Implementation for Begin and Rollback clientside statements #1041

Merged
merged 13 commits into from
Dec 4, 2023

Conversation

ankiaga
Copy link
Contributor

@ankiaga ankiaga commented Nov 24, 2023

No description provided.

@ankiaga ankiaga requested review from a team as code owners November 24, 2023 12:23
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. api: spanner Issues related to the googleapis/python-spanner API. labels Nov 24, 2023
@ankiaga ankiaga requested a review from olavloite November 24, 2023 12:58
@ankiaga ankiaga self-assigned this Nov 27, 2023
@ankiaga ankiaga requested a review from aseering November 28, 2023 10:02
Copy link

@aseering aseering left a comment

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.

google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
tests/unit/spanner_dbapi/test_connection.py Outdated Show resolved Hide resolved
tests/unit/spanner_dbapi/test_connection.py Outdated Show resolved Hide resolved
tests/system/test_dbapi.py Outdated Show resolved Hide resolved
conn.commit()
checksum = hashlib.sha256()
checksum.update(pickle.dumps(got_rows[0]))
checksum.update(pickle.dumps(got_rows[1]))

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 😄

tests/system/test_dbapi.py Show resolved Hide resolved
tests/system/test_dbapi.py Show resolved Hide resolved
tests/system/test_dbapi.py Show resolved Hide resolved
tests/system/test_dbapi.py Show resolved Hide resolved
Copy link
Contributor

@olavloite olavloite left a comment

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?

google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
@ankiaga ankiaga merged commit 15623cd into googleapis:main Dec 4, 2023
16 of 17 checks passed
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 googleapis/python-spanner API. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants