From 6361574a20ff173ef8c39af7444e236cea3fbc1b Mon Sep 17 00:00:00 2001 From: jope-bm Date: Mon, 28 Jul 2025 14:50:47 -0600 Subject: [PATCH] fix: basic memory home env var not respected when project path is changed. (#239) Signed-off-by: Joe P --- .../api/routers/project_router.py | 34 ++- src/basic_memory/cli/commands/project.py | 37 +++ src/basic_memory/config.py | 6 +- .../repository/project_repository.py | 18 ++ src/basic_memory/services/project_service.py | 41 +++ src/basic_memory/utils.py | 26 +- tests/api/test_project_router.py | 233 ++++++++++++++++++ tests/cli/test_project_commands.py | 38 +++ tests/repository/test_project_repository.py | 28 +++ tests/services/test_project_service.py | 120 +++++++++ 10 files changed, 551 insertions(+), 30 deletions(-) diff --git a/src/basic_memory/api/routers/project_router.py b/src/basic_memory/api/routers/project_router.py index 4e555b64..bbfc11c3 100644 --- a/src/basic_memory/api/routers/project_router.py +++ b/src/basic_memory/api/routers/project_router.py @@ -1,5 +1,6 @@ """Router for project management.""" +import os from fastapi import APIRouter, HTTPException, Path, Body from typing import Optional @@ -32,40 +33,47 @@ async def get_project_info( @project_router.patch("/{name}", response_model=ProjectStatusResponse) async def update_project( project_service: ProjectServiceDep, - project_name: str = Path(..., description="Name of the project to update"), - path: Optional[str] = Body(None, description="New path for the project"), + name: str = Path(..., description="Name of the project to update"), + path: Optional[str] = Body(None, description="New absolute path for the project"), is_active: Optional[bool] = Body(None, description="Status of the project (active/inactive)"), ) -> ProjectStatusResponse: """Update a project's information in configuration and database. Args: - project_name: The name of the project to update - path: Optional new path for the project + name: The name of the project to update + path: Optional new absolute path for the project is_active: Optional status update for the project Returns: Response confirming the project was updated """ - try: # pragma: no cover + try: + # Validate that path is absolute if provided + if path and not os.path.isabs(path): + raise HTTPException(status_code=400, detail="Path must be absolute") + # Get original project info for the response old_project_info = ProjectItem( - name=project_name, - path=project_service.projects.get(project_name, ""), + name=name, + path=project_service.projects.get(name, ""), ) - await project_service.update_project(project_name, updated_path=path, is_active=is_active) + if path: + await project_service.move_project(name, path) + elif is_active is not None: + await project_service.update_project(name, is_active=is_active) # Get updated project info - updated_path = path if path else project_service.projects.get(project_name, "") + updated_path = path if path else project_service.projects.get(name, "") return ProjectStatusResponse( - message=f"Project '{project_name}' updated successfully", + message=f"Project '{name}' updated successfully", status="success", - default=(project_name == project_service.default_project), + default=(name == project_service.default_project), old_project=old_project_info, - new_project=ProjectItem(name=project_name, path=updated_path), + new_project=ProjectItem(name=name, path=updated_path), ) - except ValueError as e: # pragma: no cover + except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) diff --git a/src/basic_memory/cli/commands/project.py b/src/basic_memory/cli/commands/project.py index c9ab726a..3ef02c47 100644 --- a/src/basic_memory/cli/commands/project.py +++ b/src/basic_memory/cli/commands/project.py @@ -23,6 +23,7 @@ from basic_memory.mcp.tools.utils import call_post from basic_memory.schemas.project_info import ProjectStatusResponse from basic_memory.mcp.tools.utils import call_delete from basic_memory.mcp.tools.utils import call_put +from basic_memory.mcp.tools.utils import call_patch from basic_memory.utils import generate_permalink console = Console() @@ -148,6 +149,42 @@ def synchronize_projects() -> None: raise typer.Exit(1) +@project_app.command("move") +def move_project( + name: str = typer.Argument(..., help="Name of the project to move"), + new_path: str = typer.Argument(..., help="New absolute path for the project"), +) -> None: + """Move a project to a new location.""" + # Resolve to absolute path + resolved_path = os.path.abspath(os.path.expanduser(new_path)) + + try: + data = {"path": resolved_path} + project_name = generate_permalink(name) + + current_project = session.get_current_project() + response = asyncio.run(call_patch(client, f"/{current_project}/project/{project_name}", json=data)) + result = ProjectStatusResponse.model_validate(response.json()) + + console.print(f"[green]{result.message}[/green]") + + # Show important file movement reminder + console.print() # Empty line for spacing + console.print(Panel( + "[bold red]IMPORTANT:[/bold red] Project configuration updated successfully.\n\n" + "[yellow]You must manually move your project files from the old location to:[/yellow]\n" + f"[cyan]{resolved_path}[/cyan]\n\n" + "[dim]Basic Memory has only updated the configuration - your files remain in their original location.[/dim]", + title="⚠️ Manual File Movement Required", + border_style="yellow", + expand=False + )) + + except Exception as e: + console.print(f"[red]Error moving project: {str(e)}[/red]") + raise typer.Exit(1) + + @project_app.command("info") def display_project_info( json_output: bool = typer.Option(False, "--json", help="Output in JSON format"), diff --git a/src/basic_memory/config.py b/src/basic_memory/config.py index 998c7c6b..8e67eb1e 100644 --- a/src/basic_memory/config.py +++ b/src/basic_memory/config.py @@ -182,10 +182,8 @@ class ConfigManager: data = json.loads(self.config_file.read_text(encoding="utf-8")) return BasicMemoryConfig(**data) except Exception as e: # pragma: no cover - logger.error(f"Failed to load config: {e}") - config = BasicMemoryConfig() - self.save_config(config) - return config + logger.exception(f"Failed to load config: {e}") + raise e else: config = BasicMemoryConfig() self.save_config(config) diff --git a/src/basic_memory/repository/project_repository.py b/src/basic_memory/repository/project_repository.py index 99383ad3..630510f5 100644 --- a/src/basic_memory/repository/project_repository.py +++ b/src/basic_memory/repository/project_repository.py @@ -83,3 +83,21 @@ class ProjectRepository(Repository[Project]): await session.flush() return target_project return None # pragma: no cover + + async def update_path(self, project_id: int, new_path: str) -> Optional[Project]: + """Update project path. + + Args: + project_id: ID of the project to update + new_path: New filesystem path for the project + + Returns: + The updated project if found, None otherwise + """ + async with db.scoped_session(self.session_maker) as session: + project = await self.select_by_id(session, project_id) + if project: + project.path = new_path + await session.flush() + return project + return None diff --git a/src/basic_memory/services/project_service.py b/src/basic_memory/services/project_service.py index 85741bca..48518df1 100644 --- a/src/basic_memory/services/project_service.py +++ b/src/basic_memory/services/project_service.py @@ -309,6 +309,47 @@ class ProjectService: # MCP components might not be available in all contexts logger.debug("MCP session not available, skipping session refresh") + async def move_project(self, name: str, new_path: str) -> None: + """Move a project to a new location. + + Args: + name: The name of the project to move + new_path: The new absolute path for the project + + Raises: + ValueError: If the project doesn't exist or repository isn't initialized + """ + if not self.repository: + raise ValueError("Repository is required for move_project") + + # Resolve to absolute path + resolved_path = os.path.abspath(os.path.expanduser(new_path)) + + # Validate project exists in config + if name not in self.config_manager.projects: + raise ValueError(f"Project '{name}' not found in configuration") + + # Create the new directory if it doesn't exist + Path(resolved_path).mkdir(parents=True, exist_ok=True) + + # Update in configuration + config = self.config_manager.load_config() + old_path = config.projects[name] + config.projects[name] = resolved_path + self.config_manager.save_config(config) + + # Update in database + project = await self.repository.get_by_name(name) + if project: + await self.repository.update_path(project.id, resolved_path) + logger.info(f"Moved project '{name}' from {old_path} to {resolved_path}") + else: + logger.error(f"Project '{name}' exists in config but not in database") + # Restore the old path in config since DB update failed + config.projects[name] = old_path + self.config_manager.save_config(config) + raise ValueError(f"Project '{name}' not found in database") + async def update_project( # pragma: no cover self, name: str, updated_path: Optional[str] = None, is_active: Optional[bool] = None ) -> None: diff --git a/src/basic_memory/utils.py b/src/basic_memory/utils.py index dab43373..33ffa139 100644 --- a/src/basic_memory/utils.py +++ b/src/basic_memory/utils.py @@ -146,19 +146,19 @@ def setup_logging( # logger.remove() # Add file handler if we are not running tests and a log file is specified - # if log_file and env != "test": - # # Setup file logger - # log_path = home_dir / log_file - # logger.add( - # str(log_path), - # level=log_level, - # rotation="10 MB", - # retention="10 days", - # backtrace=True, - # diagnose=True, - # enqueue=True, - # colorize=False, - # ) + if log_file and env != "test": + # Setup file logger + log_path = home_dir / log_file + logger.add( + str(log_path), + level=log_level, + rotation="10 MB", + retention="10 days", + backtrace=True, + diagnose=True, + enqueue=True, + colorize=False, + ) # Add console logger if requested or in test mode # if env == "test" or console: diff --git a/tests/api/test_project_router.py b/tests/api/test_project_router.py index 21448db5..9966e1fb 100644 --- a/tests/api/test_project_router.py +++ b/tests/api/test_project_router.py @@ -159,3 +159,236 @@ async def test_set_default_project_endpoint(test_config, client, project_service # Verify it's actually set as default assert project_service.default_project == test_project_name + + +@pytest.mark.asyncio +async def test_update_project_path_endpoint(test_config, client, project_service, project_url, tmp_path): + """Test the update project endpoint for changing project path.""" + # Create a test project to update + test_project_name = "test-update-project" + old_path = str(tmp_path / "old-location") + new_path = str(tmp_path / "new-location") + + await project_service.add_project(test_project_name, old_path) + + try: + # Verify initial state + project = await project_service.get_project(test_project_name) + assert project is not None + assert project.path == old_path + + # Update the project path + response = await client.patch( + f"{project_url}/project/{test_project_name}", + json={"path": new_path} + ) + + # Verify response + assert response.status_code == 200 + data = response.json() + + # Check response structure + assert "message" in data + assert "status" in data + assert data["status"] == "success" + assert "old_project" in data + assert "new_project" in data + + # Check old project data + assert data["old_project"]["name"] == test_project_name + assert data["old_project"]["path"] == old_path + + # Check new project data + assert data["new_project"]["name"] == test_project_name + assert data["new_project"]["path"] == new_path + + # Verify project was actually updated in database + updated_project = await project_service.get_project(test_project_name) + assert updated_project is not None + assert updated_project.path == new_path + + finally: + # Clean up + try: + await project_service.remove_project(test_project_name) + except Exception: + pass + + +@pytest.mark.asyncio +async def test_update_project_is_active_endpoint(test_config, client, project_service, project_url): + """Test the update project endpoint for changing is_active status.""" + # Create a test project to update + test_project_name = "test-update-active-project" + test_path = "/tmp/test-update-active" + + await project_service.add_project(test_project_name, test_path) + + try: + # Update the project is_active status + response = await client.patch( + f"{project_url}/project/{test_project_name}", + json={"is_active": False} + ) + + # Verify response + assert response.status_code == 200 + data = response.json() + + # Check response structure + assert "message" in data + assert "status" in data + assert data["status"] == "success" + assert f"Project '{test_project_name}' updated successfully" == data["message"] + + finally: + # Clean up + try: + await project_service.remove_project(test_project_name) + except Exception: + pass + + +@pytest.mark.asyncio +async def test_update_project_both_params_endpoint(test_config, client, project_service, project_url, tmp_path): + """Test the update project endpoint with both path and is_active parameters.""" + # Create a test project to update + test_project_name = "test-update-both-project" + old_path = str(tmp_path / "old-location") + new_path = str(tmp_path / "new-location") + + await project_service.add_project(test_project_name, old_path) + + try: + # Update both path and is_active (path should take precedence) + response = await client.patch( + f"{project_url}/project/{test_project_name}", + json={"path": new_path, "is_active": False} + ) + + # Verify response + assert response.status_code == 200 + data = response.json() + + # Check that path update was performed (takes precedence) + assert data["new_project"]["path"] == new_path + + # Verify project was actually updated in database + updated_project = await project_service.get_project(test_project_name) + assert updated_project is not None + assert updated_project.path == new_path + + finally: + # Clean up + try: + await project_service.remove_project(test_project_name) + except Exception: + pass + + +@pytest.mark.asyncio +async def test_update_project_nonexistent_endpoint(client, project_url): + """Test the update project endpoint with a nonexistent project.""" + # Try to update a project that doesn't exist + response = await client.patch( + f"{project_url}/project/nonexistent-project", + json={"path": "/tmp/new-path"} + ) + + # Should return 400 error + assert response.status_code == 400 + data = response.json() + assert "detail" in data + assert "not found in configuration" in data["detail"] + + +@pytest.mark.asyncio +async def test_update_project_relative_path_error_endpoint(test_config, client, project_service, project_url): + """Test the update project endpoint with relative path (should fail).""" + # Create a test project to update + test_project_name = "test-update-relative-project" + test_path = "/tmp/test-update-relative" + + await project_service.add_project(test_project_name, test_path) + + try: + # Try to update with relative path + response = await client.patch( + f"{project_url}/project/{test_project_name}", + json={"path": "./relative-path"} + ) + + # Should return 400 error + assert response.status_code == 400 + data = response.json() + assert "detail" in data + assert "Path must be absolute" in data["detail"] + + finally: + # Clean up + try: + await project_service.remove_project(test_project_name) + except Exception: + pass + + +@pytest.mark.asyncio +async def test_update_project_no_params_endpoint(test_config, client, project_service, project_url): + """Test the update project endpoint with no parameters (should fail).""" + # Create a test project to update + test_project_name = "test-update-no-params-project" + test_path = "/tmp/test-update-no-params" + + await project_service.add_project(test_project_name, test_path) + proj_info = await project_service.get_project(test_project_name) + assert proj_info.name == test_project_name + assert proj_info.path == test_path + + try: + # Try to update with no parameters + response = await client.patch( + f"{project_url}/project/{test_project_name}", + json={} + ) + + # Should return 200 (no-op) + assert response.status_code == 200 + proj_info = await project_service.get_project(test_project_name) + assert proj_info.name == test_project_name + assert proj_info.path == test_path + + finally: + # Clean up + try: + await project_service.remove_project(test_project_name) + except Exception: + pass + + +@pytest.mark.asyncio +async def test_update_project_empty_path_endpoint(test_config, client, project_service, project_url): + """Test the update project endpoint with empty path parameter.""" + # Create a test project to update + test_project_name = "test-update-empty-path-project" + test_path = "/tmp/test-update-empty-path" + + await project_service.add_project(test_project_name, test_path) + + try: + # Try to update with empty/null path - should be treated as no path update + response = await client.patch( + f"{project_url}/project/{test_project_name}", + json={"path": None, "is_active": True} + ) + + # Should succeed and perform is_active update + assert response.status_code == 200 + data = response.json() + assert data["status"] == "success" + + finally: + # Clean up + try: + await project_service.remove_project(test_project_name) + except Exception: + pass diff --git a/tests/cli/test_project_commands.py b/tests/cli/test_project_commands.py index 4af7c9ae..2bc040fe 100644 --- a/tests/cli/test_project_commands.py +++ b/tests/cli/test_project_commands.py @@ -141,3 +141,41 @@ def test_project_failure_exits_with_error(mock_run, cli_env): assert default_result.exit_code == 1 assert "Error setting default project" in default_result.output + + +@patch("basic_memory.cli.commands.project.asyncio.run") +def test_project_move_command(mock_run, cli_env): + """Test the 'project move' command with mocked API.""" + # Mock the API response + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = { + "message": "Project 'test-project' updated successfully", + "status": "success", + "default": False, + } + mock_run.return_value = mock_response + + runner = CliRunner() + result = runner.invoke(cli_app, ["project", "move", "test-project", "/new/path/to/project"]) + + # Verify it runs without exception + assert result.exit_code == 0 + # Verify the important warning message is displayed + assert "Manual File Movement Required" in result.output + assert "You must manually move your project files" in result.output + assert "/new/path/to/project" in result.output + + +@patch("basic_memory.cli.commands.project.asyncio.run") +def test_project_move_command_failure(mock_run, cli_env): + """Test the 'project move' command with API failure.""" + # Mock an exception being raised + mock_run.side_effect = Exception("Project not found") + + runner = CliRunner() + result = runner.invoke(cli_app, ["project", "move", "nonexistent-project", "/new/path"]) + + # Should exit with code 1 and show error message + assert result.exit_code == 1 + assert "Error moving project" in result.output diff --git a/tests/repository/test_project_repository.py b/tests/repository/test_project_repository.py index e30406bf..b2fd4385 100644 --- a/tests/repository/test_project_repository.py +++ b/tests/repository/test_project_repository.py @@ -267,3 +267,31 @@ async def test_delete_nonexistent_project(project_repository: ProjectRepository) """Test deleting a project that doesn't exist.""" result = await project_repository.delete(999) # Non-existent ID assert result is False + + +@pytest.mark.asyncio +async def test_update_path(project_repository: ProjectRepository, sample_project: Project): + """Test updating a project's path.""" + new_path = "/new/project/path" + + # Update the project path + updated_project = await project_repository.update_path(sample_project.id, new_path) + + # Verify returned object + assert updated_project is not None + assert updated_project.id == sample_project.id + assert updated_project.path == new_path + assert updated_project.name == sample_project.name # Other fields unchanged + + # Verify in database + found = await project_repository.find_by_id(sample_project.id) + assert found is not None + assert found.path == new_path + assert found.name == sample_project.name + + +@pytest.mark.asyncio +async def test_update_path_nonexistent_project(project_repository: ProjectRepository): + """Test updating path for a project that doesn't exist.""" + result = await project_repository.update_path(999, "/some/path") # Non-existent ID + assert result is None diff --git a/tests/services/test_project_service.py b/tests/services/test_project_service.py index 9150826e..49076005 100644 --- a/tests/services/test_project_service.py +++ b/tests/services/test_project_service.py @@ -535,6 +535,124 @@ async def test_synchronize_projects_normalizes_project_names( await project_service.repository.delete(db_project.id) +@pytest.mark.asyncio +async def test_move_project(project_service: ProjectService, tmp_path): + """Test moving a project to a new location.""" + test_project_name = f"test-move-project-{os.urandom(4).hex()}" + old_path = str(tmp_path / "old-location") + new_path = str(tmp_path / "new-location") + + # Create old directory + os.makedirs(old_path, exist_ok=True) + + try: + # Add project with initial path + await project_service.add_project(test_project_name, old_path) + + # Verify initial state + assert test_project_name in project_service.projects + assert project_service.projects[test_project_name] == old_path + + project = await project_service.repository.get_by_name(test_project_name) + assert project is not None + assert project.path == old_path + + # Move project to new location + await project_service.move_project(test_project_name, new_path) + + # Verify config was updated + assert project_service.projects[test_project_name] == new_path + + # Verify database was updated + updated_project = await project_service.repository.get_by_name(test_project_name) + assert updated_project is not None + assert updated_project.path == new_path + + # Verify new directory was created + assert os.path.exists(new_path) + + finally: + # Clean up + if test_project_name in project_service.projects: + await project_service.remove_project(test_project_name) + + +@pytest.mark.asyncio +async def test_move_project_nonexistent(project_service: ProjectService, tmp_path): + """Test moving a project that doesn't exist.""" + new_path = str(tmp_path / "new-location") + + with pytest.raises(ValueError, match="not found in configuration"): + await project_service.move_project("nonexistent-project", new_path) + + +@pytest.mark.asyncio +async def test_move_project_db_mismatch(project_service: ProjectService, tmp_path): + """Test moving a project that exists in config but not in database.""" + test_project_name = f"test-move-mismatch-{os.urandom(4).hex()}" + old_path = str(tmp_path / "old-location") + new_path = str(tmp_path / "new-location") + + # Create directories + os.makedirs(old_path, exist_ok=True) + + config_manager = project_service.config_manager + + try: + # Add project to config only (not to database) + config_manager.add_project(test_project_name, old_path) + + # Verify it's in config but not in database + assert test_project_name in project_service.projects + db_project = await project_service.repository.get_by_name(test_project_name) + assert db_project is None + + # Try to move project - should fail and restore config + with pytest.raises(ValueError, match="not found in database"): + await project_service.move_project(test_project_name, new_path) + + # Verify config was restored to original path + assert project_service.projects[test_project_name] == old_path + + finally: + # Clean up + if test_project_name in project_service.projects: + config_manager.remove_project(test_project_name) + + +@pytest.mark.asyncio +async def test_move_project_expands_path(project_service: ProjectService, tmp_path): + """Test that move_project expands ~ and relative paths.""" + test_project_name = f"test-move-expand-{os.urandom(4).hex()}" + old_path = str(tmp_path / "old-location") + + # Create old directory + os.makedirs(old_path, exist_ok=True) + + try: + # Add project with initial path + await project_service.add_project(test_project_name, old_path) + + # Use a relative path for the move + relative_new_path = "./new-location" + expected_absolute_path = os.path.abspath(relative_new_path) + + # Move project using relative path + await project_service.move_project(test_project_name, relative_new_path) + + # Verify the path was expanded to absolute + assert project_service.projects[test_project_name] == expected_absolute_path + + updated_project = await project_service.repository.get_by_name(test_project_name) + assert updated_project is not None + assert updated_project.path == expected_absolute_path + + finally: + # Clean up + if test_project_name in project_service.projects: + await project_service.remove_project(test_project_name) + + @pytest.mark.asyncio async def test_synchronize_projects_handles_case_sensitivity_bug( project_service: ProjectService, tmp_path @@ -595,3 +713,5 @@ async def test_synchronize_projects_handles_case_sensitivity_bug( db_project = await project_service.repository.get_by_name(name) if db_project: await project_service.repository.delete(db_project.id) + +