-
Notifications
You must be signed in to change notification settings - Fork 0
Fix CI blockers for auth integration tests #565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -243,15 +243,15 @@ def _parse_json_fields(self, row: Dict[str, Any], fields: List[str]) -> Dict[str | |
| """ | ||
| if not row: | ||
| return row | ||
| for field in row: | ||
| for field in fields: | ||
| if field in row and isinstance(row[field], str): | ||
| try: | ||
| row[field] = json.loads(row[field]) | ||
| except json.JSONDecodeError: | ||
| logger.warning( | ||
| f"Failed to parse JSON for field {field} in row {row.get(FIELD_ID)}" | ||
| ) | ||
| if field in (FIELD_ANALYSIS_METADATA, "metadata"): | ||
| if field in (FIELD_ANALYSIS_METADATA, "metadata", "filterResults"): | ||
| row[field] = {} | ||
| else: | ||
| row[field] = [] | ||
|
|
@@ -692,6 +692,9 @@ async def get_weekly_growth(self) -> Dict[str, Any]: | |
| # This is initialized via FastAPI startup event | ||
| _db_manager_instance = None | ||
|
|
||
| # A singleton for imports assuming a single database manager exists initially (used by main) | ||
| db_manager = DatabaseManager() | ||
| _db_manager_instance = db_manager | ||
|
Comment on lines
+695
to
+697
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This eager singleton now skips disk initialization. By setting Suggested fix # A singleton for imports assuming a single database manager exists initially (used by main)
db_manager = DatabaseManager()
_db_manager_instance = db_manager
async def get_db() -> DatabaseManager:
@@
global _db_manager_instance
if _db_manager_instance is None:
# This should ideally not be reached if startup event is properly set
- _db_manager_instance = DatabaseManager()
- await _db_manager_instance._ensure_initialized()
+ _db_manager_instance = db_manager
+ await _db_manager_instance._ensure_initialized()
return _db_manager_instance
async def initialize_db():
@@
global _db_manager_instance
if _db_manager_instance is None:
- _db_manager_instance = DatabaseManager()
- await _db_manager_instance._ensure_initialized()
+ _db_manager_instance = db_manager
+ await _db_manager_instance._ensure_initialized()🤖 Prompt for AI Agents |
||
|
|
||
| async def get_db() -> DatabaseManager: | ||
| """ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,3 +19,14 @@ def get_active_models(self) -> List[Dict[str, Any]]: | |
| def check_health(self) -> bool: | ||
| """Check model manager health. Stub implementation.""" | ||
| return True | ||
|
|
||
| def discover_models(self) -> None: | ||
| """Discover models. Stub implementation.""" | ||
| pass | ||
|
|
||
| def list_models(self) -> List[Dict[str, Any]]: | ||
| """List models. Stub implementation.""" | ||
| return [] | ||
|
|
||
|
|
||
| model_manager = ModelManager() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instantiating |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -146,9 +146,14 @@ class EmailCache: | |||||||||||||||||||||||||||||||||||
| def __init__(self, cache_path: str = str(DEFAULT_CACHE_PATH)): | ||||||||||||||||||||||||||||||||||||
| """Initializes the EmailCache.""" | ||||||||||||||||||||||||||||||||||||
| # Secure path validation | ||||||||||||||||||||||||||||||||||||
| self.cache_path = str( | ||||||||||||||||||||||||||||||||||||
| PathValidator.validate_database_path(cache_path, Path(cache_path).parent) | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||
| self.cache_path = str( | ||||||||||||||||||||||||||||||||||||
| PathValidator.validate_database_path(cache_path, Path(cache_path).parent) | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| except AttributeError: | ||||||||||||||||||||||||||||||||||||
| self.cache_path = str( | ||||||||||||||||||||||||||||||||||||
| PathValidator.validate_and_resolve_db_path(cache_path, Path(cache_path).parent) | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+149
to
+156
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a
Suggested change
Comment on lines
+149
to
+156
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use a fixed trusted base directory for cache-path validation. On Line 151 and Line 155, 🔧 Proposed fix def __init__(self, cache_path: str = str(DEFAULT_CACHE_PATH)):
"""Initializes the EmailCache."""
- # Secure path validation
- try:
- self.cache_path = str(
- PathValidator.validate_database_path(cache_path, Path(cache_path).parent)
- )
- except AttributeError:
- self.cache_path = str(
- PathValidator.validate_and_resolve_db_path(cache_path, Path(cache_path).parent)
- )
+ candidate_path = Path(cache_path)
+ if not candidate_path.is_absolute():
+ candidate_path = PROJECT_ROOT / PathValidator.sanitize_filename(candidate_path.name)
+
+ validator = getattr(PathValidator, "validate_database_path", None)
+ if callable(validator):
+ self.cache_path = str(validator(candidate_path, PROJECT_ROOT))
+ else:
+ self.cache_path = str(
+ PathValidator.validate_and_resolve_db_path(candidate_path, PROJECT_ROOT)
+ )
+
+ Path(self.cache_path).parent.mkdir(parents=True, exist_ok=True)
self.conn = sqlite3.connect(self.cache_path, check_same_thread=False)
self._init_cache()🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| self.conn = sqlite3.connect(self.cache_path, check_same_thread=False) | ||||||||||||||||||||||||||||||||||||
| self._init_cache() | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -115,7 +115,10 @@ def __init__(self, db_path: str = None): | |||||||||||||||||||
| db_path = os.path.join(DATA_DIR, filename) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| # Validate the final path | ||||||||||||||||||||
| self.db_path = str(PathValidator.validate_database_path(db_path, DATA_DIR)) | ||||||||||||||||||||
| try: | ||||||||||||||||||||
| self.db_path = str(PathValidator.validate_database_path(db_path, DATA_DIR)) | ||||||||||||||||||||
| except AttributeError: | ||||||||||||||||||||
|
Comment on lines
+118
to
+120
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (bug_risk): The Catching all |
||||||||||||||||||||
| self.db_path = str(PathValidator.validate_and_resolve_db_path(db_path, DATA_DIR)) | ||||||||||||||||||||
|
Comment on lines
+118
to
+121
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
Suggested change
|
||||||||||||||||||||
| self.logger = logging.getLogger(__name__) | ||||||||||||||||||||
| self.conn = None | ||||||||||||||||||||
| if self.db_path == ":memory:": | ||||||||||||||||||||
|
|
@@ -129,6 +132,10 @@ def _get_db_connection(self) -> sqlite3.Connection: | |||||||||||||||||||
| """Establishes and returns a database connection.""" | ||||||||||||||||||||
| if self.conn: | ||||||||||||||||||||
| return self.conn | ||||||||||||||||||||
| # Ensure parent directory exists before connecting | ||||||||||||||||||||
| db_dir = os.path.dirname(self.db_path) | ||||||||||||||||||||
| if db_dir and not os.path.exists(db_dir): | ||||||||||||||||||||
| os.makedirs(db_dir, exist_ok=True) | ||||||||||||||||||||
| conn = sqlite3.connect(self.db_path) | ||||||||||||||||||||
| conn.row_factory = sqlite3.Row | ||||||||||||||||||||
| return conn | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instantiating
DatabaseManagerat the module level introduces side effects on import (e.g., file system operations) and makes the code harder to test. It's better to manage singletons through a factory function or a dependency injection framework. This allows for lazy initialization and easier mocking in tests.