✨ added a --mysql-socket option to connect to MySQL using a Unix socket #129
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduced in this pull request add a new command-line option, Changes
Possibly related PRs
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 (
|
![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
🧹 Outside diff range and nitpick comments (4)
docs/README.rst (2)
32-32: Documentation needs more details about the socket option.Whilst the basic documentation is correct, consider enhancing it with:
- A brief description of when to use socket connections
- The default socket path if any
- Example usage in the command-line examples section
Apply this diff to improve the documentation:
-- ``--mysql-socket TEXT``: Path to MySQL unix socket file. +- ``--mysql-socket TEXT``: Path to MySQL Unix socket file (e.g., '/var/run/mysqld/mysqld.sock'). Use this instead of host/port for local connections with better performance. Common paths include '/tmp/mysql.sock' on macOS and '/var/run/mysqld/mysqld.sock' on Linux.
32-32: Update Docker documentation for socket connections.The Docker section should explain how to use Unix sockets, as it requires additional volume mounting.
Add this section after the existing Docker example:
+Socket Connections in Docker +''''''''''''''''''''''''''' + +To use MySQL socket connections with Docker, you need to mount the socket file from the host: + +.. code:: bash + + docker run -it \ + --workdir $(pwd) \ + --volume $(pwd):$(pwd) \ + --volume /var/run/mysqld/mysqld.sock:/var/run/mysqld/mysqld.sock \ + --rm ghcr.io/techouse/sqlite3-to-mysql:latest \ + --sqlite-file baz.db \ + --mysql-user foo \ + --mysql-password bar \ + --mysql-database baz \ + --mysql-socket /var/run/mysqld/mysqld.sock + +Note: The socket path may vary depending on your MySQL installation and operating system.README.md (1)
54-54: Documentation could be more comprehensiveThe addition of the
--mysql-socketoption is well-placed and formatted consistently. However, consider enhancing the documentation with:
- Default behaviour (if any)
- Platform-specific information (Unix/Linux systems only)
- Example usage in the command-line examples section
Apply this diff to enhance the documentation:
- --mysql-socket TEXT Path to MySQL unix socket file. + --mysql-socket TEXT Path to MySQL Unix socket file. Only available + on Unix-like systems. Takes precedence over + host/port if specified.Consider adding this example to the usage section:
# Connect using Unix socket sqlite3mysql -f database.db -d mydb -u myuser --mysql-socket /var/run/mysqld/mysqld.socksrc/sqlite3_to_mysql/cli.py (1)
65-65: Enhance the --mysql-socket option documentation and validationThe help text could be more descriptive. Consider expanding it to clarify:
- This is specifically for Unix socket connections
- Common socket path locations
- That this is an alternative to host/port connection
Consider updating the help text:
-@click.option("--mysql-socket", default=None, help="Path to MySQL unix socket file.") +@click.option("--mysql-socket", default=None, help="Path to MySQL Unix socket file (e.g., /var/run/mysqld/mysqld.sock). When specified, takes precedence over host/port.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- README.md (1 hunks)
- docs/README.rst (1 hunks)
- src/sqlite3_to_mysql/cli.py (3 hunks)
- src/sqlite3_to_mysql/transporter.py (2 hunks)
- src/sqlite3_to_mysql/types.py (2 hunks)
🔇 Additional comments (4)
src/sqlite3_to_mysql/types.py (2)
23-23: LGTM: Proper type annotation for MySQL socket parameterThe
mysql_socketparameter is correctly typed asOptional[str]and well-positioned amongst other connection-related parameters.
23-23: Verify socket parameter handling in connection logicLet's ensure the socket parameter is properly handled in the connection establishment code.
Also applies to: 53-53
✅ Verification successful
Based on the search results, I can see that the MySQL socket parameter (
unix_socket) is properly handled in the connection logic withinsrc/sqlite3_to_mysql/transporter.py. Specifically, it's used in the MySQL connection setup:_mysql_connection = mysql.connector.connect( user=self._mysql_user, password=self._mysql_password, host=self._mysql_host, port=self._mysql_port, unix_socket=self._mysql_socket, # Socket parameter is properly used here ssl_disabled=self._mysql_ssl_disabled, use_pure=True, charset=self._mysql_charset, collation=self._mysql_collation, )Socket parameter is properly handled in MySQL connection logic
The
mysql_socketparameter from the configuration is correctly passed through to the MySQL connector's connection setup asunix_socket.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how the socket parameter is used in connection logic # Search for MySQL connection setup code rg -A 10 "MySQLConnection|connect" src/Length of output: 16993
src/sqlite3_to_mysql/cli.py (1)
188-188: Add validation for MySQL socket configurationConsider adding validation to handle these scenarios:
- Verify socket file existence before attempting connection
- Clarify behaviour when both socket and host/port are specified
- Add warning logs for unused connection parameters
Let's check if similar validation exists in other files:
Consider adding validation before instantiation:
if mysql_socket and not os.path.exists(mysql_socket): raise click.ClickException(f"MySQL socket file not found: {mysql_socket}") if mysql_socket and (mysql_host != "localhost" or mysql_port != 3306): click.echo("Warning: MySQL socket connection will ignore host/port settings")src/sqlite3_to_mysql/transporter.py (1)
77-78: LGTM! Clean implementation of the socket path handling.The implementation properly handles type conversion and fallback to None, following the class's naming conventions.
| @@ -49,6 +50,7 @@ class SQLite3toMySQLAttributes: | |||
| _mysql_password: t.Optional[str] | |||
| _mysql_host: str | |||
| _mysql_port: int | |||
| _mysql_socket: str | |||
There was a problem hiding this comment.
Type inconsistency between parameter and attribute definitions
The mysql_socket is defined as Optional[str] in SQLite3toMySQLParams but as a required str in SQLite3toMySQLAttributes. This could lead to runtime errors if the socket parameter isn't provided.
Apply this change to maintain consistency:
- _mysql_socket: str
+ _mysql_socket: t.Optional[str]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _mysql_socket: str | |
| _mysql_socket: t.Optional[str] |
| @@ -137,6 +138,7 @@ def cli( | |||
| mysql_database: str, | |||
| mysql_host: str, | |||
| mysql_port: int, | |||
| mysql_socket: str, | |||
There was a problem hiding this comment.
Update type annotation for mysql_socket parameter
The parameter should be marked as optional since it has a None default value.
- mysql_socket: str,
+ mysql_socket: t.Optional[str],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mysql_socket: str, | |
| mysql_socket: t.Optional[str], |
| @@ -143,6 +145,7 @@ def __init__(self, **kwargs: tx.Unpack[SQLite3toMySQLParams]): | |||
| password=self._mysql_password, | |||
| host=self._mysql_host, | |||
| port=self._mysql_port, | |||
| unix_socket=self._mysql_socket, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding socket path validation.
While the implementation is correct, consider validating the socket path's existence before attempting the connection to provide a more user-friendly error message.
+ unix_socket=(
+ self._mysql_socket
+ if self._mysql_socket and os.path.exists(self._mysql_socket)
+ else None
+ ),
- unix_socket=self._mysql_socket,This change would help users identify socket path issues before MySQL Connector attempts the connection.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unix_socket=self._mysql_socket, | |
| unix_socket=( | |
| self._mysql_socket | |
| if self._mysql_socket and os.path.exists(self._mysql_socket) | |
| else None | |
| ), |
|
Good idea 😇 Could you please look at the comments CodeRabbit made? They seem quite valid. Also, since this is a new feature, could you please add some tests? |
techouse
changed the title
Description
This adds a
--mysql-socketoption to the CLI which passes aunix_socketparameter to the MySQL Connector, which lets you connect using a Unix socket instead of a TCP port.Type of change
How Has This Been Tested?
Tested with my local database with disabled networking (MacPorts default), with and without the option
--mysql-socket /opt/local/var/run/mysql8/mysqld.sock.Checklist: