-
Notifications
You must be signed in to change notification settings - Fork 31
DRAFT: google-cloud-spanner-driver project setup #739
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @bhatt4982, 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 lays the groundwork for a new Python driver for Google Cloud Spanner. It focuses on creating a complete, self-contained project with all necessary configuration, licensing, and documentation. The primary goal is to provide a DBAPI 2.0 compliant interface, enabling Python applications to interact with Spanner in a standardized manner. The inclusion of extensive testing infrastructure ensures the reliability and correctness of the driver's implementation from its inception. Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request sets up the initial project structure for the google-cloud-spanner-driver, including basic DBAPI 2.0 compliance, error handling, and testing infrastructure. The changes introduce necessary files like .gitignore, LICENSE, README.md, pyproject.toml, setup.py, noxfile.py, and the core driver components (__init__.py, connection.py, cursor.py, dbapi.py, errors.py, types.py). Unit, compliance, and system tests are also included, along with helper utilities. While the overall setup is comprehensive, there are several critical and high-severity issues related to DBAPI compliance, error handling, and type mapping that need to be addressed before this driver can be considered robust. Specifically, silent exception handling in transaction methods, a mismatch between declared and actual parameter styles, and incomplete data type conversions are significant concerns.
| except Exception as e: | ||
| # raise errors.map_spanner_error(e) | ||
| logger.debug(f"Commit failed {e}") | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching Exception and then passing silently in the commit method can hide critical database errors, leading to data inconsistency or unexpected behavior without proper notification. It's crucial to either re-raise a mapped DBAPI error or handle the exception explicitly in a way that informs the user about the failure.
Consider re-raising a mapped error for better error visibility and handling.
| except Exception as e: | |
| # raise errors.map_spanner_error(e) | |
| logger.debug(f"Commit failed {e}") | |
| pass | |
| except Exception as e: | |
| raise errors.map_spanner_error(e) |
| except Exception as e: | ||
| # raise errors.map_spanner_error(e) | ||
| logger.debug(f"Rollback failed {e}") | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the commit method, catching Exception and passing silently in rollback can mask important issues. While rollback might sometimes be best-effort, silent failures can lead to an inconsistent state. It's generally better to re-raise a mapped DBAPI error to ensure the application is aware of the failure.
Consider re-raising a mapped error for better error visibility and handling.
| except Exception as e: | |
| # raise errors.map_spanner_error(e) | |
| logger.debug(f"Rollback failed {e}") | |
| except Exception as e: | |
| raise errors.map_spanner_error(e) |
...wrappers/spannerlib-python/google-cloud-spanner-driver/google/cloud/spanner_driver/cursor.py
Show resolved
Hide resolved
.../wrappers/spannerlib-python/google-cloud-spanner-driver/google/cloud/spanner_driver/dbapi.py
Outdated
Show resolved
Hide resolved
| finally: | ||
| con.close() | ||
|
|
||
| @unittest.skip("Failing as params are not yet handled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test_execute_with_params is currently skipped because parameters are "not yet handled". This indicates a critical gap in the driver's functionality and DBAPI 2.0 compliance. A core feature of DBAPI is the ability to execute queries with parameters to prevent SQL injection and improve performance.
This test should be enabled and passing before the driver is considered functional.
No description provided.