langgenius/dify
1
0
Fork 0
mirror of https://github.com/langgenius/dify.git synced 2026-07-21 12:05:30 -04:00
Code Issues 775 Packages Projects Releases 10 Wiki Activity

[PR #28894] feat: Add comprehensive unit tests for DatasetService validation and configuration methods #32225

New Issue
Closed
opened 2026-02-21 20:50:59 -05:00 by yindo · 0 comments
yindo commented 2026-02-21 20:50:59 -05:00
Owner
Copy Link
Copy Source

Original Pull Request: https://github.com/langgenius/dify/pull/28894

State: closed
Merged: No

Summary

This PR adds comprehensive unit tests for the dataset validation and configuration methods in the DatasetService class located in api/services/dataset_service.py. The test suite provides complete coverage for permission checking, name validation, indexing technique configuration, and embedding model setup operations, ensuring robust dataset management and access control functionality.

Fix: #28893

What's Changed

  • Test Coverage: Added comprehensive unit tests covering 5 critical validation and configuration methods in DatasetService
  • Documentation: Extensive inline documentation, comments, and docstrings explaining test logic and expected behavior
  • Test Organization: Tests organized into four main test classes:
    • TestDatasetServicePermissionChecks: Permission validation operations
    • TestDatasetServiceNameValidation: Duplicate name detection
    • TestDatasetServiceIndexingTechnique: Indexing technique change handling
    • TestDatasetServiceEmbeddingConfiguration: Embedding model configuration

Test Coverage Details

Permission Checks

  • ✅ check_dataset_permission: User access validation

    • Tenant isolation enforcement (raises NoPermissionError for different tenants)
    • OWNER role bypass validation
    • ONLY_ME permission (creator vs non-creator access)
    • ALL_TEAM permission (team member access)
    • PARTIAL_TEAM permission (explicit permission validation)
    • Database query verification for permission records
    • Logger debug message verification

  • ✅ check_dataset_operator_permission: Operator permission validation

    • ValueError handling for missing dataset/user
    • OWNER role bypass
    • ONLY_ME permission for operators
    • PARTIAL_TEAM permission with explicit DatasetPermission records
    • Database query verification

Name Validation

  • ✅ _has_dataset_same_name: Duplicate name detection
    • Returns True when duplicate name exists within tenant
    • Returns False when name is unique
    • Excludes current dataset from duplicate check (by dataset_id)
    • Tenant isolation (same name in different tenant is allowed)
    • Database query filtering verification

Indexing Technique Configuration

  • ✅ _handle_indexing_technique_change: Indexing technique change handling
    • Change from economy to high_quality (calls embedding configuration)
    • Change from high_quality to economy (removes embedding model config)
    • No change when technique remains the same (delegates to update handler)
    • Return value handling ('add', 'remove', 'update', or None)
    • filtered_data modification verification

Embedding Model Configuration

  • ✅ _configure_embedding_model_for_high_quality: Embedding model setup
    • Valid embedding model provider and model configuration
    • ModelManager.get_model_instance() call verification
    • DatasetCollectionBindingService.get_dataset_collection_binding() call
    • filtered_data update (embedding_model, embedding_model_provider, collection_binding_id)
    • LLMBadRequestError handling (raises ValueError with helpful message)
    • ProviderTokenNotInitError handling (raises ValueError with error description)
    • current_user and tenant_id assertion verification

Key Features

  • 100% Function Coverage: All specified methods in DatasetService are tested
  • Edge Case Handling: Tests cover empty inputs, NotFound exceptions, permission denials, error conditions
  • Security Testing: Permission checks, tenant isolation, role-based access control
  • Mocking Strategy: All external dependencies (database, ModelManager, DatasetCollectionBindingService, current_user, logger) are properly mocked
  • Factory Pattern: Uses DatasetValidationTestDataFactory for consistent test data creation
  • AAA Pattern: All tests follow Arrange-Act-Assert structure

Testing Approach

  • Mocked database session and queries for fast, isolated unit tests
  • Mocked ModelManager and DatasetCollectionBindingService for embedding model operations
  • Mocked current_user for authentication context
  • Comprehensive error handling tests (NoPermissionError, ValueError, AssertionError)
  • Validation tests for permissions, names, and configuration logic
  • Tenant isolation verification for all operations

Screenshots

N/A (Backend unit tests, no UI changes)

Checklist

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods

Related Files

  • api/services/dataset_service.py - Service under test (methods: check_dataset_permission, check_dataset_operator_permission, _has_dataset_same_name, _handle_indexing_technique_change, _configure_embedding_model_for_high_quality)
  • api/tests/unit_tests/services/test_dataset_validation.py - Test suite (1,295+ lines)
  • api/models/dataset.py - Dataset model and DatasetPermissionEnum
  • api/models/model.py - DatasetPermission model
  • api/core/model_manager.py - ModelManager for embedding model configuration
  • api/services/dataset_collection_binding_service.py - DatasetCollectionBindingService for collection bindings

Benefits

  • Improved Code Reliability: Comprehensive test coverage ensures dataset validation and configuration operations work correctly
  • Enhanced Security: Tests validate security controls (permissions, tenant isolation, role-based access)
  • Easier Refactoring: Tests provide confidence when making changes to dataset validation logic
  • Better Documentation: Extensive comments serve as documentation for expected behavior
  • Regression Prevention: Tests catch regressions when making changes to dataset-related code
  • Multi-tenancy Validation: Tests verify tenant isolation is properly enforced
  • Access Control Verification: Tests confirm role-based access control and permission levels work correctly
  • Configuration Logic: Tests verify indexing technique changes and embedding model configuration

Contribution by Gittensor, learn more at https://gittensor.io/

**Original Pull Request:** https://github.com/langgenius/dify/pull/28894 **State:** closed **Merged:** No --- ## Summary This PR adds comprehensive unit tests for the dataset validation and configuration methods in the `DatasetService` class located in `api/services/dataset_service.py`. The test suite provides complete coverage for permission checking, name validation, indexing technique configuration, and embedding model setup operations, ensuring robust dataset management and access control functionality. Fix: #28893 ### What's Changed - **Test Coverage**: Added comprehensive unit tests covering 5 critical validation and configuration methods in `DatasetService` - **Documentation**: Extensive inline documentation, comments, and docstrings explaining test logic and expected behavior - **Test Organization**: Tests organized into four main test classes: - `TestDatasetServicePermissionChecks`: Permission validation operations - `TestDatasetServiceNameValidation`: Duplicate name detection - `TestDatasetServiceIndexingTechnique`: Indexing technique change handling - `TestDatasetServiceEmbeddingConfiguration`: Embedding model configuration ### Test Coverage Details #### Permission Checks - ✅ `check_dataset_permission`: User access validation - Tenant isolation enforcement (raises NoPermissionError for different tenants) - OWNER role bypass validation - ONLY_ME permission (creator vs non-creator access) - ALL_TEAM permission (team member access) - PARTIAL_TEAM permission (explicit permission validation) - Database query verification for permission records - Logger debug message verification - ✅ `check_dataset_operator_permission`: Operator permission validation - ValueError handling for missing dataset/user - OWNER role bypass - ONLY_ME permission for operators - PARTIAL_TEAM permission with explicit DatasetPermission records - Database query verification #### Name Validation - ✅ `_has_dataset_same_name`: Duplicate name detection - Returns True when duplicate name exists within tenant - Returns False when name is unique - Excludes current dataset from duplicate check (by dataset_id) - Tenant isolation (same name in different tenant is allowed) - Database query filtering verification #### Indexing Technique Configuration - ✅ `_handle_indexing_technique_change`: Indexing technique change handling - Change from economy to high_quality (calls embedding configuration) - Change from high_quality to economy (removes embedding model config) - No change when technique remains the same (delegates to update handler) - Return value handling ('add', 'remove', 'update', or None) - filtered_data modification verification #### Embedding Model Configuration - ✅ `_configure_embedding_model_for_high_quality`: Embedding model setup - Valid embedding model provider and model configuration - ModelManager.get_model_instance() call verification - DatasetCollectionBindingService.get_dataset_collection_binding() call - filtered_data update (embedding_model, embedding_model_provider, collection_binding_id) - LLMBadRequestError handling (raises ValueError with helpful message) - ProviderTokenNotInitError handling (raises ValueError with error description) - current_user and tenant_id assertion verification ### Key Features - **100% Function Coverage**: All specified methods in `DatasetService` are tested - **Edge Case Handling**: Tests cover empty inputs, NotFound exceptions, permission denials, error conditions - **Security Testing**: Permission checks, tenant isolation, role-based access control - **Mocking Strategy**: All external dependencies (database, ModelManager, DatasetCollectionBindingService, current_user, logger) are properly mocked - **Factory Pattern**: Uses `DatasetValidationTestDataFactory` for consistent test data creation - **AAA Pattern**: All tests follow Arrange-Act-Assert structure ### Testing Approach - Mocked database session and queries for fast, isolated unit tests - Mocked ModelManager and DatasetCollectionBindingService for embedding model operations - Mocked current_user for authentication context - Comprehensive error handling tests (NoPermissionError, ValueError, AssertionError) - Validation tests for permissions, names, and configuration logic - Tenant isolation verification for all operations ## Screenshots N/A (Backend unit tests, no UI changes) ## Checklist - [x] This change requires a documentation update, included: [Dify Document](https://github.com/langgenius/dify-docs) - [x] I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!) - [x] I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change. - [x] I've updated the documentation accordingly. - [x] I ran `dev/reformat`(backend) and `cd web && npx lint-staged`(frontend) to appease the lint gods ### Related Files - `api/services/dataset_service.py` - Service under test (methods: check_dataset_permission, check_dataset_operator_permission, _has_dataset_same_name, _handle_indexing_technique_change, _configure_embedding_model_for_high_quality) - `api/tests/unit_tests/services/test_dataset_validation.py` - Test suite (1,295+ lines) - `api/models/dataset.py` - Dataset model and DatasetPermissionEnum - `api/models/model.py` - DatasetPermission model - `api/core/model_manager.py` - ModelManager for embedding model configuration - `api/services/dataset_collection_binding_service.py` - DatasetCollectionBindingService for collection bindings ### Benefits - **Improved Code Reliability**: Comprehensive test coverage ensures dataset validation and configuration operations work correctly - **Enhanced Security**: Tests validate security controls (permissions, tenant isolation, role-based access) - **Easier Refactoring**: Tests provide confidence when making changes to dataset validation logic - **Better Documentation**: Extensive comments serve as documentation for expected behavior - **Regression Prevention**: Tests catch regressions when making changes to dataset-related code - **Multi-tenancy Validation**: Tests verify tenant isolation is properly enforced - **Access Control Verification**: Tests confirm role-based access control and permission levels work correctly - **Configuration Logic**: Tests verify indexing technique changes and embedding model configuration Contribution by Gittensor, learn more at https://gittensor.io/
yindo added the pull-request label 2026-02-21 20:50:59 -05:00
yindo closed this issue 2026-02-21 20:50:59 -05:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Knowledge Pipeline
Proposal
WIP
cloud
dependencies
feat:webapp
good first issue
outdated
performance
pull-request
Mirrored from GitHub Pull Request
 python:uv
refactor
size:L
stale
status: accepting prs
status: blocked
status: in discussion
stt & tts
vulnerablity
web
wont-fix
🌊 feat:workflow
🌚 invalid
🐞 bug
👻 feat:rag
💪 enhancement
📚 documentation
🔨 feat:tools
🙋‍♂️ question
🙏 help wanted
🤔 cant-reproduce
🤖 feat:agent
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
yindo
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: langgenius/dify#32225
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?