Repository navigation
fix(tool/list-tables): trim whitespace in mysql table_names filter - #4321
beskvladimir-create wants to merge 1 commit into
Conversation
mysql-list-tables filters table_names with FIND_IN_SET, which compares each comma-separated element exactly and does not trim surrounding whitespace, so a value like "t1, t2" silently omits t2. Normalize the whitespace around commas with REGEXP_REPLACE before FIND_IN_SET, consistent with the mssql (LTRIM/RTRIM) and spanner (regexp_split_to_array + TRIM) list-tables tools. Added an integration test case with surrounding whitespace.
There was a problem hiding this comment.
Code Review
This pull request updates the MySQL list_tables tool to support comma-separated table names with surrounding whitespace by using REGEXP_REPLACE and TRIM in the SQL query. An integration test has been added to verify this behavior. The reviewer suggested a performance optimization to perform this string normalization once during the variable assignment in the CROSS JOIN clause, rather than evaluating it for every row in the WHERE clause.
| CROSS JOIN (SELECT @table_names := ?, @output_format := ?) AS variables | ||
| WHERE | ||
| T.TABLE_SCHEMA NOT IN ('mysql', 'information_schema', 'performance_schema', 'sys') | ||
| AND (NULLIF(TRIM(@table_names), '') IS NULL OR FIND_IN_SET(T.TABLE_NAME, @table_names)) | ||
| AND (NULLIF(TRIM(@table_names), '') IS NULL OR FIND_IN_SET(T.TABLE_NAME, TRIM(REGEXP_REPLACE(@table_names, '[[:space:]]*,[[:space:]]*', ',')))) |
There was a problem hiding this comment.
Instead of evaluating REGEXP_REPLACE and TRIM on @table_names for every row in INFORMATION_SCHEMA.TABLES (which can be slow and inefficient), you can perform this normalization once during the variable assignment in the CROSS JOIN clause. This simplifies the WHERE clause and improves query performance. Ensure that the regex and delimiter strings are defined as constants if they are reused elsewhere in the package.
| CROSS JOIN (SELECT @table_names := ?, @output_format := ?) AS variables | |
| WHERE | |
| T.TABLE_SCHEMA NOT IN ('mysql', 'information_schema', 'performance_schema', 'sys') | |
| AND (NULLIF(TRIM(@table_names), '') IS NULL OR FIND_IN_SET(T.TABLE_NAME, @table_names)) | |
| AND (NULLIF(TRIM(@table_names), '') IS NULL OR FIND_IN_SET(T.TABLE_NAME, TRIM(REGEXP_REPLACE(@table_names, '[[:space:]]*,[[:space:]]*', ',')))) | |
| CROSS JOIN (SELECT @table_names := TRIM(REGEXP_REPLACE(?, '[[:space:]]*,[[:space:]]*', ',')), @output_format := ?) AS variables | |
| WHERE | |
| T.TABLE_SCHEMA NOT IN ('mysql', 'information_schema', 'performance_schema', 'sys') | |
| AND (NULLIF(@table_names, '') IS NULL OR FIND_IN_SET(T.TABLE_NAME, @table_names)) |
References
- Use hardcoded string literals for values used only once. Centralize values into constants if they are used across multiple locations to maintain consistency and avoid typos.
|
Duplicated with another PR, pasting here for visibility: After looking closely at the changes in this PR, I don't think we should clean the table name whitespace within the SQL code itself. If a user provides a table name with leading or trailing whitespace, it is generally considered a typo on their end. It is safer to let the underlying database driver return an error for the invalid table name rather than having the tool attempt to implicitly modify or "correct" the input. If the database driver is silently dropping this and it wasn't an expected behavior, this will probably need a fix on the database driver's end. I'll go ahead and close this PR. Thank you! |
Description
mysql-list-tables filters table_names with FIND_IN_SET(T.TABLE_NAME, @table_names), which compares each comma-separated element exactly and does not trim surrounding whitespace. A value like "t1, t2" splits to {"t1", " t2"} and the second element never matches, so the table is silently omitted with no error (to an agent this reads as "the table doesn't exist"). This normalizes the whitespace around commas with REGEXP_REPLACE before FIND_IN_SET, consistent with mssql-list-tables (LTRIM/RTRIM) and spanner-list-tables (regexp_split_to_array plus TRIM), and with the postgres/cockroachdb fix in #4306. Added an integration test case with surrounding whitespace to RunMySQLListTablesTest.
Security: no change to parameter binding. table_names is still passed as the bound session variable; this only changes how the already-bound value is normalized before comparison, so there is no new SQL/query-injection surface.
Note: REGEXP_REPLACE requires MySQL 8.0+ (the repo docs reference the 8.4 manual). I was not able to run this against a live MySQL in my environment, so I am relying on the MySQL / Cloud SQL MySQL integration CI to validate it.
PR Checklist
Opened an issue first (#4320). Reviewed the full diff. Tests and linter to be validated by CI. An integration test case was added, so coverage does not decrease. No docs needed. Not a breaking change.
Issue Reference
Fixes #4320 🦕