From 220914879bc11cac9b640ec7604b784c6ccce6db Mon Sep 17 00:00:00 2001 From: phernandez Date: Wed, 22 Jan 2025 15:30:57 -0600 Subject: [PATCH] delete relations/observations from search_index on delete entity during sync --- src/basic_memory/models/knowledge.py | 36 +++++++--- .../repository/entity_repository.py | 2 +- src/basic_memory/services/search_service.py | 17 +---- src/basic_memory/sync/sync_service.py | 39 ++++++++--- tests/sync/test_sync_service.py | 66 +++++++++++++++---- 5 files changed, 114 insertions(+), 46 deletions(-) diff --git a/src/basic_memory/models/knowledge.py b/src/basic_memory/models/knowledge.py index 6dbdae5d..64152c1d 100644 --- a/src/basic_memory/models/knowledge.py +++ b/src/basic_memory/models/knowledge.py @@ -1,10 +1,8 @@ """Knowledge graph models.""" import re -import os from datetime import datetime from typing import Optional -from unidecode import unidecode from sqlalchemy import ( Integer, @@ -22,7 +20,7 @@ from sqlalchemy.orm import Mapped, mapped_column, relationship, validates from basic_memory.models.base import Base from enum import Enum - +from basic_memory.utils import generate_permalink class Entity(Base): @@ -89,10 +87,10 @@ class Entity(Base): def relations(self): return self.incoming_relations + self.outgoing_relations - @validates('permalink') + @validates("permalink") def validate_permalink(self, key, value): """Validate permalink format. - + Requirements: 1. Must be valid URI path component 2. Only lowercase letters, numbers, and hyphens (no underscores) @@ -101,8 +99,8 @@ class Entity(Base): """ if not value: raise ValueError("Permalink must not be None") - - if not re.match(r'^[a-z0-9][a-z0-9\-/]*[a-z0-9]$', value): + + if not re.match(r"^[a-z0-9][a-z0-9\-/]*[a-z0-9]$", value): raise ValueError( f"Invalid permalink format: {value}. " "Use only lowercase letters, numbers, and hyphens." @@ -159,6 +157,17 @@ class Observation(Base): # Relationships entity = relationship("Entity", back_populates="observations") + @property + def permalink(self) -> str: + """ + Create synthetic permalink for the observation + We can construct these because observations are always + defined in and owned by a single entity + """ + return generate_permalink( + f"{self.entity.permalink}/observations/{self.category}/{self.content}" + ) + def __repr__(self) -> str: return f"Observation(id={self.id}, entity_id={self.entity_id}, content='{self.content}')" @@ -194,5 +203,16 @@ class Relation(Base): ) to_entity = relationship("Entity", foreign_keys=[to_id], back_populates="incoming_relations") + @property + def permalink(self) -> str: + """Create relation permalink showing the semantic connection: + source/relation_type/target + e.g., "specs/search/implements/features/search-ui" + """ + + return generate_permalink( + f"{self.from_entity.permalink}/{self.relation_type}/{self.to_entity.permalink}" + ) + def __repr__(self) -> str: - return f"Relation(id={self.id}, from_id={self.from_id}, to_id={self.to_id}, type='{self.relation_type}')" \ No newline at end of file + return f"Relation(id={self.id}, from_id={self.from_id}, to_id={self.to_id}, type='{self.relation_type}')" diff --git a/src/basic_memory/repository/entity_repository.py b/src/basic_memory/repository/entity_repository.py index 8fb13077..f1582b53 100644 --- a/src/basic_memory/repository/entity_repository.py +++ b/src/basic_memory/repository/entity_repository.py @@ -116,7 +116,7 @@ class EntityRepository(Repository[Entity]): def get_load_options(self) -> List[LoaderOption]: return [ - selectinload(Entity.observations), + selectinload(Entity.observations).selectinload(Observation.entity), # Load from_relations and both entities for each relation selectinload(Entity.outgoing_relations).selectinload(Relation.from_entity), selectinload(Entity.outgoing_relations).selectinload(Relation.to_entity), diff --git a/src/basic_memory/services/search_service.py b/src/basic_memory/services/search_service.py index a3f34680..d5b87dbe 100644 --- a/src/basic_memory/services/search_service.py +++ b/src/basic_memory/services/search_service.py @@ -152,12 +152,6 @@ class SearchService: # Index each observation with synthetic permalink for obs in entity.observations: - # Create synthetic permalink for the observation - # We can construct these because observations are always - # defined in and owned by a single entity - observation_permalink = ( - generate_permalink(f"{entity.permalink}/observations/{obs.category}/{obs.content}") - ) # Index with parent entity's file path since that's where it's defined await self._do_index( @@ -166,7 +160,7 @@ class SearchService: type=SearchItemType.OBSERVATION.value, title=f"{obs.category}: {obs.content[:50]}...", content=obs.content, - permalink=observation_permalink, + permalink=obs.permalink, file_path=entity.file_path, category=obs.category, entity_id=entity.id, @@ -182,13 +176,6 @@ class SearchService: # Only index outgoing relations (ones defined in this file) for rel in entity.outgoing_relations: - # Create relation permalink showing the semantic connection: - # source/relation_type/target - # e.g., "specs/search/implements/features/search-ui" - relation_permalink = ( - generate_permalink(f"{rel.from_entity.permalink}/{rel.relation_type}/{rel.to_entity.permalink}") - ) - # Create descriptive title showing the relationship relation_title = f"{rel.from_entity.title} → {rel.to_entity.title}" @@ -197,7 +184,7 @@ class SearchService: id=rel.id, title=relation_title, content=rel.context or "", - permalink=relation_permalink, + permalink=rel.permalink, file_path=entity.file_path, type=SearchItemType.RELATION.value, from_id=rel.from_id, diff --git a/src/basic_memory/sync/sync_service.py b/src/basic_memory/sync/sync_service.py index 03337e9f..45fa4a49 100644 --- a/src/basic_memory/sync/sync_service.py +++ b/src/basic_memory/sync/sync_service.py @@ -35,6 +35,29 @@ class SyncService: self.entity_repository = entity_repository self.search_service = search_service + async def handle_entity_deletion(self, file_path: str): + """Handle complete entity deletion including search index cleanup.""" + # First get entity to get permalink before deletion + entity = await self.entity_repository.get_by_file_path(file_path) + if entity: + logger.debug(f"Deleting entity and cleaning up search index: {file_path}") + + # Delete from db (this cascades to observations/relations) + await self.entity_sync_service.delete_entity_by_file_path(file_path) + + # Clean up search index + permalinks = ( + [entity.permalink] + + [o.permalink for o in entity.observations] + + [r.permalink for r in entity.relations] + ) + logger.debug(f"Deleting from search index: {permalinks}") + for permalink in permalinks: + await self.search_service.delete_by_permalink(permalink) + + else: + logger.debug(f"No entity found to delete: {file_path}") + async def sync(self, directory: Path) -> SyncReport: """Sync knowledge files with database.""" changes = await self.scanner.find_knowledge_changes(directory) @@ -47,15 +70,13 @@ class SyncService: if entity: # Update file_path but keep the same permalink for link stability await self.entity_repository.update( - entity.id, - {"file_path": new_path, "checksum": changes.checksums[new_path]} + entity.id, {"file_path": new_path, "checksum": changes.checksums[new_path]} ) - + # Handle deletions next # remove rows from db for files no longer present for file_path in changes.deleted: - logger.debug(f"Deleting entity from db: {file_path}") - await self.entity_sync_service.delete_entity_by_file_path(file_path) + await self.handle_entity_deletion(file_path) # Parse files that need updating parsed_entities: Dict[str, EntityMarkdown] = {} @@ -83,11 +104,13 @@ class SyncService: # Second pass for file_path, entity_markdown in parsed_entities.items(): logger.debug(f"Updating relations for: {file_path}") - + # Process relations checksum = changes.checksums[file_path] - entity = await self.entity_sync_service.update_entity_relations(file_path, entity_markdown) - + entity = await self.entity_sync_service.update_entity_relations( + file_path, entity_markdown + ) + # add to search index await self.search_service.index_entity(entity) diff --git a/tests/sync/test_sync_service.py b/tests/sync/test_sync_service.py index c192c8d4..cd664548 100644 --- a/tests/sync/test_sync_service.py +++ b/tests/sync/test_sync_service.py @@ -7,7 +7,10 @@ import pytest from basic_memory.config import ProjectConfig from basic_memory.models import Entity -from basic_memory.services import EntityService +from basic_memory.repository import EntityRepository +from basic_memory.schemas.search import SearchQuery +from basic_memory.services import EntityService, ObservationService +from basic_memory.services.search_service import SearchService from basic_memory.sync.sync_service import SyncService @@ -420,21 +423,24 @@ modified: 2024-01-01 assert doc is not None # File should have a checksum, even if it's from either version assert doc.checksum is not None - + + @pytest.mark.asyncio -async def test_permalink_formatting(sync_service: SyncService, test_config: ProjectConfig, entity_service: EntityService): +async def test_permalink_formatting( + sync_service: SyncService, test_config: ProjectConfig, entity_service: EntityService +): """Test that permalinks are properly formatted during sync.""" - + # Test cases with different filename formats test_files = { - # filename -> expected permalink - "my_awesome_feature.md": "my-awesome-feature", - "MIXED_CASE_NAME.md": "mixed-case-name", - "spaces and_underscores.md": "spaces-and-underscores", - "design/model_refactor.md": "design/model-refactor", - "test/multiple_word_directory/feature_name.md": "test/multiple-word-directory/feature-name", + # filename -> expected permalink + "my_awesome_feature.md": "my-awesome-feature", + "MIXED_CASE_NAME.md": "mixed-case-name", + "spaces and_underscores.md": "spaces-and-underscores", + "design/model_refactor.md": "design/model-refactor", + "test/multiple_word_directory/feature_name.md": "test/multiple-word-directory/feature-name", } - + # Create test files for filename, _ in test_files.items(): content: str = """ @@ -448,16 +454,48 @@ modified: 2024-01-01 Testing permalink generation. """ await create_test_file(test_config.home / filename, content) - + # Run sync await sync_service.sync(test_config.home) - + # Verify permalinks entities = await entity_service.repository.find_all() for filename, expected_permalink in test_files.items(): # Find entity for this file entity = next(e for e in entities if e.file_path == filename) - assert entity.permalink == expected_permalink, f"File {filename} should have permalink {expected_permalink}" + assert ( + entity.permalink == expected_permalink + ), f"File {filename} should have permalink {expected_permalink}" + + +@pytest.mark.asyncio +async def test_handle_entity_deletion( + test_graph, + sync_service: SyncService, + test_config: ProjectConfig, + entity_repository: EntityRepository, + observation_service: ObservationService, + search_service: SearchService, +): + """Test deletion of entity cleans up search index.""" + + root_entity = test_graph["root"] + # Delete the entity + await sync_service.handle_entity_deletion(root_entity.file_path) + + # Verify entity is gone from db + assert await entity_repository.get_by_permalink(root_entity.permalink) is None + + # Verify entity is gone from search index + entity_results = await search_service.search(SearchQuery(text=root_entity.title)) + assert len(entity_results) == 0 + + obs_results = await search_service.search(SearchQuery(text="Root note 1")) + assert len(obs_results) == 0 + + rel_results = await search_service.search(SearchQuery(text="connects_to")) + assert len(rel_results) == 0 + @pytest.mark.asyncio async def test_sync_null_checksum_cleanup(