🐛 fix typo in --mysql-insert-method parameter #130
Conversation
WalkthroughThe changes introduce a new function, Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
techouse
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #130 +/- ##
==========================================
+ Coverage 88.65% 89.15% +0.49%
==========================================
Files 8 8
Lines 670 664 -6
==========================================
- Hits 594 592 -2
+ Misses 76 72 -4 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/sqlite3_to_mysql/mysql_utils.py (2 hunks)
- src/sqlite3_to_mysql/transporter.py (3 hunks)
🧰 Additional context used
🪛 Ruff
src/sqlite3_to_mysql/mysql_utils.py
138-140: Return the condition
mysql_version.micro >= 19directlyReplace with
return mysql_version.micro >= 19(SIM103)
🪛 GitHub Check: codecov/patch
src/sqlite3_to_mysql/mysql_utils.py
[warning] 134-140: src/sqlite3_to_mysql/mysql_utils.py#L134-L140
Added lines #L134 - L140 were not covered by testssrc/sqlite3_to_mysql/transporter.py
[warning] 729-729: src/sqlite3_to_mysql/transporter.py#L729
Added line #L729 was not covered by tests
🔇 Additional comments (3)
src/sqlite3_to_mysql/mysql_utils.py (2)
134-140: Add test coverage for version checking logicThe version checking branches lack test coverage.
Let's verify which test cases are missing:
#!/bin/bash # Description: Check existing test coverage for version checks echo "Checking test coverage for MySQL version checks..." rg -l "check_mysql_values_alias_support" tests/ rg "def test.*check_mysql_values_alias_support" tests/Would you like me to help generate comprehensive test cases covering all version scenarios?
🧰 Tools
🪛 Ruff
138-140: Return the condition
mysql_version.micro >= 19directlyReplace with
return mysql_version.micro >= 19(SIM103)
🪛 GitHub Check: codecov/patch
[warning] 134-140: src/sqlite3_to_mysql/mysql_utils.py#L134-L140
Added lines #L134 - L140 were not covered by tests
116-116: Improved MariaDB detection looks good!The change from checking a specific suffix to checking for "mariadb" anywhere in the version string makes the detection more robust.
Let's verify this handles various MariaDB version strings correctly:
✅ Verification successful
The MariaDB version detection change is correct and well-implemented
The change to check for "mariadb" anywhere in the version string is a good improvement because:
- It correctly handles various MariaDB version string formats
- The version check logic remains intact, requiring MariaDB 10.2.7 or later for JSON support
- The implementation is consistent with other version checks in the codebase (e.g.,
check_mysql_values_alias_supportandcheck_mysql_fulltext_support)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the MariaDB version string detection handles various formats # Test cases with different MariaDB version string formats echo "Testing MariaDB version detection..." python3 -c " from sqlite3_to_mysql.mysql_utils import check_mysql_json_support test_versions = [ '10.5.8-MariaDB', '10.5.8-mariadb-1:10.5.8+maria~focal', '5.5.5-10.5.8-MariaDB-1:10.5.8+maria~focal' ] for v in test_versions: print(f'{v}: {check_mysql_json_support(v)}') "Length of output: 1104
Script:
#!/bin/bash # Let's first check the implementation of check_mysql_json_support echo "Checking implementation of check_mysql_json_support..." rg -A 10 "def check_mysql_json_support" src/ # Then check if there are any tests for this function echo -e "\nChecking tests for MariaDB version detection..." rg "def test.*check_mysql_json_support" tests/ # Also look for test cases with MariaDB version strings echo -e "\nLooking for MariaDB version string test cases..." rg "MariaDB.*version" tests/Length of output: 1352
Script:
#!/bin/bash # Let's check the test file contents for version checks echo "Checking test files for version checks..." fd -e py -e test . tests/ -x cat {} # Also check if there are any version-related test fixtures echo -e "\nChecking test fixtures..." fd fixtures -e py -e json -e yaml tests/ -x cat {}Length of output: 91808
Script:
#!/bin/bash # Let's check the actual implementation of check_mysql_json_support echo "Checking check_mysql_json_support implementation..." cat src/sqlite3_to_mysql/mysql_utils.pyLength of output: 4386
src/sqlite3_to_mysql/transporter.py (1)
757-757:⚠️ Potential issueRemove debugging
The
print(sql)statement at line 757 appears to be used for debugging. Leaving print statements can clutter the console output and potentially expose sensitive information.Apply this diff to remove the unwanted print statement:
- print(sql)Likely invalid or redundant comment.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/sqlite3_to_mysql/transporter.py (1)
726-745: MariaDB compatibility fix looks good!The code correctly handles the VALUES alias syntax based on MySQL version compatibility.
Consider refactoring SQL query construction
The SQL query construction logic could be more maintainable. Consider extracting the common parts into helper methods.
Here's a suggested refactor:
def _build_fields_clause(self, columns): return ("`{}`, " * len(columns)).rstrip(" ,").format(*columns) def _build_values_clause(self, columns, use_alias): placeholders = ("%s, " * len(columns)).rstrip(" ,") if use_alias: return f"VALUES ({placeholders}) AS `__new__`" return f"VALUES ({placeholders})" def _build_updates_clause(self, columns, use_alias): if use_alias: return ("`{}`=`__new__`.`{}`, " * len(columns)).rstrip(" ,") return ("`{}`=`{}`, " * len(columns)).rstrip(" ,") def _build_insert_sql(self, table_name, columns): use_alias = ( self._mysql_insert_method.upper() == "UPDATE" and check_mysql_values_alias_support(self._mysql_version) ) if self._mysql_insert_method.upper() == "UPDATE": return """ INSERT INTO `{table}` ({fields}) {values_clause} ON DUPLICATE KEY UPDATE {field_updates} """.format( table=safe_identifier_length(table_name), fields=self._build_fields_clause(columns), values_clause=self._build_values_clause(columns, use_alias), field_updates=self._build_updates_clause( columns, use_alias ).format(*list(chain.from_iterable((column, column) for column in columns))), ) return """ INSERT {ignore} INTO `{table}` ({fields}) VALUES ({placeholders}) """.format( ignore="IGNORE" if self._mysql_insert_method.upper() == "IGNORE" else "", table=safe_identifier_length(table_name), fields=self._build_fields_clause(columns), placeholders=("%s, " * len(columns)).rstrip(" ,"), )Then use it in the
transfermethod:- sql: str - if self._mysql_insert_method.upper() == "UPDATE": - sql = """ - INSERT - INTO `{table}` ({fields}) - {values_clause} - ON DUPLICATE KEY UPDATE {field_updates} - """.format( - table=safe_identifier_length(table["name"]), - fields=("`{}`, " * len(columns)).rstrip(" ,").format(*columns), - values_clause=( - "VALUES ({placeholders}) AS `__new__`" - if check_mysql_values_alias_support(self._mysql_version) - else "VALUES ({placeholders})" - ).format(placeholders=("%s, " * len(columns)).rstrip(" ,")), - field_updates=( - ("`{}`=`__new__`.`{}`, " * len(columns)).rstrip(" ,") - if check_mysql_values_alias_support(self._mysql_version) - else ("`{}`=`{}`, " * len(columns)).rstrip(" ,") - ).format(*list(chain.from_iterable((column, column) for column in columns))), - ) - else: - sql = """ - INSERT {ignore} - INTO `{table}` ({fields}) - VALUES ({placeholders}) - """.format( - ignore="IGNORE" if self._mysql_insert_method.upper() == "IGNORE" else "", - table=safe_identifier_length(table["name"]), - fields=("`{}`, " * len(columns)).rstrip(" ,").format(*columns), - placeholders=("%s, " * len(columns)).rstrip(" ,"), - ) + sql = self._build_insert_sql(table["name"], columns)Benefits:
- Improved readability and maintainability
- Easier to test individual components
- Reduced code duplication
- Better separation of concerns
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/sqlite3_to_mysql/mysql_utils.py (2 hunks)
- src/sqlite3_to_mysql/transporter.py (3 hunks)
🔇 Additional comments (3)
src/sqlite3_to_mysql/mysql_utils.py (1)
116-116: Improved MariaDB version detectionThe modification to use a case-insensitive substring search for "mariadb" is more robust than the previous exact suffix match. This change correctly handles various MariaDB version string formats across different distributions.
src/sqlite3_to_mysql/transporter.py (2)
42-42: LGTM!The import is correctly placed with other related MySQL utility functions.
92-94: LGTM!The parameter handling is robust with:
- Proper default value ("IGNORE")
- Case normalisation using
upper()- Validation against
MYSQL_INSERT_METHODconstant
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
tests/unit/mysql_utils_test.py (3)
11-24: Consider adding docstring to explain version boundaries.The test cases effectively cover the version boundaries for both MySQL and MariaDB, but adding a docstring would help explain why 5.7.8 and 10.2.7 are the cutoff versions for JSON support.
@pytest.mark.parametrize( "version_string,expected", [ ("5.7.7", False), ("5.7.8", True), ("8.0.0", True), ("9.0.0", True), ("10.2.6-mariadb", False), ("10.2.7-mariadb", True), ("11.4.0-mariadb", True), ], ) def test_check_mysql_json_support(self, version_string: str, expected: bool) -> None: + """Test JSON support detection across MySQL/MariaDB versions. + + MySQL added JSON support in version 5.7.8 + MariaDB added JSON support in version 10.2.7 + """ assert check_mysql_json_support(version_string) == expected
26-40: Add docstring to document MariaDB limitation.The test cases correctly reflect that VALUES alias is only supported in MySQL ≥8.0.19 and never in MariaDB. Adding a docstring would help explain this limitation.
@pytest.mark.parametrize( "version_string,expected", [ ("5.7.8", False), ("8.0.0", False), ("8.0.18", False), ("8.0.19", True), ("9.0.0", True), ("10.2.6-mariadb", False), ("10.2.7-mariadb", False), ("11.4.0-mariadb", False), ], ) def test_check_mysql_values_alias_support(self, version_string: str, expected: bool) -> None: + """Test VALUES alias support detection across MySQL/MariaDB versions. + + MySQL added VALUES alias support in version 8.0.19 + MariaDB does not support VALUES alias in any version + """ assert check_mysql_values_alias_support(version_string) == expected
42-56: Add docstring to document version requirements.The test cases effectively cover FULLTEXT support boundaries for both MySQL and MariaDB. Adding a docstring would help explain the version requirements.
@pytest.mark.parametrize( "version_string,expected", [ ("5.0.0", False), ("5.5.0", False), ("5.6.0", True), ("8.0.0", True), ("10.0.4-mariadb", False), ("10.0.5-mariadb", True), ("10.2.6-mariadb", True), ("11.4.0-mariadb", True), ], ) def test_check_mysql_fulltext_support(self, version_string: str, expected: bool) -> None: + """Test FULLTEXT support detection across MySQL/MariaDB versions. + + MySQL added FULLTEXT support in version 5.6.0 + MariaDB added FULLTEXT support in version 10.0.5 + """ assert check_mysql_fulltext_support(version_string) == expected
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/sqlite3_to_mysql/mysql_utils.py (2 hunks)
- tests/func/sqlite3_to_mysql_test.py (1 hunks)
- tests/unit/mysql_utils_test.py (1 hunks)
🔇 Additional comments (6)
tests/unit/mysql_utils_test.py (1)
1-10: LGTM! Clean and well-structured imports.The imports are explicit and well-organised, following Python best practices.
src/sqlite3_to_mysql/mysql_utils.py (2)
129-144: Function implementation is correctThe version check logic accurately identifies MySQL 8.0.19+ for VALUES alias support whilst correctly handling MariaDB versions.
116-120: Improved MariaDB version detection looks goodThe change from suffix-based to substring-based MariaDB detection is more robust and handles various MariaDB version string formats correctly.
Let's verify the version string handling with different formats:
✅ Verification successful
MariaDB version detection is well-tested and correctly implemented
The test suite demonstrates comprehensive coverage of MariaDB version string handling with various formats, including:
- Standard MariaDB format (e.g., "10.2.7-mariadb")
- Different major versions (10.x and 11.x)
- Version boundary cases for JSON support:
- "10.2.6-mariadb" (unsupported)
- "10.2.7-mariadb" (supported)
- "11.4.0-mariadb" (supported)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for version string handling test cases # Look for test coverage of different MariaDB version formats rg -A 3 "mariadb.*version|version.*mariadb" tests/Length of output: 52
Script:
#!/bin/bash # Let's check for test files related to MySQL version checks fd -e py "test.*mysql" tests/ # Then look for any version-related test cases rg -l "test.*version|version.*test" tests/ # Finally, let's examine the actual test implementations ast-grep --pattern 'def test_$_version$_($_) { $$$ }'Length of output: 186
Script:
#!/bin/bash # Let's examine the test cases in mysql_utils_test.py cat tests/unit/mysql_utils_test.pyLength of output: 1768
tests/func/sqlite3_to_mysql_test.py (3)
62-62: LGTM! Return type annotation improves type safety.The addition of
-> Nonereturn type annotation follows Python type hinting best practices and improves code clarity.
Line range hint
447-448: Well-implemented version-specific SQL syntax handling.The conditional JOIN syntax based on MySQL version (8.0.19) ensures compatibility across different MySQL and MariaDB versions. This is particularly important for the foreign key information retrieval.
Line range hint
324-349: Consider adding MariaDB-specific test parameters.While the test coverage is comprehensive for MySQL scenarios, consider adding specific test parameters or markers for MariaDB compatibility testing, especially given the PR's focus on MariaDB compatibility issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/unit/mysql_utils_test.py (2)
14-28: Consider adding edge cases for version parsingWhilst the current test cases cover valid version strings well, consider adding tests for:
- Malformed version strings (e.g., "8.0", "not-a-version")
- Empty strings
- None values
Example additions:
@pytest.mark.parametrize( "version_string,expected", [ # ... existing cases ... pytest.param("8.0", pytest.raises(ValueError), id="incomplete_version"), pytest.param("not-a-version", pytest.raises(ValueError), id="invalid_version"), pytest.param("", pytest.raises(ValueError), id="empty_string"), pytest.param(None, pytest.raises(TypeError), id="none_value"), ], )
76-87: Consider additional identifier length test casesWhilst the current test cases cover length boundaries well, consider adding tests for:
- Unicode characters (which might have different byte lengths)
- Special characters in identifiers
- Empty strings
Example additions:
@pytest.mark.parametrize( "identifier,expected", [ # ... existing cases ... ("🐰" * 64, "🐰" * 32), # Unicode characters ("test@#$%^&*", "test@#$%^&*"), # Special characters ("", ""), # Empty string ], )src/sqlite3_to_mysql/mysql_utils.py (1)
114-139: Overall changes improve MariaDB compatibilityThe modifications across all version check functions create a more robust and consistent approach to handling MariaDB versions. This effectively addresses the compatibility issues mentioned in the PR objectives.
Consider adding integration tests that verify the behaviour with actual MariaDB instances, particularly focusing on the VALUES alias support detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/sqlite3_to_mysql/mysql_utils.py (1 hunks)
- tests/unit/mysql_utils_test.py (1 hunks)
🔇 Additional comments (7)
tests/unit/mysql_utils_test.py (4)
1-13: Well-structured imports and class organisationThe imports are properly organised, with explicit imports and clear type hints. The test class structure follows pytest best practices.
29-43: Comprehensive JSON support test coverageThe test cases effectively cover the version boundaries for JSON support in both MySQL and MariaDB. The parameterised test data is well-organised and the assertions are clear.
44-59: Well-aligned test cases for VALUES alias supportThe test cases effectively validate the PR's main objective regarding MariaDB compatibility. The version boundary at MySQL 8.0.19 is properly tested, and the distinction between MySQL and MariaDB support is clearly demonstrated.
60-75: Thorough fulltext support test coverageThe test cases effectively cover the version boundaries for fulltext support in both MySQL and MariaDB. The test data is well-structured and provides good coverage of the feature's availability across different versions.
src/sqlite3_to_mysql/mysql_utils.py (3)
114-118: Improved MariaDB detection looks good!The change from suffix-based to substring-based MariaDB detection is more robust, and the version comparison logic is now more concise.
136-139: Consistent MariaDB detection approach looks good!The changes align well with the other version check functions, making the codebase more consistent and maintainable.
120-131: Add test coverage for MariaDB edge casesWhilst the implementation looks correct, we should ensure comprehensive test coverage for MariaDB-specific scenarios:
- MariaDB versions that might include "mariadb" in different cases (e.g., "MariaDB", "MARIADB")
- MariaDB versions with different version number formats
Let's verify the existing test coverage:
Pull Request Template
Description
While reviewing #127 I noticed that MariaDB does not support aliasing
VALUESthat's why the tests were failing with MariaDB but worked with MySQL. Additionally, MySQL <8.0.19 also does not support this feature, so I have added an additional condition to work around this issue.Supersedes #127 originally authored by @mackuba
Type of change
How Has This Been Tested?
The test suite passes with both MySQL and MariaDB.
Checklist: