fix(mysql): apply _quote_ident to all SQL paths (#442 follow-up)#514
Conversation
Replace raw backtick-quoted table names with _quote_ident() in _load_replace, _build_insert_sql, _build_upsert_sql, and get_row_count so schema-qualified table names (e.g. mydb.scores) work in all modes. Closes drt-hub#511
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Pull request overview
This PR applies MySQL table identifier quoting consistently across row-count, replace, insert, and upsert SQL generation so schema-qualified table names are rendered correctly.
Changes:
- Reuses
_quote_identinget_row_countand replace-modeTRUNCATE. - Uses
_quote_identin insert and upsert SQL builders. - Aligns MySQL qualified identifier handling with the existing swap path.
Comments suppressed due to low confidence (3)
drt/destinations/mysql.py:366
- These updated return expressions exceed the repository's configured 100-character Ruff line length (pyproject.toml:99), so
ruff check drt testswill fail. Please wrap them before merging.
f"INSERT INTO {MySQLDestination._quote_ident(table)} ({cols_str}) VALUES ({placeholders}) "
f"ON DUPLICATE KEY UPDATE {set_clause}"
)
# All columns are part of the key — just ignore duplicates
return f"INSERT IGNORE INTO {MySQLDestination._quote_ident(table)} ({cols_str}) VALUES ({placeholders})"
drt/destinations/mysql.py:347
- This introduces the schema-qualified table behavior for the INSERT path, but the PR does not add MySQL regression coverage for schema-qualified insert/replace/upsert paths. Existing MySQL tests only cover unqualified insert/upsert SQL, while schema-qualified coverage exists only for get_row_count, so this could regress without detection.
return f"INSERT INTO {MySQLDestination._quote_ident(table)} ({cols_str}) VALUES ({placeholders})"
drt/destinations/mysql.py:366
- The qualified identifier change for the upsert SQL paths lacks MySQL regression coverage. Existing upsert tests assert unqualified table names only, so both the ON DUPLICATE KEY UPDATE and INSERT IGNORE schema-qualified cases can regress without failing tests.
f"INSERT INTO {MySQLDestination._quote_ident(table)} ({cols_str}) VALUES ({placeholders}) "
f"ON DUPLICATE KEY UPDATE {set_clause}"
)
# All columns are part of the key — just ignore duplicates
return f"INSERT IGNORE INTO {MySQLDestination._quote_ident(table)} ({cols_str}) VALUES ({placeholders})"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cols_str = ", ".join(f"`{c}`" for c in columns) | ||
| placeholders = ", ".join(["%s"] * len(columns)) | ||
| return f"INSERT INTO `{table}` ({cols_str}) VALUES ({placeholders})" | ||
| return f"INSERT INTO {MySQLDestination._quote_ident(table)} ({cols_str}) VALUES ({placeholders})" |
There was a problem hiding this comment.
@copilot apply changes based on this feedback
|
|
||
| if not self._replace_truncated: | ||
| cur.execute(f"TRUNCATE TABLE `{table}`") | ||
| cur.execute(f"TRUNCATE TABLE {self._quote_ident(table)}") |
There was a problem hiding this comment.
@copilot apply changes based on this feedback
|
Welcome to drt, @Godzilaa! 🎉 Thanks for the clean MySQL-side follow-up to I also compared this against #498 (Postgres), and the divergence in A few things to address before merge. 🚫 Blockers1. CLA signatureThe CLA Assistant bot is blocking CI. Could you post the following comment
2. Missing regression tests for schema-qualified table namesIssue #511's AC requires:
This is currently unmet. Given that the substantive bug fix here is exactly PR #498's At minimum — the following 3 tests satisfy the AC: # Add to TestUpsertSql
def test_qualified_table_on_duplicate_key(self) -> None:
sql = MySQLDestination._build_upsert_sql(
table="mydb.scores",
columns=["id", "score"],
update_cols=["score"],
)
assert "INSERT INTO `mydb`.`scores`" in sql
assert "`mydb.scores`" not in sql # regression guard for the old bug
# Add to TestMySQLReplaceMode
@patch("drt.destinations.mysql.MySQLDestination._connect")
def test_replace_uses_qualified_identifier(
self, mock_connect: MagicMock
) -> None:
conn = _fake_connection()
cur = conn.cursor()
mock_connect.return_value = conn
MySQLDestination().load(
[{"user_id": 1, "company_id": 5, "score": 0.5}],
_config(table="mydb.learning_profiles"),
_options(mode="replace"),
)
sqls = [c[0][0] for c in cur.execute.call_args_list]
assert any("TRUNCATE TABLE `mydb`.`learning_profiles`" in s for s in sqls)
assert any("INSERT INTO `mydb`.`learning_profiles`" in s for s in sqls)
# Add to TestMySQLDestinationLoad
@patch("drt.destinations.mysql.MySQLDestination._connect")
def test_get_row_count_with_qualified_table(
self, mock_connect: MagicMock
) -> None:
conn = _fake_connection()
cur = conn.cursor()
cur.fetchone.return_value = (42,)
mock_connect.return_value = conn
count = MySQLDestination().get_row_count(
_config(table="mydb.learning_profiles")
)
assert count == 42
sql = cur.execute.call_args.args[0]
assert "`mydb`.`learning_profiles`" in sql
assert "`mydb.learning_profiles`" not in sqlOptional (for better coverage) — one test each for the qualified 3. Line length exceeds ruff's 100-char limitAs Copilot's review also flagged, # drt/destinations/mysql.py L347 (_build_insert_sql)
table_q = MySQLDestination._quote_ident(table)
return f"INSERT INTO {table_q} ({cols_str}) VALUES ({placeholders})"
# Same pattern for L362, L366 in _build_upsert_sql — pull table_q at the top
table_q = MySQLDestination._quote_ident(table)
if update_cols:
set_clause = ", ".join(f"`{c}` = VALUES(`{c}`)" for c in update_cols)
return (
f"INSERT INTO {table_q} ({cols_str}) VALUES ({placeholders}) "
f"ON DUPLICATE KEY UPDATE {set_clause}"
)
return f"INSERT IGNORE INTO {table_q} ({cols_str}) VALUES ({placeholders})"A note on the 🔧 Nice-to-have (optional, fine to fold into the same push)4. CHANGELOG.md entryA one-liner under the - **MySQL destination correctly quotes schema-qualified table names**
(#511): `mydb.scores` now produces `` `mydb`.`scores` `` across replace,
insert, and upsert paths (was treated as a single identifier).Next stepOnce CLA signature + 3 tests + line-length fix are pushed, I'll re-check CI Don't hesitate to drop a comment here if you get stuck or want a sanity |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck @yodakanohoshi |
masukai
left a comment
masukai
left a comment
There was a problem hiding this comment.
This is the textbook tight follow-up to #442 — applies the existing _quote_ident to the four remaining call sites consistently, and the refactor to introduce table_q = MySQLDestination._quote_ident(table) as a local before the f-strings is genuinely cleaner than just wrapping the long lines.
What I especially like about the new tests:
- Each qualified-table assertion pairs a positive check (
INSERT INTO \mydb`.`scores`) with a **negative check (`mydb.scores`` not in sql)**. The negative one is what would have caught the original bug, and is what locks down the fix against future regressions. Nice design. - Coverage now hits all four affected paths:
get_row_count,_load_replace(TRUNCATE),_build_insert_sql,_build_upsert_sql(bothON DUPLICATE KEY UPDATEandINSERT IGNOREbranches). No gaps left from Copilot's earlier callout.
CI all green, no further notes from me. Approving 🚀
Thanks @Godzilaa for picking this one up cleanly and turning the Copilot review around so quickly — that's a great first-contribution shape 🙏
A few engagement pointers if you'd like to stay involved:
- If drt is useful to you, a ⭐ on the repo really helps signal traction: https://github.com/drt-hub/drt
- The exact same shape of work is open for ClickHouse in #512 (identifier quoting audit across SQL command paths) — unassigned, and your MySQL approach here would translate directly. No pressure though.
|
@yodakanohoshi — when you have a minute, would you mind dropping a formal approval here? I've reviewed and approved on my side, and we'd talked through the changes already, but branch protection is (correctly) gating on your second review so the merge is held. 30-second job, no need to re-read in depth — just a green tick for the audit trail. Thanks 🙏 |
yodakanohoshi
left a comment
yodakanohoshi
left a comment
There was a problem hiding this comment.
Sorry for the lag on my end — thanks @masukai for picking it up. Coverage confirmed across all four affected paths, and the table_q local read better than wrapped f-strings — exactly the cleanup direction I'd hoped for. CLA + tests + lint + CHANGELOG all in. Thanks for the quick turnaround @Godzilaa. Merging.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Closes #511
Summary
MySQLDestination._quote_identalready exists and correctly handlesschema-qualified table names (`mydb.scores` → `mydb`.`scores`), but
was only used in the swap path. This applies it to the remaining paths.
Changes
get_row_count: Replaced inline split+backtick logic with`self._quote_ident(config.table)`.
_load_replace: Replaced raw `TRUNCATE TABLE `{table}`` with`self._quote_ident(table)`.
_build_insert_sql: Replaced raw ``{table}`` with`MySQLDestination._quote_ident(table)`.
_build_upsert_sql: Same change for both the `INSERT ... ONDUPLICATE KEY UPDATE` and `INSERT IGNORE` paths.
Follows the pattern established in PR #498 for Postgres.