✨ transfer SQLite views as MySQL native views #148
Conversation
WalkthroughAdds sqlglot as a dependency; introduces a CLI flag to treat SQLite views as tables; implements SQLite view translation and MySQL view creation via sqlglot; extends transporter to fetch/process views, updates types to include the view flag; and adds extensive unit tests for these behaviours. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI handler
participant Transporter as SQLite3toMySQL
participant SQLite as SQLite DB
participant SQLGlot as sqlglot
participant MySQL as MySQL DB
User->>CLI: start with -A / --sqlite-views-as-tables
CLI->>Transporter: __init__(..., sqlite_views_as_tables=True)
rect rgb(249,250,255)
Note over Transporter,SQLite: Discover and transfer tables
Transporter->>SQLite: _fetch_sqlite_master_rows(['table'])
SQLite-->>Transporter: table rows
Transporter->>MySQL: create & populate tables
end
alt sqlite_views_as_tables enabled
rect rgb(237,250,237)
Note over Transporter,SQLGlot: Translate views and create MySQL views
Transporter->>SQLite: _fetch_sqlite_master_rows(['view'], include_sql=True)
SQLite-->>Transporter: view rows (name, sql)
Transporter->>SQLGlot: _translate_sqlite_view_definition(name, sql)
SQLGlot-->>Transporter: translated MySQL SQL
Transporter->>MySQL: _create_mysql_view(name, translated_sql)
MySQL-->>Transporter: success / error
end
end
Transporter-->>User: transfer complete / errors reported
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #148 +/- ##
==========================================
+ Coverage 92.83% 92.85% +0.02%
==========================================
Files 8 8
Lines 823 966 +143
==========================================
+ Hits 764 897 +133
- Misses 59 69 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
pyproject.toml (1)
48-48: Pinsqlglotto a major range to avoid silent breakages.
sqlglotevolves quickly; you rely on dialect specifics and AST APIs. Prefer an upper bound to keep builds reproducible.Apply:
- "sqlglot>=27.27.0", + "sqlglot>=27.27.0,<28",Based on learnings.
requirements_dev.txt (1)
16-16: Mirror the runtime pin for dev to keep local/CI reproducible.Align dev requirement with the runtime constraint.
-sqlglot>=27.27.0 +sqlglot>=27.27.0,<28Based on learnings.
src/sqlite3_to_mysql/types.py (1)
26-26: LGTM: config surface for views.New params/attrs (
sqlite_views_as_tables) integrate cleanly with CLI and transporter. Ensure README/CLI docs mention defaults and interaction with--mysql-skip-create-tables.Also applies to: 59-59
src/sqlite3_to_mysql/cli.py (1)
54-59: Clarify flag interplay in help text.If
-K/--mysql-skip-create-tablesis used without--sqlite-views-as-tables, views won’t be created. Consider hinting this in the help.@click.option( "-A", "--sqlite-views-as-tables", is_flag=True, - help="Materialize SQLite views as tables in MySQL instead of creating matching MySQL views.", + help="Materialize SQLite views as tables in MySQL instead of creating matching MySQL views " + "(note: if you skip creating tables, views are not created unless materialised).", )tests/unit/sqlite3_to_mysql_test.py (1)
26-64: Minor test nits for style and warnings.
- Prefer list unpack over concatenation for readability.
- Avoid ARG001 by prefixing unused fn params with “_”.
- result = cli_runner.invoke(sqlite3mysql, common_args + ["--sqlite-views-as-tables"]) + result = cli_runner.invoke(sqlite3mysql, [*common_args, "--sqlite-views-as-tables"])- def fetch_rows(object_types, include_sql=False): + def fetch_rows(_object_types, include_sql=False):Also applies to: 299-339
src/sqlite3_to_mysql/transporter.py (4)
591-627: Extend STRFTIME/now() rewrite to support modifiers.
strftime('%f','now','localtime'|'utc')and similar forms won’t match the current 2‑arg check. Consider accepting extra args when arg[1] is 'now'.- if name == "STRFTIME" and len(args) >= 2 and isinstance(args[0], exp.Literal) and _is_now_literal(args[1]): + if ( + name == "STRFTIME" + and len(args) >= 2 + and isinstance(args[0], exp.Literal) + and _is_now_literal(args[1]) + ): return exp.Anonymous( this=exp.Var(this="DATE_FORMAT"), expressions=[exp.CurrentTimestamp(), exp.Literal.string(str(args[0].this))], )And optionally ignore/consume trailing
'localtime'/'utc'modifiers if present. This improves compatibility with common SQLite patterns.
628-644: Preferlogging.exceptionwhen re‑raising unexpected DROP errors.Keeps traceback in logs.
- except mysql.connector.Error as err: + except mysql.connector.Error as err: if err.errno not in { errorcode.ER_BAD_TABLE_ERROR, errorcode.ER_WRONG_OBJECT, errorcode.ER_UNKNOWN_TABLE, }: - raise + self._logger.exception("Unexpected error dropping potential table %s", safe_name) + raise
1025-1033: Optional tuple build nit.Slightly cleaner tuple extension:
- table_types: t.Tuple[str, ...] = ("table",) - if self._sqlite_views_as_tables: - table_types = table_types + ("view",) + table_types: t.Tuple[str, ...] = ("table",) if not self._sqlite_views_as_tables else ("table", "view")
1113-1119: Log exceptions with tracebacks.Use
logger.exceptionin these except blocks to capture stack traces.- self._logger.error( + self._logger.exception( "MySQL transfer failed inserting data into %s %s: %s", "view" if object_type == "view" else "table", safe_identifier_length(table_name), err, ) @@ - self._logger.error( + self._logger.exception( "Failed translating view %s: %s", safe_identifier_length(view_name), err, ) @@ - self._logger.error( + self._logger.exception( "MySQL failed creating view %s: %s", safe_identifier_length(view_name), err, )Also applies to: 1143-1149, 1150-1155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pyproject.toml(1 hunks)requirements_dev.txt(1 hunks)src/sqlite3_to_mysql/cli.py(3 hunks)src/sqlite3_to_mysql/transporter.py(6 hunks)src/sqlite3_to_mysql/types.py(2 hunks)tests/unit/sqlite3_to_mysql_test.py(2 hunks)tests/unit/types_test.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/sqlite3_to_mysql_test.py (3)
src/sqlite3_to_mysql/transporter.py (12)
SQLite3toMySQL(61-1160)transfer(1023-1160)_fetch_sqlite_master_rows(534-572)_create_mysql_view(628-643)_translate_sqlite_view_definition(574-589)_rewrite_sqlite_view_functions(592-626)_sqlite_table_has_rowid(282-288)_create_table(645-736)_truncate_table(738-751)_add_indices(753-848)_add_foreign_keys(950-1000)_transfer_table_data(1002-1021)src/sqlite3_to_mysql/cli.py (1)
cli(152-247)tests/conftest.py (3)
MySQLCredentials(195-202)cli_runner(333-334)mysql_credentials(206-232)
src/sqlite3_to_mysql/transporter.py (1)
src/sqlite3_to_mysql/mysql_utils.py (1)
safe_identifier_length(166-168)
🪛 Ruff (0.14.0)
tests/unit/sqlite3_to_mysql_test.py
60-60: Consider [*common_args, "--sqlite-views-as-tables"] instead of concatenation
Replace with [*common_args, "--sqlite-views-as-tables"]
(RUF005)
299-299: Unused function argument: object_types
(ARG001)
317-317: Unused function argument: object_types
(ARG001)
335-335: Unused function argument: params
(ARG001)
src/sqlite3_to_mysql/transporter.py
548-556: Possible SQL injection vector through string-based query construction
(S608)
579-579: Avoid specifying long messages outside the exception class
(TRY003)
589-589: Avoid specifying long messages outside the exception class
(TRY003)
1027-1027: Consider (*table_types, "view") instead of concatenation
Replace with (*table_types, "view")
(RUF005)
1056-1056: Possible SQL injection vector through string-based query construction
(S608)
1113-1118: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1143-1147: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1150-1154: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
src/sqlite3_to_mysql/cli.py (2)
156-156: Parameter plumbing looks correct.
213-214: Constructor wiring is correct.tests/unit/types_test.py (1)
21-23: Tests cover new keys and defaults well.Good additions; typings and attribute defaults are exercised.
Also applies to: 51-53, 94-96, 136-138
tests/unit/sqlite3_to_mysql_test.py (1)
1098-1117: Nice: golden tests for view translation.These validate key rewrites (
datetime('now'),strftime) and name truncation.src/sqlite3_to_mysql/transporter.py (2)
534-573: Query builder for sqlite_master is sound.Uses placeholders for values; includes include/exclude filters; returns dicts — solid.
574-590: View AST translation via sqlglot looks correct.Setting
replace=True, forcing identifier, thenidentify=Trueon render matches the intended “CREATE OR REPLACE VIEW...…”.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sqlite3_to_mysql/transporter.py (1)
129-131: Fix chunk size conversion bug and add unit test for parameter validation.The bug is confirmed: casting to bool turns any positive chunk into True, breaking batching logic since fetchmany() expects an integer size, not a boolean. The codebase declares
_chunk_size: Optional[int](types.py line 66), the CLI accepts--chunkastype=int, and at line 1019 code callsfetchmany(self._chunk_size)with an integer argument expected. However,bool(kwargs.get("chunk"))converts chunk=2 to True, chunk=None to False—neither preserves the actual chunk size needed for batching.- self._chunk_size = bool(kwargs.get("chunk")) + # Expect an integer chunk size; normalise to None when unset/invalid or <= 0 + _chunk = kwargs.get("chunk") + self._chunk_size = int(_chunk) if isinstance(_chunk, int) and _chunk > 0 else NoneAdd a unit test in
tests/unit/sqlite3_to_mysql_test.pyto verify parameter conversion during__init__:def test_init_chunk_parameter_conversion(mocker: MockFixture) -> None: """Verify chunk parameter is correctly converted to integer _chunk_size.""" mocker.patch("sqlite3_to_mysql.transporter.SQLite3toMySQL._create_logger") mocker.patch("sqlite3_to_mysql.transporter.SQLite3toMySQL._create_database") mocker.patch("sqlite3_to_mysql.transporter.SQLite3toMySQL._check_mysql_connection") # Chunk=2 should yield _chunk_size == 2 instance = SQLite3toMySQL( sqlite_file="test.db", mysql_user="user", chunk=2 ) assert instance._chunk_size == 2 # Chunk=None should yield _chunk_size == None instance = SQLite3toMySQL( sqlite_file="test.db", mysql_user="user", chunk=None ) assert instance._chunk_size is None
♻️ Duplicate comments (1)
src/sqlite3_to_mysql/transporter.py (1)
282-286: SQLite identifier quoting helper closes prior injection/escaping gap.Great addition; this addresses the earlier “escape double quotes” review.
🧹 Nitpick comments (11)
tests/unit/sqlite3_to_mysql_test.py (3)
36-49: Prefer list spread over concatenation for args.Slightly cleaner and avoids creating a new list via “+”.
- result = cli_runner.invoke(sqlite3mysql, common_args + ["--sqlite-views-as-tables"]) + result = cli_runner.invoke(sqlite3mysql, [*common_args, "--sqlite-views-as-tables"])Also applies to: 60-64
309-315: Silence unused-param lints in test stubs.Rename unused parameters to “_object_types” to appease linters without changing behaviour.
- def fetch_rows(object_types, include_sql=False): + def fetch_rows(_object_types, include_sql=False):Also applies to: 327-333
345-351: Rename unused varargs to underscore.Minor lint-friendly tweak.
- def execute_side_effect(sql, *params): + def execute_side_effect(sql, *_args):Also applies to: 365-371
src/sqlite3_to_mysql/transporter.py (8)
540-579: Deterministic ordering can help reproducibility.Optionally add ORDER BY type, name to the sqlite_master scan so runs are stable.
- query: str = ( + query: str = ( """ SELECT {columns} FROM sqlite_master WHERE type IN ({object_types}) AND name NOT LIKE 'sqlite_%' + ORDER BY type, name """.format(
580-596: View translation: solid structure and errors surfaced.Good use of sqlglot with OR REPLACE and identifier normalisation.
Pin sqlglot to a safe 27.x range in prod to avoid AST/renderer surprises across micro-releases. Based on learnings.
597-633: Extend function rewrites to handle 'localtime'/'utc' modifiers (optional).Current rewrite covers only “…('now')”. SQLite also uses “…('now','localtime'|'utc')”. Consider mapping those to MySQL CURRENT_* with CONVERT_TZ when feasible, or at least handling them to avoid falling back unmodified.
Add tests for:
- DATETIME('now','utc') → DATE_FORMAT(CURRENT_TIMESTAMP(), …) or CONVERT_TZ(…,'SYSTEM','UTC')
- TIME('now','localtime') → CURRENT_TIME[(fsp)]
634-650: Optional: drop pre-existing view explicitly.OR REPLACE handles it, but issuing “DROP VIEW IF EXISTS” before create can make intent clearer when a view already exists (permissions permitting). Not required.
1037-1042: Tiny tuple style tweak.Readability: use tuple spread.
- table_types: t.Tuple[str, ...] = ("table",) - if self._sqlite_views_as_tables: - table_types = table_types + ("view",) + table_types: t.Tuple[str, ...] = ("table",) + if self._sqlite_views_as_tables: + table_types = (*table_types, "view")
1069-1071: Rowid alias may collide with a real “rowid” column.Edge case: a user table with a literal “rowid” column would clash with rowid as "rowid". Consider a less generic alias (e.g. "rowid") or collision check.
- if transfer_rowid: - select_list: str = 'rowid as "rowid", *' + if transfer_rowid: + select_list: str = 'rowid as "__rowid__", *'And update the insert column list accordingly.
Also applies to: 1082-1087
1125-1131: Prefer logger.exception to keep tracebacks.Improves diagnostics when data insert fails.
- self._logger.error( + self._logger.exception( "MySQL transfer failed inserting data into %s %s: %s", "view" if object_type == "view" else "table", safe_identifier_length(table_name), err, )
1155-1167: Prefer logger.exception for view translation/creation failures.Keeps stack traces in logs.
- self._logger.error( + self._logger.exception( "Failed translating view %s: %s", safe_identifier_length(view_name), err, ) @@ - self._logger.error( + self._logger.exception( "MySQL failed creating view %s: %s", safe_identifier_length(view_name), err, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sqlite3_to_mysql/transporter.py(10 hunks)tests/unit/sqlite3_to_mysql_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/sqlite3_to_mysql_test.py (3)
src/sqlite3_to_mysql/transporter.py (12)
SQLite3toMySQL(61-1172)transfer(1035-1172)_fetch_sqlite_master_rows(540-578)_sqlite_table_has_rowid(287-294)_create_mysql_view(634-649)_translate_sqlite_view_definition(580-595)_rewrite_sqlite_view_functions(598-632)_create_table(651-744)_truncate_table(746-759)_add_indices(761-859)_add_foreign_keys(961-1012)_transfer_table_data(1014-1033)src/sqlite3_to_mysql/cli.py (1)
cli(152-247)tests/conftest.py (2)
MySQLCredentials(195-202)cli_runner(333-334)
src/sqlite3_to_mysql/transporter.py (1)
src/sqlite3_to_mysql/mysql_utils.py (1)
safe_identifier_length(166-168)
🪛 Ruff (0.14.0)
tests/unit/sqlite3_to_mysql_test.py
60-60: Consider [*common_args, "--sqlite-views-as-tables"] instead of concatenation
Replace with [*common_args, "--sqlite-views-as-tables"]
(RUF005)
309-309: Unused function argument: object_types
(ARG001)
327-327: Unused function argument: object_types
(ARG001)
345-345: Unused function argument: params
(ARG001)
365-365: Unused function argument: params
(ARG001)
src/sqlite3_to_mysql/transporter.py
290-290: Possible SQL injection vector through string-based query construction
(S608)
554-562: Possible SQL injection vector through string-based query construction
(S608)
585-585: Avoid specifying long messages outside the exception class
(TRY003)
595-595: Avoid specifying long messages outside the exception class
(TRY003)
1039-1039: Consider (*table_types, "view") instead of concatenation
Replace with (*table_types, "view")
(RUF005)
1069-1069: Possible SQL injection vector through string-based query construction
(S608)
1086-1086: Possible SQL injection vector through string-based query construction
(S608)
1125-1130: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1155-1159: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1162-1166: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
tests/unit/sqlite3_to_mysql_test.py (1)
1129-1135: Good end-to-end assertion for CURRENT_TIMESTAMP mapping.Covers the critical path for DATETIME('now') → MySQL view SQL.
src/sqlite3_to_mysql/transporter.py (3)
117-118: Flag wiring LGTM.Boolean flag for sqlite_views_as_tables is correctly plumbed.
287-294: Rowid probe now safely quotes identifiers.Correct fix; avoids breakage on names with embedded quotes.
659-665: Consistent use of _sqlite_quote_ident in PRAGMAs is spot on.Reduces risk on odd names across table_info/index_list/index_info/foreign_key_list.
Also applies to: 761-779, 778-779, 962-964
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/sqlite3_to_mysql/transporter.py (1)
1144-1170: Consider using logging.exception for better debugging.The view creation loop correctly:
- Only processes views when not treating them as tables
- Skips views without SQL definitions
- Handles translation and creation errors appropriately
However, consider using
logging.exceptioninstead oflogging.errorin the exception handlers (lines 1158, 1165) to automatically include stack traces, which aids debugging without requiring explicit error parameter.Apply these diffs if you'd like more detailed error logs:
except ValueError as err: - self._logger.error( + self._logger.exception( "Failed translating view %s: %s", safe_identifier_length(view_name), err, ) raise except mysql.connector.Error as err: - self._logger.error( + self._logger.exception( "MySQL failed creating view %s: %s", safe_identifier_length(view_name), err, ) raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sqlite3_to_mysql/transporter.py(11 hunks)tests/unit/sqlite3_to_mysql_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sqlite3_to_mysql/transporter.py (1)
src/sqlite3_to_mysql/mysql_utils.py (1)
safe_identifier_length(166-168)
tests/unit/sqlite3_to_mysql_test.py (1)
src/sqlite3_to_mysql/transporter.py (11)
transfer(1038-1175)_fetch_sqlite_master_rows(542-581)_sqlite_table_has_rowid(289-296)_create_mysql_view(637-652)_translate_sqlite_view_definition(583-598)_rewrite_sqlite_view_functions(601-635)_create_table(654-747)_truncate_table(749-762)_add_indices(764-862)_add_foreign_keys(964-1015)_transfer_table_data(1017-1036)
🪛 Ruff (0.14.0)
src/sqlite3_to_mysql/transporter.py
292-292: Possible SQL injection vector through string-based query construction
(S608)
556-565: Possible SQL injection vector through string-based query construction
(S608)
588-588: Avoid specifying long messages outside the exception class
(TRY003)
598-598: Avoid specifying long messages outside the exception class
(TRY003)
1072-1072: Possible SQL injection vector through string-based query construction
(S608)
1089-1089: Possible SQL injection vector through string-based query construction
(S608)
1128-1133: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1158-1162: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1165-1169: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/unit/sqlite3_to_mysql_test.py
60-60: Consider [*common_args, "--sqlite-views-as-tables"] instead of concatenation
Replace with [*common_args, "--sqlite-views-as-tables"]
(RUF005)
309-309: Unused function argument: object_types
(ARG001)
327-327: Unused function argument: object_types
(ARG001)
345-345: Unused function argument: params
(ARG001)
365-365: Unused function argument: params
(ARG001)
1154-1154: Unused method argument: mocker
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (27)
tests/unit/sqlite3_to_mysql_test.py (14)
26-64: LGTM! Comprehensive flag propagation test.The test correctly verifies that the new
--sqlite-views-as-tablesflag is properly propagated from the CLI to the SQLite3toMySQL constructor, covering both the default (False) and explicit (True) cases.
66-101: LGTM! Proper collation validation test.The test correctly verifies that invalid collation combinations are rejected by the CLI with appropriate error messages.
103-116: LGTM! Fallback mechanism test is sound.The test correctly verifies that the typing_extensions fallback works when TypedDict is unavailable in the typing module.
118-134: LGTM! Inclusion filter test is thorough.The test correctly verifies that
_fetch_sqlite_master_rowsproperly applies the inclusion filter when_sqlite_tablesis set, checking both the generated query and the returned results.
136-150: LGTM! Exclusion filter test is correct.The test properly verifies that
_fetch_sqlite_master_rowsapplies the exclusion filter when_exclude_sqlite_tablesis set.
152-160: LGTM! Identifier escaping is properly tested.The test correctly verifies that double quotes in SQLite table names are properly escaped by doubling them, preventing SQL injection and supporting odd table names.
162-168: LGTM! Edge case is properly handled.The test correctly verifies that calling
_fetch_sqlite_master_rowswith an empty object_types sequence returns an empty list without executing any query.
170-221: LGTM! Comprehensive MySQL view creation tests.The three tests properly cover:
- Successful view creation with DROP TABLE and CREATE VIEW
- Known drop errors (ER_WRONG_OBJECT, etc.) that should be ignored
- Unexpected errors that should be propagated
223-244: LGTM! Error handling tests are correct.The tests properly verify that parsing and rendering errors during view translation are caught and wrapped in ValueError with descriptive messages.
246-270: LGTM! Function rewriting tests are comprehensive.The tests correctly verify that SQLite-specific time functions with 'now' are properly rewritten to MySQL equivalents:
datetime('now')→CURRENT_TIMESTAMP()strftime(...)→DATE_FORMAT(...)- TimeToStr expressions →
DATE_FORMAT(...)
272-304: LGTM! Well-structured test helper.The
_make_transfer_stubhelper appropriately creates a test instance with controlled state, avoiding full initialization while setting up all necessary mocks and attributes for transfer workflow tests.
306-394: LGTM! Transfer workflow tests are thorough.The five tests comprehensively verify:
- MySQL views are created from SQLite views
- Views can be treated as tables when requested
- Data transfer is invoked correctly
- SQLite identifiers with special characters are properly escaped
- Views without SQL definitions are gracefully skipped
396-425: LGTM! Chunking tests are correct.The tests properly verify both chunked and non-chunked data transfer paths, asserting the correct fetch methods and commit behavior.
1129-1179: LGTM! Additional view and parameter tests are sound.The four tests properly verify:
- SQLite
datetime('now')in views translates to MySQLCURRENT_TIMESTAMP()- SQLite
strftimein views translates to MySQLDATE_FORMAT- Long view names are truncated to 64 characters (MySQL limit)
- Chunk parameter is correctly normalized to integer or None
Note: The unused
mockerparameter in line 1154 is a fixture requirement, not an issue.src/sqlite3_to_mysql/transporter.py (13)
117-117: LGTM! New attribute initialization is correct.The
_sqlite_views_as_tablesattribute is properly initialized from kwargs with appropriate default (False).
129-131: LGTM! Robust chunk size normalization.The chunk size validation correctly accepts only positive integers and defaults to None otherwise, preventing issues with invalid chunk values.
284-287: LGTM! Critical identifier escaping method.The
_sqlite_quote_identmethod correctly escapes SQLite identifiers by doubling internal double quotes, preventing SQL injection when identifiers contain special characters. This addresses security concerns raised in previous reviews.
289-296: LGTM! Identifier escaping properly applied.The method now uses
_sqlite_quote_identto safely escape table names, preventing issues with special characters. The static analysis warning (S608) is a false positive—the identifier is properly escaped before interpolation.
542-581: LGTM! Parameterised query prevents injection.The method correctly uses parameterised queries with placeholders for all user-supplied values (object_types, table names). The
format()calls only insert safe, generated strings (column names and placeholders), not user input. The static analysis warning (S608) is a false positive.
583-598: LGTM! View translation with proper error handling.The method correctly:
- Truncates long view names to MySQL's 64-character limit
- Parses SQLite view SQL with sqlglot
- Transforms SQLite-specific functions to MySQL equivalents
- Handles both parse and render errors with descriptive messages
The static analysis warnings (TRY003) about exception messages are stylistic—dynamic context (view name) in error messages is valuable for debugging.
600-635: LGTM! Sophisticated function transformation logic.The static method correctly rewrites SQLite view functions to MySQL equivalents using sqlglot's expression tree transformation:
datetime('now')→CURRENT_TIMESTAMP()date('now')→CURRENT_DATE()time('now')→CURRENT_TIME()strftime(..., 'now')→DATE_FORMAT(CURRENT_TIMESTAMP(), ...)The transformation logic is well-structured and handles both
AnonymousandTimeToStrexpression types.
637-652: LGTM! View creation with proper cleanup.The method correctly:
- Drops any conflicting table before creating the view
- Ignores expected "not a table" errors during DROP
- Logs view creation
- Commits the transaction
662-667: LGTM! Identifier escaping in PRAGMA commands.The method now properly escapes table names in PRAGMA commands using
_sqlite_quote_ident, preventing issues with special characters.
765-781: LGTM! Comprehensive identifier escaping in index operations.The method now properly escapes both table and index names in all PRAGMA commands, ensuring correct handling of identifiers with special characters.
965-966: LGTM! Identifier escaping in foreign key operations.The method now properly escapes table names in the PRAGMA foreign_key_list command.
1040-1049: LGTM! Correct view handling logic.The method correctly handles two workflows:
- When
_sqlite_views_as_tablesis True, views are fetched with tables and treated as materialized tables- When False, views are fetched separately (with SQL definitions) for translation to MySQL views
1054-1142: LGTM! Table/view transfer loop with proper escaping.The loop correctly:
- Handles both tables and views uniformly when
_sqlite_views_as_tablesis enabled- Uses
_sqlite_quote_identfor all SQLite queries- Logs object type (view vs table) appropriately
- Includes proper error messages distinguishing views from tables
The static analysis warnings (S608, lines 1072 and 1089) are false positives—all identifiers are properly escaped before interpolation.
…dling" This reverts commit 894c89f.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/sqlite3_to_mysql/transporter.py (5)
129-131: Consider simplifying the chunk size validation.The logic is correct, but it could be more explicit about handling None:
- # Expect an integer chunk size; normalize to None when unset/invalid or <= 0 - _chunk = kwargs.get("chunk") - self._chunk_size = int(_chunk) if isinstance(_chunk, int) and _chunk > 0 else None + # Expect an integer chunk size; normalize to None when unset/invalid or <= 0 + _chunk = kwargs.get("chunk") + self._chunk_size = _chunk if isinstance(_chunk, int) and _chunk > 0 else NoneThe
int(_chunk)cast is redundant since you've already verified it's an integer.
582-597: LGTM!The method correctly uses sqlglot to parse SQLite view definitions and translate them to MySQL syntax with proper error handling. The identifier length truncation is appropriate for MySQL's 64-character limit.
The static analysis hint (TRY003) suggests moving long exception messages to custom exception classes, but this is a minor stylistic concern and can be deferred.
1039-1048: Consider tuple unpacking for cleaner code.The logic is correct, but line 1041 can be simplified using tuple unpacking.
- table_types: t.Tuple[str, ...] = ("table",) - if self._sqlite_views_as_tables: - table_types = table_types + ("view",) + table_types: t.Tuple[str, ...] = ("table",) + if self._sqlite_views_as_tables: + table_types = (*table_types, "view")
1127-1132: Uselogging.exceptionfor better error diagnostics.In exception handlers,
logging.exceptionautomatically includes the stack trace, which aids debugging.- except mysql.connector.Error as err: - self._logger.error( - "MySQL transfer failed inserting data into %s %s: %s", - "view" if object_type == "view" else "table", - safe_identifier_length(table_name), - err, - ) - raise + except mysql.connector.Error as err: + self._logger.exception( + "MySQL transfer failed inserting data into %s %s: %s", + "view" if object_type == "view" else "table", + safe_identifier_length(table_name), + err, + ) + raise
1143-1169: Uselogging.exceptionfor better error diagnostics.The view creation logic is correct, but both exception handlers should use
logging.exceptioninstead oflogging.errorto include stack traces.try: mysql_view_sql: str = self._translate_sqlite_view_definition(view_name, sql_definition) self._create_mysql_view(view_name, mysql_view_sql) except ValueError as err: - self._logger.error( + self._logger.exception( "Failed translating view %s: %s", safe_identifier_length(view_name), err, ) raise except mysql.connector.Error as err: - self._logger.error( + self._logger.exception( "MySQL failed creating view %s: %s", safe_identifier_length(view_name), err, ) raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sqlite3_to_mysql/transporter.py(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sqlite3_to_mysql/transporter.py (1)
src/sqlite3_to_mysql/mysql_utils.py (1)
safe_identifier_length(166-168)
🪛 Ruff (0.14.0)
src/sqlite3_to_mysql/transporter.py
292-292: Possible SQL injection vector through string-based query construction
(S608)
556-564: Possible SQL injection vector through string-based query construction
(S608)
587-587: Avoid specifying long messages outside the exception class
(TRY003)
597-597: Avoid specifying long messages outside the exception class
(TRY003)
1041-1041: Consider (*table_types, "view") instead of concatenation
Replace with (*table_types, "view")
(RUF005)
1071-1071: Possible SQL injection vector through string-based query construction
(S608)
1088-1088: Possible SQL injection vector through string-based query construction
(S608)
1127-1132: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1157-1161: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1164-1168: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
src/sqlite3_to_mysql/transporter.py (9)
117-118: LGTM!The
_sqlite_views_as_tablesattribute is correctly initialised from kwargs with an appropriate default value.
284-287: LGTM!The identifier quoting implementation is correct per SQLite specification. This properly addresses the past review comment about escaping double quotes in SQLite identifiers.
289-296: LGTM!The method now correctly uses
_sqlite_quote_identto safely handle table names with special characters.
542-580: LGTM!The method correctly uses parameterised queries to safely construct the sqlite_master query with dynamic filtering. The static analysis warning (S608) is a false positive.
636-651: LGTM!The method correctly handles the view creation flow, including the defensive
DROP TABLE IF EXISTSto handle cases where a table was previously created with the view name. Error handling is appropriate.
661-666: LGTM!The method now correctly quotes table names for PRAGMA commands using
_sqlite_quote_ident. The static analysis warnings (S608) are false positives since identifiers are properly escaped.
764-780: LGTM!The method now correctly quotes identifiers in all PRAGMA commands, ensuring robustness with special characters in table and index names.
964-965: LGTM!Consistent identifier quoting applied to the PRAGMA foreign_key_list command.
1053-1088: LGTM!The transfer loop correctly handles both tables and views, with proper identifier quoting and appropriate logging. The static analysis warnings (S608) on lines 1071 and 1088 are false positives since identifiers are properly escaped via
_sqlite_quote_ident.
…prove time formatting
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/sqlite3_to_mysql/transporter.py (1)
1127-1132: Consider usinglogging.exceptionfor better error context.When logging errors from exception handlers, use
logging.exceptioninstead oflogging.errorto automatically include the stack trace. This aids debugging.Apply this diff:
- self._logger.error( + self._logger.exception( "MySQL transfer failed inserting data into %s %s: %s", "view" if object_type == "view" else "table", safe_identifier_length(table_name), err, )Also applies to: 1157-1161, 1164-1168
As per coding guidelines
tests/unit/sqlite3_to_mysql_test.py (1)
26-64: LGTM!The test correctly verifies that the
--sqlite-views-as-tablesCLI flag propagates through to the transporter constructor.Optional: Use unpacking syntax for list concatenation (more Pythonic):
- result = cli_runner.invoke(sqlite3mysql, common_args + ["--sqlite-views-as-tables"]) + result = cli_runner.invoke(sqlite3mysql, [*common_args, "--sqlite-views-as-tables"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sqlite3_to_mysql/transporter.py(11 hunks)tests/unit/sqlite3_to_mysql_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sqlite3_to_mysql/transporter.py (1)
src/sqlite3_to_mysql/mysql_utils.py (1)
safe_identifier_length(166-168)
tests/unit/sqlite3_to_mysql_test.py (3)
src/sqlite3_to_mysql/transporter.py (9)
SQLite3toMySQL(61-1174)transfer(1037-1174)_fetch_sqlite_master_rows(542-580)_sqlite_table_has_rowid(289-296)_create_mysql_view(636-651)_translate_sqlite_view_definition(582-597)_rewrite_sqlite_view_functions(600-634)_create_table(653-746)_truncate_table(748-761)src/sqlite3_to_mysql/cli.py (1)
cli(152-247)tests/conftest.py (3)
MySQLCredentials(195-202)cli_runner(333-334)mysql_database(321-329)
🪛 Ruff (0.14.1)
src/sqlite3_to_mysql/transporter.py
292-292: Possible SQL injection vector through string-based query construction
(S608)
556-564: Possible SQL injection vector through string-based query construction
(S608)
587-587: Avoid specifying long messages outside the exception class
(TRY003)
597-597: Avoid specifying long messages outside the exception class
(TRY003)
1071-1071: Possible SQL injection vector through string-based query construction
(S608)
1088-1088: Possible SQL injection vector through string-based query construction
(S608)
1127-1132: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1157-1161: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1164-1168: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/unit/sqlite3_to_mysql_test.py
60-60: Consider [*common_args, "--sqlite-views-as-tables"] instead of concatenation
Replace with [*common_args, "--sqlite-views-as-tables"]
(RUF005)
310-310: Unused function argument: object_types
(ARG001)
328-328: Unused function argument: object_types
(ARG001)
346-346: Unused function argument: params
(ARG001)
366-366: Unused function argument: params
(ARG001)
1178-1178: Unused method argument: mocker
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (16)
src/sqlite3_to_mysql/transporter.py (9)
16-22: LGTM!The imports of
sqlglot,sqlglot.errors, andsqlglot.expressionsare appropriate for the new SQL translation functionality.
117-131: LGTM!The initialization of
_sqlite_views_as_tablesand the improved chunk size normalization logic are appropriate. The chunk size now properly validates that it's an integer greater than zero before accepting it.
284-287: LGTM!The
_sqlite_quote_identhelper correctly escapes SQLite identifiers by doubling internal quotes, addressing the SQL injection concern raised in the past review. This is the standard SQLite escaping mechanism.
542-580: LGTM!The
_fetch_sqlite_master_rowsmethod correctly uses parameterized queries to prevent SQL injection. The filtering logic for inclusion/exclusion lists is appropriate.
636-651: LGTM!The
_create_mysql_viewmethod correctly handles dropping any existing table/view and creates the MySQL view. The error handling for known drop errors is appropriate.
661-666: LGTM!The consistent use of
_sqlite_quote_identfor SQLite identifiers in PRAGMA queries properly escapes table names containing special characters.Also applies to: 764-780, 964-965
1037-1062: LGTM!The transfer workflow correctly handles both modes: treating views as tables (materialising them) or creating native MySQL views. The logic for fetching and processing views is appropriate.
1071-1088: Verify identifier escaping in all SQLite queries.The code correctly uses
_sqlite_quote_identfor the COUNT and SELECT queries. This properly handles table/view names containing special characters like double quotes.
1143-1169: LGTM!The view creation workflow properly handles missing SQL definitions, translation errors, and MySQL creation errors. The logic flow is clear and appropriate.
tests/unit/sqlite3_to_mysql_test.py (7)
1-23: LGTM!The new imports are appropriate for testing CLI integration, view translation, and sqlglot functionality.
66-101: LGTM!The test correctly verifies that invalid collation values for a given charset are rejected by the CLI.
103-116: LGTM!The test correctly verifies the TypedDict fallback behaviour for compatibility with older Python versions.
118-221: LGTM!The tests provide comprehensive coverage of the new
_fetch_sqlite_master_rows, identifier quoting, and MySQL view creation functionality. Edge cases and error handling are well tested.
223-271: LGTM!The tests verify view translation error handling and function rewriting logic. However, note that these tests don't verify comprehensive format code translation between SQLite and MySQL, which remains a critical issue in the implementation.
273-442: LGTM!The transfer workflow tests provide excellent coverage of view handling, data transfer, identifier escaping, and chunking behaviour. The helper function
_make_transfer_stubis well-structured for test isolation.
1146-1203: LGTM!The additional view translation tests verify the mapping of SQLite functions to MySQL equivalents and view name truncation. The chunk parameter conversion test ensures proper initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sqlite3_to_mysql/transporter.py(12 hunks)tests/unit/sqlite3_to_mysql_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sqlite3_to_mysql/transporter.py (1)
src/sqlite3_to_mysql/mysql_utils.py (1)
safe_identifier_length(166-168)
tests/unit/sqlite3_to_mysql_test.py (3)
src/sqlite3_to_mysql/transporter.py (10)
SQLite3toMySQL(61-1206)transfer(1069-1206)_fetch_sqlite_master_rows(572-610)_sqlite_table_has_rowid(311-318)_create_mysql_view(668-683)_translate_sqlite_view_definition(612-627)_rewrite_sqlite_view_functions(630-666)_create_table(685-778)_truncate_table(780-793)_transfer_table_data(1048-1067)src/sqlite3_to_mysql/cli.py (1)
cli(152-247)tests/conftest.py (3)
MySQLCredentials(195-202)cli_runner(333-334)mysql_database(321-329)
🪛 Ruff (0.14.1)
src/sqlite3_to_mysql/transporter.py
83-103: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
314-314: Possible SQL injection vector through string-based query construction
(S608)
586-594: Possible SQL injection vector through string-based query construction
(S608)
617-617: Avoid specifying long messages outside the exception class
(TRY003)
627-627: Avoid specifying long messages outside the exception class
(TRY003)
1103-1103: Possible SQL injection vector through string-based query construction
(S608)
1120-1120: Possible SQL injection vector through string-based query construction
(S608)
1159-1164: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1189-1193: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1196-1200: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/unit/sqlite3_to_mysql_test.py
60-60: Consider [*common_args, "--sqlite-views-as-tables"] instead of concatenation
Replace with [*common_args, "--sqlite-views-as-tables"]
(RUF005)
310-310: Unused function argument: object_types
(ARG001)
328-328: Unused function argument: object_types
(ARG001)
346-346: Unused function argument: params
(ARG001)
366-366: Unused function argument: params
(ARG001)
1178-1178: Unused method argument: mocker
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
- GitHub Check: Codacy Static Code Analysis
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/sqlite3_to_mysql/transporter.py (1)
293-297: SQLite identifier quoting helper correctly applied.Escaping embedded double quotes fixes the prior breakages across PRAGMA/SELECT calls. Nice reuse.
Also applies to: 300-305, 681-687, 784-787, 792-801, 984-986
🧹 Nitpick comments (5)
src/sqlite3_to_mysql/transporter.py (5)
421-534: DEFAULT translator: strong coverage; consider UTC-aware fallbacks.For SQLite defaults like datetime('now','utc')/time('now','utc')/date('now','utc'), map to MySQL UTC_* functions to better preserve semantics.
Apply this diff selectively inside the TIMESTAMP/DATETIME/DATE/TIME branches:
@@ - if base.startswith("TIMESTAMP") and ( + if base.startswith("TIMESTAMP") and ( self.CURRENT_TS.match(s) or (self.SQLITE_NOW_FUNC.match(s) and s.lower().startswith("datetime")) or self.STRFTIME_NOW.match(s) ): len_match: t.Optional[re.Match[str]] = self.COLUMN_LENGTH_PATTERN.search(column_type) fsp: str = "" @@ - return f"CURRENT_TIMESTAMP{fsp}" + if "utc" in s.lower(): + return f"UTC_TIMESTAMP{fsp}" + return f"CURRENT_TIMESTAMP{fsp}" @@ - if base.startswith("DATETIME") and ( + if base.startswith("DATETIME") and ( self.CURRENT_TS.match(s) or (self.SQLITE_NOW_FUNC.match(s) and s.lower().startswith("datetime")) or self.STRFTIME_NOW.match(s) ): if not self._allow_current_ts_dt: return "" @@ - return f"CURRENT_TIMESTAMP{fsp}" + if "utc" in s.lower(): + return f"UTC_TIMESTAMP{fsp}" + return f"CURRENT_TIMESTAMP{fsp}" @@ - if ( + if ( base.startswith("DATE") and ( self.CURRENT_DATE.match(s) or self.CURRENT_TS.match(s) # map CURRENT_TIMESTAMP → CURRENT_DATE for DATE or (self.SQLITE_NOW_FUNC.match(s) and s.lower().startswith("date")) or self.STRFTIME_NOW.match(s) ) and self._allow_expr_defaults ): - return "CURRENT_DATE" + return "UTC_DATE" if "utc" in s.lower() else "CURRENT_DATE" @@ - if ( + if ( base.startswith("TIME") and ( self.CURRENT_TIME.match(s) or self.CURRENT_TS.match(s) # map CURRENT_TIMESTAMP → CURRENT_TIME for TIME or (self.SQLITE_NOW_FUNC.match(s) and s.lower().startswith("time")) or self.STRFTIME_NOW.match(s) ) and self._allow_expr_defaults ): len_match = self.COLUMN_LENGTH_PATTERN.search(column_type) fsp = "" @@ - return f"CURRENT_TIME{fsp}" + return (f"UTC_TIME{fsp}" if "utc" in s.lower() else f"CURRENT_TIME{fsp}")
560-599: Parameterised IN-lists: safe; silence S608 to reduce noise.Query is parameterised; the dynamic identifier list only affects placeholder count. Consider silencing Ruff’s S608 at the execute site.
- self._sqlite_cur.execute(query, params) + self._sqlite_cur.execute(query, params) # noqa: S608 - parameterised; query text assembled from constants/placeholders
656-672: Also drop existing VIEW to avoid stale artefacts.CREATE OR REPLACE should suffice, but explicitly dropping a same‑named VIEW mirrors the DROP TABLE guard and removes edge cases when ‘OR REPLACE’ isn’t supported.
try: self._mysql_cur.execute(f"DROP TABLE IF EXISTS `{safe_name}`") self._mysql.commit() except mysql.connector.Error as err: @@ raise + + # Ensure a stale VIEW does not block creation (defensive) + try: + self._mysql_cur.execute(f"DROP VIEW IF EXISTS `{safe_name}`") + self._mysql.commit() + except mysql.connector.Error: + pass
1147-1152: Log stack traces for failures creating views/inserting rows.Use logger.exception() in except blocks to retain tracebacks; keeps logs actionable.
- self._logger.error( + self._logger.exception( "MySQL transfer failed inserting data into %s %s: %s", "view" if object_type == "view" else "table", safe_identifier_length(table_name), err, ) @@ - self._logger.error( + self._logger.exception( "Failed translating view %s: %s", safe_identifier_length(view_name), err, ) @@ - self._logger.error( + self._logger.exception( "MySQL failed creating view %s: %s", safe_identifier_length(view_name), err, )Also applies to: 1177-1181, 1184-1189
64-68: Add inline doc for excluding "%H:%M:%S"
Include a brief comment aboveSQLGLOT_MYSQL_INVERSE_TIME_MAPPINGexplaining that"%H:%M:%S"is excluded to prevent greedy composite matching; existing tests intests/unit/sqlite3_to_mysql_test.pyalready verify the mapping to"%H:%i:%s".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sqlite3_to_mysql/transporter.py(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sqlite3_to_mysql/transporter.py (1)
src/sqlite3_to_mysql/mysql_utils.py (1)
safe_identifier_length(166-168)
🪛 Ruff (0.14.1)
src/sqlite3_to_mysql/transporter.py
301-301: Possible SQL injection vector through string-based query construction
(S608)
574-582: Possible SQL injection vector through string-based query construction
(S608)
605-605: Avoid specifying long messages outside the exception class
(TRY003)
615-615: Avoid specifying long messages outside the exception class
(TRY003)
1091-1091: Possible SQL injection vector through string-based query construction
(S608)
1108-1108: Possible SQL injection vector through string-based query construction
(S608)
1147-1152: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1177-1181: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1184-1188: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/sqlite3_to_mysql/transporter.py (3)
138-141: Chunk normalisation looks good.Clear handling of <=0 as unset. LGTM.
600-616: Translate, then render with identify=True: LGTM.Nice, robust failure messages and identifier handling.
551-559: No additional tests required: minutes/seconds (%M/%S) and week tokens (%W/%w) are already covered in existing tests.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/sqlite3_to_mysql_test.py (1)
60-60: Consider using spread operator for list concatenation.The static analysis tool suggests replacing list concatenation with the spread operator for cleaner syntax.
- result = cli_runner.invoke(sqlite3mysql, common_args + ["--sqlite-views-as-tables"]) + result = cli_runner.invoke(sqlite3mysql, [*common_args, "--sqlite-views-as-tables"])src/sqlite3_to_mysql/transporter.py (1)
1186-1191: Consider usinglogging.exceptionfor better stack traces.When logging within exception handlers,
logging.exceptionautomatically includes the stack trace, providing more debugging context thanlogging.error.- self._logger.error( - "MySQL transfer failed inserting data into %s %s: %s", - "view" if object_type == "view" else "table", - safe_identifier_length(table_name), - err, - ) + self._logger.exception( + "MySQL transfer failed inserting data into %s %s", + "view" if object_type == "view" else "table", + safe_identifier_length(table_name), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sqlite3_to_mysql/transporter.py(15 hunks)tests/unit/sqlite3_to_mysql_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sqlite3_to_mysql/transporter.py (1)
src/sqlite3_to_mysql/mysql_utils.py (1)
safe_identifier_length(166-168)
tests/unit/sqlite3_to_mysql_test.py (3)
src/sqlite3_to_mysql/transporter.py (10)
SQLite3toMySQL(70-1233)transfer(1096-1233)_fetch_sqlite_master_rows(564-602)_sqlite_table_has_rowid(298-305)_create_mysql_view(691-710)_translate_sqlite_view_definition(604-619)_rewrite_sqlite_view_functions(622-689)_create_table(712-805)_truncate_table(807-820)_transfer_table_data(1075-1094)src/sqlite3_to_mysql/cli.py (1)
cli(152-247)tests/conftest.py (3)
MySQLCredentials(195-202)cli_runner(333-334)mysql_database(321-329)
🪛 Ruff (0.14.1)
src/sqlite3_to_mysql/transporter.py
301-301: Possible SQL injection vector through string-based query construction
(S608)
578-586: Possible SQL injection vector through string-based query construction
(S608)
609-609: Avoid specifying long messages outside the exception class
(TRY003)
619-619: Avoid specifying long messages outside the exception class
(TRY003)
1130-1130: Possible SQL injection vector through string-based query construction
(S608)
1147-1147: Possible SQL injection vector through string-based query construction
(S608)
1186-1191: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1216-1220: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1223-1227: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/unit/sqlite3_to_mysql_test.py
60-60: Consider [*common_args, "--sqlite-views-as-tables"] instead of concatenation
Replace with [*common_args, "--sqlite-views-as-tables"]
(RUF005)
364-364: Unused function argument: object_types
(ARG001)
382-382: Unused function argument: object_types
(ARG001)
400-400: Unused function argument: params
(ARG001)
420-420: Unused function argument: params
(ARG001)
1260-1260: Unused method argument: mocker
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test (python3.10, mariadb:10.0, 1, true, 3.10)
- GitHub Check: Test (python3.11, mariadb:10.0, 1, true, 3.11)
- GitHub Check: Test (python3.9, mariadb:10.0, 1, true, 3.9)
- GitHub Check: Test (python3.13, mariadb:5.5, 1, true, 3.13)
- GitHub Check: Test (python3.10, mariadb:5.5, 1, true, 3.10)
- GitHub Check: Test (python3.12, mariadb:5.5, 1, true, 3.12)
- GitHub Check: Test (python3.9, mariadb:5.5, 1, true, 3.9)
- GitHub Check: Test (python3.11, mariadb:5.5, 1, true, 3.11)
🔇 Additional comments (29)
tests/unit/sqlite3_to_mysql_test.py (10)
66-101: LGTM!The collation validation test correctly verifies that invalid collations are rejected with appropriate error messages.
103-116: LGTM!The TypedDict fallback test correctly validates backwards compatibility with older Python versions.
118-168: LGTM!These tests comprehensively verify the
_fetch_sqlite_master_rowsmethod with inclusion/exclusion filters and proper identifier quoting.
170-225: LGTM!These tests thoroughly validate the
_create_mysql_viewmethod, including success paths and both expected and unexpected error handling.
227-248: LGTM!These tests correctly validate error handling in the view definition translation pipeline.
250-326: LGTM!These tests comprehensively validate the
_rewrite_sqlite_view_functionsmethod, including proper handling of SQLite datetime functions, UTC/localtime modifiers, and strftime format translation.
327-359: LGTM!This helper function provides clean test isolation by creating a controlled instance for testing transfer logic without database connections.
361-496: LGTM!These tests comprehensively validate the transfer logic, including view handling, data transfer invocation, and proper identifier escaping for SQLite names containing quotes.
Note: The unused arguments flagged by static analysis in the mock side_effect functions (lines 364, 382, 400, 420) are intentional test helpers that match expected signatures.
1200-1255: LGTM!These tests thoroughly validate end-to-end SQLite view definition translation, including datetime functions, strftime format conversion, UTC/localtime modifiers, and identifier length truncation.
1256-1285: LGTM!This test correctly validates the chunk parameter conversion logic, ensuring proper normalization to
_chunk_size.Note: The unused
mockerargument flagged by static analysis may be required by the test framework fixture system.src/sqlite3_to_mysql/transporter.py (19)
16-68: LGTM!The sqlglot imports and time mapping constants are correctly configured for SQLite-to-MySQL view definition translation.
126-126: LGTM!The
sqlite_views_as_tablesflag is correctly initialized with proper default value.
138-140: LGTM!The chunk size normalization correctly handles invalid or non-positive values by setting
_chunk_sizetoNone.
293-296: LGTM!The
_sqlite_quote_identmethod correctly implements SQLite identifier escaping by doubling internal quotes, addressing SQL injection concerns from previous reviews.
298-305: LGTM!The method now correctly uses
_sqlite_quote_identto safely escape table names, addressing the SQL injection concerns.Note: The static analysis SQL injection warning (S608) is a false positive as the identifier is now properly escaped.
555-562: LGTM!The
_translate_strftime_formatmethod correctly leverages sqlglot's format translation capabilities to convert SQLite strftime formats to MySQL equivalents.
564-602: LGTM!The
_fetch_sqlite_master_rowsmethod correctly implements parameterised queries with proper filtering logic for object types and table inclusion/exclusion.Note: The static analysis SQL injection warning (S608) is a false positive as all dynamic values are passed via parameterised query placeholders.
604-619: LGTM!The
_translate_sqlite_view_definitionmethod correctly implements view definition translation using sqlglot's AST parsing and rendering capabilities with proper error handling.Note: The static analysis warnings about long exception messages (TRY003) can be safely ignored as detailed error messages aid debugging.
621-689: LGTM!The
_rewrite_sqlite_view_functionsmethod comprehensively handles SQLite datetime function translation, including proper support for UTC/localtime modifiers and strftime format conversion.
691-710: LGTM!The
_create_mysql_viewmethod correctly handles view creation with proper cleanup of existing objects and appropriate error handling for known MySQL error codes.
720-726: LGTM!The
_create_tablemethod now correctly uses_sqlite_quote_identto safely escape table names in PRAGMA queries.
823-840: LGTM!The
_add_indicesmethod now correctly uses_sqlite_quote_identto safely escape table and index names in PRAGMA queries.
1023-1024: LGTM!The
_add_foreign_keysmethod now correctly uses_sqlite_quote_identto safely escape table names in PRAGMA queries.
1098-1107: LGTM!The table/view fetching logic correctly determines whether to treat views as tables or handle them separately based on the
_sqlite_views_as_tablesflag.
1112-1201: LGTM!The main transfer loop correctly handles both tables and views with proper identifier quoting, appropriate logging, and comprehensive error handling.
Note: The static analysis SQL injection warnings (S608) are false positives as
quoted_table_nameis properly escaped via_sqlite_quote_ident.
1202-1228: LGTM!The view creation loop correctly translates and creates MySQL views with proper error handling and logging.
Note: The static analysis suggestions to use
logging.exception(lines 1216-1220, 1223-1227) would provide better stack traces but the current implementation is acceptable.
459-461: LGTM!The TIMESTAMP default translation correctly detects UTC modifiers and maps to
UTC_TIMESTAMPwhen appropriate.
480-482: LGTM!The DATETIME default translation correctly detects UTC modifiers and maps to
UTC_TIMESTAMPwhen appropriate.
519-519: LGTM!The TIME default translation correctly detects UTC modifiers and maps to
UTC_TIMEwhen appropriate.
This pull request adds support for materializing SQLite views as tables in MySQL or creating matching MySQL views, based on a new CLI option. It also introduces the
sqlglotlibrary to handle SQL translation and refactors the transfer logic to accommodate these changes. Dependency files and type definitions are updated accordingly.Feature: SQLite Views Transfer
--sqlite-views-as-tablesto allow users to choose whether to materialize SQLite views as tables in MySQL or create corresponding MySQL views. [1] [2] [3]transporter.pyto fetch, translate, and transfer SQLite views either as tables or as MySQL views, using the new option. [1] [2]SQL Translation and Dependencies
sqlglotlibrary for robust SQL parsing and translation from SQLite to MySQL, including custom translation for SQLite-specific functions in views. [1] [2] [3] [4]Type Definitions
types.pyto support the newsqlite_views_as_tablesparameter and attribute. [1] [2]