mirror of
https://github.com/basicmachines-co/basic-memory
synced 2026-06-21 13:47:35 +00:00
fix: fix permalink uniqueness violations on create/update/sync
This commit is contained in:
@@ -181,6 +181,9 @@ class Entity(BaseModel):
|
||||
- Optional relations to other entities
|
||||
- Optional description for high-level overview
|
||||
"""
|
||||
|
||||
# private field to override permalink
|
||||
_permalink: Optional[str] = None
|
||||
|
||||
title: str
|
||||
content: Optional[str] = None
|
||||
@@ -199,8 +202,8 @@ class Entity(BaseModel):
|
||||
|
||||
@property
|
||||
def permalink(self) -> PathId:
|
||||
"""Get the path ID in format {snake_case_title}."""
|
||||
return generate_permalink(self.file_path)
|
||||
"""Get a url friendly path}."""
|
||||
return self._permalink or generate_permalink(self.file_path)
|
||||
|
||||
@model_validator(mode="after")
|
||||
@classmethod
|
||||
|
||||
@@ -19,6 +19,7 @@ from basic_memory.services import FileService
|
||||
from basic_memory.services import BaseService
|
||||
from basic_memory.services.link_resolver import LinkResolver
|
||||
from basic_memory.markdown.entity_parser import EntityParser
|
||||
from basic_memory.utils import generate_permalink
|
||||
|
||||
|
||||
class EntityService(BaseService[EntityModel]):
|
||||
@@ -40,6 +41,40 @@ class EntityService(BaseService[EntityModel]):
|
||||
self.file_service = file_service
|
||||
self.link_resolver = link_resolver
|
||||
|
||||
async def resolve_permalink(
|
||||
self,
|
||||
file_path: Path,
|
||||
markdown: Optional[EntityMarkdown] = None
|
||||
) -> str:
|
||||
"""Get or generate unique permalink for an entity.
|
||||
|
||||
Priority:
|
||||
1. Use explicit permalink from markdown frontmatter if present
|
||||
2. For existing files, keep current permalink
|
||||
3. Generate new unique permalink for new files
|
||||
"""
|
||||
# If markdown has explicit permalink, try to use it
|
||||
if markdown and markdown.frontmatter.permalink:
|
||||
desired_permalink = markdown.frontmatter.permalink
|
||||
else:
|
||||
# For existing files, try to find current permalink
|
||||
existing = await self.repository.get_by_file_path(str(file_path))
|
||||
if existing:
|
||||
return existing.permalink
|
||||
|
||||
# New file - generate permalink
|
||||
desired_permalink = generate_permalink(file_path)
|
||||
|
||||
# Make unique if needed
|
||||
permalink = desired_permalink
|
||||
suffix = 1
|
||||
while await self.repository.get_by_permalink(permalink):
|
||||
permalink = f"{desired_permalink}-{suffix}"
|
||||
suffix += 1
|
||||
logger.debug(f"creating unique permalink: {permalink}")
|
||||
|
||||
return permalink
|
||||
|
||||
async def create_or_update_entity(self, schema: EntitySchema) -> (EntityModel, bool):
|
||||
"""Create new entity or update existing one.
|
||||
if a new entity is created, the return value is (entity, True)
|
||||
@@ -66,9 +101,13 @@ class EntityService(BaseService[EntityModel]):
|
||||
|
||||
if await self.file_service.exists(file_path):
|
||||
raise EntityCreationError(
|
||||
f"file_path {file_path} for entity {schema.permalink} already exists: {file_path}"
|
||||
f"file for entity {schema.folder}/{schema.title} already exists: {file_path}"
|
||||
)
|
||||
|
||||
# Get unique permalink
|
||||
permalink = await self.resolve_permalink(schema.permalink or file_path)
|
||||
schema._permalink = permalink
|
||||
|
||||
post = await schema_to_markdown(schema)
|
||||
|
||||
# write file
|
||||
@@ -184,7 +223,7 @@ class EntityService(BaseService[EntityModel]):
|
||||
Creates the entity with null checksum to indicate sync not complete.
|
||||
Relations will be added in second pass.
|
||||
"""
|
||||
logger.debug(f"Creating entity: {markdown.frontmatter.title}")
|
||||
logger.debug(f"Creating entity: {markdown.frontmatter.title}")
|
||||
model = entity_model_from_markdown(file_path, markdown)
|
||||
|
||||
# Mark as incomplete sync
|
||||
|
||||
@@ -35,7 +35,6 @@ class FileService:
|
||||
"""Generate absolute filesystem path for entity."""
|
||||
return self.base_path / f"{entity.file_path}"
|
||||
|
||||
# TODO move to tests
|
||||
async def write_entity_file(
|
||||
self,
|
||||
entity: EntityModel,
|
||||
|
||||
@@ -92,8 +92,28 @@ class SyncService:
|
||||
# First pass: Create/update entities
|
||||
# entities will have a null checksum to indicate they are not complete
|
||||
for file_path, entity_markdown in parsed_entities.items():
|
||||
|
||||
# Get unique permalink and update markdown if needed
|
||||
permalink = await self.entity_service.resolve_permalink(
|
||||
file_path,
|
||||
markdown=entity_markdown
|
||||
)
|
||||
|
||||
if permalink != entity_markdown.frontmatter.permalink:
|
||||
# Permalink changed - update markdown and rewrite file
|
||||
entity_markdown.frontmatter.metadata["permalink"] = permalink
|
||||
|
||||
# update file
|
||||
logger.info(f"Adding permalink '{permalink}' to file: {file_path}")
|
||||
updated_checksum = await self.entity_service.file_service.markdown_processor.write_file(
|
||||
directory / file_path, entity_markdown)
|
||||
|
||||
# Update checksum in changes report since file was modified
|
||||
changes.checksums[file_path] = updated_checksum
|
||||
|
||||
# if the file is new, create an entity
|
||||
if file_path in changes.new:
|
||||
# Create entity with final permalink
|
||||
logger.debug(f"Creating new entity_markdown: {file_path}")
|
||||
await self.entity_service.create_entity_from_markdown(
|
||||
file_path, entity_markdown
|
||||
|
||||
@@ -6,11 +6,12 @@ import pytest
|
||||
import yaml
|
||||
|
||||
from basic_memory.models import Entity as EntityModel
|
||||
from basic_memory.repository import EntityRepository
|
||||
from basic_memory.schemas import Entity as EntitySchema
|
||||
from basic_memory.services import FileService
|
||||
from basic_memory.services.entity_service import EntityService
|
||||
from basic_memory.services.exceptions import EntityNotFoundError
|
||||
|
||||
from basic_memory.utils import generate_permalink
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -51,7 +52,36 @@ async def test_create_entity(entity_service: EntityService, file_service: FileSe
|
||||
assert metadata["permalink"] == entity.permalink
|
||||
assert metadata["type"] == entity.entity_type
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_entity_unique_permalink(test_config, entity_service: EntityService, file_service: FileService, entity_repository: EntityRepository):
|
||||
"""Test successful entity creation."""
|
||||
entity_data = EntitySchema(
|
||||
title="Test Entity",
|
||||
folder="test",
|
||||
entity_type="test",
|
||||
)
|
||||
|
||||
entity = await entity_service.create_entity(entity_data)
|
||||
|
||||
# default permalink
|
||||
assert entity.permalink == generate_permalink(entity.file_path)
|
||||
|
||||
# move file
|
||||
file_path = file_service.get_entity_path(entity)
|
||||
file_path.rename(test_config.home / "new_path.md")
|
||||
await entity_repository.update(entity.id, {"file_path": "new_path.md"})
|
||||
|
||||
# create again
|
||||
entity2 = await entity_service.create_entity(entity_data)
|
||||
assert entity2.permalink == f"{entity.permalink}-1"
|
||||
|
||||
file_path = file_service.get_entity_path(entity2)
|
||||
file_content, _ = await file_service.read_file(file_path)
|
||||
_, frontmatter, doc_content = file_content.split("---", 2)
|
||||
metadata = yaml.safe_load(frontmatter)
|
||||
|
||||
# Verify frontmatter contents
|
||||
assert metadata["permalink"] == entity2.permalink
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_by_permalink(entity_service: EntityService):
|
||||
@@ -440,4 +470,3 @@ See the [[Git Cheat Sheet]] for reference.
|
||||
|
||||
|
||||
|
||||
# TODO handle permalink conflicts
|
||||
|
||||
@@ -10,7 +10,7 @@ from basic_memory.config import ProjectConfig
|
||||
from basic_memory.models import Entity
|
||||
from basic_memory.repository import EntityRepository
|
||||
from basic_memory.schemas.search import SearchQuery
|
||||
from basic_memory.services import EntityService
|
||||
from basic_memory.services import EntityService, FileService
|
||||
from basic_memory.services.search_service import SearchService
|
||||
from basic_memory.sync.sync_service import SyncService
|
||||
|
||||
@@ -557,9 +557,6 @@ async def test_handle_entity_deletion(
|
||||
rel_results = await search_service.search(SearchQuery(text="connects_to"))
|
||||
assert len(rel_results) == 0
|
||||
|
||||
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_preserves_timestamps(
|
||||
sync_service: SyncService,
|
||||
@@ -681,3 +678,104 @@ modified: 2024-01-01
|
||||
# Verify entity was properly synced
|
||||
updated = await entity_service.get_by_permalink("concept/incomplete")
|
||||
assert updated.checksum is not None
|
||||
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_permalink_resolved(
|
||||
sync_service: SyncService,
|
||||
test_config: ProjectConfig,
|
||||
file_service: FileService,
|
||||
):
|
||||
"""Test that we resolve duplicate permalinks on sync ."""
|
||||
project_dir = test_config.home
|
||||
|
||||
# Create initial file
|
||||
content = """
|
||||
---
|
||||
type: knowledge
|
||||
---
|
||||
# Test Move
|
||||
Content for move test
|
||||
"""
|
||||
old_path = project_dir / "old" / "test_move.md"
|
||||
old_path.parent.mkdir(parents=True)
|
||||
await create_test_file(old_path, content)
|
||||
|
||||
# Initial sync
|
||||
await sync_service.sync(test_config.home)
|
||||
|
||||
# Move the file
|
||||
new_path = project_dir / "new" / "moved_file.md"
|
||||
new_path.parent.mkdir(parents=True)
|
||||
old_path.rename(new_path)
|
||||
|
||||
# Sync again
|
||||
await sync_service.sync(test_config.home)
|
||||
|
||||
file_content, _ = await file_service.read_file(new_path)
|
||||
assert "permalink: old/test-move" in file_content
|
||||
|
||||
# Create another that has the same permalink
|
||||
content = """
|
||||
---
|
||||
type: knowledge
|
||||
---
|
||||
# Test Move
|
||||
Content for move test
|
||||
"""
|
||||
old_path = project_dir / "old" / "test_move.md"
|
||||
old_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
await create_test_file(old_path, content)
|
||||
|
||||
# Sync new file
|
||||
await sync_service.sync(test_config.home)
|
||||
|
||||
# assert permalink is unique
|
||||
file_content, _ = await file_service.read_file(old_path)
|
||||
assert "permalink: old/test-move-1" in file_content
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_sync_permalink_resolved_on_update(
|
||||
sync_service: SyncService,
|
||||
test_config: ProjectConfig,
|
||||
file_service: FileService,
|
||||
):
|
||||
"""Test that sync resolves permalink conflicts on update."""
|
||||
project_dir = test_config.home
|
||||
|
||||
one_file = project_dir / "one.md"
|
||||
two_file = project_dir / "two.md"
|
||||
await create_test_file(one_file)
|
||||
await create_test_file(two_file)
|
||||
|
||||
# Run sync
|
||||
await sync_service.sync(test_config.home)
|
||||
|
||||
# Check permalinks
|
||||
file_one_content, _ = await file_service.read_file(one_file)
|
||||
assert "permalink: one" in file_one_content
|
||||
|
||||
file_two_content, _ = await file_service.read_file(two_file)
|
||||
assert "permalink: two" in file_two_content
|
||||
|
||||
# update the second file with a duplicate permalink
|
||||
updated_content = """
|
||||
---
|
||||
title: two.md
|
||||
type: note
|
||||
permalink: one
|
||||
tags: []
|
||||
---
|
||||
|
||||
test content
|
||||
"""
|
||||
two_file.write_text(updated_content)
|
||||
|
||||
# Run sync
|
||||
await sync_service.sync(test_config.home)
|
||||
|
||||
# Check permalinks
|
||||
file_one_content, _ = await file_service.read_file(two_file)
|
||||
assert "permalink: one-1" in file_one_content
|
||||
|
||||
Reference in New Issue
Block a user