Skip to content

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.

Sign up for GitHub

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

Closed
DineshThumma9 wants to merge 6 commits into from
Closed

fix:mysql reserved keyword issue #4106

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cfcc15d
fix(sessions): Escape MySQL reserved keyword 'key' in schema queries
DineshThumma9 Jan 9, 2026
7a27084
fix(sessions): Escape MySQL reserved keyword 'key' in schema queries …
DineshThumma9 Jan 9, 2026
0c03ba4
refactored with more dialect-aware method for quoting identifiers
DineshThumma9 Jan 9, 2026
aee5fa8
corrected misplaced comment
DineshThumma9 Jan 9, 2026
8c242a4
corrected misplaced comment and linted
DineshThumma9 Jan 9, 2026
e39d0d0
Merge branch 'main' into fix/mysql-reserved-keyword-issue
wuliang229 Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/google/adk/sessions/migration/_schema_check_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
"""Database schema version check utility."""

from __future__ import annotations

import logging
Expand All @@ -32,8 +33,11 @@ def _get_schema_version_impl(inspector, connection) -> str:
"""Gets DB schema version using inspector and connection."""
if inspector.has_table("adk_internal_metadata"):
try:
key_col = inspector.dialect.identifier_preparer.quote("key")
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()
Comment on lines 37 to 42
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jan 9, 2026

Choose a reason for hiding this comment

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

high

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()

Comment on lines 37 to 42
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jan 9, 2026

Choose a reason for hiding this comment

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

medium

For improved readability, the formatting of this execute call can be made more concise.

      result = connection.execute(
          text(f"SELECT value FROM adk_internal_metadata WHERE {key_col} = :key"),
          {"key": SCHEMA_VERSION_KEY},
      ).fetchone()

if result:
Expand All @@ -48,8 +52,7 @@ def _get_schema_version_impl(inspector, connection) -> str:
"Failed to query schema version from adk_internal_metadata: %s.",
e,
)
raise
# Metadata table doesn't exist, check for v0 schema.
raise # Metadata table doesn't exist, check for v0 schema.
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jan 9, 2026

Choose a reason for hiding this comment

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

medium

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.

Suggested change
raise # Metadata table doesn't exist, check for v0 schema.
raise
# Metadata table doesn't exist, check for v0 schema.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jan 9, 2026

Choose a reason for hiding this comment

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

medium

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.

Suggested change
raise # Metadata table doesn't exist, check for v0 schema.
raise

# V0 schema has an 'events' table with an 'actions' column.
if inspector.has_table("events"):
try:
Expand Down
18 changes: 12 additions & 6 deletions tests/unittests/sessions/migration/test_database_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,18 @@ async def test_new_db_uses_latest_schema(tmp_path):
lambda sync_conn: inspect(sync_conn).has_table('adk_internal_metadata')
)
assert has_metadata_table
schema_version = await conn.run_sync(
lambda sync_conn: sync_conn.execute(
text('SELECT value FROM adk_internal_metadata WHERE key = :key'),
{'key': _schema_check_utils.SCHEMA_VERSION_KEY},
).scalar_one_or_none()
)

def get_schema_version(sync_conn):
inspector = inspect(sync_conn)
key_col = inspector.dialect.identifier_preparer.quote('key')
return sync_conn.execute(
text(
f'SELECT value FROM adk_internal_metadata WHERE {key_col} = :key'
),
{'key': _schema_check_utils.SCHEMA_VERSION_KEY},
).scalar_one_or_none()

schema_version = await conn.run_sync(get_schema_version)
assert schema_version == _schema_check_utils.LATEST_SCHEMA_VERSION

# Verify events table columns for v1
Expand Down