feat(spanner): add ClientContext support to options#1495
feat(spanner): add ClientContext support to options#1495
Conversation
This change adds support for ClientContext in Options and ensures it is propagated to ExecuteSql, Read, Commit, and BeginTransaction requests. It aligns with go/spanner-client-scoped-session-state design. ClientContext allows passing opaque, RPC-scoped side-channel information (like application-level user context) to Spanner. This implementation supports setting ClientContext at the Client, Database, and Request levels, with request-level options taking precedence. Key changes: - Added ClientContext to types/spanner.py and exposed it. - Updated Client.__init__ to accept a default client_context. - Added helpers for merging ClientContext with correct precedence. - Updated Snapshot, Transaction, Batch, and Database wrappers to propagate the context. - Added comprehensive unit tests in tests/unit/test_client_context.py.
product-auto-label
bot
added
size: l
Summary of ChangesHello @aseering, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 adds support for ClientContext throughout the Spanner client library, enabling its propagation to various requests. However, a critical flaw was identified in the _merge_client_context helper function, which modifies the base context object in place. This can lead to cross-request context leakage, potentially resulting in unauthorized data access or privilege escalation if the context is used for security-sensitive decisions. Additionally, there are suggestions to enhance code quality, such as replacing type() is dict checks with isinstance() for type validation and a refactoring suggestion to improve logic clarity.
aseering
force-pushed
the
feat/client-context-support
branch
from
76b129a to
eaeeb39
Compare
- Update Session.transaction to accept client_context. - Update unit tests to support client_context propagation. - Update mock objects in tests to match the expected attribute hierarchy. - Clean up nested imports in test files.
Implement a double-checked locking pattern in Transaction and _SnapshotBase methods. When multiple threads attempt to use a lazy transaction simultaneously, they race to acquire the lock. Previously, losing threads would acquire the lock and blindly send another 'begin transaction' request, ignoring that the winner had already initialized the transaction ID. This change ensures that threads re-check self._transaction_id after acquiring the lock. If the ID is present, they skip the initialization request and use the established ID.
aseering
force-pushed
the
feat/client-context-support
branch
from
eaeeb39 to
3611aec
Compare
- Fix critical security vulnerability in where in-place modification of the base object could lead to context leakage across requests. - Replace with throughout , , , and for better robustness and subclass support. - Simplify construction logic in for better readability. Based on suggestions from Gemini Code Assist.
aseering
force-pushed
the
feat/client-context-support
branch
from
e3d89e9 to
d249c67
Compare
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
aseering
force-pushed
the
feat/client-context-support
branch
from
9005b2b to
d249c67
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for ClientContext in Spanner options, enabling the propagation of RPC-scoped side-channel information. The changes are consistently applied across the client, database, and request levels, with correct precedence handling for merging contexts. The addition of helper functions for this purpose is a good structural improvement. The new functionality is well-supported by a comprehensive suite of unit tests. I have one suggestion to improve the clarity of a race condition fix.
| if self._transaction_id is not None: | ||
| is_inline_begin = False | ||
| self._lock.release() | ||
| request.transaction = TransactionSelector(id=self._transaction_id) |
There was a problem hiding this comment.
This line is redundant. The _restart_on_unavailable function, called below, will overwrite request.transaction by calling self._build_transaction_selector_pb(). This method already correctly handles the case where _transaction_id has been set by another thread. You can remove this line for cleaner code, similar to the pattern used for the same race condition fix in transaction.py.
| if self._transaction_id is not None: | ||
| is_inline_begin = False | ||
| self._lock.release() | ||
| request.transaction = TransactionSelector(id=self._transaction_id) |
There was a problem hiding this comment.
This whole block looks weird. Why is it being added in this change? It does not appear to be related to ClientContext.
| if self._transaction_id is not None: | ||
| is_inline_begin = False | ||
| self._lock.release() |
There was a problem hiding this comment.
This also does not appear to be related to ClientContext. Is it fixing some other issue?
| if self._transaction_id is not None: | ||
| is_inline_begin = False | ||
| self._lock.release() |
4 participants
This change adds support for ClientContext in Options and ensures it is propagated to ExecuteSql, Read, Commit, and BeginTransaction requests. It aligns with go/spanner-client-scoped-session-state design.
ClientContext allows passing opaque, RPC-scoped side-channel information (like application-level user context) to Spanner. This implementation supports setting ClientContext at the Client, Database, and Request levels, with request-level options taking precedence.
Key changes:
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕