UN-2408 [FEAT] Added per-organization configuration overrides for system settings#1330
Conversation
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughA new Django app named "configuration" is introduced, including its model, enums, and migration for storing per-organization configuration values. The Django project settings are updated to include this app. The workflow batching logic is refactored to fetch its configuration dynamically from the database using the new configuration system instead of a static settings value. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WorkflowHelper
participant Configuration
participant Database
User->>WorkflowHelper: get_file_batches(user, files)
WorkflowHelper->>Configuration: get_value_by_organization(ConfigKey, user.organization)
Configuration->>Database: Query Configuration for (organization, ConfigKey)
Database-->>Configuration: Return config value (or None)
Configuration-->>WorkflowHelper: Return typed config value (or default)
WorkflowHelper-->>User: Return file batches (using config value)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
backend/workflow_manager/workflow_v2/workflow_helper.py (1)
118-125: Clarify the fallback logic and improve readability.The implementation successfully introduces dynamic configuration, but there are several concerns:
Redundant fallback: The
or 1condition may be unnecessary ifConfiguration.get_value_by_organizationalready returns the default value when no configuration is found.Hardcoded default: The hardcoded
1should be extracted as a named constant for better readability.Missing organization handling: Consider what happens when
UserContext.get_organization()returnsNone.Apply this refactor to improve readability and remove redundancy:
+DEFAULT_BATCH_SIZE = 1 + # Prepare batches of files for parallel processing organization = UserContext.get_organization() -BATCH_SIZE = ( - Configuration.get_value_by_organization( - config_key=ConfigKey.MAX_PARALLEL_FILE_BATCHES, organization=organization - ) - or 1 -) # Max number of batches +BATCH_SIZE = Configuration.get_value_by_organization( + config_key=ConfigKey.MAX_PARALLEL_FILE_BATCHES, organization=organization +) or DEFAULT_BATCH_SIZELet me verify if the
orcondition is needed by examining the Configuration model implementation:#!/bin/bash # Check if Configuration.get_value_by_organization can return None/falsy values ast-grep --pattern 'def get_value_by_organization($$$) { $$$ }'backend/configuration/models.py (2)
11-31: Consider the relationship with existing platform settings.The past review comment raises a valid concern about whether this should be part of existing platform settings. Consider documenting the distinction between platform-wide settings and organization-specific configurations to clarify when to use each approach.
Document the design decision about when to use this Configuration model versus existing platform settings to help future developers understand the separation of concerns.
18-18: Consider using an enum for the key field to improve type safety.Using a string field for the key reduces type safety and makes it prone to typos. Consider using an enum or choices to constrain the possible values.
Apply this change to improve type safety:
+from django.db import models +from configuration.enums import ConfigKey + class Configuration(BaseModel): # ... other fields ... - key = models.CharField(max_length=100) + key = models.CharField( + max_length=100, + choices=[(config_key.name, config_key.value.name) for config_key in ConfigKey] + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (6)
backend/backend/settings/base.py(1 hunks)backend/configuration/apps.py(1 hunks)backend/configuration/enums.py(1 hunks)backend/configuration/migrations/0001_initial.py(1 hunks)backend/configuration/models.py(1 hunks)backend/workflow_manager/workflow_v2/workflow_helper.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
backend/backend/settings/base.py (1)
297-297: LGTM! Configuration app properly integrated.The addition of the "configuration" app to SHARED_APPS is correctly placed and follows Django conventions for registering new apps.
backend/configuration/apps.py (1)
1-7: LGTM! Standard Django app configuration.The ConfigurationConfig class follows Django best practices and is consistent with the project's field type conventions.
backend/workflow_manager/workflow_v2/workflow_helper.py (1)
14-15: LGTM! Imports updated for dynamic configuration.The import changes correctly replace the static Django settings with the new dynamic configuration system.
backend/configuration/migrations/0001_initial.py (1)
1-56: LGTM! Well-structured migration for Configuration model.The migration correctly defines the Configuration model with:
- Proper UUID primary key
- Foreign key relationship to Organization
- Unique constraint on organization+key for data integrity
- Appropriate field types and constraints
The migration follows Django best practices and will support the dynamic configuration system effectively.
backend/configuration/models.py (1)
41-56: The method correctly handles default values on errors.The
get_value_by_organizationmethod properly returns the default value from the ConfigKey when:
- Organization is None
- Configuration doesn't exist
- Configuration is disabled
- Value parsing fails
This addresses the concern raised in the past review comment about returning default values on errors.
backend/configuration/enums.py (4)
1-7: LGTM: Clean imports and dependenciesThe import structure is well-organized and includes all necessary dependencies for the configuration system.
9-13: LGTM: Well-defined configuration typesThe
ConfigTypeenum provides a clear set of supported configuration value types that covers common use cases.
16-20: LGTM: Immutable configuration specificationThe frozen dataclass provides a clean, immutable way to define configuration specifications with proper type hints.
24-28: Django setting “MAX_PARALLEL_FILE_BATCHES” is defined
Confirmed thatMAX_PARALLEL_FILE_BATCHESis declared inbackend/backend/settings/base.py, so referencingsettings.MAX_PARALLEL_FILE_BATCHESinbackend/configuration/enums.pywill not raise an import-time error.
- Convert Configuration.key to enum validation with clean() method - Add robust value validation with min/max constraints - Remove redundant DEFAULT_BATCH_SIZE and fallback logic - Add environment-configurable MAX_PARALLEL_FILE_BATCHES_MAX_VALUE - Update to modern Python type hints (Organization | None) - Enhance error handling with comprehensive logging - Add comprehensive docstrings explaining architectural decisions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Fix typed_value property to catch specific exceptions (ValueError, KeyError, TypeError) - Fix redundant exception handling in get_value_by_organization (json.JSONDecodeError inherits from ValueError) - Use generic Exception for comprehensive error handling in configuration retrieval - Maintain robust fallback behavior while improving code quality 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…pstack/unstract into UN-2408-support-dynamic-configuration
|
|
…tem settings (#1330) * dynamic env configurations with Db table * Address PR review comments and enhance configuration validation - Convert Configuration.key to enum validation with clean() method - Add robust value validation with min/max constraints - Remove redundant DEFAULT_BATCH_SIZE and fallback logic - Add environment-configurable MAX_PARALLEL_FILE_BATCHES_MAX_VALUE - Update to modern Python type hints (Organization | None) - Enhance error handling with comprehensive logging - Add comprehensive docstrings explaining architectural decisions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix Sonar and CodeRabbit exception handling issues - Fix typed_value property to catch specific exceptions (ValueError, KeyError, TypeError) - Fix redundant exception handling in get_value_by_organization (json.JSONDecodeError inherits from ValueError) - Use generic Exception for comprehensive error handling in configuration retrieval - Maintain robust fallback behavior while improving code quality 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * added docstring --------- Co-authored-by: Gayathri <142381512+gaya3-zipstack@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
What
Configurationmodel with type-safe value validation and enum-based key managementMAX_PARALLEL_FILE_BATCHESwith environment-configurable limitsWhy
How
Configurationmodel in theconfigurationapp with organization-scoped key-value overridesMAX_PARALLEL_FILE_BATCHES_MAX_VALUEorganization override > default system configCan this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
No - This PR is designed to be backward compatible:
Database Migrations
configurationtable added for storing organization-level configuration overridesEnv Config
MAX_PARALLEL_FILE_BATCHES_MAX_VALUEenvironment variable for configurable upper limitssample.envwith the new configuration optionRelevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
N/A - Backend configuration system
Checklist
I have read and understood the Contribution Guidelines.