Fix unbound variable and read_text() bugs in event services (#12297)

Co-authored-by: openhands <openhands@all-hands.dev>

openhands/app_server/event/event_service_base.py

@@ -107,6 +107,7 @@ class EventServiceBase(EventService, ABC):
             )
 
         start_offset = 0
+        next_page_id = None
         if page_id:
             start_offset = int(page_id)
             paths = paths[start_offset:]

openhands/app_server/event/filesystem_event_service.py

@@ -22,7 +22,7 @@ class FilesystemEventService(EventServiceBase):
 
     def _load_event(self, path: Path) -> Event | None:
         try:
-            content = path.read_text(str(path))
+            content = path.read_text()
             content = Event.model_validate_json(content)
             return content
         except Exception:

tests/unit/app_server/test_filesystem_event_service.py

@@ -0,0 +1,535 @@
+"""Tests for FilesystemEventService.
+
+This module tests the filesystem-based implementation of EventService,
+focusing on basic CRUD operations, search functionality, and file I/O handling.
+
+The tests use mocking to avoid the complex dependency chain of the actual module.
+"""
+
+import glob
+import json
+import tempfile
+from dataclasses import dataclass
+from datetime import datetime, timezone
+from pathlib import Path
+from uuid import UUID, uuid4
+
+import pytest
+
+
+class MockEvent:
+    """Mock Event class for testing without requiring the full SDK."""
+
+    def __init__(
+        self,
+        id: str | UUID | None = None,
+        kind: str = 'test_event',
+        timestamp: datetime | None = None,
+    ):
+        self.id = id if id is not None else uuid4()
+        self.kind = kind
+        self.timestamp = timestamp or datetime.now(timezone.utc)
+
+    def model_dump_json(self, indent: int = 2) -> str:
+        """Serialize to JSON string."""
+        id_str = str(self.id) if isinstance(self.id, UUID) else self.id
+        return json.dumps(
+            {
+                'id': id_str,
+                'kind': self.kind,
+                'timestamp': self.timestamp.isoformat(),
+            },
+            indent=indent,
+        )
+
+    @classmethod
+    def model_validate_json(cls, json_str: str) -> 'MockEvent':
+        """Deserialize from JSON string."""
+        data = json.loads(json_str)
+        return cls(
+            id=data['id'],
+            kind=data['kind'],
+            timestamp=datetime.fromisoformat(data['timestamp']),
+        )
+
+
+@dataclass
+class MockFilesystemEventService:
+    """
+    A mock implementation of FilesystemEventService for testing.
+
+    This replicates the core logic of the actual service without importing it,
+    allowing us to test the algorithms in isolation.
+    """
+
+    prefix: Path
+    user_id: str | None
+    app_conversation_info_service: None
+    app_conversation_info_load_tasks: dict
+    limit: int = 500
+
+    def _load_event(self, path: Path) -> MockEvent | None:
+        """Get the event at the path given."""
+        try:
+            content = path.read_text()
+            event = MockEvent.model_validate_json(content)
+            return event
+        except Exception:
+            return None
+
+    def _store_event(self, path: Path, event: MockEvent):
+        """Store the event given at the path given."""
+        path.parent.mkdir(parents=True, exist_ok=True)
+        content = event.model_dump_json(indent=2)
+        path.write_text(content)
+
+    def _search_paths(self, prefix: Path, page_id: str | None = None) -> list[Path]:
+        """Search paths."""
+        search_path = f'{prefix}*'
+        files = glob.glob(str(search_path))
+        paths = [Path(file) for file in files]
+        return paths
+
+    async def get_conversation_path(self, conversation_id: UUID) -> Path:
+        """Get a path for a conversation."""
+        path = self.prefix
+        if self.user_id:
+            path /= self.user_id
+        path = path / 'v1_conversations' / conversation_id.hex
+        return path
+
+    async def save_event(self, conversation_id: UUID, event: MockEvent):
+        """Save an event."""
+        if isinstance(event.id, str):
+            id_hex = event.id.replace('-', '')
+        else:
+            id_hex = event.id.hex
+        path = (await self.get_conversation_path(conversation_id)) / f'{id_hex}.json'
+        self._store_event(path, event)
+
+    async def get_event(
+        self, conversation_id: UUID, event_id: UUID
+    ) -> MockEvent | None:
+        """Get an event."""
+        conversation_path = await self.get_conversation_path(conversation_id)
+        path = conversation_path / f'{event_id.hex}.json'
+        return self._load_event(path)
+
+
+@pytest.fixture
+def temp_dir():
+    """Create a temporary directory for test files."""
+    with tempfile.TemporaryDirectory() as tmpdir:
+        yield Path(tmpdir)
+
+
+@pytest.fixture
+def service(temp_dir: Path) -> MockFilesystemEventService:
+    """Create a MockFilesystemEventService instance for testing."""
+    return MockFilesystemEventService(
+        prefix=temp_dir,
+        user_id='test_user',
+        app_conversation_info_service=None,
+        app_conversation_info_load_tasks={},
+    )
+
+
+@pytest.fixture
+def service_no_user(temp_dir: Path) -> MockFilesystemEventService:
+    """Create a MockFilesystemEventService instance without user_id."""
+    return MockFilesystemEventService(
+        prefix=temp_dir,
+        user_id=None,
+        app_conversation_info_service=None,
+        app_conversation_info_load_tasks={},
+    )
+
+
+class TestFilesystemEventServiceLoadEvent:
+    """Test cases for _load_event method."""
+
+    def test_load_event_success(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test loading an event from a valid JSON file."""
+        event_id = uuid4()
+        event_data = {
+            'id': str(event_id),
+            'kind': 'test_event',
+            'timestamp': datetime.now(timezone.utc).isoformat(),
+        }
+
+        test_file = temp_dir / f'{event_id.hex}.json'
+        test_file.write_text(json.dumps(event_data))
+
+        result = service._load_event(test_file)
+        assert result is not None
+        assert result.id == str(event_id)
+        assert result.kind == 'test_event'
+
+    def test_load_event_file_not_found(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test loading an event from a non-existent file."""
+        non_existent_path = temp_dir / 'non_existent.json'
+        result = service._load_event(non_existent_path)
+        assert result is None
+
+    def test_load_event_invalid_json(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test loading an event from an invalid JSON file."""
+        test_file = temp_dir / 'invalid.json'
+        test_file.write_text('not valid json {{{')
+
+        result = service._load_event(test_file)
+        assert result is None
+
+    def test_load_event_empty_file(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test loading an event from an empty file."""
+        test_file = temp_dir / 'empty.json'
+        test_file.write_text('')
+
+        result = service._load_event(test_file)
+        assert result is None
+
+    def test_load_event_missing_fields(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test loading an event with missing required fields."""
+        test_file = temp_dir / 'missing_fields.json'
+        test_file.write_text(json.dumps({'id': str(uuid4())}))  # Missing kind/timestamp
+
+        result = service._load_event(test_file)
+        assert result is None
+
+
+class TestFilesystemEventServiceStoreEvent:
+    """Test cases for _store_event method."""
+
+    def test_store_event_creates_file(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test that _store_event creates a file with correct content."""
+        event = MockEvent()
+        test_path = temp_dir / 'events' / f'{event.id}.json'
+
+        service._store_event(test_path, event)
+
+        assert test_path.exists()
+        content = test_path.read_text()
+        assert str(event.id) in content
+        assert event.kind in content
+
+    def test_store_event_creates_parent_directories(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test that _store_event creates parent directories if needed."""
+        event = MockEvent()
+        nested_path = temp_dir / 'a' / 'b' / 'c' / f'{event.id}.json'
+
+        assert not nested_path.parent.exists()
+
+        service._store_event(nested_path, event)
+
+        assert nested_path.exists()
+        assert nested_path.parent.exists()
+
+    def test_store_event_overwrites_existing(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test that _store_event overwrites an existing file."""
+        event1 = MockEvent(kind='original')
+        event2 = MockEvent(id=event1.id, kind='updated')
+        test_path = temp_dir / f'{event1.id}.json'
+
+        service._store_event(test_path, event1)
+        content1 = test_path.read_text()
+        assert 'original' in content1
+
+        service._store_event(test_path, event2)
+        content2 = test_path.read_text()
+        assert 'updated' in content2
+        assert 'original' not in content2
+
+    def test_store_event_json_format(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test that stored event is valid JSON."""
+        event = MockEvent(kind='json_test')
+        test_path = temp_dir / 'json_test.json'
+
+        service._store_event(test_path, event)
+
+        content = test_path.read_text()
+        parsed = json.loads(content)
+        assert parsed['kind'] == 'json_test'
+        assert 'id' in parsed
+        assert 'timestamp' in parsed
+
+
+class TestFilesystemEventServiceSearchPaths:
+    """Test cases for _search_paths method."""
+
+    def test_search_paths_empty_directory(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test searching in an empty directory."""
+        search_prefix = temp_dir / 'events'
+        search_prefix.mkdir(parents=True)
+
+        result = service._search_paths(search_prefix / 'nonexistent')
+        assert result == []
+
+    def test_search_paths_with_glob_pattern(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test _search_paths with files matching glob pattern."""
+        (temp_dir / 'event1.json').write_text('{}')
+        (temp_dir / 'event2.json').write_text('{}')
+        (temp_dir / 'other.txt').write_text('not json')
+
+        result = service._search_paths(temp_dir / 'event')
+
+        assert len(result) == 2
+        assert all('event' in str(p) for p in result)
+
+    def test_search_paths_no_matches(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test _search_paths with no matching files."""
+        (temp_dir / 'file1.json').write_text('{}')
+        (temp_dir / 'file2.json').write_text('{}')
+
+        result = service._search_paths(temp_dir / 'nonexistent')
+
+        assert result == []
+
+    def test_search_paths_returns_path_objects(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test that _search_paths returns Path objects."""
+        (temp_dir / 'test1.json').write_text('{}')
+
+        result = service._search_paths(temp_dir / 'test')
+
+        assert len(result) == 1
+        assert isinstance(result[0], Path)
+
+
+class TestFilesystemEventServiceIntegration:
+    """Integration tests for MockFilesystemEventService."""
+
+    @pytest.mark.asyncio
+    async def test_get_conversation_path_with_user_id(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test conversation path generation with user_id."""
+        conversation_id = uuid4()
+
+        path = await service.get_conversation_path(conversation_id)
+
+        assert str(temp_dir) in str(path)
+        assert 'test_user' in str(path)
+        assert 'v1_conversations' in str(path)
+        assert conversation_id.hex in str(path)
+
+    @pytest.mark.asyncio
+    async def test_get_conversation_path_without_user_id(
+        self, service_no_user: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test conversation path generation without user_id."""
+        conversation_id = uuid4()
+
+        path = await service_no_user.get_conversation_path(conversation_id)
+
+        assert str(temp_dir) in str(path)
+        assert 'test_user' not in str(path)
+        assert 'v1_conversations' in str(path)
+        assert conversation_id.hex in str(path)
+
+    @pytest.mark.asyncio
+    async def test_save_and_get_event(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test saving and retrieving an event."""
+        conversation_id = uuid4()
+        event = MockEvent()
+
+        await service.save_event(conversation_id, event)
+
+        conversation_path = await service.get_conversation_path(conversation_id)
+        event_file = conversation_path / f'{event.id.hex}.json'
+        assert event_file.exists()
+
+        retrieved = await service.get_event(conversation_id, event.id)
+        assert retrieved is not None
+        assert str(retrieved.id) == str(event.id)
+
+    @pytest.mark.asyncio
+    async def test_get_nonexistent_event(self, service: MockFilesystemEventService):
+        """Test getting an event that doesn't exist."""
+        conversation_id = uuid4()
+        event_id = uuid4()
+
+        result = await service.get_event(conversation_id, event_id)
+        assert result is None
+
+    @pytest.mark.asyncio
+    async def test_save_multiple_events(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test saving multiple events to the same conversation."""
+        conversation_id = uuid4()
+        events = [MockEvent(kind=f'event_{i}') for i in range(3)]
+
+        for event in events:
+            await service.save_event(conversation_id, event)
+
+        for event in events:
+            retrieved = await service.get_event(conversation_id, event.id)
+            assert retrieved is not None
+            assert retrieved.kind == event.kind
+
+
+class TestFilesystemEventServiceEdgeCases:
+    """Edge case tests for MockFilesystemEventService."""
+
+    def test_load_event_with_unicode_content(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test loading an event with unicode characters."""
+        event_data = {
+            'id': str(uuid4()),
+            'kind': 'unicode_event_你好_🌍',
+            'timestamp': datetime.now(timezone.utc).isoformat(),
+        }
+
+        test_file = temp_dir / 'unicode.json'
+        test_file.write_text(json.dumps(event_data, ensure_ascii=False))
+
+        result = service._load_event(test_file)
+        assert result is not None
+        assert '你好' in result.kind
+        assert '🌍' in result.kind
+
+    def test_store_event_with_special_characters_in_path(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test storing an event in a path with spaces."""
+        event = MockEvent()
+        test_path = temp_dir / 'path with spaces' / f'{event.id}.json'
+
+        service._store_event(test_path, event)
+        assert test_path.exists()
+
+    @pytest.mark.asyncio
+    async def test_save_event_with_string_id(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test saving an event where id is a string (not UUID)."""
+        conversation_id = uuid4()
+        string_id = str(uuid4())
+        event = MockEvent(id=string_id)
+
+        await service.save_event(conversation_id, event)
+
+        conversation_path = await service.get_conversation_path(conversation_id)
+        id_hex = string_id.replace('-', '')
+        event_file = conversation_path / f'{id_hex}.json'
+        assert event_file.exists()
+
+    def test_load_event_preserves_timestamp(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test that loading an event preserves the timestamp."""
+        original_timestamp = datetime(2024, 1, 15, 10, 30, 0, tzinfo=timezone.utc)
+        event_data = {
+            'id': str(uuid4()),
+            'kind': 'timestamp_test',
+            'timestamp': original_timestamp.isoformat(),
+        }
+
+        test_file = temp_dir / 'timestamp.json'
+        test_file.write_text(json.dumps(event_data))
+
+        result = service._load_event(test_file)
+        assert result is not None
+        assert result.timestamp == original_timestamp
+
+
+class TestFilesystemEventServiceLimit:
+    """Test cases for the limit attribute."""
+
+    def test_default_limit(self, temp_dir: Path):
+        """Test that default limit is 500."""
+        service = MockFilesystemEventService(
+            prefix=temp_dir,
+            user_id='test_user',
+            app_conversation_info_service=None,
+            app_conversation_info_load_tasks={},
+        )
+        assert service.limit == 500
+
+    def test_custom_limit(self, temp_dir: Path):
+        """Test setting a custom limit."""
+        service = MockFilesystemEventService(
+            prefix=temp_dir,
+            user_id='test_user',
+            app_conversation_info_service=None,
+            app_conversation_info_load_tasks={},
+            limit=100,
+        )
+        assert service.limit == 100
+
+
+class TestReadTextBugFix:
+    """Tests specifically for the read_text() bug fix.
+
+    The original bug was: path.read_text(str(path))
+    Which incorrectly passed the path string as the 'encoding' parameter.
+    The fix is: path.read_text()
+    """
+
+    def test_read_text_no_argument(self, temp_dir: Path):
+        """Verify that Path.read_text() works correctly without arguments."""
+        test_file = temp_dir / 'test.txt'
+        test_content = 'Hello, World!'
+        test_file.write_text(test_content)
+
+        # This is the correct way (after the fix)
+        result = test_file.read_text()
+        assert result == test_content
+
+    def test_read_text_with_path_as_encoding_fails(self, temp_dir: Path):
+        """Demonstrate that the bug would cause a LookupError."""
+        test_file = temp_dir / 'test.txt'
+        test_content = 'Hello, World!'
+        test_file.write_text(test_content)
+
+        # This is what the bug was doing - passing path as encoding
+        with pytest.raises(LookupError):
+            test_file.read_text(str(test_file))
+
+    def test_load_event_uses_correct_read_text(
+        self, service: MockFilesystemEventService, temp_dir: Path
+    ):
+        """Test that _load_event correctly reads files.
+
+        This test verifies the fix works end-to-end.
+        """
+        event_data = {
+            'id': str(uuid4()),
+            'kind': 'read_test',
+            'timestamp': datetime.now(timezone.utc).isoformat(),
+        }
+
+        test_file = temp_dir / 'read_test.json'
+        test_file.write_text(json.dumps(event_data))
+
+        # If the bug existed, this would raise LookupError
+        result = service._load_event(test_file)
+        assert result is not None
+        assert result.kind == 'read_test'