refactor: [#281] strengthen domain invariant enforcement #282
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…g types Apply the DDD validated constructor pattern to UdpTrackerConfig, HttpTrackerConfig, and HealthCheckApiConfig (Phase 0, Proposal #1). Domain layer changes: - Make all fields private with getter methods - Add ::new() constructors with domain invariant validation - Implement custom Deserialize via serde deserialize_with - Create domain error types with help() methods: - UdpTrackerConfigError (port validation) - HttpTrackerConfigError (port + TLS validation) - HealthCheckApiConfigError (port + TLS validation) Application layer changes: - Add TryFrom<Section> for Config implementations - Replace to_*_config() methods with TryFrom trait - Add From<DomainError> for CreateConfigError variants - Update tracker_section to use .try_into() conversions Infrastructure and test updates: - Replace all field accesses with getter method calls - Update TrackerConfig::default() to use ::new() constructors - Add test helper functions for creating valid test instances - Remove redundant localhost+TLS tests (now enforced at construction) This completes Phase 0 of issue #281, establishing the validated constructor pattern across all tracker configuration types.
…oposal #2) - Make TrackerConfig fields private with getter methods - Add TrackerConfig::new() validated constructor - Remove public validate() method (internalize as check_socket_address_conflicts) - Remove LocalhostWithTls error variant (child types now enforce this) - Add custom Deserialize via TrackerConfigRaw intermediate struct - Update Default impl to use TrackerConfig::new() - Update all field access across codebase to use getter methods - Update refactor plan with implementation notes
…posal #3) Phase 1, Proposal #3 - TrackerCoreConfig and DatabaseConfig Validation: Domain Layer Changes: - SqliteConfig: Made database_name field private, added new() constructor with empty validation, added database_name() getter, custom Deserialize via SqliteConfigRaw, SqliteConfigError enum with help() method - MysqlConfig: Made all 5 fields private (host, port, database_name, username, password), added new() constructor with validation (empty checks for host/database_name/username, port 0 check), added 5 getters, custom Deserialize via MysqlConfigRaw, MysqlConfigError enum with help() - TrackerCoreConfig: Made database and private fields private, added new() constructor, added database() and private() getters, custom Deserialize via TrackerCoreConfigRaw Application Layer Changes: - Updated CreateConfigError with SqliteConfigInvalid and MysqlConfigInvalid variants using #[from] for automatic conversion - Updated TrackerCoreSection::to_database_config() to use domain constructors - Updated TrackerCoreSection::to_tracker_core_config() to use constructors Infrastructure/Test Updates: - Updated all field access to use getter methods (database(), private()) - Updated all test code to use constructors instead of struct literals - Added test helper functions for creating test configurations - Fixed clippy doc_markdown warnings for SQLite/MySQL in doc comments Completes Phase 1 with 2/2 proposals done (100%).
- Created UserInputsError enum with GrafanaRequiresPrometheus, HttpsSectionWithoutTlsServices, TlsServicesWithoutHttpsSection variants - Made UserInputs fields private with getter methods - Changed UserInputs::new() and with_tracker() to return Result - Added cross-service validation in with_tracker(): - Grafana requires Prometheus as data source - HTTPS section requires at least one TLS service - TLS services require HTTPS section for Caddy - Added TrackerConfig::has_any_tls_configured() method - Updated EnvironmentContext::with_working_dir_and_tracker() to return Result - Updated Environment::with_working_dir_and_tracker() to return Result - Added CrossServiceValidation variant to CreateConfigError - Removed duplicate validation from app layer: - GrafanaRequiresPrometheus check from to_environment_params() - TlsWithoutHttpsSection and HttpsSectionWithoutTls error variants - Simplified validate_https_config() to only check admin email - Updated all field access across codebase to use getters - All 1917 tests pass
- Add email validation to HttpsConfig::new() returning Result<Self, HttpsConfigError> - Create HttpsConfigError with InvalidEmail variant and help() method - Update CreateConfigError to wrap HttpsConfigError (From implementation) - Remove duplicate validation from HttpsSection::validate() - Remove validate_https_config() method (now redundant) - Update tests in domain, application, and user_inputs modules - Mark Proposal #5 as COMPLETED in refactor plan This completes Phase 2 of Issue #281 - all 6 proposals are now implemented.
…r config - Remove from_service_endpoints method (was using runtime state lacking TLS info) - Update show handler to always use from_tracker_config with instance IP - Split from_tracker_config into focused helper methods: - build_udp_tracker_urls - build_http_tracker_info - build_api_endpoint_info - build_health_check_info - collect_grafana_tls_domain - Remove #[allow(clippy::too_many_lines)] attribute This simplifies the code by establishing from_tracker_config as the single source of truth for ServiceInfo construction, using the tracker configuration plus instance IP rather than storing incomplete runtime endpoints.
Extract focused helper methods for each service type: - render_udp_trackers - render_http_trackers (handles HTTPS, direct, and localhost-only) - render_api_endpoint - render_health_check This improves readability by separating rendering logic for each tracker service category into its own method.
…ersions - Convert GrafanaSection, PrometheusSection, SshCredentialsConfig, DatabaseSection, TrackerCoreSection, TrackerSection, and ProviderSection to use TryFrom trait for domain type conversion - Update environment_config.rs to use try_into() for all conversions - Remove old to_* methods (no backward compatibility needed pre-v1.0) Follows pattern documented in docs/decisions/tryfrom-for-dto-to-domain-conversion.md
Replace references to deprecated to_*_config() methods with the new TryFrom trait conversion pattern in the module documentation.
…entParams TryFrom - Create ValidatedEnvironmentParams struct with 9 named fields - Implement TryFrom<EnvironmentCreationConfig> for ValidatedEnvironmentParams - Update handler and config_loader to use new pattern - Remove deprecated to_environment_params() method - Update all tests and documentation This eliminates the Clippy warning about tuple complexity and provides a more idiomatic, self-documenting API following the established TryFrom pattern used for other DTO conversions.
Rename with_working_dir_and_tracker() to create() for cleaner, more idiomatic API. The new name is concise and clearly indicates this is the primary factory method for creating a fully-configured Environment aggregate.
- Create EnvironmentParams as domain value object (Factory Input Pattern) - Update Environment::create() to accept single params argument - Keep TryFrom<EnvironmentCreationConfig> in application layer - Cleaner API: Environment::create(params, working_dir, timestamp) This follows DDD principles - the params struct contains only domain types so it belongs in the domain layer, not application layer.
- Make all RuntimeOutputs fields private (instance_ip, provision_method, service_endpoints) - Add RuntimeOutputs::new() constructor for empty outputs - Add semantic setters that indicate when data is set: - record_provisioning(ip) - after provision command - record_registration(ip) - after register command - record_services_started(endpoints) - after run command - Add low-level setters (set_instance_ip, set_provision_method) for compatibility with existing code - Add getter methods: instance_ip(), provision_method(), service_endpoints() - Implement Default trait for RuntimeOutputs The setter names now clearly document when data is populated during the deployment lifecycle, improving code readability and traceability.
Member
Author
|
ACK 2ce6c6b |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Implements the DDD validated constructor pattern for domain types, ensuring invariants are enforced at construction time rather than relying on separate validation calls. Also improves encapsulation across key domain structs.
Changes
Phase 0, Proposal 1 (Completed) - Tracker Config Validation
Applied the validated constructor pattern to:
UdpTrackerConfig: Port must be non-zeroHttpTrackerConfig: Port must be non-zero, TLS requires domain, localhost cannot use TLSHealthCheckApiConfig: Port must be non-zero, TLS requires domain, localhost cannot use TLSPattern Applied:
::new()constructors with domain validationDeserializevia serdedeserialize_withhelp()method for actionable error messagesTryFrom<DtoConfig> for DomainConfigfor DTO→Domain conversionsPhase 0, Proposals 2-6 (Completed) - Core Domain Improvements
HttpApiConfig- validated constructor pattern with TLS validationHttpApiSection-TryFromDTO conversion with error propagationTrackerSection- comprehensiveTryFromimplementation for all nested configsValidatedEnvironmentParams- replaced 9-element tuple with named structEnvironmentParams- moved from Application to Domain layerAdditional Encapsulation (Completed)
RuntimeOutputs: Made all fields private with semantic settersrecord_provisioning(ip)- afterprovisioncommandrecord_registration(ip)- afterregistercommandrecord_services_started(endpoints)- afterruncommandinstance_ip(),provision_method(),service_endpoints()DefaulttraitFiles Modified
Domain Layer:
src/domain/tracker/config/udp.rs- Validated constructor patternsrc/domain/tracker/config/http.rs- Validated constructor patternsrc/domain/tracker/config/health_check_api.rs- Validated constructor patternsrc/domain/tracker/config/http_api.rs- Validated constructor patternsrc/domain/tracker/config/mod.rs- Updated defaults, tests, helper functionssrc/domain/environment/params.rs- NEW - Moved from Application layersrc/domain/environment/runtime_outputs.rs- Encapsulated with semantic settersApplication Layer:
*_section.rsfiles - AddedTryFromimplementationsvalidated_params.rs- NEW - Named struct replacing tupleerrors.rs- Added error variants for invalid configsInfrastructure Layer:
::new()constructors and getter methodsRemaining Work
All proposals from Issue #281 are now complete. Additional improvements may include:
TrackerConfigvalidates at construction (Phase 1)TrackerCoreConfigdatabase validation (Phase 2)UserInputsvalidated constructor (Phase 2)Testing
Related
Closes #281