mirror of
https://github.com/langgenius/dify.git
synced 2026-10-04 13:47:55 +08:00
fix(api): bind dataset operations to owners (#40149)
This commit is contained in:
1 parent
bee269afe8
commit
ef8544b173
35 files changed
+1520
-496
No files matched your search
+13
-38
@@ -11,13 +11,13 @@ from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from sqlalchemy.orm import Session
|
||||
from werkzeug.exceptions import NotFound
|
||||
|
||||
from core.rag.index_processor.constant.index_type import IndexTechniqueType
|
||||
from models import Account, AccountStatus, Tenant, TenantAccountJoin, TenantAccountRole, TenantStatus
|
||||
from models.dataset import AppDatasetJoin, Dataset, DatasetPermissionEnum
|
||||
from models.enums import DataSourceType
|
||||
from models.model import App
|
||||
from services.dataset_ref_service import DatasetRefService
|
||||
from services.dataset_service import DatasetService
|
||||
from services.errors.account import NoPermissionError
|
||||
|
||||
@@ -228,9 +228,10 @@ class TestDatasetServiceDatasetUseCheck:
|
||||
dataset = DatasetUpdateDeleteTestDataFactory.create_dataset(db_session_with_containers, tenant.id, owner.id)
|
||||
app = DatasetUpdateDeleteTestDataFactory.create_app(db_session_with_containers, tenant.id, owner.id)
|
||||
DatasetUpdateDeleteTestDataFactory.create_app_dataset_join(db_session_with_containers, app.id, dataset.id)
|
||||
dataset_ref = DatasetRefService.create_dataset_ref(dataset)
|
||||
|
||||
# Act
|
||||
result = DatasetService.dataset_use_check(dataset.id, session=db_session_with_containers)
|
||||
result = DatasetService.dataset_use_check(dataset_ref, session=db_session_with_containers)
|
||||
|
||||
# Assert
|
||||
assert result is True
|
||||
@@ -252,9 +253,10 @@ class TestDatasetServiceDatasetUseCheck:
|
||||
db_session_with_containers, role=TenantAccountRole.OWNER
|
||||
)
|
||||
dataset = DatasetUpdateDeleteTestDataFactory.create_dataset(db_session_with_containers, tenant.id, owner.id)
|
||||
dataset_ref = DatasetRefService.create_dataset_ref(dataset)
|
||||
|
||||
# Act
|
||||
result = DatasetService.dataset_use_check(dataset.id, session=db_session_with_containers)
|
||||
result = DatasetService.dataset_use_check(dataset_ref, session=db_session_with_containers)
|
||||
|
||||
# Assert
|
||||
assert result is False
|
||||
@@ -288,11 +290,8 @@ class TestDatasetServiceUpdateDatasetApiStatus:
|
||||
current_time = datetime.datetime(2023, 1, 1, 12, 0, 0)
|
||||
|
||||
# Act
|
||||
with (
|
||||
patch("services.dataset_service.current_user", owner),
|
||||
patch("services.dataset_service.naive_utc_now", return_value=current_time),
|
||||
):
|
||||
DatasetService.update_dataset_api_status(dataset.id, True, session=db_session_with_containers)
|
||||
with patch("services.dataset_service.naive_utc_now", return_value=current_time):
|
||||
DatasetService.update_dataset_api_status(dataset, True, owner, session=db_session_with_containers)
|
||||
|
||||
# Assert
|
||||
db_session_with_containers.refresh(dataset)
|
||||
@@ -323,36 +322,14 @@ class TestDatasetServiceUpdateDatasetApiStatus:
|
||||
current_time = datetime.datetime(2023, 1, 1, 12, 0, 0)
|
||||
|
||||
# Act
|
||||
with (
|
||||
patch("services.dataset_service.current_user", owner),
|
||||
patch("services.dataset_service.naive_utc_now", return_value=current_time),
|
||||
):
|
||||
DatasetService.update_dataset_api_status(dataset.id, False, session=db_session_with_containers)
|
||||
with patch("services.dataset_service.naive_utc_now", return_value=current_time):
|
||||
DatasetService.update_dataset_api_status(dataset, False, owner, session=db_session_with_containers)
|
||||
|
||||
# Assert
|
||||
db_session_with_containers.refresh(dataset)
|
||||
assert dataset.enable_api is False
|
||||
assert dataset.updated_by == owner.id
|
||||
|
||||
def test_update_dataset_api_status_not_found_error(self, db_session_with_containers: Session):
|
||||
"""
|
||||
Test error handling when dataset is not found.
|
||||
|
||||
Verifies that when the dataset ID doesn't exist, a NotFound
|
||||
exception is raised.
|
||||
|
||||
This test ensures:
|
||||
- NotFound exception is raised
|
||||
- No updates are performed
|
||||
- Error message is appropriate
|
||||
"""
|
||||
# Arrange
|
||||
dataset_id = str(uuid4())
|
||||
|
||||
# Act & Assert
|
||||
with pytest.raises(NotFound, match="Dataset not found"):
|
||||
DatasetService.update_dataset_api_status(dataset_id, True, session=db_session_with_containers)
|
||||
|
||||
def test_update_dataset_api_status_missing_current_user_error(self, db_session_with_containers: Session):
|
||||
"""
|
||||
Test error handling when current_user is missing.
|
||||
@@ -372,13 +349,11 @@ class TestDatasetServiceUpdateDatasetApiStatus:
|
||||
dataset = DatasetUpdateDeleteTestDataFactory.create_dataset(
|
||||
db_session_with_containers, tenant.id, owner.id, enable_api=False
|
||||
)
|
||||
|
||||
actor = Account(name="missing-id", email="missing-id@example.com")
|
||||
actor.id = ""
|
||||
# Act & Assert
|
||||
with (
|
||||
patch("services.dataset_service.current_user", None),
|
||||
pytest.raises(ValueError, match="Current user or current user id not found"),
|
||||
):
|
||||
DatasetService.update_dataset_api_status(dataset.id, True, session=db_session_with_containers)
|
||||
with pytest.raises(ValueError, match="Current user or current user id not found"):
|
||||
DatasetService.update_dataset_api_status(dataset, True, actor, session=db_session_with_containers)
|
||||
|
||||
# Verify no commit was attempted
|
||||
db_session_with_containers.rollback()
|
||||
|
||||
+11
-6
@@ -142,7 +142,9 @@ def test_get_document_queries_by_dataset_and_document_id(db_session_with_contain
|
||||
def test_get_documents_by_ids_returns_empty_for_empty_input(db_session_with_containers: Session):
|
||||
dataset = DocumentServiceIntegrationFactory.create_dataset(db_session_with_containers)
|
||||
|
||||
result = DocumentService.get_documents_by_ids(dataset.id, [], session=db_session_with_containers)
|
||||
result = DocumentService.get_documents_by_ids(
|
||||
DatasetRefService.create_dataset_ref(dataset), [], session=db_session_with_containers
|
||||
)
|
||||
|
||||
assert result == []
|
||||
|
||||
@@ -157,7 +159,9 @@ def test_get_documents_by_ids_uses_single_batch_query(db_session_with_containers
|
||||
position=2,
|
||||
)
|
||||
|
||||
result = DocumentService.get_documents_by_ids(dataset.id, [doc_a.id, doc_b.id], db_session_with_containers)
|
||||
result = DocumentService.get_documents_by_ids(
|
||||
DatasetRefService.create_dataset_ref(dataset), [doc_a.id, doc_b.id], db_session_with_containers
|
||||
)
|
||||
|
||||
assert {document.id for document in result} == {doc_a.id, doc_b.id}
|
||||
|
||||
@@ -319,7 +323,7 @@ def test_get_upload_files_by_document_id_for_zip_download_raises_for_missing_doc
|
||||
)
|
||||
|
||||
|
||||
def test_get_upload_files_by_document_id_for_zip_download_rejects_cross_tenant_access(
|
||||
def test_get_upload_files_by_document_id_for_zip_download_hides_cross_tenant_documents(
|
||||
db_session_with_containers: Session,
|
||||
):
|
||||
dataset = DocumentServiceIntegrationFactory.create_dataset(db_session_with_containers)
|
||||
@@ -335,7 +339,7 @@ def test_get_upload_files_by_document_id_for_zip_download_rejects_cross_tenant_a
|
||||
data_source_info={"upload_file_id": upload_file.id},
|
||||
)
|
||||
|
||||
with pytest.raises(Forbidden, match="No permission"):
|
||||
with pytest.raises(NotFound, match="Document not found"):
|
||||
DocumentService._get_upload_files_by_document_id_for_zip_download(
|
||||
dataset_id=dataset.id,
|
||||
document_ids=[document.id],
|
||||
@@ -527,7 +531,7 @@ def test_get_working_documents_by_dataset_id_returns_completed_enabled_unarchive
|
||||
assert [document.id for document in result] == [available_document.id]
|
||||
|
||||
|
||||
def test_get_error_documents_by_dataset_id_returns_error_and_paused_documents(db_session_with_containers: Session):
|
||||
def test_get_error_documents_by_dataset_ref_returns_error_and_paused_documents(db_session_with_containers: Session):
|
||||
dataset = DocumentServiceIntegrationFactory.create_dataset(db_session_with_containers)
|
||||
error_document = DocumentServiceIntegrationFactory.create_document(
|
||||
db_session_with_containers,
|
||||
@@ -547,7 +551,8 @@ def test_get_error_documents_by_dataset_id_returns_error_and_paused_documents(db
|
||||
indexing_status=IndexingStatus.COMPLETED,
|
||||
)
|
||||
|
||||
result = DocumentService.get_error_documents_by_dataset_id(dataset.id, session=db_session_with_containers)
|
||||
dataset_ref = DatasetRefService.create_dataset_ref(dataset)
|
||||
result = DocumentService.get_error_documents_by_dataset_ref(dataset_ref, session=db_session_with_containers)
|
||||
|
||||
assert {document.id for document in result} == {error_document.id, paused_document.id}
|
||||
|
||||
|
||||
+19
-29
@@ -8,7 +8,6 @@ from uuid import uuid4
|
||||
import pytest
|
||||
from flask import Flask
|
||||
from sqlalchemy.orm import Session
|
||||
from werkzeug.exceptions import NotFound
|
||||
|
||||
from core.rag.index_processor.constant.index_type import IndexTechniqueType
|
||||
from models.account import Account, Tenant, TenantAccountJoin, TenantAccountRole
|
||||
@@ -21,6 +20,7 @@ from models.dataset import (
|
||||
DatasetPermissionEnum,
|
||||
)
|
||||
from models.enums import DataSourceType
|
||||
from services.dataset_ref_service import DatasetRef, DatasetRefService
|
||||
from services.dataset_service import DatasetCollectionBindingService, DatasetPermissionService, DatasetService
|
||||
from services.errors.account import NoPermissionError
|
||||
|
||||
@@ -213,7 +213,9 @@ class TestDatasetServicePermissionsAndLifecycle:
|
||||
dataset_id=dataset.id,
|
||||
)
|
||||
|
||||
assert DatasetService.dataset_use_check(dataset.id, session=db_session_with_containers) is True
|
||||
dataset_ref = DatasetRefService.create_dataset_ref(dataset)
|
||||
|
||||
assert DatasetService.dataset_use_check(dataset_ref, session=db_session_with_containers) is True
|
||||
|
||||
def test_dataset_use_check_returns_false_when_join_missing(self, db_session_with_containers: Session):
|
||||
owner, tenant = DatasetPermissionIntegrationFactory.create_account_with_tenant(db_session_with_containers)
|
||||
@@ -223,7 +225,9 @@ class TestDatasetServicePermissionsAndLifecycle:
|
||||
created_by=owner.id,
|
||||
)
|
||||
|
||||
assert DatasetService.dataset_use_check(dataset.id, session=db_session_with_containers) is False
|
||||
dataset_ref = DatasetRefService.create_dataset_ref(dataset)
|
||||
|
||||
assert DatasetService.dataset_use_check(dataset_ref, session=db_session_with_containers) is False
|
||||
|
||||
def test_check_dataset_permission_rejects_cross_tenant_access(self, db_session_with_containers: Session):
|
||||
owner, tenant = DatasetPermissionIntegrationFactory.create_account_with_tenant(db_session_with_containers)
|
||||
@@ -371,13 +375,6 @@ class TestDatasetServicePermissionsAndLifecycle:
|
||||
user=operator, dataset=dataset, session=db_session_with_containers
|
||||
)
|
||||
|
||||
def test_update_dataset_api_status_raises_not_found_for_missing_dataset(
|
||||
self, flask_app_with_containers: Flask, db_session_with_containers: Session
|
||||
):
|
||||
with flask_app_with_containers.app_context():
|
||||
with pytest.raises(NotFound, match="Dataset not found"):
|
||||
DatasetService.update_dataset_api_status(str(uuid4()), True, session=db_session_with_containers)
|
||||
|
||||
def test_update_dataset_api_status_requires_current_user_id(self, db_session_with_containers: Session):
|
||||
owner, tenant = DatasetPermissionIntegrationFactory.create_account_with_tenant(db_session_with_containers)
|
||||
dataset = DatasetPermissionIntegrationFactory.create_dataset(
|
||||
@@ -386,10 +383,10 @@ class TestDatasetServicePermissionsAndLifecycle:
|
||||
created_by=owner.id,
|
||||
enable_api=False,
|
||||
)
|
||||
|
||||
with patch("services.dataset_service.current_user", SimpleNamespace(id=None)):
|
||||
with pytest.raises(ValueError, match="Current user or current user id not found"):
|
||||
DatasetService.update_dataset_api_status(dataset.id, True, session=db_session_with_containers)
|
||||
actor = Account(name="missing-id", email="missing-id@example.com")
|
||||
actor.id = ""
|
||||
with pytest.raises(ValueError, match="Current user or current user id not found"):
|
||||
DatasetService.update_dataset_api_status(dataset, True, actor, session=db_session_with_containers)
|
||||
|
||||
def test_update_dataset_api_status_updates_fields_and_commits(self, db_session_with_containers: Session):
|
||||
owner, tenant = DatasetPermissionIntegrationFactory.create_account_with_tenant(db_session_with_containers)
|
||||
@@ -401,11 +398,8 @@ class TestDatasetServicePermissionsAndLifecycle:
|
||||
)
|
||||
now = datetime(2026, 4, 14, 18, 0, 0)
|
||||
|
||||
with (
|
||||
patch("services.dataset_service.current_user", owner),
|
||||
patch("services.dataset_service.naive_utc_now", return_value=now),
|
||||
):
|
||||
DatasetService.update_dataset_api_status(dataset.id, True, session=db_session_with_containers)
|
||||
with patch("services.dataset_service.naive_utc_now", return_value=now):
|
||||
DatasetService.update_dataset_api_status(dataset, True, owner, session=db_session_with_containers)
|
||||
|
||||
db_session_with_containers.refresh(dataset)
|
||||
assert dataset.enable_api is True
|
||||
@@ -419,12 +413,10 @@ class TestDatasetServicePermissionsAndLifecycle:
|
||||
features = SimpleNamespace(
|
||||
billing=SimpleNamespace(enabled=False, subscription=SimpleNamespace(plan="professional"))
|
||||
)
|
||||
dataset_ref = DatasetRef(tenant_id=tenant.id, dataset_id=str(uuid4()))
|
||||
|
||||
with (
|
||||
patch("services.dataset_service.current_user", owner),
|
||||
patch("services.dataset_service.FeatureService.get_features", return_value=features),
|
||||
):
|
||||
result = DatasetService.get_dataset_auto_disable_logs(str(uuid4()), session=db_session_with_containers)
|
||||
with patch("services.dataset_service.FeatureService.get_features", return_value=features):
|
||||
result = DatasetService.get_dataset_auto_disable_logs(dataset_ref, session=db_session_with_containers)
|
||||
|
||||
assert result == {"document_ids": [], "count": 0}
|
||||
|
||||
@@ -450,12 +442,10 @@ class TestDatasetServicePermissionsAndLifecycle:
|
||||
features = SimpleNamespace(
|
||||
billing=SimpleNamespace(enabled=True, subscription=SimpleNamespace(plan="professional"))
|
||||
)
|
||||
dataset_ref = DatasetRefService.create_dataset_ref(dataset)
|
||||
|
||||
with (
|
||||
patch("services.dataset_service.current_user", owner),
|
||||
patch("services.dataset_service.FeatureService.get_features", return_value=features),
|
||||
):
|
||||
result = DatasetService.get_dataset_auto_disable_logs(dataset.id, session=db_session_with_containers)
|
||||
with patch("services.dataset_service.FeatureService.get_features", return_value=features):
|
||||
result = DatasetService.get_dataset_auto_disable_logs(dataset_ref, session=db_session_with_containers)
|
||||
|
||||
assert result["count"] == 2
|
||||
assert len(result["document_ids"]) == 2
|
||||
|
||||
+117
-14
@@ -1,16 +1,18 @@
|
||||
from __future__ import annotations
|
||||
|
||||
from concurrent.futures import ThreadPoolExecutor
|
||||
from threading import Barrier
|
||||
from unittest.mock import patch
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from flask import Flask
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy.orm import Session
|
||||
from sqlalchemy import event, select
|
||||
from sqlalchemy.orm import Session, sessionmaker
|
||||
|
||||
from models import Account, Tenant
|
||||
from models.dataset import Dataset, DatasetMetadataBinding, Document
|
||||
from models.enums import DataSourceType, DocumentCreatedFrom
|
||||
from models.dataset import Dataset, DatasetMetadata, DatasetMetadataBinding, Document
|
||||
from models.enums import DatasetMetadataType, DataSourceType, DocumentCreatedFrom
|
||||
from services.entities.knowledge_entities.knowledge_entities import (
|
||||
DocumentMetadataOperation,
|
||||
MetadataDetail,
|
||||
@@ -54,6 +56,19 @@ def _create_document(
|
||||
return document
|
||||
|
||||
|
||||
def _create_metadata(db_session: Session, *, dataset: Dataset, created_by: str, name: str) -> DatasetMetadata:
|
||||
metadata = DatasetMetadata(
|
||||
tenant_id=dataset.tenant_id,
|
||||
dataset_id=dataset.id,
|
||||
type=DatasetMetadataType.STRING,
|
||||
name=name,
|
||||
created_by=created_by,
|
||||
)
|
||||
db_session.add(metadata)
|
||||
db_session.commit()
|
||||
return metadata
|
||||
|
||||
|
||||
class TestMetadataPartialUpdate:
|
||||
@pytest.fixture
|
||||
def tenant_id(self) -> str:
|
||||
@@ -87,10 +102,12 @@ class TestMetadataPartialUpdate:
|
||||
doc_metadata={"existing_key": "existing_value"},
|
||||
)
|
||||
|
||||
meta_id = str(uuid4())
|
||||
metadata = _create_metadata(
|
||||
db_session_with_containers, dataset=dataset, created_by=current_account.id, name="new_key"
|
||||
)
|
||||
operation = DocumentMetadataOperation(
|
||||
document_id=document.id,
|
||||
metadata_list=[MetadataDetail(id=meta_id, name="new_key", value="new_value")],
|
||||
metadata_list=[MetadataDetail(id=metadata.id, name=metadata.name, value="new_value")],
|
||||
partial_update=True,
|
||||
)
|
||||
metadata_args = MetadataOperationData(operation_data=[operation])
|
||||
@@ -105,6 +122,86 @@ class TestMetadataPartialUpdate:
|
||||
assert updated_doc.doc_metadata["existing_key"] == "existing_value"
|
||||
assert updated_doc.doc_metadata["new_key"] == "new_value"
|
||||
|
||||
def test_concurrent_partial_updates_keep_both_values(
|
||||
self,
|
||||
db_session_with_containers: Session,
|
||||
tenant_id: str,
|
||||
current_account: Account,
|
||||
) -> None:
|
||||
dataset = _create_dataset(db_session_with_containers, tenant_id=tenant_id)
|
||||
document = _create_document(
|
||||
db_session_with_containers,
|
||||
dataset_id=dataset.id,
|
||||
tenant_id=tenant_id,
|
||||
doc_metadata={},
|
||||
)
|
||||
metadatas = [
|
||||
_create_metadata(
|
||||
db_session_with_containers,
|
||||
dataset=dataset,
|
||||
created_by=current_account.id,
|
||||
name=name,
|
||||
)
|
||||
for name in ("author", "region")
|
||||
]
|
||||
operations = [
|
||||
MetadataOperationData(
|
||||
operation_data=[
|
||||
DocumentMetadataOperation(
|
||||
document_id=document.id,
|
||||
metadata_list=[MetadataDetail(id=metadata.id, name=metadata.name, value=value)],
|
||||
partial_update=True,
|
||||
)
|
||||
]
|
||||
)
|
||||
for metadata, value in zip(metadatas, ("Alice", "EU"), strict=True)
|
||||
]
|
||||
dataset_id = dataset.id
|
||||
document_id = document.id
|
||||
session_factory = sessionmaker(bind=db_session_with_containers.get_bind())
|
||||
engine = db_session_with_containers.get_bind()
|
||||
locked_select_barrier = Barrier(2)
|
||||
locked_selects: list[None] = []
|
||||
|
||||
def synchronize_locked_select(
|
||||
_conn: object,
|
||||
_cursor: object,
|
||||
statement: str,
|
||||
_parameters: object,
|
||||
_context: object,
|
||||
_executemany: bool,
|
||||
) -> None:
|
||||
if "FROM documents" in statement and "FOR UPDATE" in statement:
|
||||
locked_selects.append(None)
|
||||
locked_select_barrier.wait(timeout=10)
|
||||
|
||||
def update(metadata_args: MetadataOperationData) -> None:
|
||||
with session_factory() as session:
|
||||
owned_dataset = session.get(Dataset, dataset_id)
|
||||
assert owned_dataset is not None
|
||||
MetadataService.update_documents_metadata(
|
||||
owned_dataset, metadata_args, current_account, session=session
|
||||
)
|
||||
|
||||
event.listen(engine, "before_cursor_execute", synchronize_locked_select)
|
||||
try:
|
||||
with (
|
||||
patch.object(MetadataService, "knowledge_base_metadata_lock_check"),
|
||||
patch("services.metadata_service.redis_client.delete"),
|
||||
ThreadPoolExecutor(max_workers=2) as executor,
|
||||
):
|
||||
futures = [executor.submit(update, operation) for operation in operations]
|
||||
for future in futures:
|
||||
future.result(timeout=20)
|
||||
finally:
|
||||
event.remove(engine, "before_cursor_execute", synchronize_locked_select)
|
||||
|
||||
db_session_with_containers.expire_all()
|
||||
updated_doc = db_session_with_containers.get(Document, document_id)
|
||||
assert updated_doc is not None
|
||||
assert len(locked_selects) == 2
|
||||
assert updated_doc.doc_metadata == {"author": "Alice", "region": "EU"}
|
||||
|
||||
def test_full_update_replaces_metadata(
|
||||
self,
|
||||
flask_app_with_containers: Flask,
|
||||
@@ -120,10 +217,12 @@ class TestMetadataPartialUpdate:
|
||||
doc_metadata={"existing_key": "existing_value"},
|
||||
)
|
||||
|
||||
meta_id = str(uuid4())
|
||||
metadata = _create_metadata(
|
||||
db_session_with_containers, dataset=dataset, created_by=current_account.id, name="new_key"
|
||||
)
|
||||
operation = DocumentMetadataOperation(
|
||||
document_id=document.id,
|
||||
metadata_list=[MetadataDetail(id=meta_id, name="new_key", value="new_value")],
|
||||
metadata_list=[MetadataDetail(id=metadata.id, name=metadata.name, value="new_value")],
|
||||
partial_update=False,
|
||||
)
|
||||
metadata_args = MetadataOperationData(operation_data=[operation])
|
||||
@@ -154,12 +253,14 @@ class TestMetadataPartialUpdate:
|
||||
doc_metadata={"existing_key": "existing_value"},
|
||||
)
|
||||
|
||||
meta_id = str(uuid4())
|
||||
metadata = _create_metadata(
|
||||
db_session_with_containers, dataset=dataset, created_by=current_account.id, name="existing_key"
|
||||
)
|
||||
existing_binding = DatasetMetadataBinding(
|
||||
tenant_id=tenant_id,
|
||||
dataset_id=dataset.id,
|
||||
document_id=document.id,
|
||||
metadata_id=meta_id,
|
||||
metadata_id=metadata.id,
|
||||
created_by=user_id,
|
||||
)
|
||||
db_session_with_containers.add(existing_binding)
|
||||
@@ -167,7 +268,7 @@ class TestMetadataPartialUpdate:
|
||||
|
||||
operation = DocumentMetadataOperation(
|
||||
document_id=document.id,
|
||||
metadata_list=[MetadataDetail(id=meta_id, name="existing_key", value="existing_value")],
|
||||
metadata_list=[MetadataDetail(id=metadata.id, name=metadata.name, value="existing_value")],
|
||||
partial_update=True,
|
||||
)
|
||||
metadata_args = MetadataOperationData(operation_data=[operation])
|
||||
@@ -180,7 +281,7 @@ class TestMetadataPartialUpdate:
|
||||
bindings = db_session_with_containers.scalars(
|
||||
select(DatasetMetadataBinding).where(
|
||||
DatasetMetadataBinding.document_id == document.id,
|
||||
DatasetMetadataBinding.metadata_id == meta_id,
|
||||
DatasetMetadataBinding.metadata_id == metadata.id,
|
||||
)
|
||||
).all()
|
||||
assert len(bindings) == 1
|
||||
@@ -200,10 +301,12 @@ class TestMetadataPartialUpdate:
|
||||
doc_metadata={"existing_key": "existing_value"},
|
||||
)
|
||||
|
||||
meta_id = str(uuid4())
|
||||
metadata = _create_metadata(
|
||||
db_session_with_containers, dataset=dataset, created_by=current_account.id, name="key"
|
||||
)
|
||||
operation = DocumentMetadataOperation(
|
||||
document_id=document.id,
|
||||
metadata_list=[MetadataDetail(id=meta_id, name="key", value="value")],
|
||||
metadata_list=[MetadataDetail(id=metadata.id, name=metadata.name, value="value")],
|
||||
partial_update=True,
|
||||
)
|
||||
metadata_args = MetadataOperationData(operation_data=[operation])
|
||||
|
||||
@@ -10,8 +10,14 @@ from core.rag.index_processor.constant.built_in_field import BuiltInField
|
||||
from core.rag.index_processor.constant.index_type import IndexStructureType
|
||||
from models import Account, AccountStatus, Tenant, TenantAccountJoin, TenantAccountRole, TenantStatus
|
||||
from models.dataset import Dataset, DatasetMetadata, DatasetMetadataBinding, Document
|
||||
from models.enums import DataSourceType, DocumentCreatedFrom
|
||||
from services.entities.knowledge_entities.knowledge_entities import MetadataArgs
|
||||
from models.enums import DataSourceType, DocumentCreatedFrom, IndexingStatus
|
||||
from services.entities.knowledge_entities.knowledge_entities import (
|
||||
DocumentMetadataOperation,
|
||||
MetadataArgs,
|
||||
MetadataDetail,
|
||||
MetadataOperationData,
|
||||
)
|
||||
from services.errors.metadata import MetadataResourceNotFoundError
|
||||
from services.metadata_service import MetadataService
|
||||
|
||||
|
||||
@@ -300,7 +306,7 @@ class TestMetadataService:
|
||||
# Act: Execute the method under test
|
||||
new_name = "new_name"
|
||||
result = MetadataService.update_metadata_name(
|
||||
dataset.id, metadata.id, new_name, account, tenant.id, session=db_session_with_containers
|
||||
dataset, metadata.id, new_name, account, session=db_session_with_containers
|
||||
)
|
||||
|
||||
# Assert: Verify the expected outcomes
|
||||
@@ -340,7 +346,7 @@ class TestMetadataService:
|
||||
# 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, session=db_session_with_containers
|
||||
dataset, metadata.id, long_name, account, session=db_session_with_containers
|
||||
)
|
||||
|
||||
def test_update_metadata_name_already_exists(
|
||||
@@ -371,7 +377,7 @@ class TestMetadataService:
|
||||
# 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, session=db_session_with_containers
|
||||
dataset, first_metadata.id, "second_metadata", account, session=db_session_with_containers
|
||||
)
|
||||
|
||||
def test_update_metadata_name_conflicts_with_built_in_field(
|
||||
@@ -399,7 +405,7 @@ class TestMetadataService:
|
||||
|
||||
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, session=db_session_with_containers
|
||||
dataset, metadata.id, built_in_field_name, account, session=db_session_with_containers
|
||||
)
|
||||
|
||||
def test_update_metadata_name_not_found(
|
||||
@@ -424,7 +430,7 @@ class TestMetadataService:
|
||||
|
||||
# Act: Execute the method under test
|
||||
result = MetadataService.update_metadata_name(
|
||||
dataset.id, fake_metadata_id, new_name, account, tenant.id, session=db_session_with_containers
|
||||
dataset, fake_metadata_id, new_name, account, session=db_session_with_containers
|
||||
)
|
||||
|
||||
# Assert: Verify the method returns None when metadata is not found
|
||||
@@ -451,7 +457,7 @@ class TestMetadataService:
|
||||
)
|
||||
|
||||
# Act: Execute the method under test
|
||||
result = MetadataService.delete_metadata(dataset.id, metadata.id, session=db_session_with_containers)
|
||||
result = MetadataService.delete_metadata(dataset, metadata.id, session=db_session_with_containers)
|
||||
|
||||
# Assert: Verify the expected outcomes
|
||||
assert result is not None
|
||||
@@ -482,7 +488,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, session=db_session_with_containers)
|
||||
result = MetadataService.delete_metadata(dataset, fake_metadata_id, session=db_session_with_containers)
|
||||
|
||||
# Assert: Verify the method returns None when metadata is not found
|
||||
assert result is None
|
||||
@@ -528,7 +534,7 @@ class TestMetadataService:
|
||||
db_session_with_containers.commit()
|
||||
|
||||
# Act: Execute the method under test
|
||||
result = MetadataService.delete_metadata(dataset.id, metadata.id, session=db_session_with_containers)
|
||||
result = MetadataService.delete_metadata(dataset, metadata.id, session=db_session_with_containers)
|
||||
|
||||
# Assert: Verify the expected outcomes
|
||||
assert result is not None
|
||||
@@ -540,6 +546,69 @@ class TestMetadataService:
|
||||
# Note: The service attempts to update document metadata but may not succeed
|
||||
# due to mock configuration. The main functionality (metadata deletion) is verified.
|
||||
|
||||
@pytest.mark.parametrize("operation", ["rename", "delete"])
|
||||
@pytest.mark.parametrize("binding_owner", ["metadata", "document"])
|
||||
def test_metadata_changes_ignore_historical_foreign_document_binding(
|
||||
self,
|
||||
operation: str,
|
||||
binding_owner: str,
|
||||
db_session_with_containers: Session,
|
||||
mock_external_service_dependencies: MetadataServiceDeps,
|
||||
) -> None:
|
||||
account, tenant = self._create_test_account_and_tenant(
|
||||
db_session_with_containers, mock_external_service_dependencies
|
||||
)
|
||||
dataset = self._create_test_dataset(
|
||||
db_session_with_containers, mock_external_service_dependencies, account, tenant
|
||||
)
|
||||
foreign_account, foreign_tenant = self._create_test_account_and_tenant(
|
||||
db_session_with_containers, mock_external_service_dependencies
|
||||
)
|
||||
foreign_dataset = self._create_test_dataset(
|
||||
db_session_with_containers,
|
||||
mock_external_service_dependencies,
|
||||
foreign_account,
|
||||
foreign_tenant,
|
||||
)
|
||||
foreign_document = self._create_test_document(
|
||||
db_session_with_containers,
|
||||
mock_external_service_dependencies,
|
||||
foreign_dataset,
|
||||
foreign_account,
|
||||
)
|
||||
foreign_document.enabled = True
|
||||
foreign_document.archived = False
|
||||
foreign_document.indexing_status = IndexingStatus.COMPLETED
|
||||
foreign_document.doc_metadata = {"old_name": "foreign-value"}
|
||||
|
||||
metadata = MetadataService.create_metadata(
|
||||
dataset.id,
|
||||
MetadataArgs(type="string", name="old_name"),
|
||||
account,
|
||||
tenant.id,
|
||||
session=db_session_with_containers,
|
||||
)
|
||||
db_session_with_containers.add(
|
||||
DatasetMetadataBinding(
|
||||
tenant_id=dataset.tenant_id if binding_owner == "metadata" else foreign_dataset.tenant_id,
|
||||
dataset_id=dataset.id if binding_owner == "metadata" else foreign_dataset.id,
|
||||
metadata_id=metadata.id,
|
||||
document_id=foreign_document.id,
|
||||
created_by=account.id,
|
||||
)
|
||||
)
|
||||
db_session_with_containers.commit()
|
||||
|
||||
if operation == "rename":
|
||||
MetadataService.update_metadata_name(
|
||||
dataset, metadata.id, "new_name", account, session=db_session_with_containers
|
||||
)
|
||||
else:
|
||||
MetadataService.delete_metadata(dataset, metadata.id, session=db_session_with_containers)
|
||||
|
||||
db_session_with_containers.refresh(foreign_document)
|
||||
assert foreign_document.doc_metadata == {"old_name": "foreign-value"}
|
||||
|
||||
def test_get_built_in_fields_success(
|
||||
self, db_session_with_containers: Session, mock_external_service_dependencies: MetadataServiceDeps
|
||||
) -> None:
|
||||
@@ -799,13 +868,6 @@ class TestMetadataService:
|
||||
# Mock DocumentService.get_document
|
||||
mock_external_service_dependencies["document_service"].get_document.return_value = document
|
||||
|
||||
# Create metadata operation data
|
||||
from services.entities.knowledge_entities.knowledge_entities import (
|
||||
DocumentMetadataOperation,
|
||||
MetadataDetail,
|
||||
MetadataOperationData,
|
||||
)
|
||||
|
||||
metadata_detail = MetadataDetail(id=metadata.id, name=metadata.name, value="test_value")
|
||||
|
||||
operation = DocumentMetadataOperation(document_id=document.id, metadata_list=[metadata_detail])
|
||||
@@ -833,6 +895,77 @@ class TestMetadataService:
|
||||
assert binding.tenant_id == tenant.id
|
||||
assert binding.dataset_id == dataset.id
|
||||
|
||||
@pytest.mark.parametrize("foreign_resource", ["metadata", "document"])
|
||||
def test_update_documents_metadata_rejects_foreign_owner_before_writes(
|
||||
self,
|
||||
foreign_resource: str,
|
||||
db_session_with_containers: Session,
|
||||
mock_external_service_dependencies: MetadataServiceDeps,
|
||||
) -> None:
|
||||
account, tenant = self._create_test_account_and_tenant(
|
||||
db_session_with_containers, mock_external_service_dependencies
|
||||
)
|
||||
dataset = self._create_test_dataset(
|
||||
db_session_with_containers, mock_external_service_dependencies, account, tenant
|
||||
)
|
||||
document = self._create_test_document(
|
||||
db_session_with_containers, mock_external_service_dependencies, dataset, account
|
||||
)
|
||||
metadata = MetadataService.create_metadata(
|
||||
dataset.id,
|
||||
MetadataArgs(type="string", name="owned"),
|
||||
account,
|
||||
tenant.id,
|
||||
session=db_session_with_containers,
|
||||
)
|
||||
|
||||
foreign_account, foreign_tenant = self._create_test_account_and_tenant(
|
||||
db_session_with_containers, mock_external_service_dependencies
|
||||
)
|
||||
foreign_dataset = self._create_test_dataset(
|
||||
db_session_with_containers,
|
||||
mock_external_service_dependencies,
|
||||
foreign_account,
|
||||
foreign_tenant,
|
||||
)
|
||||
foreign_document = self._create_test_document(
|
||||
db_session_with_containers,
|
||||
mock_external_service_dependencies,
|
||||
foreign_dataset,
|
||||
foreign_account,
|
||||
)
|
||||
foreign_metadata = MetadataService.create_metadata(
|
||||
foreign_dataset.id,
|
||||
MetadataArgs(type="string", name="foreign"),
|
||||
foreign_account,
|
||||
foreign_tenant.id,
|
||||
session=db_session_with_containers,
|
||||
)
|
||||
operation = DocumentMetadataOperation(
|
||||
document_id=foreign_document.id if foreign_resource == "document" else document.id,
|
||||
metadata_list=[
|
||||
MetadataDetail(
|
||||
id=foreign_metadata.id if foreign_resource == "metadata" else metadata.id,
|
||||
name="ignored",
|
||||
value="value",
|
||||
)
|
||||
],
|
||||
)
|
||||
|
||||
with pytest.raises(MetadataResourceNotFoundError, match=f"{foreign_resource.capitalize()} not found"):
|
||||
MetadataService.update_documents_metadata(
|
||||
dataset,
|
||||
MetadataOperationData(operation_data=[operation]),
|
||||
account,
|
||||
session=db_session_with_containers,
|
||||
)
|
||||
|
||||
db_session_with_containers.refresh(document)
|
||||
db_session_with_containers.refresh(foreign_document)
|
||||
assert document.doc_metadata is None
|
||||
assert foreign_document.doc_metadata is None
|
||||
assert db_session_with_containers.query(DatasetMetadataBinding).count() == 0
|
||||
|
||||
def test_update_documents_metadata_with_built_in_fields_enabled(
|
||||
self, db_session_with_containers: Session, mock_external_service_dependencies: MetadataServiceDeps
|
||||
) -> None:
|
||||
@@ -865,13 +998,6 @@ class TestMetadataService:
|
||||
# Mock DocumentService.get_document
|
||||
mock_external_service_dependencies["document_service"].get_document.return_value = document
|
||||
|
||||
# Create metadata operation data
|
||||
from services.entities.knowledge_entities.knowledge_entities import (
|
||||
DocumentMetadataOperation,
|
||||
MetadataDetail,
|
||||
MetadataOperationData,
|
||||
)
|
||||
|
||||
metadata_detail = MetadataDetail(id=metadata.id, name=metadata.name, value="test_value")
|
||||
|
||||
operation = DocumentMetadataOperation(document_id=document.id, metadata_list=[metadata_detail])
|
||||
@@ -911,13 +1037,6 @@ class TestMetadataService:
|
||||
dataset.id, metadata_args, account, tenant.id, session=db_session_with_containers
|
||||
)
|
||||
|
||||
# Create metadata operation data
|
||||
from services.entities.knowledge_entities.knowledge_entities import (
|
||||
DocumentMetadataOperation,
|
||||
MetadataDetail,
|
||||
MetadataOperationData,
|
||||
)
|
||||
|
||||
metadata_detail = MetadataDetail(id=metadata.id, name=metadata.name, value="test_value")
|
||||
|
||||
# Use a valid UUID format that does not exist in the database
|
||||
@@ -927,9 +1046,7 @@ class TestMetadataService:
|
||||
|
||||
operation_data = MetadataOperationData(operation_data=[operation])
|
||||
|
||||
# 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"):
|
||||
with pytest.raises(MetadataResourceNotFoundError, match="Document not found"):
|
||||
MetadataService.update_documents_metadata(
|
||||
dataset, operation_data, account, session=db_session_with_containers
|
||||
)
|
||||
|
||||
Reference in new issue
Block a user