add support for configurable sqlite db path #93
Conversation
WalkthroughThe changes include the addition of a new environment variable Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SQLiteDB
participant Environment
App->>Environment: Get SQLITE_DB_PATH
Environment-->>App: Return path or default "director.db"
App->>SQLiteDB: Initialize with db_path
SQLiteDB-->>App: Database connection established
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: 2
🧹 Outside diff range and nitpick comments (1)
backend/.env.sample (1)
5-7: Add example value for SQLITE_DB_PATHWhile the new section and variable are well-organized, providing an example value would help users understand the expected format.
# Database -SQLITE_DB_PATH= +SQLITE_DB_PATH=director.db # Example: /path/to/your/database.db
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
backend/.env.sample(2 hunks)backend/director/db/sqlite/db.py(2 hunks)backend/director/db/sqlite/initialize.py(2 hunks)
🔇 Additional comments (3)
backend/.env.sample (1)
41-41: LGTM: Added newline at end of file
The addition of a newline at the end of file follows good Unix file format conventions.
backend/director/db/sqlite/db.py (1)
26-26: Review thread safety configuration
The check_same_thread=True parameter enforces SQLite connections to be used only in the thread they were created in. Ensure this aligns with your application's threading model.
✅ Verification successful
Let me check the content of these files to understand the threading usage.
Let me check how the database is being used in the application's main entry points to understand the threading model better.
Let me check the FastAPI implementation to understand how the database is used in the web application context.
Based on the gathered information, I can now provide a final response:
SQLite connection's thread safety setting is appropriate
The check_same_thread=True setting is correct for this application because:
- The database is initialized per-request through the
load_db()function in API routes - The concurrent operations in the codebase (in prompt_clip.py and meme_maker.py) use ThreadPoolExecutor for parallel API calls and video processing, but don't share the database connection
- Each HTTP request gets its own database connection, maintaining thread safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for threading-related code
rg -l 'threading|Thread|asyncio|concurrent' | grep -v 'test'
Length of output: 137
Script:
#!/bin/bash
# Check content of files for threading usage
echo "=== prompt_clip.py ==="
rg -A 5 'threading|Thread|asyncio|concurrent' backend/director/agents/prompt_clip.py
echo -e "\n=== meme_maker.py ==="
rg -A 5 'threading|Thread|asyncio|concurrent' backend/director/agents/meme_maker.py
# Also check if there's any database usage in these files
echo -e "\n=== Database usage in these files ==="
rg "sqlite|\.conn|\.execute|cursor" backend/director/agents/prompt_clip.py backend/director/agents/meme_maker.py
# Check for any other database access patterns in the codebase
echo -e "\n=== Database access patterns ==="
rg -A 3 "sqlite3\.connect|\.conn\." backend/director/db/
Length of output: 5271
Script:
#!/bin/bash
# Check for FastAPI/Flask/web framework usage
rg -l "FastAPI|Flask|web\.Application"
# Check main application files
echo -e "\n=== Main application setup ==="
fd "main\.py|app\.py" --exec rg -A 5 "SQLite|Database|db\."
# Check how database instance is created and used
echo -e "\n=== Database instance creation and usage ==="
rg -A 5 "SQLiteDatabase|Database" --type py
Length of output: 898
Script:
#!/bin/bash
# Check FastAPI implementation
echo "=== FastAPI implementation ==="
rg -A 10 "FastAPI" backend/director/entrypoint/api/
# Check database initialization in the API
echo -e "\n=== Database initialization in API ==="
rg -A 10 "db\s*=|database\s*=" backend/director/entrypoint/api/
# Check for any concurrent request handling
echo -e "\n=== Concurrent request handling ==="
rg -B 2 -A 5 "@app\.|@router\." backend/director/entrypoint/api/
Length of output: 7184
backend/director/db/sqlite/initialize.py (1)
Line range hint 44-44: Verify database initialization across the codebase
The changes to database path configuration might affect other parts of the codebase that initialize or access the database.
Also applies to: 61-62
✅ Verification successful
Let me check the actual database initialization and path usage patterns to verify the impact.
Let me check how the database is actually used in the application to understand the initialization flow better.
Database initialization changes are safely handled
The codebase consistently uses the SQLITE_DB_PATH environment variable as the primary configuration method for the database path, with "director.db" as the default fallback. The database initialization is properly encapsulated in the SQLite-specific modules:
backend/director/db/sqlite/initialize.pyhandles initialization using the environment variablebackend/director/db/sqlite/db.pyuses the same environment variable in theSQLiteDBclass- No direct database initialization calls found in the application flow (handler.py or session.py)
The changes maintain consistency with the existing pattern of database configuration and initialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for database initialization and access patterns
rg -l 'sqlite3\.connect|initialize_sqlite|SQLiteDB' | grep -v 'test'
# Search for hardcoded database paths
rg -l 'director\.db' | grep -v 'test'
Length of output: 528
Script:
#!/bin/bash
# Check database initialization implementation
ast-grep --pattern 'def initialize_sqlite($$$)' -f backend/director/db/sqlite/initialize.py
# Check database path configuration
rg -A 5 'director\.db' backend/director/db/sqlite/initialize.py
rg -A 5 'director\.db' backend/director/db/sqlite/db.py
# Check how the initialization is used
rg -A 3 'initialize_sqlite' backend/director/db/__init__.py
Length of output: 1214
Script:
#!/bin/bash
# Check how SQLiteDB is instantiated and used
rg -A 3 'SQLiteDB\('
# Check environment variable usage
rg -A 2 'SQLITE_DB_PATH'
# Check database initialization in main application flow
rg -A 5 'initialize_sqlite' backend/director/handler.py
rg -A 5 'initialize_sqlite' backend/director/core/session.py
Length of output: 826
| def __init__(self, db_path: str = None): | ||
| """ | ||
| :param db_path: Path to the SQLite database file. | ||
| """ | ||
| self.db_type = DBType.SQLITE | ||
| self.db_path = db_path | ||
| if db_path is None: | ||
| self.db_path = os.getenv("SQLITE_DB_PATH", "director.db") | ||
| else: | ||
| self.db_path = db_path |
There was a problem hiding this comment.
Add path validation for security
The database path from environment variable or parameter should be validated to prevent directory traversal attacks and ensure it's within allowed directories.
def __init__(self, db_path: str = None):
"""
:param db_path: Path to the SQLite database file.
"""
self.db_type = DBType.SQLITE
if db_path is None:
self.db_path = os.getenv("SQLITE_DB_PATH", "director.db")
else:
self.db_path = db_path
+ # Validate path is absolute and within allowed directory
+ self.db_path = os.path.abspath(self.db_path)
+ allowed_dir = os.path.abspath(os.getenv("SQLITE_DB_DIR", "."))
+ if not self.db_path.startswith(allowed_dir):
+ raise ValueError(f"Database path must be within {allowed_dir}")
self.conn = sqlite3.connect(self.db_path, check_same_thread=True)📝 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.
| def __init__(self, db_path: str = None): | |
| """ | |
| :param db_path: Path to the SQLite database file. | |
| """ | |
| self.db_type = DBType.SQLITE | |
| self.db_path = db_path | |
| if db_path is None: | |
| self.db_path = os.getenv("SQLITE_DB_PATH", "director.db") | |
| else: | |
| self.db_path = db_path | |
| def __init__(self, db_path: str = None): | |
| """ | |
| :param db_path: Path to the SQLite database file. | |
| """ | |
| self.db_type = DBType.SQLITE | |
| if db_path is None: | |
| self.db_path = os.getenv("SQLITE_DB_PATH", "director.db") | |
| else: | |
| self.db_path = db_path | |
| # Validate path is absolute and within allowed directory | |
| self.db_path = os.path.abspath(self.db_path) | |
| allowed_dir = os.path.abspath(os.getenv("SQLITE_DB_DIR", ".")) | |
| if not self.db_path.startswith(allowed_dir): | |
| raise ValueError(f"Database path must be within {allowed_dir}") | |
| self.conn = sqlite3.connect(self.db_path, check_same_thread=True) |
| db_path = os.getenv("SQLITE_DB_PATH", "director.db") | ||
| initialize_sqlite(db_path) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and logging
The database initialization should include error handling for invalid paths and log the database path being used.
if __name__ == "__main__":
db_path = os.getenv("SQLITE_DB_PATH", "director.db")
+ try:
+ db_path = os.path.abspath(db_path)
+ logger.info(f"Initializing SQLite database at: {db_path}")
+ if not os.access(os.path.dirname(db_path), os.W_OK):
+ raise PermissionError(f"No write access to directory: {os.path.dirname(db_path)}")
initialize_sqlite(db_path)
+ except Exception as e:
+ logger.error(f"Failed to initialize database: {e}")
+ sys.exit(1)Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
SQLITE_DB_PATHfor flexible database path configuration.SQLiteDBclass to dynamically retrieve the database path from the environment variable.initialize_sqlitefunction to accept the database path from the environment variable, defaulting to "director.db" if not set.