opensource/dify
3
0
Fork 0
mirror of https://github.com/langgenius/dify.git synced 2026-09-24 23:22:26 +08:00
Code Issues Packages Projects Releases Wiki Activity

refactor: for db.session feedback service.export feedbacks (#37763)

Browse Source 
Co-authored-by: kunalj1-arch <kunal.j1@turing.com>
Co-authored-by: Asuka Minato <i@asukaminato.eu.org>
This commit is contained in:
kunal
2026-06-23 14:01:40 +00:00
committed by GitHub
co-authored by kunalj1-arch Asuka Minato
parent acf6d0ddc9
commit 5b453069d1
11 changed files with 173 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files
api/tests/test_containers_integration_tests/services/test_feedback_service.py
+22 -11
View File
@@ -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))
api/tests/test_containers_integration_tests/services/test_metadata_partial_update.py
+7 -5
View File
@@ -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
)
api/tests/test_containers_integration_tests/services/test_metadata_service.py
+75 -37
View File
@@ -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
api/tests/unit_tests/controllers/service_api/dataset/test_metadata.py
+3 -3
View File
@@ -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(
api/tests/unit_tests/services/test_metadata_bug_complete.py
+5 -3
View File
@@ -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."""
api/tests/unit_tests/services/test_metadata_nullable_bug.py
+4 -2
View File
@@ -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."""