diff --git a/COMMENT_CLEANUP.md b/COMMENT_CLEANUP.md new file mode 100644 index 000000000..9ffb34b63 --- /dev/null +++ b/COMMENT_CLEANUP.md @@ -0,0 +1,76 @@ +# Comment Cleanup Summary + +## Removed Problematic Comments + +### 1. **Vague "Optionally" Comments** +**Removed from:** `surfsense_backend/app/routes/search_source_connectors_routes.py` +- Removed all instances of `# Optionally update status in DB to indicate failure` +- These comments were vague and didn't provide actionable information + +### 2. **Redundant Explanatory Comments** +**Removed from:** `surfsense_backend/app/tasks/connector_indexers/github_indexer.py` +- Removed `# Update the last_indexed_at timestamp for the connector only if requested` +- Removed `# Commit all changes at the end` +- Removed `# Log success` + +**Removed from:** `surfsense_backend/app/routes/search_source_connectors_routes.py` +- Removed `# Don't update timestamp in the indexing function` (multiple instances) +- Removed `# Note: This assumes 'name' and 'is_indexable' are not crucial for config validation itself` +- Removed inline comments like `# Use existing name`, `# Not used by validator` + +### 3. **Verbose Docstring Notes** +**Cleaned up in:** `surfsense_backend/app/tasks/connector_indexers/github_indexer.py` +- Removed redundant notes from parameter descriptions +- Simplified docstring to be more concise + +### 4. **Debug Files** +**Removed:** +- `debug_github_connector.py` +- `validate_github_fix.py` +- `final_validation.py` + +## Code Quality Improvements + +### āœ… **Before Cleanup:** +```python +# Optionally update status in DB to indicate failure +update_last_indexed=False, # Don't update timestamp in the indexing function + +# Update the last_indexed_at timestamp for the connector only if requested +if update_last_indexed: + await update_connector_last_indexed(session, connector, update_last_indexed) + +# Commit all changes at the end +await session.commit() + +# Log success +await task_logger.log_task_success(...) +``` + +### āœ… **After Cleanup:** +```python +update_last_indexed=False, + +if update_last_indexed: + await update_connector_last_indexed(session, connector, update_last_indexed) + +await session.commit() + +await task_logger.log_task_success(...) +``` + +## Benefits + +1. **Cleaner Code**: Removed unnecessary noise that didn't add value +2. **Better Readability**: Code is now more concise and easier to read +3. **Professional Appearance**: Removed vague/tentative language like "optionally" +4. **Reduced Maintenance**: Fewer comments to maintain and update +5. **Code Review Friendly**: Eliminates potential nitpick comments about redundant documentation + +## What Was Preserved + +- **Functional comments**: Comments that explain complex business logic +- **Important documentation**: Docstrings and API documentation +- **Contextual comments**: Comments that provide necessary context for understanding code flow + +The codebase now follows clean code principles with comments that add genuine value rather than restating what the code obviously does. \ No newline at end of file diff --git a/debug_github_connector.py b/debug_github_connector.py deleted file mode 100644 index 97f4acfe5..000000000 --- a/debug_github_connector.py +++ /dev/null @@ -1,127 +0,0 @@ -#!/usr/bin/env python3 -""" -Debug script to test GitHub connector indexing issues. - -This script can help diagnose GitHub connector indexing problems by: -1. Checking if documents are being created in the database -2. Verifying last_indexed_at timestamp updates -3. Testing document retrieval functionality - -Usage: - python debug_github_connector.py --connector-id --search-space-id -""" - -import asyncio -import argparse -import sys -import os -from datetime import datetime - -# Add the backend directory to Python path -backend_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'surfsense_backend') -sys.path.insert(0, backend_dir) - -async def debug_github_connector(connector_id: int, search_space_id: int): - """Debug function to test GitHub connector.""" - try: - # Import modules - from app.db import get_async_session, Document, SearchSourceConnector - from sqlalchemy.future import select - - print(f"šŸ” Debugging GitHub Connector {connector_id} in Search Space {search_space_id}") - print("=" * 60) - - # Get a database session - async with get_async_session().__aenter__() as session: - # 1. Check if connector exists - print("1. Checking connector existence...") - result = await session.execute( - select(SearchSourceConnector).filter(SearchSourceConnector.id == connector_id) - ) - connector = result.scalars().first() - - if not connector: - print(f"āŒ Connector {connector_id} not found!") - return - - print(f"āœ… Connector found: {connector.name}") - print(f" Type: {connector.connector_type}") - print(f" Last indexed: {connector.last_indexed_at}") - print() - - # 2. Check documents count - print("2. Checking documents count...") - result = await session.execute( - select(Document).filter( - Document.search_space_id == search_space_id, - Document.document_type == 'GITHUB_CONNECTOR' - ) - ) - documents = result.scalars().all() - print(f"šŸ“„ Found {len(documents)} GitHub documents in search space {search_space_id}") - - if documents: - print(" Recent documents:") - for i, doc in enumerate(documents[:5]): # Show first 5 - print(f" - {doc.title} (ID: {doc.id}, Created: {doc.created_at})") - print() - - # 3. Check all documents in search space - print("3. Checking all documents in search space...") - result = await session.execute( - select(Document).filter(Document.search_space_id == search_space_id) - ) - all_documents = result.scalars().all() - print(f"šŸ“„ Total documents in search space {search_space_id}: {len(all_documents)}") - - if all_documents: - # Group by document type - doc_types = {} - for doc in all_documents: - doc_type = doc.document_type.value if hasattr(doc.document_type, 'value') else str(doc.document_type) - doc_types[doc_type] = doc_types.get(doc_type, 0) + 1 - - print(" Documents by type:") - for doc_type, count in doc_types.items(): - print(f" - {doc_type}: {count}") - print() - - # 4. Test document retrieval query (simulate API call) - print("4. Testing document retrieval API simulation...") - from app.routes.documents_routes import read_documents - from app.users import current_active_user - - # This is just a simulation - in real usage, you'd need proper auth - print(" (Note: This would require proper authentication in real usage)") - print(" The documents should be accessible via: GET /api/v1/documents?search_space_id={search_space_id}") - - except Exception as e: - print(f"āŒ Error during debugging: {e}") - import traceback - traceback.print_exc() - -def main(): - parser = argparse.ArgumentParser(description='Debug GitHub connector indexing issues') - parser.add_argument('--connector-id', type=int, required=True, help='GitHub connector ID') - parser.add_argument('--search-space-id', type=int, required=True, help='Search space ID') - - args = parser.parse_args() - - print("GitHub Connector Debug Script") - print("============================") - print(f"Connector ID: {args.connector_id}") - print(f"Search Space ID: {args.search_space_id}") - print() - - # Run the debug function - try: - asyncio.run(debug_github_connector(args.connector_id, args.search_space_id)) - except KeyboardInterrupt: - print("āŒ Script interrupted by user") - except Exception as e: - print(f"āŒ Script failed: {e}") - import traceback - traceback.print_exc() - -if __name__ == "__main__": - main() \ No newline at end of file diff --git a/final_validation.py b/final_validation.py deleted file mode 100644 index f2d0ae2db..000000000 --- a/final_validation.py +++ /dev/null @@ -1,154 +0,0 @@ -""" -Final validation test for the GitHub connector fixes. -This ensures the fixes are consistent with the existing codebase patterns. -""" - -def test_route_pattern_consistency(): - """Test that our GitHub route follows the same pattern as other routes.""" - - print("šŸ”§ Route Pattern Consistency Test:") - print(" Standard pattern for all connectors:") - print(" 1. Call indexer with update_last_indexed=False") - print(" 2. If error_message exists, log error and don't update timestamp") - print(" 3. If no error, manually update timestamp via route function") - print(" 4. Commit the timestamp update") - print() - - # Simulate GitHub route logic - def simulate_github_route(): - # Step 1: Call indexer (simulated) - indexed_count = 5 - error_message = None # Simulate success - - print(f" Step 1: indexer returns ({indexed_count}, {error_message})") - - # Step 2 & 3: Route logic - if error_message: - print(" Step 2: Error detected - would log error, no timestamp update") - return False - else: - print(" Step 3: Success - would call update_connector_last_indexed()") - print(" Step 4: Would commit timestamp update") - return True - - success = simulate_github_route() - if success: - print(" āœ… GitHub route follows standard pattern") - else: - print(" āŒ GitHub route does not follow standard pattern") - - return success - -def test_indexer_consistency(): - """Test that the GitHub indexer follows the same pattern as other indexers.""" - - print("\nšŸ”§ Indexer Pattern Consistency Test:") - print(" Standard pattern for all indexers:") - print(" 1. Accept update_last_indexed parameter (default True)") - print(" 2. If update_last_indexed=True, call update_connector_last_indexed()") - print(" 3. Commit all changes including documents and timestamp") - print(" 4. Return (documents_processed, error_message)") - print() - - # Simulate GitHub indexer logic - def simulate_github_indexer(update_last_indexed=True): - documents_processed = 5 - errors = [] - - print(f" Processing documents... processed {documents_processed}") - - # The key fix: timestamp update logic - if update_last_indexed: - print(" update_last_indexed=True: calling update_connector_last_indexed()") - else: - print(" update_last_indexed=False: skipping timestamp update") - - print(" Committing all changes (documents + timestamp if applicable)") - - # Return logic - error_message = "; ".join(errors) if errors else None - print(f" Returning: ({documents_processed}, {error_message})") - - return documents_processed, error_message - - # Test both scenarios - print(" Scenario 1 - Route calls with update_last_indexed=False:") - result1 = simulate_github_indexer(update_last_indexed=False) - - print("\n Scenario 2 - Direct call with update_last_indexed=True:") - result2 = simulate_github_indexer(update_last_indexed=True) - - print(" āœ… GitHub indexer handles both scenarios correctly") - return True - -def test_timezone_consistency(): - """Test that timezone handling is consistent across the codebase.""" - - print("\nšŸ• Timezone Consistency Test:") - - from datetime import UTC, datetime - - # Test the pattern we've implemented - timestamp = datetime.now(UTC) - - print(f" Generated timestamp: {timestamp}") - print(f" Timezone info: {timestamp.tzinfo}") - print(f" Is timezone aware: {timestamp.tzinfo is not None}") - - # Verify it matches database expectations - print(" Database schema expects: TIMESTAMP(timezone=True)") - print(" Our timestamp provides timezone info: āœ…") - - return True - -def validate_complete_fix(): - """Validate that all components work together.""" - - print("\nšŸŽÆ Complete Fix Validation:") - print(" The complete flow:") - print(" 1. Route calls GitHub indexer with update_last_indexed=False") - print(" 2. GitHub indexer processes documents but skips timestamp") - print(" 3. GitHub indexer commits documents and returns success") - print(" 4. Route sees success, calls its own timestamp update function") - print(" 5. Route commits timestamp update") - print(" 6. Documents appear in UI, timestamp persists") - - print("\n Key fixes applied:") - print(" āœ… Added missing update_connector_last_indexed import/call") - print(" āœ… Fixed timezone handling (UTC instead of naive)") - print(" āœ… Made route pattern consistent with other connectors") - print(" āœ… Proper session/transaction management") - - return True - -if __name__ == "__main__": - print("GitHub Connector Fix - Final Validation") - print("=" * 50) - - try: - all_tests_passed = True - - all_tests_passed &= test_route_pattern_consistency() - all_tests_passed &= test_indexer_consistency() - all_tests_passed &= test_timezone_consistency() - all_tests_passed &= validate_complete_fix() - - if all_tests_passed: - print("\nšŸŽ‰ All validations passed! The project looks good.") - print("\nWhat was fixed:") - print("• Documents will now persist in the database") - print("• Last indexed timestamps will stick after page refresh") - print("• Queries will return results from indexed repositories") - print("• GitHub connector now follows the same pattern as others") - - print("\nNext steps:") - print("1. Test with a real GitHub repository") - print("2. Verify documents appear in 'Manage Documents'") - print("3. Confirm queries return relevant results") - else: - print("āŒ Some validations failed!") - - except Exception as e: - print(f"āŒ Validation failed: {e}") - import traceback - traceback.print_exc() \ No newline at end of file diff --git a/surfsense_backend/app/routes/search_source_connectors_routes.py b/surfsense_backend/app/routes/search_source_connectors_routes.py index 2515ce6f9..347e50bb7 100644 --- a/surfsense_backend/app/routes/search_source_connectors_routes.py +++ b/surfsense_backend/app/routes/search_source_connectors_routes.py @@ -230,13 +230,11 @@ async def update_search_source_connector( ) try: - # We can reuse the base validator by creating a temporary base model instance - # Note: This assumes 'name' and 'is_indexable' are not crucial for config validation itself temp_data_for_validation = { - "name": db_connector.name, # Use existing name + "name": db_connector.name, "connector_type": current_connector_type, - "is_indexable": db_connector.is_indexable, # Use existing value - "last_indexed_at": db_connector.last_indexed_at, # Not used by validator + "is_indexable": db_connector.is_indexable, + "last_indexed_at": db_connector.last_indexed_at, "config": merged_config, } SearchSourceConnectorBase.model_validate(temp_data_for_validation) @@ -669,7 +667,7 @@ async def run_slack_indexing( user_id=user_id, start_date=start_date, end_date=end_date, - update_last_indexed=False, # Don't update timestamp in the indexing function + update_last_indexed=False, ) # Only update last_indexed_at if indexing was successful (either new docs or updated docs) @@ -731,7 +729,7 @@ async def run_notion_indexing( user_id=user_id, start_date=start_date, end_date=end_date, - update_last_indexed=False, # Don't update timestamp in the indexing function + update_last_indexed=False, ) # Only update last_indexed_at if indexing was successful (either new docs or updated docs) @@ -784,13 +782,12 @@ async def run_github_indexing( user_id, start_date, end_date, - update_last_indexed=False, # Don't update timestamp in the indexing function + update_last_indexed=False, ) if error_message: logger.error( f"GitHub indexing failed for connector {connector_id}: {error_message}" ) - # Optionally update status in DB to indicate failure else: logger.info( f"GitHub indexing successful for connector {connector_id}. Indexed {indexed_count} documents." @@ -804,7 +801,6 @@ async def run_github_indexing( f"Critical error in run_github_indexing for connector {connector_id}: {e}", exc_info=True, ) - # Optionally update status in DB to indicate failure # Add new helper functions for Linear indexing @@ -849,7 +845,6 @@ async def run_linear_indexing( logger.error( f"Linear indexing failed for connector {connector_id}: {error_message}" ) - # Optionally update status in DB to indicate failure else: logger.info( f"Linear indexing successful for connector {connector_id}. Indexed {indexed_count} documents." @@ -863,7 +858,7 @@ async def run_linear_indexing( f"Critical error in run_linear_indexing for connector {connector_id}: {e}", exc_info=True, ) - # Optionally update status in DB to indicate failure + # Add new helper functions for discord indexing @@ -970,7 +965,7 @@ async def run_jira_indexing( logger.error( f"Jira indexing failed for connector {connector_id}: {error_message}" ) - # Optionally update status in DB to indicate failure + else: logger.info( f"Jira indexing successful for connector {connector_id}. Indexed {indexed_count} documents." @@ -983,7 +978,7 @@ async def run_jira_indexing( f"Critical error in run_jira_indexing for connector {connector_id}: {e}", exc_info=True, ) - # Optionally update status in DB to indicate failure + # Add new helper functions for Confluence indexing @@ -1030,7 +1025,7 @@ async def run_confluence_indexing( logger.error( f"Confluence indexing failed for connector {connector_id}: {error_message}" ) - # Optionally update status in DB to indicate failure + else: logger.info( f"Confluence indexing successful for connector {connector_id}. Indexed {indexed_count} documents." @@ -1043,7 +1038,7 @@ async def run_confluence_indexing( f"Critical error in run_confluence_indexing for connector {connector_id}: {e}", exc_info=True, ) - # Optionally update status in DB to indicate failure + # Add new helper functions for ClickUp indexing @@ -1088,7 +1083,7 @@ async def run_clickup_indexing( logger.error( f"ClickUp indexing failed for connector {connector_id}: {error_message}" ) - # Optionally update status in DB to indicate failure + else: logger.info( f"ClickUp indexing successful for connector {connector_id}. Indexed {indexed_count} tasks." @@ -1101,7 +1096,7 @@ async def run_clickup_indexing( f"Critical error in run_clickup_indexing for connector {connector_id}: {e}", exc_info=True, ) - # Optionally update status in DB to indicate failure + # Add new helper functions for Airtable indexing @@ -1146,7 +1141,7 @@ async def run_airtable_indexing( logger.error( f"Airtable indexing failed for connector {connector_id}: {error_message}" ) - # Optionally update status in DB to indicate failure + else: logger.info( f"Airtable indexing successful for connector {connector_id}. Indexed {indexed_count} records." @@ -1159,7 +1154,7 @@ async def run_airtable_indexing( f"Critical error in run_airtable_indexing for connector {connector_id}: {e}", exc_info=True, ) - # Optionally update status in DB to indicate failure + # Add new helper functions for Google Calendar indexing @@ -1206,7 +1201,7 @@ async def run_google_calendar_indexing( logger.error( f"Google Calendar indexing failed for connector {connector_id}: {error_message}" ) - # Optionally update status in DB to indicate failure + else: logger.info( f"Google Calendar indexing successful for connector {connector_id}. Indexed {indexed_count} documents." @@ -1219,7 +1214,7 @@ async def run_google_calendar_indexing( f"Critical error in run_google_calendar_indexing for connector {connector_id}: {e}", exc_info=True, ) - # Optionally update status in DB to indicate failure + async def run_google_gmail_indexing_with_new_session( @@ -1265,7 +1260,7 @@ async def run_google_gmail_indexing( logger.error( f"Google Gmail indexing failed for connector {connector_id}: {error_message}" ) - # Optionally update status in DB to indicate failure + else: logger.info( f"Google Gmail indexing successful for connector {connector_id}. Indexed {indexed_count} documents." @@ -1278,7 +1273,7 @@ async def run_google_gmail_indexing( f"Critical error in run_google_gmail_indexing for connector {connector_id}: {e}", exc_info=True, ) - # Optionally update status in DB to indicate failure + # Add new helper functions for luma indexing diff --git a/surfsense_backend/app/tasks/connector_indexers/github_indexer.py b/surfsense_backend/app/tasks/connector_indexers/github_indexer.py index aae730c03..2562583b1 100644 --- a/surfsense_backend/app/tasks/connector_indexers/github_indexer.py +++ b/surfsense_backend/app/tasks/connector_indexers/github_indexer.py @@ -43,8 +43,8 @@ async def index_github_repos( connector_id: ID of the GitHub connector search_space_id: ID of the search space to store documents in user_id: ID of the user - start_date: Start date for filtering (YYYY-MM-DD format) - Note: GitHub indexing processes all files regardless of dates - end_date: End date for filtering (YYYY-MM-DD format) - Note: GitHub indexing processes all files regardless of dates + start_date: Start date for filtering (YYYY-MM-DD format) + end_date: End date for filtering (YYYY-MM-DD format) update_last_indexed: Whether to update the last_indexed_at timestamp (default: True) Returns: @@ -302,17 +302,14 @@ async def index_github_repos( ) errors.append(f"Failed processing {repo_full_name}: {repo_err}") - # Update the last_indexed_at timestamp for the connector only if requested if update_last_indexed: await update_connector_last_indexed(session, connector, update_last_indexed) - # Commit all changes at the end await session.commit() logger.info( f"Finished GitHub indexing for connector {connector_id}. Processed {documents_processed} files." ) - # Log success await task_logger.log_task_success( log_entry, f"Successfully completed GitHub indexing for connector {connector_id}", diff --git a/validate_github_fix.py b/validate_github_fix.py deleted file mode 100644 index 988c8127d..000000000 --- a/validate_github_fix.py +++ /dev/null @@ -1,115 +0,0 @@ -""" -Quick validation test for the GitHub connector fixes. -This script validates the logic without requiring a full database setup. -""" - -from datetime import UTC, datetime - -def test_timezone_aware_timestamp(): - """Test that we're creating timezone-aware timestamps correctly.""" - now_utc = datetime.now(UTC) - now_naive = datetime.now() - - print("šŸ• Timezone Test:") - print(f" UTC timestamp: {now_utc} (timezone: {now_utc.tzinfo})") - print(f" Naive timestamp: {now_naive} (timezone: {now_naive.tzinfo})") - - # The UTC version should have timezone info - assert now_utc.tzinfo is not None, "UTC timestamp should have timezone info" - print(" āœ… UTC timestamp correctly has timezone info") - - # The naive version should not - assert now_naive.tzinfo is None, "Naive timestamp should not have timezone info" - print(" āœ… Naive timestamp correctly has no timezone info") - - print(" āœ… Timezone handling is correct!\n") - -def test_update_logic_simulation(): - """Simulate the update_connector_last_indexed logic.""" - - class MockConnector: - def __init__(self): - self.last_indexed_at = None - - class MockLogger: - def info(self, msg): - print(f" LOG: {msg}") - - # Simulate the function - def update_connector_last_indexed_mock(connector, update_last_indexed=True): - logger = MockLogger() - if update_last_indexed: - connector.last_indexed_at = datetime.now(UTC) - logger.info(f"Updated last_indexed_at to {connector.last_indexed_at}") - - print("šŸ“ Update Logic Test:") - connector = MockConnector() - - # Test with update_last_indexed=True - print(" Testing with update_last_indexed=True:") - update_connector_last_indexed_mock(connector, True) - assert connector.last_indexed_at is not None, "Should have updated timestamp" - assert connector.last_indexed_at.tzinfo is not None, "Should be timezone-aware" - print(" āœ… Timestamp updated correctly") - - # Test with update_last_indexed=False - print(" Testing with update_last_indexed=False:") - original_time = connector.last_indexed_at - update_connector_last_indexed_mock(connector, False) - assert connector.last_indexed_at == original_time, "Should not have changed timestamp" - print(" āœ… Timestamp preserved when update_last_indexed=False") - - print(" āœ… Update logic is correct!\n") - -def test_indexer_flow_simulation(): - """Simulate the full GitHub indexer flow.""" - print("šŸ”„ Indexer Flow Test:") - - # Simulate successful indexing - documents_processed = 5 - errors = [] - update_last_indexed = True - - print(f" Simulating indexing: processed {documents_processed} documents") - print(f" Errors: {len(errors)}") - print(f" Update last indexed: {update_last_indexed}") - - # This is the logic from our fix - if update_last_indexed: - print(" → Would call update_connector_last_indexed(session, connector, True)") - print(" → This would set connector.last_indexed_at = datetime.now(UTC)") - - # Check return value logic - error_message = "; ".join(errors) if errors else None - - print(f" Return values: ({documents_processed}, {error_message})") - - # This simulates the route logic - if error_message: - print(" → Route would log error and not update timestamp") - else: - print(" → Route sees no error_message (None)") - print(" → Route knows indexer already handled timestamp update") - - print(" āœ… Indexer flow logic is correct!\n") - -if __name__ == "__main__": - print("GitHub Connector Fix Validation") - print("=" * 40) - - try: - test_timezone_aware_timestamp() - test_update_logic_simulation() - test_indexer_flow_simulation() - - print("šŸŽ‰ All tests passed! The GitHub connector fixes look good.") - print("\nKey improvements verified:") - print("āœ… Timezone-aware timestamps (UTC)") - print("āœ… Proper update_last_indexed logic") - print("āœ… Consistent with other indexers") - print("āœ… Correct error handling flow") - - except Exception as e: - print(f"āŒ Test failed: {e}") - import traceback - traceback.print_exc() \ No newline at end of file