🐛 prevent non-unique indices from being quietly omitted #109
Conversation
WalkthroughThis pull request implements automatic suffixing of duplicate SQLite index names when prefixing is disabled. The solution introduces internal state tracking to detect collisions and append numeric suffixes (_2, _3, etc.) to ensure global index name uniqueness across the database. Changes
Sequence Diagram(s)sequenceDiagram
participant Table Table Processing
participant _build_create_table_sql
participant _get_unique_index_name
participant State Storage
Table->>_build_create_table_sql: Process indexes
_build_create_table_sql->>_build_create_table_sql: Compute proposed_index_name
alt Prefixing enabled
_build_create_table_sql->>_build_create_table_sql: Use proposed_index_name directly
else Prefixing disabled
_build_create_table_sql->>_get_unique_index_name: Call with proposed_index_name
_get_unique_index_name->>State Storage: Check _seen_sqlite_index_names
alt Index name unseen
State Storage-->>_get_unique_index_name: Not found
_get_unique_index_name->>State Storage: Record base_name
_get_unique_index_name-->>_build_create_table_sql: Return base_name
else Index name collision
State Storage-->>_get_unique_index_name: Found in seen set
_get_unique_index_name->>State Storage: Increment counter
_get_unique_index_name->>_get_unique_index_name: Append suffix (_2, _3, ...)
_get_unique_index_name->>State Storage: Record unique name
_get_unique_index_name-->>_build_create_table_sql: Return suffixed_name (log rename)
end
end
_build_create_table_sql->>_build_create_table_sql: Use final index name in CREATE INDEX
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes implement a straightforward suffix-based collision resolution pattern. The logic is easy to follow, and the scope is well-contained within the transporter module. However, careful attention is needed to verify the state initialisation order, the correctness of the collision detection logic, and that the test coverage adequately exercises edge cases. Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mysql_to_sqlite3/transporter.py (1)
417-437: Consider logging when index names are modified.The implementation correctly generates unique index names using numeric suffixes. However, issue #108 suggested providing notice to users when index names are modified. Consider adding a log message (e.g.,
self._logger.info) when a suffix is appended, informing the user that indexbase_namewas renamed tocandidateto avoid conflicts.Example:
# Record chosen candidate and bump counter for the base name self._seen_sqlite_index_names.add(candidate) self._sqlite_index_name_counters[base_name] = next_num + 1 + self._logger.info( + 'Index "%s" renamed to "%s" to ensure uniqueness across the SQLite database.', + base_name, + candidate, + ) return candidate
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mysql_to_sqlite3/transporter.py(3 hunks)src/mysql_to_sqlite3/types.py(1 hunks)tests/unit/mysql_to_sqlite3_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/mysql_to_sqlite3_test.py (1)
src/mysql_to_sqlite3/transporter.py (1)
_get_unique_index_name(417-437)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
src/mysql_to_sqlite3/types.py (1)
84-86: LGTM! Clear documentation of the new tracking attributes.The comment clearly explains the purpose of these fields, and the type annotations are correct.
tests/unit/mysql_to_sqlite3_test.py (1)
584-605: LGTM! Comprehensive test of the suffixing sequence.The test correctly validates the uniqueness logic: first occurrence returns the base name, subsequent occurrences append numeric suffixes starting from 2, and different base names are tracked independently.
src/mysql_to_sqlite3/transporter.py (2)
149-152: LGTM! Proper initialisation of tracking state.The tracking attributes are correctly initialised as empty collections, and the comments clearly document their purpose.
553-569: LGTM! Integration correctly applies uniqueness logic when prefixing is disabled.The logic appropriately handles both table name collisions and the prefix option, then ensures global uniqueness by calling
_get_unique_index_namewhen prefixing is disabled. The implementation matches the PR objectives.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #109 +/- ##
==========================================
- Coverage 94.23% 94.11% -0.12%
==========================================
Files 8 8
Lines 659 680 +21
==========================================
+ Hits 621 640 +19
- Misses 38 40 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tox.ini (1)
115-115: Consider inline pylint disables for more targeted suppression.Whilst adding C0103 to the global disable list works, it affects the entire codebase rather than just the specific attributes that need it (
_seen_sqlite_index_names,_sqlite_index_name_counters). For more granular control, you could use inline# pylint: disable=invalid-namecomments next to those specific attribute definitions instead.tests/unit/test_transporter.py (1)
47-66: Excellent test coverage for the suffixing logic.The test correctly verifies that:
- First occurrence returns the base name without suffix
- Subsequent occurrences append numeric suffixes (_2, _3, ...)
- Different base names are tracked independently
Optionally, you could enhance the test by asserting that
instance._logger.infowas called with the expected rename messages for the suffixed cases (lines 59, 61, 66), which would verify the logging behaviour as well.src/mysql_to_sqlite3/transporter.py (1)
558-574: Correct implementation of index name uniqueness when prefixing is disabled.The logic correctly:
- Computes the proposed index name based on table name collisions and the prefix option
- Applies global uniqueness checking via
_get_unique_index_nameonly when prefixing is disabled- Uses the resulting unique name in the CREATE INDEX statement
This directly addresses the stated issue of indexes being silently omitted due to name collisions when the
-Koption is not used.Note: When
prefix_indices=True, no uniqueness check is applied. Whilst table prefixing typically ensures uniqueness, edge cases could theoretically still produce collisions (e.g., tableuserswith indexidx_nameand tableusers_idxwith indexnameboth becomeusers_idx_name). However, this is the existing behaviour and outside the scope of this PR, which specifically addresses the non-prefixed case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mysql_to_sqlite3/transporter.py(3 hunks)tests/unit/test_transporter.py(1 hunks)tox.ini(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_transporter.py (1)
src/mysql_to_sqlite3/transporter.py (2)
MySQLtoSQLite(42-820)_get_unique_index_name(417-442)
⏰ 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.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:10.0, 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.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 (2)
src/mysql_to_sqlite3/transporter.py (2)
149-152: LGTM! Proper initialization of tracking state.The tracking attributes are correctly initialised early in the constructor, ensuring they're ready before index generation occurs. The type hints clearly indicate their purpose.
417-442: Well-implemented uniqueness helper.The method correctly:
- Returns the base name on first use and records it
- Generates sequential suffixes (_2, _3, ...) for subsequent collisions
- Uses the counter dictionary to efficiently find the next available suffix
- Logs renames to inform users when disambiguation occurs
The while loop guards against potential gaps in the sequence, ensuring robustness.
This pull request improves the handling of SQLite index name collisions when migrating from MySQL, ensuring that index names are globally unique in the target SQLite database, especially when index prefixing is disabled. It introduces a helper method for generating unique index names, updates internal tracking attributes, and adds a unit test for the new logic.
Index name uniqueness improvements:
_seen_sqlite_index_namesand_sqlite_index_name_countersto bothMySQLtoSQLiteAttributesand the main transporter class to record used index names and manage numeric suffixes. [1] [2]_get_unique_index_namemethod intransporter.pyto generate globally unique SQLite index names by appending numeric suffixes when needed._build_create_table_sqlto use the new uniqueness helper, preventing index name collisions when prefixing is disabled.Testing:
test_get_unique_index_name_suffixing_sequenceto verify correct suffixing and uniqueness behavior for index names.Lint configuration:
C0103naming style warning intox.inito accommodate the new internal attribute naming.Fixes #108