diff --git a/api/controllers/console/app/message.py b/api/controllers/console/app/message.py index 726bd94cd7e..195a41f2888 100644 --- a/api/controllers/console/app/message.py +++ b/api/controllers/console/app/message.py @@ -341,8 +341,8 @@ class MessageFeedbackExportApi(Resource): try: export_data = FeedbackService.export_feedbacks( - db.session(), - app_id=app_model.id, + app_model.id, + session=db.session(), from_source=args.from_source, rating=args.rating, has_comment=args.has_comment, diff --git a/api/controllers/console/datasets/metadata.py b/api/controllers/console/datasets/metadata.py index 7195fe066fd..ebb490cd9e8 100644 --- a/api/controllers/console/datasets/metadata.py +++ b/api/controllers/console/datasets/metadata.py @@ -17,6 +17,7 @@ from controllers.console.wraps import ( with_current_tenant_id, with_current_user, ) +from extensions.ext_database import db from fields.dataset_fields import ( DatasetMetadataBuiltInFieldsResponse, DatasetMetadataListResponse, @@ -65,7 +66,9 @@ class DatasetMetadataCreateApi(Resource): raise NotFound("Dataset not found.") DatasetService.check_dataset_permission(dataset, current_user) - metadata = MetadataService.create_metadata(dataset_id_str, metadata_args, current_user, current_tenant_id) + metadata = MetadataService.create_metadata( + db.session(), dataset_id_str, metadata_args, current_user, current_tenant_id + ) return dump_response(DatasetMetadataResponse, metadata), 201 @setup_required @@ -81,7 +84,7 @@ class DatasetMetadataCreateApi(Resource): dataset = DatasetService.get_dataset(dataset_id_str) if dataset is None: raise NotFound("Dataset not found.") - metadata = MetadataService.get_dataset_metadatas(dataset) + metadata = MetadataService.get_dataset_metadatas(db.session(), dataset) return dump_response(DatasetMetadataListResponse, metadata), 200 @@ -108,7 +111,7 @@ class DatasetMetadataApi(Resource): DatasetService.check_dataset_permission(dataset, current_user) metadata = MetadataService.update_metadata_name( - dataset_id_str, metadata_id_str, name, current_user, current_tenant_id + db.session(), dataset_id_str, metadata_id_str, name, current_user, current_tenant_id ) return dump_response(DatasetMetadataResponse, metadata), 200 @@ -127,7 +130,7 @@ class DatasetMetadataApi(Resource): raise NotFound("Dataset not found.") DatasetService.check_dataset_permission(dataset, current_user) - MetadataService.delete_metadata(dataset_id_str, metadata_id_str) + MetadataService.delete_metadata(db.session(), dataset_id_str, metadata_id_str) # Frontend callers only await success and invalidate metadata caches; no response body is consumed. return "", 204 @@ -166,9 +169,9 @@ class DatasetMetadataBuiltInFieldActionApi(Resource): match action: case "enable": - MetadataService.enable_built_in_field(dataset) + MetadataService.enable_built_in_field(db.session(), dataset) case "disable": - MetadataService.disable_built_in_field(dataset) + MetadataService.disable_built_in_field(db.session(), dataset) # Frontend callers only await success and invalidate metadata caches; no response body is consumed. return "", 204 @@ -195,7 +198,7 @@ class DocumentMetadataEditApi(Resource): metadata_args = MetadataOperationData.model_validate(console_ns.payload or {}) - MetadataService.update_documents_metadata(dataset, metadata_args, current_user) + MetadataService.update_documents_metadata(db.session(), dataset, metadata_args, current_user) # Frontend callers only await success and invalidate caches; no response body is consumed. return "", 204 diff --git a/api/controllers/service_api/dataset/metadata.py b/api/controllers/service_api/dataset/metadata.py index 426d008c412..7363e6bdfd4 100644 --- a/api/controllers/service_api/dataset/metadata.py +++ b/api/controllers/service_api/dataset/metadata.py @@ -8,6 +8,7 @@ from controllers.common.controller_schemas import MetadataUpdatePayload from controllers.common.schema import register_response_schema_models, register_schema_model, register_schema_models from controllers.service_api import service_api_ns from controllers.service_api.wraps import DatasetApiResource, cloud_edition_billing_rate_limit_check +from extensions.ext_database import db from fields.dataset_fields import ( DatasetMetadataActionResponse, DatasetMetadataBuiltInFieldsResponse, @@ -85,7 +86,7 @@ class DatasetMetadataCreateServiceApi(DatasetApiResource): raise NotFound("Dataset not found.") DatasetService.check_dataset_permission(dataset, current_user) - metadata = MetadataService.create_metadata(dataset_id_str, metadata_args) + metadata = MetadataService.create_metadata(db.session(), dataset_id_str, metadata_args) return dump_response(DatasetMetadataResponse, metadata), 201 @service_api_ns.doc( @@ -118,7 +119,7 @@ class DatasetMetadataCreateServiceApi(DatasetApiResource): dataset = DatasetService.get_dataset(dataset_id_str) if dataset is None: raise NotFound("Dataset not found.") - metadata = MetadataService.get_dataset_metadatas(dataset) + metadata = MetadataService.get_dataset_metadatas(db.session(), dataset) return dump_response(DatasetMetadataListResponse, metadata), 200 @@ -158,7 +159,7 @@ class DatasetMetadataServiceApi(DatasetApiResource): raise NotFound("Dataset not found.") DatasetService.check_dataset_permission(dataset, current_user) - metadata = MetadataService.update_metadata_name(dataset_id_str, metadata_id_str, payload.name) + metadata = MetadataService.update_metadata_name(db.session(), dataset_id_str, metadata_id_str, payload.name) return dump_response(DatasetMetadataResponse, metadata), 200 @service_api_ns.doc( @@ -193,7 +194,7 @@ class DatasetMetadataServiceApi(DatasetApiResource): raise NotFound("Dataset not found.") DatasetService.check_dataset_permission(dataset, current_user) - MetadataService.delete_metadata(dataset_id_str, metadata_id_str) + MetadataService.delete_metadata(db.session(), dataset_id_str, metadata_id_str) return "", 204 @@ -263,9 +264,9 @@ class DatasetMetadataBuiltInFieldActionServiceApi(DatasetApiResource): match action: case "enable": - MetadataService.enable_built_in_field(dataset) + MetadataService.enable_built_in_field(db.session(), dataset) case "disable": - MetadataService.disable_built_in_field(dataset) + MetadataService.disable_built_in_field(db.session(), dataset) return dump_response(DatasetMetadataActionResponse, {"result": "success"}), 200 @@ -309,6 +310,6 @@ class DocumentMetadataEditServiceApi(DatasetApiResource): metadata_args = MetadataOperationData.model_validate(service_api_ns.payload or {}) - MetadataService.update_documents_metadata(dataset, metadata_args) + MetadataService.update_documents_metadata(db.session(), dataset, metadata_args) return dump_response(DatasetMetadataActionResponse, {"result": "success"}), 200 diff --git a/api/services/feedback_service.py b/api/services/feedback_service.py index 24cfb8aa852..62885c901b7 100644 --- a/api/services/feedback_service.py +++ b/api/services/feedback_service.py @@ -14,8 +14,9 @@ from models.model import Account, App, Conversation, Message, MessageFeedback class FeedbackService: @staticmethod def export_feedbacks( - session: Session, app_id: str, + *, + session: Session, from_source: str | None = None, rating: str | None = None, has_comment: bool | None = None, @@ -28,6 +29,7 @@ class FeedbackService: Args: app_id: Application ID + session: Database session used to run the export query from_source: Filter by feedback source ('user' or 'admin') rating: Filter by rating ('like' or 'dislike') has_comment: Only include feedback with comments diff --git a/api/services/metadata_service.py b/api/services/metadata_service.py index f9dcfd25c7f..d9cd65b2b39 100644 --- a/api/services/metadata_service.py +++ b/api/services/metadata_service.py @@ -2,9 +2,9 @@ import copy import logging from sqlalchemy import delete, func, select +from sqlalchemy.orm import Session from core.rag.index_processor.constant.built_in_field import BuiltInField, MetadataDataSource -from extensions.ext_database import db from extensions.ext_redis import redis_client from libs.datetime_utils import naive_utc_now from libs.login import resolve_account_fallback @@ -23,6 +23,7 @@ logger = logging.getLogger(__name__) class MetadataService: @staticmethod def create_metadata( + session: Session, dataset_id: str, metadata_args: MetadataArgs, current_user: Account | None = None, # TODO: the service_api is not migrated yet @@ -33,7 +34,7 @@ class MetadataService: raise ValueError("Metadata name cannot exceed 255 characters.") current_user, current_tenant_id = resolve_account_fallback(current_user, current_tenant_id) # check if metadata name already exists - if db.session.scalar( + if session.scalar( select(DatasetMetadata) .where( DatasetMetadata.tenant_id == current_tenant_id, @@ -53,12 +54,13 @@ class MetadataService: name=metadata_args.name, created_by=current_user.id, ) - db.session.add(metadata) - db.session.commit() + session.add(metadata) + session.commit() return metadata @staticmethod def update_metadata_name( + session: Session, dataset_id: str, metadata_id: str, name: str, @@ -72,7 +74,7 @@ class MetadataService: lock_key = f"dataset_metadata_lock_{dataset_id}" # check if metadata name already exists current_user, current_tenant_id = resolve_account_fallback(current_user, current_tenant_id) - if db.session.scalar( + if session.scalar( select(DatasetMetadata) .where( DatasetMetadata.tenant_id == current_tenant_id, @@ -87,7 +89,7 @@ class MetadataService: raise ValueError("Metadata name already exists in Built-in fields.") try: MetadataService.knowledge_base_metadata_lock_check(dataset_id, None) - metadata = db.session.scalar( + metadata = session.scalar( select(DatasetMetadata) .where(DatasetMetadata.id == metadata_id, DatasetMetadata.dataset_id == dataset_id) .limit(1) @@ -100,7 +102,7 @@ class MetadataService: metadata.updated_at = naive_utc_now() # update related documents - dataset_metadata_bindings = db.session.scalars( + dataset_metadata_bindings = session.scalars( select(DatasetMetadataBinding).where(DatasetMetadataBinding.metadata_id == metadata_id) ).all() if dataset_metadata_bindings: @@ -114,8 +116,8 @@ class MetadataService: value = doc_metadata.pop(old_name, None) doc_metadata[name] = value document.doc_metadata = doc_metadata - db.session.add(document) - db.session.commit() + session.add(document) + session.commit() return metadata except Exception: logger.exception("Update metadata name failed") @@ -124,21 +126,21 @@ class MetadataService: redis_client.delete(lock_key) @staticmethod - def delete_metadata(dataset_id: str, metadata_id: str): + def delete_metadata(session: Session, dataset_id: str, metadata_id: str): lock_key = f"dataset_metadata_lock_{dataset_id}" try: MetadataService.knowledge_base_metadata_lock_check(dataset_id, None) - metadata = db.session.scalar( + metadata = session.scalar( select(DatasetMetadata) .where(DatasetMetadata.id == metadata_id, DatasetMetadata.dataset_id == dataset_id) .limit(1) ) if metadata is None: raise ValueError("Metadata not found.") - db.session.delete(metadata) + session.delete(metadata) # deal related documents - dataset_metadata_bindings = db.session.scalars( + dataset_metadata_bindings = session.scalars( select(DatasetMetadataBinding).where(DatasetMetadataBinding.metadata_id == metadata_id) ).all() if dataset_metadata_bindings: @@ -151,8 +153,8 @@ class MetadataService: doc_metadata = copy.deepcopy(document.doc_metadata) doc_metadata.pop(metadata.name, None) document.doc_metadata = doc_metadata - db.session.add(document) - db.session.commit() + session.add(document) + session.commit() return metadata except Exception: logger.exception("Delete metadata failed") @@ -170,13 +172,13 @@ class MetadataService: ] @staticmethod - def enable_built_in_field(dataset: Dataset): + def enable_built_in_field(session: Session, dataset: Dataset): if dataset.built_in_field_enabled: return lock_key = f"dataset_metadata_lock_{dataset.id}" try: MetadataService.knowledge_base_metadata_lock_check(dataset.id, None) - db.session.add(dataset) + session.add(dataset) documents = DocumentService.get_working_documents_by_dataset_id(dataset.id) if documents: for document in documents: @@ -190,22 +192,22 @@ class MetadataService: doc_metadata[BuiltInField.last_update_date] = document.last_update_date.timestamp() doc_metadata[BuiltInField.source] = MetadataDataSource[document.data_source_type] document.doc_metadata = doc_metadata - db.session.add(document) + session.add(document) dataset.built_in_field_enabled = True - db.session.commit() + session.commit() except Exception: logger.exception("Enable built-in field failed") finally: redis_client.delete(lock_key) @staticmethod - def disable_built_in_field(dataset: Dataset): + def disable_built_in_field(session: Session, dataset: Dataset): if not dataset.built_in_field_enabled: return lock_key = f"dataset_metadata_lock_{dataset.id}" try: MetadataService.knowledge_base_metadata_lock_check(dataset.id, None) - db.session.add(dataset) + session.add(dataset) documents = DocumentService.get_working_documents_by_dataset_id(dataset.id) document_ids = [] if documents: @@ -220,10 +222,10 @@ class MetadataService: doc_metadata.pop(BuiltInField.last_update_date, None) doc_metadata.pop(BuiltInField.source, None) document.doc_metadata = doc_metadata - db.session.add(document) + session.add(document) document_ids.append(document.id) dataset.built_in_field_enabled = False - db.session.commit() + session.commit() except Exception: logger.exception("Disable built-in field failed") finally: @@ -231,6 +233,7 @@ class MetadataService: @staticmethod def update_documents_metadata( + session: Session, dataset: Dataset, metadata_args: MetadataOperationData, current_user: Account | None = None, # TODO: the service_api is not migrated yet @@ -259,11 +262,11 @@ class MetadataService: doc_metadata[BuiltInField.last_update_date] = document.last_update_date.timestamp() doc_metadata[BuiltInField.source] = MetadataDataSource[document.data_source_type] document.doc_metadata = doc_metadata - db.session.add(document) + session.add(document) # deal metadata binding (in the same transaction as the doc_metadata update) if not operation.partial_update: - db.session.execute( + session.execute( delete(DatasetMetadataBinding).where( DatasetMetadataBinding.document_id == operation.document_id ) @@ -272,7 +275,7 @@ class MetadataService: for metadata_value in operation.metadata_list: # check if binding already exists if operation.partial_update: - existing_binding = db.session.scalar( + existing_binding = session.scalar( select(DatasetMetadataBinding) .where( DatasetMetadataBinding.document_id == operation.document_id, @@ -290,10 +293,10 @@ class MetadataService: metadata_id=metadata_value.id, created_by=current_user.id, ) - db.session.add(dataset_metadata_binding) - db.session.commit() + session.add(dataset_metadata_binding) + session.commit() except Exception: - db.session.rollback() + session.rollback() logger.exception("Update documents metadata failed") raise finally: @@ -313,14 +316,14 @@ class MetadataService: redis_client.set(lock_key, 1, ex=3600) @staticmethod - def get_dataset_metadatas(dataset: Dataset): + def get_dataset_metadatas(session: Session, dataset: Dataset): return { "doc_metadata": [ { "id": item.get("id"), "name": item.get("name"), "type": item.get("type"), - "count": db.session.scalar( + "count": session.scalar( select(func.count(DatasetMetadataBinding.id)).where( DatasetMetadataBinding.metadata_id == item.get("id"), DatasetMetadataBinding.dataset_id == dataset.id, diff --git a/api/tests/test_containers_integration_tests/services/test_feedback_service.py b/api/tests/test_containers_integration_tests/services/test_feedback_service.py index e4fd81b53e7..c2b0385fc74 100644 --- a/api/tests/test_containers_integration_tests/services/test_feedback_service.py +++ b/api/tests/test_containers_integration_tests/services/test_feedback_service.py @@ -97,8 +97,9 @@ class TestFeedbackService: ) # Test CSV export - result = FeedbackService.export_feedbacks(mock_db_session, app_id=sample_data["app"].id, format_type="csv") - + result = FeedbackService.export_feedbacks( + app_id=sample_data["app"].id, session=mock_db_session, format_type="csv" + ) # Verify response structure assert hasattr(result, "headers") assert "text/csv" in result.headers["Content-Type"] @@ -128,7 +129,9 @@ class TestFeedbackService: ) # Test JSON export - result = FeedbackService.export_feedbacks(mock_db_session, app_id=sample_data["app"].id, format_type="json") + result = FeedbackService.export_feedbacks( + app_id=sample_data["app"].id, session=mock_db_session, format_type="json" + ) # Verify response structure assert hasattr(result, "headers") @@ -158,8 +161,8 @@ class TestFeedbackService: # Test with filters result = FeedbackService.export_feedbacks( - mock_db_session, app_id=sample_data["app"].id, + session=mock_db_session, from_source=FeedbackFromSource.ADMIN, rating=FeedbackRating.DISLIKE, has_comment=True, @@ -175,7 +178,9 @@ class TestFeedbackService: """Test exporting feedback when no data exists.""" mock_db_session.execute.return_value = _execute_result([]) - result = FeedbackService.export_feedbacks(mock_db_session, app_id=sample_data["app"].id, format_type="csv") + result = FeedbackService.export_feedbacks( + app_id=sample_data["app"].id, session=mock_db_session, format_type="csv" + ) # Should return an empty CSV with headers only assert hasattr(result, "headers") @@ -194,13 +199,13 @@ class TestFeedbackService: # Test with invalid start_date with pytest.raises(ValueError, match="Invalid start_date format"): FeedbackService.export_feedbacks( - mock_db_session, app_id=sample_data["app"].id, start_date="invalid-date-format" + app_id=sample_data["app"].id, session=mock_db_session, start_date="invalid-date-format" ) # Test with invalid end_date with pytest.raises(ValueError, match="Invalid end_date format"): FeedbackService.export_feedbacks( - mock_db_session, app_id=sample_data["app"].id, end_date="invalid-date-format" + app_id=sample_data["app"].id, session=mock_db_session, end_date="invalid-date-format" ) def test_export_feedbacks_invalid_format(self, mock_db_session, sample_data): @@ -208,8 +213,8 @@ class TestFeedbackService: with pytest.raises(ValueError, match="Unsupported format"): FeedbackService.export_feedbacks( - mock_db_session, app_id=sample_data["app"].id, + session=mock_db_session, format_type="xml", # Unsupported format ) @@ -239,7 +244,9 @@ class TestFeedbackService: ) # Test export - result = FeedbackService.export_feedbacks(mock_db_session, app_id=sample_data["app"].id, format_type="json") + result = FeedbackService.export_feedbacks( + app_id=sample_data["app"].id, session=mock_db_session, format_type="json" + ) # Check JSON content json_content = json.loads(result.get_data(as_text=True)) @@ -290,7 +297,9 @@ class TestFeedbackService: ) # Test export - result = FeedbackService.export_feedbacks(mock_db_session, app_id=sample_data["app"].id, format_type="csv") + result = FeedbackService.export_feedbacks( + app_id=sample_data["app"].id, session=mock_db_session, format_type="csv" + ) # Check that unicode content is preserved csv_content = result.get_data(as_text=True) @@ -320,7 +329,9 @@ class TestFeedbackService: ) # Test export - result = FeedbackService.export_feedbacks(mock_db_session, app_id=sample_data["app"].id, format_type="json") + result = FeedbackService.export_feedbacks( + app_id=sample_data["app"].id, session=mock_db_session, format_type="json" + ) # Check JSON content for emoji ratings json_content = json.loads(result.get_data(as_text=True)) diff --git a/api/tests/test_containers_integration_tests/services/test_metadata_partial_update.py b/api/tests/test_containers_integration_tests/services/test_metadata_partial_update.py index 5844441e6a5..fbdc265265d 100644 --- a/api/tests/test_containers_integration_tests/services/test_metadata_partial_update.py +++ b/api/tests/test_containers_integration_tests/services/test_metadata_partial_update.py @@ -95,7 +95,7 @@ class TestMetadataPartialUpdate: ) metadata_args = MetadataOperationData(operation_data=[operation]) - MetadataService.update_documents_metadata(dataset, metadata_args, current_account) + MetadataService.update_documents_metadata(db_session_with_containers, dataset, metadata_args, current_account) db_session_with_containers.expire_all() updated_doc = db_session_with_containers.get(Document, document.id) @@ -126,7 +126,7 @@ class TestMetadataPartialUpdate: ) metadata_args = MetadataOperationData(operation_data=[operation]) - MetadataService.update_documents_metadata(dataset, metadata_args, current_account) + MetadataService.update_documents_metadata(db_session_with_containers, dataset, metadata_args, current_account) db_session_with_containers.expire_all() updated_doc = db_session_with_containers.get(Document, document.id) @@ -168,7 +168,7 @@ class TestMetadataPartialUpdate: ) metadata_args = MetadataOperationData(operation_data=[operation]) - MetadataService.update_documents_metadata(dataset, metadata_args, current_account) + MetadataService.update_documents_metadata(db_session_with_containers, dataset, metadata_args, current_account) db_session_with_containers.expire_all() bindings = db_session_with_containers.scalars( @@ -202,6 +202,8 @@ class TestMetadataPartialUpdate: ) metadata_args = MetadataOperationData(operation_data=[operation]) - with patch("services.metadata_service.db.session.commit", side_effect=RuntimeError("database connection lost")): + with patch.object(db_session_with_containers, "commit", side_effect=RuntimeError("database connection lost")): with pytest.raises(RuntimeError, match="database connection lost"): - MetadataService.update_documents_metadata(dataset, metadata_args, current_account) + MetadataService.update_documents_metadata( + db_session_with_containers, dataset, metadata_args, current_account + ) diff --git a/api/tests/test_containers_integration_tests/services/test_metadata_service.py b/api/tests/test_containers_integration_tests/services/test_metadata_service.py index 0c9e3830430..7cc9fc7e696 100644 --- a/api/tests/test_containers_integration_tests/services/test_metadata_service.py +++ b/api/tests/test_containers_integration_tests/services/test_metadata_service.py @@ -183,7 +183,9 @@ class TestMetadataService: metadata_args = MetadataArgs(type="string", name="test_metadata") # Act: Execute the method under test - result = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + result = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Assert: Verify the expected outcomes assert result is not None @@ -218,7 +220,7 @@ class TestMetadataService: # Act & Assert: Verify proper error handling with pytest.raises(ValueError, match="Metadata name cannot exceed 255 characters."): - MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + MetadataService.create_metadata(db_session_with_containers, dataset.id, metadata_args, account, tenant.id) def test_create_metadata_name_already_exists( self, db_session_with_containers: Session, mock_external_service_dependencies: MetadataServiceDeps @@ -236,14 +238,16 @@ class TestMetadataService: # Create first metadata first_metadata_args = MetadataArgs(type="string", name="duplicate_name") - MetadataService.create_metadata(dataset.id, first_metadata_args, account, tenant.id) + MetadataService.create_metadata(db_session_with_containers, dataset.id, first_metadata_args, account, tenant.id) # Try to create second metadata with same name second_metadata_args = MetadataArgs(type="number", name="duplicate_name") # Act & Assert: Verify proper error handling with pytest.raises(ValueError, match="Metadata name already exists."): - MetadataService.create_metadata(dataset.id, second_metadata_args, account, tenant.id) + MetadataService.create_metadata( + db_session_with_containers, dataset.id, second_metadata_args, account, tenant.id + ) def test_create_metadata_name_conflicts_with_built_in_field( self, db_session_with_containers: Session, mock_external_service_dependencies: MetadataServiceDeps @@ -265,7 +269,7 @@ class TestMetadataService: # Act & Assert: Verify proper error handling with pytest.raises(ValueError, match="Metadata name already exists in Built-in fields."): - MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + MetadataService.create_metadata(db_session_with_containers, dataset.id, metadata_args, account, tenant.id) def test_update_metadata_name_success( self, db_session_with_containers: Session, mock_external_service_dependencies: MetadataServiceDeps @@ -283,11 +287,15 @@ class TestMetadataService: # Create metadata first metadata_args = MetadataArgs(type="string", name="old_name") - metadata = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Act: Execute the method under test new_name = "new_name" - result = MetadataService.update_metadata_name(dataset.id, metadata.id, new_name, account, tenant.id) + result = MetadataService.update_metadata_name( + db_session_with_containers, dataset.id, metadata.id, new_name, account, tenant.id + ) # Assert: Verify the expected outcomes assert result is not None @@ -316,14 +324,18 @@ class TestMetadataService: # Create metadata first metadata_args = MetadataArgs(type="string", name="old_name") - metadata = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Try to update with too long name long_name = "a" * 256 # 256 characters, exceeding 255 limit # Act & Assert: Verify proper error handling with pytest.raises(ValueError, match="Metadata name cannot exceed 255 characters."): - MetadataService.update_metadata_name(dataset.id, metadata.id, long_name, account, tenant.id) + MetadataService.update_metadata_name( + db_session_with_containers, dataset.id, metadata.id, long_name, account, tenant.id + ) def test_update_metadata_name_already_exists( self, db_session_with_containers: Session, mock_external_service_dependencies: MetadataServiceDeps @@ -341,14 +353,20 @@ class TestMetadataService: # Create two metadata entries first_metadata_args = MetadataArgs(type="string", name="first_metadata") - first_metadata = MetadataService.create_metadata(dataset.id, first_metadata_args, account, tenant.id) + first_metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, first_metadata_args, account, tenant.id + ) second_metadata_args = MetadataArgs(type="number", name="second_metadata") - second_metadata = MetadataService.create_metadata(dataset.id, second_metadata_args, account, tenant.id) + second_metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, second_metadata_args, account, tenant.id + ) # Try to update first metadata with second metadata's name with pytest.raises(ValueError, match="Metadata name already exists."): - MetadataService.update_metadata_name(dataset.id, first_metadata.id, "second_metadata", account, tenant.id) + MetadataService.update_metadata_name( + db_session_with_containers, dataset.id, first_metadata.id, "second_metadata", account, tenant.id + ) def test_update_metadata_name_conflicts_with_built_in_field( self, db_session_with_containers: Session, mock_external_service_dependencies: MetadataServiceDeps @@ -366,13 +384,17 @@ class TestMetadataService: # Create metadata first metadata_args = MetadataArgs(type="string", name="old_name") - metadata = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Try to update with built-in field name built_in_field_name = BuiltInField.document_name with pytest.raises(ValueError, match="Metadata name already exists in Built-in fields."): - MetadataService.update_metadata_name(dataset.id, metadata.id, built_in_field_name, account, tenant.id) + MetadataService.update_metadata_name( + db_session_with_containers, dataset.id, metadata.id, built_in_field_name, account, tenant.id + ) def test_update_metadata_name_not_found( self, db_session_with_containers: Session, mock_external_service_dependencies: MetadataServiceDeps @@ -395,7 +417,9 @@ class TestMetadataService: new_name = "new_name" # Act: Execute the method under test - result = MetadataService.update_metadata_name(dataset.id, fake_metadata_id, new_name, account, tenant.id) + result = MetadataService.update_metadata_name( + db_session_with_containers, dataset.id, fake_metadata_id, new_name, account, tenant.id + ) # Assert: Verify the method returns None when metadata is not found assert result is None @@ -416,10 +440,12 @@ class TestMetadataService: # Create metadata first metadata_args = MetadataArgs(type="string", name="to_be_deleted") - metadata = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Act: Execute the method under test - result = MetadataService.delete_metadata(dataset.id, metadata.id) + result = MetadataService.delete_metadata(db_session_with_containers, dataset.id, metadata.id) # Assert: Verify the expected outcomes assert result is not None @@ -450,7 +476,7 @@ class TestMetadataService: fake_metadata_id = str(uuid.uuid4()) # Use valid UUID format # Act: Execute the method under test - result = MetadataService.delete_metadata(dataset.id, fake_metadata_id) + result = MetadataService.delete_metadata(db_session_with_containers, dataset.id, fake_metadata_id) # Assert: Verify the method returns None when metadata is not found assert result is None @@ -474,7 +500,9 @@ class TestMetadataService: # Create metadata metadata_args = MetadataArgs(type="string", name="test_metadata") - metadata = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Create metadata binding binding = DatasetMetadataBinding( @@ -494,7 +522,7 @@ class TestMetadataService: db_session_with_containers.commit() # Act: Execute the method under test - result = MetadataService.delete_metadata(dataset.id, metadata.id) + result = MetadataService.delete_metadata(db_session_with_containers, dataset.id, metadata.id) # Assert: Verify the expected outcomes assert result is not None @@ -559,7 +587,7 @@ class TestMetadataService: assert dataset.built_in_field_enabled is False # Act: Execute the method under test - MetadataService.enable_built_in_field(dataset) + MetadataService.enable_built_in_field(db_session_with_containers, dataset) # Assert: Verify the expected outcomes @@ -595,7 +623,7 @@ class TestMetadataService: ]() # Act: Execute the method under test - MetadataService.enable_built_in_field(dataset) + MetadataService.enable_built_in_field(db_session_with_containers, dataset) # Assert: Verify the method returns early without changes db_session_with_containers.refresh(dataset) @@ -621,7 +649,7 @@ class TestMetadataService: ]() # Act: Execute the method under test - MetadataService.enable_built_in_field(dataset) + MetadataService.enable_built_in_field(db_session_with_containers, dataset) # Assert: Verify the expected outcomes @@ -668,7 +696,7 @@ class TestMetadataService: ] # Act: Execute the method under test - MetadataService.disable_built_in_field(dataset) + MetadataService.disable_built_in_field(db_session_with_containers, dataset) # Assert: Verify the expected outcomes db_session_with_containers.refresh(dataset) @@ -700,7 +728,7 @@ class TestMetadataService: ]() # Act: Execute the method under test - MetadataService.disable_built_in_field(dataset) + MetadataService.disable_built_in_field(db_session_with_containers, dataset) # Assert: Verify the method returns early without changes @@ -733,7 +761,7 @@ class TestMetadataService: ]() # Act: Execute the method under test - MetadataService.disable_built_in_field(dataset) + MetadataService.disable_built_in_field(db_session_with_containers, dataset) # Assert: Verify the expected outcomes db_session_with_containers.refresh(dataset) @@ -758,7 +786,9 @@ class TestMetadataService: # Create metadata metadata_args = MetadataArgs(type="string", name="test_metadata") - metadata = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Mock DocumentService.get_document mock_external_service_dependencies["document_service"].get_document.return_value = document @@ -777,7 +807,7 @@ class TestMetadataService: operation_data = MetadataOperationData(operation_data=[operation]) # Act: Execute the method under test - MetadataService.update_documents_metadata(dataset, operation_data, account) + MetadataService.update_documents_metadata(db_session_with_containers, dataset, operation_data, account) # Assert: Verify the expected outcomes @@ -822,7 +852,9 @@ class TestMetadataService: # Create metadata metadata_args = MetadataArgs(type="string", name="test_metadata") - metadata = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Mock DocumentService.get_document mock_external_service_dependencies["document_service"].get_document.return_value = document @@ -841,7 +873,7 @@ class TestMetadataService: operation_data = MetadataOperationData(operation_data=[operation]) # Act: Execute the method under test - MetadataService.update_documents_metadata(dataset, operation_data, account) + MetadataService.update_documents_metadata(db_session_with_containers, dataset, operation_data, account) # Assert: Verify the expected outcomes # Verify document metadata was updated with both custom and built-in fields @@ -869,7 +901,9 @@ class TestMetadataService: # Create metadata metadata_args = MetadataArgs(type="string", name="test_metadata") - metadata = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Create metadata operation data from services.entities.knowledge_entities.knowledge_entities import ( @@ -890,7 +924,7 @@ class TestMetadataService: # Act & Assert: The method should raise ValueError("Document not found.") # because the exception is now re-raised after rollback with pytest.raises(ValueError, match="Document not found"): - MetadataService.update_documents_metadata(dataset, operation_data, account) + MetadataService.update_documents_metadata(db_session_with_containers, dataset, operation_data, account) def test_knowledge_base_metadata_lock_check_dataset_id( self, db_session_with_containers: Session, mock_external_service_dependencies: MetadataServiceDeps @@ -986,7 +1020,9 @@ class TestMetadataService: # Create metadata metadata_args = MetadataArgs(type="string", name="test_metadata") - metadata = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Create document and metadata binding document = self._create_test_document( @@ -1005,7 +1041,7 @@ class TestMetadataService: db_session_with_containers.commit() # Act: Execute the method under test - result = MetadataService.get_dataset_metadatas(dataset) + result = MetadataService.get_dataset_metadatas(db_session_with_containers, dataset) # Assert: Verify the expected outcomes assert result is not None @@ -1045,10 +1081,12 @@ class TestMetadataService: # Create metadata metadata_args = MetadataArgs(type="string", name="test_metadata") - metadata = MetadataService.create_metadata(dataset.id, metadata_args, account, tenant.id) + metadata = MetadataService.create_metadata( + db_session_with_containers, dataset.id, metadata_args, account, tenant.id + ) # Act: Execute the method under test - result = MetadataService.get_dataset_metadatas(dataset) + result = MetadataService.get_dataset_metadatas(db_session_with_containers, dataset) # Assert: Verify the expected outcomes assert result is not None @@ -1077,7 +1115,7 @@ class TestMetadataService: ) # Act: Execute the method under test - result = MetadataService.get_dataset_metadatas(dataset) + result = MetadataService.get_dataset_metadatas(db_session_with_containers, dataset) # Assert: Verify the expected outcomes assert result is not None diff --git a/api/tests/unit_tests/controllers/service_api/dataset/test_metadata.py b/api/tests/unit_tests/controllers/service_api/dataset/test_metadata.py index 7a9978e742a..b77c783ae16 100644 --- a/api/tests/unit_tests/controllers/service_api/dataset/test_metadata.py +++ b/api/tests/unit_tests/controllers/service_api/dataset/test_metadata.py @@ -17,7 +17,7 @@ Decorator strategy: import uuid from inspect import unwrap -from unittest.mock import Mock, patch +from unittest.mock import ANY, Mock, patch import pytest from flask import Flask @@ -408,7 +408,7 @@ class TestDatasetMetadataBuiltInFieldAction: assert status == 200 assert response["result"] == "success" - mock_meta_svc.enable_built_in_field.assert_called_once_with(mock_dataset) + mock_meta_svc.enable_built_in_field.assert_called_once_with(ANY, mock_dataset) @patch("controllers.service_api.dataset.metadata.MetadataService") @patch("controllers.service_api.dataset.metadata.DatasetService") @@ -439,7 +439,7 @@ class TestDatasetMetadataBuiltInFieldAction: ) assert status == 200 - mock_meta_svc.disable_built_in_field.assert_called_once_with(mock_dataset) + mock_meta_svc.disable_built_in_field.assert_called_once_with(ANY, mock_dataset) @patch("controllers.service_api.dataset.metadata.DatasetService") def test_action_dataset_not_found( diff --git a/api/tests/unit_tests/services/test_metadata_bug_complete.py b/api/tests/unit_tests/services/test_metadata_bug_complete.py index 36ea1fac1a4..6792243e9d0 100644 --- a/api/tests/unit_tests/services/test_metadata_bug_complete.py +++ b/api/tests/unit_tests/services/test_metadata_bug_complete.py @@ -48,13 +48,15 @@ class TestMetadataBugCompleteValidation: account = _make_account() # Should crash with TypeError with pytest.raises(TypeError, match="object of type 'NoneType' has no len"): - MetadataService.create_metadata("dataset-123", mock_metadata_args, account, "tenant-123") + MetadataService.create_metadata(Mock(), "dataset-123", mock_metadata_args, account, "tenant-123") # Test update method as well account = _make_account() none_name = cast(str, None) with pytest.raises(TypeError, match="object of type 'NoneType' has no len"): - MetadataService.update_metadata_name("dataset-123", "metadata-456", none_name, account, "tenant-123") + MetadataService.update_metadata_name( + Mock(), "dataset-123", "metadata-456", none_name, account, "tenant-123" + ) def test_3_database_constraints_verification(self) -> None: """Test Layer 3: Verify database model has nullable=False constraints.""" @@ -97,7 +99,7 @@ class TestMetadataBugCompleteValidation: account = _make_account() with pytest.raises(TypeError, match="object of type 'NoneType' has no len"): - MetadataService.create_metadata("dataset-123", mock_metadata_args, account, "tenant-123") + MetadataService.create_metadata(Mock(), "dataset-123", mock_metadata_args, account, "tenant-123") def test_7_end_to_end_validation_layers(self) -> None: """Test all validation layers work together correctly.""" diff --git a/api/tests/unit_tests/services/test_metadata_nullable_bug.py b/api/tests/unit_tests/services/test_metadata_nullable_bug.py index 27570a86f1a..ae93fe5ef51 100644 --- a/api/tests/unit_tests/services/test_metadata_nullable_bug.py +++ b/api/tests/unit_tests/services/test_metadata_nullable_bug.py @@ -37,7 +37,7 @@ class TestMetadataNullableBug: account = _make_account() # This should crash with TypeError when calling len(None) with pytest.raises(TypeError, match="object of type 'NoneType' has no len"): - MetadataService.create_metadata("dataset-123", mock_metadata_args, account, "tenant-123") + MetadataService.create_metadata(Mock(), "dataset-123", mock_metadata_args, account, "tenant-123") def test_metadata_service_update_with_none_name_crashes(self) -> None: """Test that MetadataService.update_metadata_name crashes when name is None.""" @@ -45,7 +45,9 @@ class TestMetadataNullableBug: none_name = cast(str, None) # This should crash with TypeError when calling len(None) with pytest.raises(TypeError, match="object of type 'NoneType' has no len"): - MetadataService.update_metadata_name("dataset-123", "metadata-456", none_name, account, "tenant-123") + MetadataService.update_metadata_name( + Mock(), "dataset-123", "metadata-456", none_name, account, "tenant-123" + ) def test_api_layer_now_uses_pydantic_validation(self) -> None: """Verify that API layer relies on Pydantic validation instead of reqparse."""