-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix:mysql reserved keyword issue #4106
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:mysql reserved keyword issue #4106
Conversation
Summary of ChangesHello @DineshThumma9, 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 resolves a critical database compatibility issue by correctly handling a MySQL reserved keyword in SQL queries. By escaping the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 addresses an issue with the MySQL reserved keyword key by escaping it with backticks in SQL queries. While this fixes the problem for MySQL, the use of backticks is not portable and will cause issues with other database systems like PostgreSQL. My review provides suggestions to use a database-agnostic approach for quoting identifiers, ensuring the code remains portable across different SQL dialects supported by SQLAlchemy.
| result = connection.execute( | ||
| text("SELECT value FROM adk_internal_metadata WHERE key = :key"), | ||
| text("SELECT value FROM adk_internal_metadata WHERE `key` = :key"), | ||
| {"key": SCHEMA_VERSION_KEY}, | ||
| ).fetchone() |
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 use of backticks (`key`) to quote the key column is specific to MySQL and will cause syntax errors on other SQL databases like PostgreSQL. The pull request description incorrectly states that backticks are part of the SQL-92 standard and work on PostgreSQL; the standard uses double quotes. To ensure database portability, you should use a dialect-aware method for quoting identifiers. Since an inspector object is available, inspector.dialect.identifier_preparer.quote() can be used to get the correct quoting for the current database dialect.
key_col = inspector.dialect.identifier_preparer.quote("key")
result = connection.execute(
text(f"SELECT value FROM adk_internal_metadata WHERE {key_col} = :key"),
{"key": SCHEMA_VERSION_KEY},
).fetchone()| lambda sync_conn: sync_conn.execute( | ||
| text('SELECT value FROM adk_internal_metadata WHERE key = :key'), | ||
| text('SELECT value FROM adk_internal_metadata WHERE `key` = :key'), | ||
| {'key': _schema_check_utils.SCHEMA_VERSION_KEY}, | ||
| ).scalar_one_or_none() |
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 change in _schema_check_utils.py, using backticks for quoting is not portable across different database systems. To make this test compatible with various database backends, you should generate the quoted identifier dynamically. You can create an inspector from the sync_conn object and use it to quote the key column correctly for the dialect under test.
lambda sync_conn: sync_conn.execute(
text(
"SELECT value FROM adk_internal_metadata WHERE"
f" {inspect(sync_conn).dialect.identifier_preparer.quote('key')} = :key"
),
{'key': _schema_check_utils.SCHEMA_VERSION_KEY},
).scalar_one_or_none()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 correctly fixes an issue where a MySQL reserved keyword key was used in a raw SQL query, causing syntax errors. The solution uses SQLAlchemy's dialect-specific identifier quoting, which is a robust and portable approach. The corresponding test has also been updated. I have one minor suggestion to improve code readability by relocating a comment to its correct logical position.
| ) | ||
| raise | ||
| # Metadata table doesn't exist, check for v0 schema. | ||
| raise # Metadata table doesn't exist, check for v0 schema. |
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.
This comment appears to be misplaced. It describes the logic for when the adk_internal_metadata table doesn't exist, which corresponds to the code path taken after the if inspector.has_table("adk_internal_metadata"): block. Placing it here as an inline comment for the raise statement is misleading, as this except block is only entered if there's an error querying the metadata table that is known to exist. The original placement on a separate line was clearer.
| raise # Metadata table doesn't exist, check for v0 schema. | |
| raise | |
| # Metadata table doesn't exist, check for v0 schema. |
|
@gemini-code-assist review my changes now |
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 correctly fixes a MySQL syntax error by properly escaping a reserved keyword. The use of SQLAlchemy's dialect-aware quoting is a robust approach. The unit tests are also updated accordingly. I have one suggestion regarding a misplaced comment that affects code readability, but otherwise, the changes are good.
| ) | ||
| raise | ||
| # Metadata table doesn't exist, check for v0 schema. | ||
| raise # Metadata table doesn't exist, check for v0 schema. |
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 comment # Metadata table doesn't exist, check for v0 schema. has been moved to an incorrect location. It correctly describes the logic that follows when the adk_internal_metadata table doesn't exist (i.e., the code path after the if inspector.has_table(...) block). By attaching it to this raise statement inside the except block, it becomes misleading, as this block handles any query exception, not just the table's absence.
Please move the comment back to its original location, on a new line after the if block, to accurately describe the fallback logic for checking the v0 schema.
| raise # Metadata table doesn't exist, check for v0 schema. | |
| raise |
|
Hello @DineshThumma9, it looks like you might have intended to ask for another review. I've already provided an initial summary of your changes in the first comment on this pull request. If you'd like me to perform a new code review based on the current state of the pull request, please use the command |
|
/gemini review |
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 effectively resolves a MySQL-specific bug caused by an unescaped reserved keyword (key) in a raw SQL query. The fix is robust, employing SQLAlchemy's dialect-aware quoting mechanism, which is the correct approach. The corresponding test has also been updated appropriately. Overall, this is a high-quality contribution that correctly fixes the issue. I've added a couple of minor suggestions to improve code formatting and readability in the changed sections.
| result = connection.execute( | ||
| text("SELECT value FROM adk_internal_metadata WHERE key = :key"), | ||
| text( | ||
| f"SELECT value FROM adk_internal_metadata WHERE {key_col} = :key" | ||
| ), | ||
| {"key": SCHEMA_VERSION_KEY}, | ||
| ).fetchone() |
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.
|
Thank you @DineshThumma9 for your contribution! 🎉 Your changes have been successfully imported and merged via Copybara in commit 94d48fc. Closing this PR as the changes are now in the main branch. |
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
_If applicable, please follow the issue templates to provide as much detail as
possible.
Problem:
In the _get_schema_version_impl function of google/adk/sessions/migration/_schema_check_utils.py file, the SQL query statement uses the MySQL reserved keyword key without escaping, causing a syntax error when executing in MySQL database.
SELECT value FROM adk_internal_metadata WHERE key = :key
Solution:
changed from key to
keyto use backticks both in _schema_check_utils.py and also in test fileTesting Plan
Unit Tests:
Please include a summary of passed
pytestresults.collected 4 items
tests/unittests/sessions/migration/test_database_schema.py::test_new_db_uses_latest_schema PASSED [ 25%]
tests/unittests/sessions/migration/test_database_schema.py::test_existing_v0_db_uses_v0_schema PASSED [ 50%]
tests/unittests/sessions/migration/test_database_schema.py::test_existing_latest_db_uses_latest_schema PASSED [ 75%]
tests/unittests/sessions/migration/test_migration.py::test_migrate_from_sqlalchemy_pickle PASSED [100%]
================================================================== 4 passed in 8.36s ==================================================================
==================================================================
Manual End-to-End (E2E) Tests:
Checklist
Additional context
Add any other context or screenshots about the feature request here.