diff --git a/VALIDATION-IMPROVEMENTS-PLAN.md b/VALIDATION-IMPROVEMENTS-PLAN.md new file mode 100644 index 0000000..b4832e1 --- /dev/null +++ b/VALIDATION-IMPROVEMENTS-PLAN.md @@ -0,0 +1,574 @@ +# Validation System Improvements & Rules Engine Evaluation + +## Executive Summary + +**Current State:** Custom validation system with 10 C# rules across 3 entity types. Clean architecture but hardcoded rule registration, requires recompilation for changes. + +**Critical Requirement:** Runtime-editable validation rules for different chapters (from user feedback). + +**Recommendation:** **Hybrid approach** - Improve current implementation + add JSON-based configuration storage, avoiding full rules engine adoption to maintain simplicity while enabling runtime customization. + +## Analysis Results + +### Current Implementation Assessment + +**Strengths:** +- Clean generic design (`IValidationRule`) +- Type-safe validation +- Clear separation of concerns +- Context-aware rule execution +- Well-organized directory structure (StudentRankingRules, StudentAssignmentRules, TeamRules) + +**Critical Issues:** +1. **Hardcoded rule registration** - Rules manually added in `ValidationService.InitializeRules()` (Open/Closed violation) +2. **Requires recompilation** - Cannot change rules or thresholds without code deployment +3. **Rule duplication** - StudentRankingRules and StudentAssignmentRules have duplicate logic +4. **Non-scalable configuration** - One property per rule severity (6 specific properties) +5. **No rule discovery** - Cannot dynamically load or register rules +6. **Inconsistent context handling** - `ValidateTeam()` doesn't check `AppliesTo(context)` + +**Performance Concerns:** +- Rule objects instantiated per ValidationService (no singleton/pooling) +- No short-circuit logic (all rules execute even after critical errors) +- Batch operations materialize entire dictionary in memory + +### Rules Engine Library Research + +#### Microsoft RulesEngine +[GitHub - microsoft/RulesEngine](https://github.com/microsoft/RulesEngine) | [Documentation](https://microsoft.github.io/RulesEngine/) + +**Features:** +- JSON-based rule definitions (runtime editable) +- Lambda expression support in rules +- Global/local parameters (reusable logic) +- Custom actions (v3+) +- Custom type injection +- Flexible storage (Azure Blob, Cosmos DB, files, EF Core, SQL) +- Actively maintained (tutorial from October 2025) + +**Example Rule:** +```json +{ + "WorkflowName": "StudentValidation", + "Rules": [ + { + "RuleName": "RequireRegionalEvent", + "Expression": "input.RankedEvents.Any(e => e.RegionalEvent == true)", + "ErrorMessage": "No Regional Event", + "ErrorType": "Warning" + } + ] +} +``` + +**Pros:** +- ✅ Enables runtime editing without recompilation +- ✅ Proven, maintained library (Microsoft) +- ✅ Reduces custom code maintenance +- ✅ Flexible rule storage options +- ✅ Supports complex expressions + +**Cons:** +- ❌ Significant complexity (JSON + lambda expressions as strings) +- ❌ Learning curve for team +- ❌ All rules must be rewritten in JSON +- ❌ Type safety lost (expressions are strings) +- ❌ Debugging more difficult (no compile-time checks) +- ❌ External dependency + +#### Alternative: NRules +[GitHub - NRules/NRules](https://github.com/NRules/NRules) | [Website](https://nrules.net/) + +**Characteristics:** +- Forward-chaining rules engine based on Rete algorithm +- C# internal DSL for rule authoring +- More complex than RulesEngine +- Better for complex decision trees and rule dependencies + +**Assessment:** Overkill for TSA validation use case. Designed for complex business logic scenarios with many interdependent rules. + +#### Alternative: FluentValidation +[GitHub - FluentValidation](https://github.com/FluentValidation/FluentValidation) | [NuGet](https://www.nuget.org/packages/fluentvalidation/) + +**Characteristics:** +- Validation-specific library (NOT a rules engine) +- Fluent interface for building validation rules +- Strongly-typed C# validation rules + +**Assessment:** Not applicable - FluentValidation is for object validation, not business rule execution. Different use case. + +### Decision Matrix + +| Aspect | Current Implementation | Improve Current + JSON Config | Full RulesEngine Adoption | +|--------|------------------------|------------------------------|--------------------------| +| **Runtime Editing** | ❌ No (requires recompile) | ✅ Yes (configuration only) | ✅ Yes (rules + config) | +| **Simplicity** | ✅ Simple C# rules | ✅ C# rules + JSON config | ❌ Complex JSON + lambdas | +| **Type Safety** | ✅ Full compile-time checks | ✅ Rules are type-safe | ❌ String-based expressions | +| **Maintenance** | ❌ Custom code to maintain | ⚠️ Less custom code | ✅ Library maintained | +| **Flexibility** | ❌ Can't add new rules | ⚠️ Can't add new rules | ✅ Can add new rules | +| **Learning Curve** | ✅ Familiar C# | ✅ Minimal | ❌ Significant | +| **Debugging** | ✅ Easy | ✅ Easy | ❌ Harder (runtime errors) | +| **Dependencies** | ✅ None | ✅ None | ⚠️ External library | + +## Recommended Approach: Hybrid Improvement + +### Strategy + +**Phase 1: Immediate Improvements (No Breaking Changes)** +Improve current implementation to fix code smells and add runtime configurability WITHOUT adopting a rules engine library. + +**Phase 2: JSON Configuration Storage** +Move `ValidationConfiguration` to JSON files (Data/validation-config.json) to enable per-chapter customization at runtime. + +**Phase 3: Rule Registry Pattern (Optional)** +Add plugin architecture for future extensibility if needed. + +### Why This Approach? + +1. **Meets critical requirement:** Enables runtime editing of thresholds, severities, and requirements +2. **Maintains simplicity:** Keeps familiar C# rule classes with type safety +3. **Reduces complexity:** Avoids JSON lambda expressions and external dependencies +4. **Pragmatic:** Most chapter-to-chapter differences are in THRESHOLDS (min/max events, severity levels), not rule LOGIC +5. **Incremental:** Can adopt RulesEngine later if rule logic customization becomes critical + +### What Can Be Customized at Runtime? + +With JSON configuration approach, chapters can customize: +- **Event count thresholds** (MinRecommendedEvents, MaxRecommendedEvents, etc.) +- **Requirement toggles** (RequireRegionalEvent, RequireOnSiteActivity, etc.) +- **Severity levels** (Warning vs Error for each rule type) + +This covers ~95% of expected chapter-to-chapter variation without rule engine complexity. + +### When to Adopt RulesEngine? + +Only adopt Microsoft RulesEngine if you need: +- Different chapters with **fundamentally different rules** (not just thresholds) +- Non-developers authoring new validation rules +- Complex rule dependencies/chaining +- Rule versioning and AB testing + +## Implementation Plan + +### Phase 1: Core Improvements (No External Dependencies) + +#### 1.1 Fix Rule Registration Anti-Pattern +**File:** `Core/Validation/ValidationService.cs` + +**Current Problem:** +```csharp +private void InitializeRules() +{ + _studentRules.Add(new NoRegionalEventRule()); + _studentRules.Add(new NoOnSiteActivityRule()); + // ... hardcoded rule instantiation +} +``` + +**Solution:** Use reflection-based rule discovery or explicit registration API: + +```csharp +// Option A: Reflection-based discovery (simple) +private void InitializeRules() +{ + // Auto-discover all IValidationRule implementations + _studentRules = DiscoverRules(); + _teamRules = DiscoverRules(); + _statisticsRules = DiscoverRules(); +} + +private List> DiscoverRules() +{ + var ruleType = typeof(IValidationRule); + return Assembly.GetExecutingAssembly() + .GetTypes() + .Where(t => t.IsClass && !t.IsAbstract && ruleType.IsAssignableFrom(t)) + .Select(t => (IValidationRule)Activator.CreateInstance(t)!) + .ToList(); +} +``` + +**Benefits:** +- Adding new rules just requires creating the class +- No need to modify ValidationService +- Rules automatically discovered + +#### 1.2 Eliminate Rule Duplication +**Files:** Student ranking rules vs assignment rules + +**Current Problem:** Near-duplicate code between: +- `NoRegionalEventRule` (Student) vs `NoRegionalEventAssignmentRule` (StudentEventStatistics) +- `NoOnSiteActivityRule` (Student) vs `NoOnSiteActivityAssignmentRule` (StudentEventStatistics) + +**Solution:** Create abstract base classes: + +```csharp +// Core/Validation/Rules/BaseRules/RequiredEventTypeRule.cs +public abstract class RequiredEventTypeRule : IValidationRule +{ + protected abstract string EventTypeName { get; } + protected abstract string IconIdentifier { get; } + protected abstract Func IsRequired { get; } + protected abstract Func GetSeverity { get; } + protected abstract Func HasEventType { get; } + protected abstract ValidationContext[] ApplicableContexts { get; } + + public ValidationWarning? Validate(TEntity entity, ValidationConfiguration config) + { + if (!IsRequired(config)) return null; + if (HasEventType(entity)) return null; + + return new ValidationWarning { /* ... */ }; + } + + public bool AppliesTo(ValidationContext context) => + ApplicableContexts.Contains(context); +} +``` + +Then specific rules become: +```csharp +public class NoRegionalEventRule : RequiredEventTypeRule +{ + protected override string EventTypeName => "Regional Event"; + protected override Func HasEventType => + s => s.RankedEvents.Any(e => e.RegionalEvent); + // ... etc +} +``` + +**Benefits:** +- Eliminates duplication +- Easier to maintain +- Consistent behavior across similar rules + +#### 1.3 Fix Context Handling Inconsistency +**File:** `Core/Validation/ValidationService.cs` + +**Current Problem:** `ValidateTeam()` doesn't filter by context + +**Solution:** +```csharp +public List ValidateTeam(Team team, ValidationContext context = ValidationContext.Team) +{ + return _teamRules + .Where(rule => rule.AppliesTo(context)) // ADD THIS + .Select(rule => rule.Validate(team, _config)) + .Where(warning => warning != null) + .Cast() + .ToList(); +} +``` + +#### 1.4 Add Rule Singleton Pattern +**File:** `Core/Validation/ValidationService.cs` + +**Current:** Rules instantiated per ValidationService instance + +**Solution:** Use lazy static singletons: +```csharp +private static readonly Lazy>> _studentRuleDefinitions = + new(() => DiscoverRules()); + +public ValidationService(ValidationConfiguration? config = null) +{ + _config = config ?? ValidationConfiguration.Default; + _studentRules = _studentRuleDefinitions.Value; + _teamRules = _teamRuleDefinitions.Value; + _statisticsRules = _statisticsRuleDefinitions.Value; +} +``` + +**Benefits:** +- Rules instantiated once per application lifetime +- Reduced memory allocation +- Better performance + +### Phase 2: JSON Configuration Storage + +#### 2.1 Add Configuration Serialization +**File:** `Core/Validation/ValidationConfiguration.cs` + +Add methods: +```csharp +public static ValidationConfiguration FromJson(string json) +{ + return JsonSerializer.Deserialize(json) + ?? Default; +} + +public static async Task LoadFromFileAsync(string path) +{ + if (!File.Exists(path)) return Default; + + var json = await File.ReadAllTextAsync(path); + return FromJson(json); +} + +public async Task SaveToFileAsync(string path) +{ + var json = JsonSerializer.Serialize(this, new JsonSerializerOptions + { + WriteIndented = true + }); + await File.WriteAllTextAsync(path, json); +} +``` + +#### 2.2 Create Default Configuration File +**File:** `Data/validation-config.json` (new) + +```json +{ + "MinRecommendedEvents": 2, + "MaxRecommendedEvents": 4, + "MinCriticalEvents": 1, + "MaxCriticalEvents": 6, + "RequireRegionalEvent": true, + "RequireOnSiteActivity": true, + "RequireIndividualEvent": false, + "RequireTeamCaptain": true, + "NoRegionalEventSeverity": "Warning", + "NoOnSiteActivitySeverity": "Warning", + "NoIndividualEventSeverity": "Warning", + "TeamSizeSeverity": "Warning", + "EventCountSeverity": "Warning", + "MissingCaptainSeverity": "Warning" +} +``` + +#### 2.3 Update Service Registration +**File:** `WebApp/Program.cs` (lines 176-177) + +**Current:** +```csharp +builder.Services.AddScoped(sp => + new Core.Validation.ValidationService(Core.Validation.ValidationConfiguration.Default)); +``` + +**New:** +```csharp +// Load validation configuration from Data directory (or use default) +var validationConfigPath = Path.Combine( + builder.Environment.ContentRootPath, + "Data", + "validation-config.json"); + +var validationConfig = await Core.Validation.ValidationConfiguration + .LoadFromFileAsync(validationConfigPath); + +builder.Services.AddScoped(sp => + new Core.Validation.ValidationService(validationConfig)); +``` + +#### 2.4 Add Configuration UI (Optional) +**File:** `WebApp/Components/Pages/Settings/ValidationSettings.razor` (new) + +Create admin page to edit validation configuration with live preview. + +### Phase 3: Optional Enhancements + +#### 3.1 Add Short-Circuit Logic +```csharp +public List ValidateStudentRankings( + Student student, + ValidationContext context, + bool stopOnFirstError = false) +{ + var warnings = new List(); + + foreach (var rule in _studentRules.Where(r => r.AppliesTo(context))) + { + var warning = rule.Validate(student, _config); + if (warning != null) + { + warnings.Add(warning); + if (stopOnFirstError && warning.Severity == ValidationSeverity.Error) + break; + } + } + + return warnings; +} +``` + +#### 3.2 Add Rule Metadata Interface +```csharp +public interface IValidationRuleMetadata +{ + string Name { get; } + string Description { get; } + string Category { get; } + int Priority { get; } // For ordering +} + +public interface IValidationRule : IValidationRuleMetadata +{ + ValidationWarning? Validate(TEntity entity, ValidationConfiguration config); + bool AppliesTo(ValidationContext context); +} +``` + +## Alternative: Full RulesEngine Migration Plan + +If you decide runtime rule editing (not just configuration) is critical, here's the Microsoft RulesEngine adoption plan: + +### Installation +```bash +dotnet add package RulesEngine +``` + +### Migration Strategy + +#### 1. Create Rule JSON Files +**File:** `Data/validation-rules.json` + +```json +{ + "StudentRankingWorkflow": { + "WorkflowName": "StudentRankingValidation", + "Rules": [ + { + "RuleName": "RequireRegionalEvent", + "Enabled": true, + "Expression": "RequireRegionalEvent AND input.RankedEvents.Any(e => e.RegionalEvent == false)", + "RuleExpressionType": "LambdaExpression", + "ErrorMessage": "No Regional Event", + "ErrorType": "Warning", + "SuccessEvent": "NoRegionalEventWarning" + }, + { + "RuleName": "RequireOnSiteActivity", + "Enabled": true, + "Expression": "RequireOnSiteActivity AND input.RankedEvents.Any(e => e.OnSiteActivity == false)", + "RuleExpressionType": "LambdaExpression", + "ErrorMessage": "No On-Site Activity", + "ErrorType": "Warning" + } + ] + } +} +``` + +#### 2. Create RulesEngine Wrapper +**File:** `Core/Validation/RulesEngineValidationService.cs` (new) + +```csharp +using RulesEngine.Models; + +public class RulesEngineValidationService +{ + private readonly RulesEngine.RulesEngine _rulesEngine; + private readonly ValidationConfiguration _config; + + public RulesEngineValidationService(string rulesJsonPath, ValidationConfiguration config) + { + var rulesJson = File.ReadAllText(rulesJsonPath); + var workflows = JsonSerializer.Deserialize(rulesJson); + _rulesEngine = new RulesEngine.RulesEngine(workflows); + _config = config; + } + + public async Task> ValidateStudentRankingsAsync(Student student) + { + var input = new RuleParameter("input", student); + var configParam = new RuleParameter("config", _config); + + var results = await _rulesEngine.ExecuteAllRulesAsync( + "StudentRankingValidation", + input, + configParam); + + return results + .Where(r => !r.IsSuccess) + .Select(r => new ValidationWarning + { + Code = r.Rule.RuleName, + Message = r.Rule.ErrorMessage, + Severity = r.Rule.ErrorType == "Error" + ? ValidationSeverity.Error + : ValidationSeverity.Warning, + Context = ValidationContext.StudentRanking + }) + .ToList(); + } +} +``` + +#### 3. Gradual Migration +- Keep existing ValidationService +- Add RulesEngineValidationService alongside +- Use feature flag to toggle between implementations +- Migrate one context at a time (StudentRanking → StudentAssignment → Team) + +## Critical Files + +### Current Implementation +1. `Core/Validation/ValidationService.cs` - Main orchestration service +2. `Core/Validation/ValidationConfiguration.cs` - Configuration model +3. `Core/Validation/IValidationRule.cs` - Generic rule interface +4. `Core/Validation/Rules/StudentRankingRules/*.cs` - Student ranking rules (3 files) +5. `Core/Validation/Rules/StudentAssignmentRules/*.cs` - Assignment rules (4 files) +6. `Core/Validation/Rules/TeamRules/*.cs` - Team rules (3 files) +7. `WebApp/Program.cs` - Service registration (line 176) + +### For Improvements +8. `Data/validation-config.json` - Runtime configuration file (new) +9. `Core/Validation/Rules/BaseRules/*.cs` - Abstract base rule classes (new) + +### For RulesEngine Migration +10. `Data/validation-rules.json` - RulesEngine rule definitions (new) +11. `Core/Validation/RulesEngineValidationService.cs` - Wrapper service (new) + +## Recommendations Summary + +### Recommended: Hybrid Improvement Approach + +**Do This:** +1. ✅ Fix hardcoded rule registration (reflection-based discovery) +2. ✅ Eliminate rule duplication (abstract base classes) +3. ✅ Add JSON configuration storage (`Data/validation-config.json`) +4. ✅ Enable per-chapter threshold/severity customization +5. ✅ Fix context handling inconsistency +6. ✅ Add rule singleton pattern for performance + +**Benefits:** +- Enables runtime customization without complexity +- Maintains type safety and simplicity +- No external dependencies +- Easy to debug and maintain +- Covers 95% of expected customization needs + +**Estimated Effort:** 4-6 hours + +### Not Recommended (Yet): Full RulesEngine Adoption + +**Don't Do This Unless:** +- Chapters need fundamentally different rule logic (not just thresholds) +- Non-developers need to author rules +- Complex rule dependencies are required +- A/B testing of rules is needed + +**Complexity Cost:** +- All 10 rules rewritten in JSON with lambda strings +- Team must learn RulesEngine syntax and concepts +- Type safety lost (runtime errors vs compile-time) +- Harder debugging (no breakpoints in rule logic) +- External dependency to maintain + +## Next Steps + +1. **Decision Point:** Confirm hybrid approach or full RulesEngine migration +2. **If Hybrid:** Implement Phase 1 improvements (2-3 hours) +3. **Then:** Add JSON configuration storage (1-2 hours) +4. **Test:** Deploy to test chapter with custom config +5. **Iterate:** Add Phase 3 enhancements if needed + +## Research Sources + +- [Microsoft RulesEngine - GitHub](https://github.com/microsoft/RulesEngine) +- [RulesEngine Documentation](https://microsoft.github.io/RulesEngine/) +- [Understanding Microsoft RulesEngine (Oct 2025)](https://medium.com/@sunil-bisht/understanding-microsoft-rulesengine-a-gentle-introduction-part-1-ae1585e5e279) +- [NRules - Rules Engine for .NET](https://nrules.net/) +- [Rule Engines In .NET - Yunier's Wiki](https://yunier.dev/post/2023/rule-engines-in-dotnet/) +- [Top 10 Open Source Rules Engines (2025)](https://www.nected.ai/blog/open-source-rules-engine)