Fix: Support unix socket connection to Redis#66
Fix: Support unix socket connection to Redis#66sachinjogi wants to merge 6 commits intoFalkorDB:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughFalkorDB.from_url now propagates a Unix domain socket path into the Redis connection kwargs by assigning Changes
sequenceDiagram
autonumber
participant Client
participant FalkorDB as FalkorDB.from_url
participant RedisLib as redis.from_url
participant RedisSock as UnixSocket/Redis
Client->>FalkorDB: call from_url("unix:///sockets/redis0.sock", ...)
FalkorDB->>FalkorDB: parse URL -> connection_kwargs (path="/sockets/redis0.sock")
FalkorDB->>FalkorDB: set kwargs["unix_socket_path"] = connection_kwargs.get("path")
FalkorDB->>RedisLib: call redis.from_url(..., unix_socket_path="/sockets/redis0.sock")
RedisLib->>RedisSock: open unix socket "/sockets/redis0.sock"
RedisSock-->>RedisLib: accept connection
RedisLib-->>FalkorDB: connection established
FalkorDB-->>Client: return client instance
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- falkordb/falkordb.py (1 hunks)
Additional comments not posted (2)
falkordb/falkordb.py (2)
178-178: Summary: Unix socket support added successfully.The implementation of Unix socket support in the
from_urlmethod is correct and aligns with the PR objectives. This change enhances the flexibility of theFalkorDBclass by allowing connections via Unix sockets.Key points:
- The change correctly adds
unix_socket_pathto thekwargsdictionary.- It uses the appropriate key ("path") from
connection_kwargs.- The implementation is consistent with Redis's handling of Unix socket connections.
To ensure a robust implementation:
- Verify full Unix socket support throughout the class (see the verification script in a previous comment).
- Add specific unit tests for Unix socket connections.
- Update documentation to reflect the new functionality.
- Ensure no regressions for existing TCP connections.
Overall, this change is a valuable addition to the
FalkorDBclass, addressing the issue raised in #11 and improving compatibility for users migrating from RedisGraph to FalkorDB.
178-178: LGTM! Verify full Unix socket support.The addition of
unix_socket_pathtokwargsis correct and aligns with the PR objective of supporting Unix socket connections. This change should allow theFalkorDBclass to handle Unix socket URLs as requested in the linked issue.To ensure full Unix socket support, please run the following verification script:
This script will help ensure that the
unix_socket_pathis properly handled throughout theFalkorDBclass and related files.Verification successful
Verified! Unix socket support is correctly implemented.
The
unix_socket_pathis properly set and utilized within both theFalkorDBclass and its asynchronous counterpart, ensuring full support for Unix socket connections.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Unix socket support in FalkorDB class # Test 1: Check if unix_socket_path is properly passed to the FalkorDB constructor rg -A 10 'def __init__' falkordb/falkordb.py # Test 2: Verify that the unix_socket_path is used in establishing the connection rg -A 5 'unix_socket_path' falkordb/falkordb.py # Test 3: Check for any potential issues with Unix socket handling in other parts of the code rg 'unix[_-]socket' falkordb/Length of output: 1352
| kwargs["port"] = connection_kwargs.get("port", 6379) | ||
| kwargs["username"] = connection_kwargs.get("username") | ||
| kwargs["password"] = connection_kwargs.get("password") | ||
| kwargs["unix_socket_path"] = connection_kwargs.get("path") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggest additional tests and documentation updates.
While the change correctly adds Unix socket support to the from_url method, consider the following suggestions to ensure comprehensive implementation:
- Add unit tests specifically for Unix socket connections using the
from_urlmethod. - Update the class and method docstrings to mention Unix socket support.
- Consider adding an example of using a Unix socket connection in the class docstring.
Here's a suggested update for the class docstring:
class FalkorDB:
"""
FalkorDB Class for interacting with a FalkorDB server.
Supports both TCP and Unix socket connections.
Usage examples::
from falkordb import FalkorDB
# Connect to the database using TCP and select the 'social' graph
db = FalkorDB()
graph = db.select_graph("social")
# Connect using a Unix socket
db_unix = FalkorDB.from_url("unix:///path/to/redis.sock")
graph_unix = db_unix.select_graph("social")
# Get a single 'Person' node from the graph and print its name
result = graph.query("MATCH (n:Person) RETURN n LIMIT 1").result_set
person = result[0][0]
print(person.properties['name'])
"""Also, consider adding a unit test for Unix socket connections:
def test_from_url_unix_socket():
url = "unix:///path/to/redis.sock"
db = FalkorDB.from_url(url)
assert db.connection.connection_pool.connection_kwargs['unix_socket_path'] == "/path/to/redis.sock"
assert db.connection.connection_pool.connection_kwargs['host'] is None
assert db.connection.connection_pool.connection_kwargs['port'] is None
|
@gkorland can you please review this when you're free? |
swilly22
left a comment
swilly22
left a comment
There was a problem hiding this comment.
Please add documentation & tests
@copilot can you please help here? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
=======================================
Coverage 90.45% 90.45%
=======================================
Files 36 36
Lines 2776 2777 +1
=======================================
+ Hits 2511 2512 +1
Misses 265 265 ☔ 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 (1)
falkordb/falkordb.py (1)
173-173: Conditionally assign host/port when not using Unix socket connections.When
from_url()parses aunix://URL, it correctly extracts the socket path toconnection_kwargs["path"], but the current implementation unconditionally setshostandportto defaults (lines 169-170). This results in passing redundant host/port values toFalkorDB.__init__()when a Unix socket is being used.Since redis-py uses the
"path"key inconnection_kwargsto store the Unix socket path, you can conditionally set host/port only for TCP connections:connection_kwargs = conn.connection_pool.connection_kwargs connection_class = conn.connection_pool.connection_class -kwargs["host"] = connection_kwargs.get("host", "localhost") -kwargs["port"] = connection_kwargs.get("port", 6379) kwargs["username"] = connection_kwargs.get("username") kwargs["password"] = connection_kwargs.get("password") kwargs["unix_socket_path"] = connection_kwargs.get("path") + +# Only set host/port for non-Unix socket connections +if kwargs["unix_socket_path"] is None: + kwargs["host"] = connection_kwargs.get("host", "localhost") + kwargs["port"] = connection_kwargs.get("port", 6379)
There was a problem hiding this comment.
Pull request overview
This PR adds support for connecting to Redis via Unix domain sockets by correctly mapping the socket path from Redis connection parameters to FalkorDB's initialization. The fix addresses issue #11 in the falkordb-bulk-loader project.
Changes:
- Extracts unix socket path from Redis connection pool parameters and passes it to FalkorDB initialization
- Enables
from_url()method to supportunix://URLs in addition to existingfalkor://andfalkors://schemes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| kwargs["username"] = connection_kwargs.get("username") | ||
| kwargs["password"] = connection_kwargs.get("password") | ||
| kwargs["unix_socket_path"] = connection_kwargs.get("path") | ||
|
|
There was a problem hiding this comment.
Remove trailing whitespace from this line.
| kwargs["port"] = connection_kwargs.get("port", 6379) | ||
| kwargs["username"] = connection_kwargs.get("username") | ||
| kwargs["password"] = connection_kwargs.get("password") | ||
| kwargs["unix_socket_path"] = connection_kwargs.get("path") |
There was a problem hiding this comment.
Consider adding test coverage for unix socket connections. The existing tests in tests/test_db.py::test_connect_via_url only cover TCP connections (falkor:// scheme). Adding a test case for unix:// URLs would help ensure this feature works correctly and prevent regressions.
This should fix FalkorDB/falkordb-bulk-loader#11.
Summary by CodeRabbit