✨ MCP Service Containerization Integration #1368

[Specification Details]
1. Add test cases.

Author: YehongPan

test/backend/app/test_remote_mcp_app.py

@@ -1478,5 +1478,122 @@ def test_get_container_logs_exception(self, mock_container_manager_class, mock_g
         assert "Failed to get container logs" in data["detail"]
 
 
+# ---------------------------------------------------------------------------
+# Additional test cases for upload_mcp_image validation
+# ---------------------------------------------------------------------------
+
+
+class TestUploadMCPImageValidationAdditional:
+    """Additional test cases for upload_mcp_image endpoint validation"""
+
+    @patch('apps.remote_mcp_app.get_current_user_id')
+    def test_upload_mcp_image_invalid_port_range(self, mock_get_user_id):
+        """Test upload with invalid port range (covers lines 429-430)"""
+        mock_get_user_id.return_value = ("user123", "tenant456")
+
+        # Test port <= 0
+        file_content = b"fake tar content"
+        response = client.post(
+            "/mcp/upload-image",
+            data={"port": 0},  # Invalid port
+            files={"file": ("test.tar", file_content,
+                            "application/octet-stream")},
+            headers={"Authorization": "Bearer test_token"}
+        )
+        assert response.status_code == HTTPStatus.BAD_REQUEST
+        data = response.json()
+        assert "Port must be between 1 and 65535" in data["detail"]
+
+        # Test port > 65535
+        response = client.post(
+            "/mcp/upload-image",
+            data={"port": 70000},  # Invalid port
+            files={"file": ("test.tar", file_content,
+                            "application/octet-stream")},
+            headers={"Authorization": "Bearer test_token"}
+        )
+        assert response.status_code == HTTPStatus.BAD_REQUEST
+        data = response.json()
+        assert "Port must be between 1 and 65535" in data["detail"]
+
+    @patch('apps.remote_mcp_app.get_current_user_id')
+    @patch('apps.remote_mcp_app.MCPContainerManager')
+    @patch('apps.remote_mcp_app.check_mcp_name_exists', return_value=False)
+    def test_upload_mcp_image_env_vars_not_dict(self, mock_check_name, mock_container_manager_class, mock_get_user_id):
+        """Test upload with environment variables that are not a JSON object (covers lines 459-460)"""
+        mock_get_user_id.return_value = ("user123", "tenant456")
+
+        mock_container_manager = MagicMock()
+        mock_container_manager_class.return_value = mock_container_manager
+
+        file_content = b"fake tar content"
+
+        # Test with array instead of object
+        response = client.post(
+            "/mcp/upload-image",
+            data={
+                "port": 5020,
+                "env_vars": '["VAR1", "VAR2"]'  # Array instead of object
+            },
+            files={"file": ("test.tar", file_content,
+                            "application/octet-stream")},
+            headers={"Authorization": "Bearer test_token"}
+        )
+        assert response.status_code == HTTPStatus.BAD_REQUEST
+        data = response.json()
+        assert "Invalid environment variables format" in data["detail"]
+        assert "Environment variables must be a JSON object" in data["detail"]
+
+    @patch('apps.remote_mcp_app.get_current_user_id')
+    @patch('apps.remote_mcp_app.MCPContainerManager')
+    @patch('apps.remote_mcp_app.add_remote_mcp_server_list')
+    @patch('apps.remote_mcp_app.check_mcp_name_exists', return_value=False)
+    @patch('tempfile.NamedTemporaryFile')
+    @patch('os.unlink', side_effect=OSError("Permission denied"))
+    @patch('apps.remote_mcp_app.logger')
+    def test_upload_mcp_image_temp_file_cleanup_warning(self, mock_logger, mock_unlink, mock_temp_file, mock_check_name, mock_add_server, mock_container_manager_class, mock_get_user_id):
+        """Test upload with temporary file cleanup failure (covers lines 536-537)"""
+        mock_get_user_id.return_value = ("user123", "tenant456")
+
+        # Mock tempfile.NamedTemporaryFile
+        mock_temp_file_obj = MagicMock()
+        mock_temp_file_obj.__enter__.return_value = mock_temp_file_obj
+        mock_temp_file_obj.__exit__.return_value = None
+        mock_temp_file_obj.name = "/tmp/test.tar"
+        mock_temp_file.return_value = mock_temp_file_obj
+
+        mock_container_manager = MagicMock()
+        mock_container_manager_class.return_value = mock_container_manager
+        mock_container_manager.start_mcp_container_from_tar = AsyncMock(return_value={
+            "container_id": "container-123",
+            "mcp_url": "http://localhost:5020/mcp",
+            "host_port": "5020",
+            "status": "started",
+            "container_name": "test-image-user1234"
+        })
+
+        mock_add_server.return_value = None
+
+        file_content = b"fake tar content"
+
+        response = client.post(
+            "/mcp/upload-image",
+            data={"port": 5020},
+            files={"file": ("test.tar", file_content,
+                            "application/octet-stream")},
+            headers={"Authorization": "Bearer test_token"}
+        )
+
+        # Should still succeed despite cleanup failure
+        assert response.status_code == HTTPStatus.OK
+        data = response.json()
+        assert data["status"] == "success"
+
+        # Verify warning was logged
+        mock_logger.warning.assert_called_once()
+        warning_call_args = mock_logger.warning.call_args[0][0]
+        assert "Failed to clean up temporary file /tmp/test.tar" in warning_call_args
+
+
 if __name__ == "__main__":
     pytest.main([__file__])

test/backend/services/test_mcp_container_service.py

@@ -5,6 +5,7 @@
 
 import sys
 import os
+import tempfile
 from unittest.mock import patch, MagicMock, AsyncMock
 import pytest
 
@@ -473,5 +474,208 @@ def test_get_container_logs_exception(self, mock_manager):
         assert "Error retrieving logs" in logs
 
 
+# ---------------------------------------------------------------------------
+# Test load_image_from_tar_file
+# ---------------------------------------------------------------------------
+
+
+class TestLoadImageFromTarFile:
+    """Test load_image_from_tar_file method"""
+
+    @pytest.fixture
+    def mock_manager(self):
+        """Create MCPContainerManager instance with mocked client"""
+        with patch('services.mcp_container_service.create_container_client_from_config'), \
+                patch('services.mcp_container_service.DockerContainerConfig'):
+            manager = MCPContainerManager()
+            manager.client = MagicMock()
+            return manager
+
+    @pytest.mark.asyncio
+    async def test_load_image_from_tar_file_success_with_tags(self, mock_manager):
+        """Test successful loading of image with tags"""
+        # Create a temporary file
+        with tempfile.NamedTemporaryFile(delete=False, suffix='.tar') as temp_file:
+            temp_file.write(b"fake tar content")
+            temp_file_path = temp_file.name
+
+        try:
+            mock_image = MagicMock()
+            mock_image.tags = ["test-image:latest", "test-image:v1.0"]
+            mock_image.id = "sha256:1234567890abcdef"
+
+            mock_manager.client.client.images.load.return_value = [mock_image]
+
+            result = await mock_manager.load_image_from_tar_file(temp_file_path)
+
+            assert result == "test-image:latest"
+            mock_manager.client.client.images.load.assert_called_once()
+        finally:
+            # Clean up
+            try:
+                os.unlink(temp_file_path)
+            except:
+                pass
+
+    @pytest.mark.asyncio
+    async def test_load_image_from_tar_file_success_without_tags(self, mock_manager):
+        """Test successful loading of image without tags"""
+        # Create a temporary file
+        with tempfile.NamedTemporaryFile(delete=False, suffix='.tar') as temp_file:
+            temp_file.write(b"fake tar content")
+            temp_file_path = temp_file.name
+
+        try:
+            mock_image = MagicMock()
+            mock_image.tags = []
+            mock_image.id = "sha256:1234567890abcdef"
+
+            mock_manager.client.client.images.load.return_value = [mock_image]
+
+            result = await mock_manager.load_image_from_tar_file(temp_file_path)
+
+            assert result == "sha256:1234567890abcdef"
+            mock_manager.client.client.images.load.assert_called_once()
+        finally:
+            # Clean up
+            try:
+                os.unlink(temp_file_path)
+            except:
+                pass
+
+    @pytest.mark.asyncio
+    async def test_load_image_from_tar_file_empty_images(self, mock_manager):
+        """Test loading when no images are found in tar file (covers lines 69-70)"""
+        # Create a temporary file
+        with tempfile.NamedTemporaryFile(delete=False, suffix='.tar') as temp_file:
+            temp_file.write(b"fake tar content")
+            temp_file_path = temp_file.name
+
+        try:
+            mock_manager.client.client.images.load.return_value = []
+
+            with pytest.raises(MCPContainerError, match="No images found in tar file"):
+                await mock_manager.load_image_from_tar_file(temp_file_path)
+        finally:
+            # Clean up
+            try:
+                os.unlink(temp_file_path)
+            except:
+                pass
+
+    @pytest.mark.asyncio
+    async def test_load_image_from_tar_file_exception(self, mock_manager):
+        """Test loading when exception occurs (covers lines 80-82)"""
+        # Create a temporary file
+        with tempfile.NamedTemporaryFile(delete=False, suffix='.tar') as temp_file:
+            temp_file.write(b"fake tar content")
+            temp_file_path = temp_file.name
+
+        try:
+            mock_manager.client.client.images.load.side_effect = Exception(
+                "File not found")
+
+            with pytest.raises(MCPContainerError, match="Failed to load image from tar file: File not found"):
+                await mock_manager.load_image_from_tar_file(temp_file_path)
+        finally:
+            # Clean up
+            try:
+                os.unlink(temp_file_path)
+            except:
+                pass
+
+
+# ---------------------------------------------------------------------------
+# Test start_mcp_container_from_tar
+# ---------------------------------------------------------------------------
+
+
+class TestStartMCPContainerFromTar:
+    """Test start_mcp_container_from_tar method"""
+
+    @pytest.fixture
+    def mock_manager(self):
+        """Create MCPContainerManager instance with mocked client"""
+        with patch('services.mcp_container_service.create_container_client_from_config'), \
+                patch('services.mcp_container_service.DockerContainerConfig'):
+            manager = MCPContainerManager()
+            manager.client = MagicMock()
+            return manager
+
+    @pytest.mark.asyncio
+    async def test_start_mcp_container_from_tar_success(self, mock_manager):
+        """Test successful starting of MCP container from tar file"""
+        # Mock load_image_from_tar_file
+        mock_manager.load_image_from_tar_file = AsyncMock(
+            return_value="loaded-image:latest")
+
+        # Mock start_mcp_container
+        mock_manager.start_mcp_container = AsyncMock(return_value={
+            "container_id": "container-123",
+            "mcp_url": "http://localhost:5020/mcp",
+            "host_port": "5020",
+            "status": "started",
+            "container_name": "test-service-user1234"
+        })
+
+        result = await mock_manager.start_mcp_container_from_tar(
+            tar_file_path="/path/to/image.tar",
+            service_name="test-service",
+            tenant_id="tenant123",
+            user_id="user12345",
+            env_vars={"NODE_ENV": "production"},
+            host_port=5020,
+            full_command=["npx", "-y", "test-mcp"]
+        )
+
+        assert result["container_id"] == "container-123"
+        assert result["mcp_url"] == "http://localhost:5020/mcp"
+        mock_manager.load_image_from_tar_file.assert_called_once_with(
+            "/path/to/image.tar")
+        mock_manager.start_mcp_container.assert_called_once_with(
+            service_name="test-service",
+            tenant_id="tenant123",
+            user_id="user12345",
+            env_vars={"NODE_ENV": "production"},
+            host_port=5020,
+            image="loaded-image:latest",
+            full_command=["npx", "-y", "test-mcp"]
+        )
+
+    @pytest.mark.asyncio
+    async def test_start_mcp_container_from_tar_load_image_error(self, mock_manager):
+        """Test starting container when load_image_from_tar_file fails (covers lines 178-181)"""
+        # Mock load_image_from_tar_file to raise error
+        mock_manager.load_image_from_tar_file = AsyncMock(
+            side_effect=MCPContainerError("Failed to load image"))
+
+        with pytest.raises(MCPContainerError, match="Failed to start container from tar file: Failed to load image"):
+            await mock_manager.start_mcp_container_from_tar(
+                tar_file_path="/path/to/image.tar",
+                service_name="test-service",
+                tenant_id="tenant123",
+                user_id="user12345"
+            )
+
+    @pytest.mark.asyncio
+    async def test_start_mcp_container_from_tar_start_container_error(self, mock_manager):
+        """Test starting container when start_mcp_container fails (covers lines 178-181)"""
+        # Mock load_image_from_tar_file to succeed
+        mock_manager.load_image_from_tar_file = AsyncMock(
+            return_value="loaded-image:latest")
+
+        # Mock start_mcp_container to raise error
+        mock_manager.start_mcp_container = AsyncMock(
+            side_effect=MCPContainerError("Container startup failed"))
+
+        with pytest.raises(MCPContainerError, match="Failed to start container from tar file: Container startup failed"):
+            await mock_manager.start_mcp_container_from_tar(
+                tar_file_path="/path/to/image.tar",
+                service_name="test-service",
+                tenant_id="tenant123",
+                user_id="user12345"
+            )
+
+
 if __name__ == "__main__":
     pytest.main([__file__])