From d74ba0f2598934c496265119cbb81f1cc222170f Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 16 Jun 2026 15:08:20 +0000 Subject: [PATCH] Unify report module config traversal --- internal/config/config_test.go | 182 +++++++++++++++++++++++++++++++++ internal/config/reports.go | 124 +++++++++------------- 2 files changed, 232 insertions(+), 74 deletions(-) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 25ce130..b9ba12e 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -313,6 +313,37 @@ func TestValidateReportModuleKeysWithoutMutatingOptions(t *testing.T) { } } +func TestReportModuleOverridesNormalizesConstructedOptionsWithoutMutatingConfig(t *testing.T) { + cfg := Defaults() + rawOptions := map[string]any{ + "sections": []any{"short_term"}, + } + cfg.Reports = map[string]ReportConfig{ + "daily": { + DeterministicModules: []ModuleConfigItem{ + {ID: module.Metadata}, + {ID: module.AreaForecastDiscussion, Options: rawOptions}, + }, + deterministicModulesSet: true, + }, + } + + overrides, err := cfg.ReportModuleOverrides() + if err != nil { + t.Fatalf("ReportModuleOverrides() error = %v", err) + } + options, ok := overrides[report.Daily][1].Options.(module.AreaForecastDiscussionOptions) + if !ok { + t.Fatalf("override options type = %T, want AreaForecastDiscussionOptions", overrides[report.Daily][1].Options) + } + if strings.Join(options.Sections, ",") != "short_term" { + t.Fatalf("override sections = %#v, want short_term", options.Sections) + } + if got, ok := cfg.Reports["daily"].DeterministicModules[1].Options.(map[string]any); !ok || !reflect.DeepEqual(got, rawOptions) { + t.Fatalf("config options after ReportModuleOverrides = %#v, want original raw map", cfg.Reports["daily"].DeterministicModules[1].Options) + } +} + func TestValidateReportModuleAliasesDirectly(t *testing.T) { retiredDailyKey := retiredDailyReportKeyForTest() tests := []struct { @@ -510,6 +541,157 @@ reports: } } +func TestReportModuleValidationConsistentForLoadedAndConstructedConfig(t *testing.T) { + tests := []struct { + name string + yaml string + reports map[string]ReportConfig + wantErr string + }{ + { + name: "UnknownReport", + yaml: ` +reports: + moon: + deterministic_modules: + - metadata +`, + reports: map[string]ReportConfig{ + "moon": { + DeterministicModules: []ModuleConfigItem{{ID: module.Metadata}}, + deterministicModulesSet: true, + }, + }, + wantErr: "reports.moon", + }, + { + name: "DuplicateReportAlias", + yaml: ` +reports: + three-day: + deterministic_modules: + - metadata + three_day: + deterministic_modules: + - metadata +`, + reports: map[string]ReportConfig{ + "three-day": { + DeterministicModules: []ModuleConfigItem{{ID: module.Metadata}}, + deterministicModulesSet: true, + }, + "three_day": { + DeterministicModules: []ModuleConfigItem{{ID: module.Metadata}}, + deterministicModulesSet: true, + }, + }, + wantErr: "duplicates report override", + }, + { + name: "UnknownModule", + yaml: ` +reports: + daily: + deterministic_modules: + - missing_module +`, + reports: map[string]ReportConfig{ + "daily": { + DeterministicModules: []ModuleConfigItem{{ID: module.ID("missing_module")}}, + deterministicModulesSet: true, + }, + }, + wantErr: `unknown module "missing_module"`, + }, + { + name: "DuplicateModule", + yaml: ` +reports: + daily: + deterministic_modules: + - metadata + - metadata +`, + reports: map[string]ReportConfig{ + "daily": { + DeterministicModules: []ModuleConfigItem{ + {ID: module.Metadata}, + {ID: module.Metadata}, + }, + deterministicModulesSet: true, + }, + }, + wantErr: `duplicate module "metadata"`, + }, + { + name: "IncompatibleModule", + yaml: ` +reports: + daily: + deterministic_modules: + - tomorrow_planning +`, + reports: map[string]ReportConfig{ + "daily": { + DeterministicModules: []ModuleConfigItem{{ID: module.TomorrowPlanning}}, + deterministicModulesSet: true, + }, + }, + wantErr: `not compatible with report "daily"`, + }, + { + name: "InvalidOptions", + yaml: ` +reports: + daily: + deterministic_modules: + - id: metadata + options: + sections: + - short_term +`, + reports: map[string]ReportConfig{ + "daily": { + DeterministicModules: []ModuleConfigItem{ + { + ID: module.Metadata, + Options: map[string]any{ + "sections": []any{"short_term"}, + }, + }, + }, + deterministicModulesSet: true, + }, + }, + wantErr: "options are invalid", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, loadErr := LoadFile(writeConfig(t, tt.yaml)) + assertReportModuleError(t, "LoadFile", loadErr, tt.wantErr) + + cfg := Defaults() + cfg.Reports = tt.reports + assertReportModuleError(t, "Validate", Validate(cfg), tt.wantErr) + + _, overrideErr := cfg.ReportModuleOverrides() + assertReportModuleError(t, "ReportModuleOverrides", overrideErr, tt.wantErr) + }) + } +} + +func assertReportModuleError(t *testing.T, operation string, err error, want string) { + t.Helper() + if err == nil { + t.Fatalf("%s error = nil, want %q", operation, want) + } + if !strings.Contains(err.Error(), want) { + t.Fatalf("%s error = %q, want %q", operation, err.Error(), want) + } +} + func retiredDailyReportKeyForTest() string { return strings.Join([]string{"daily", "today"}, "_") } diff --git a/internal/config/reports.go b/internal/config/reports.go index b2c37da..84ce37f 100644 --- a/internal/config/reports.go +++ b/internal/config/reports.go @@ -12,116 +12,92 @@ import ( ) func (cfg Config) ReportModuleOverrides() (map[report.ID][]module.ConfigItem, error) { - overrides := map[report.ID][]module.ConfigItem{} - seenReports := map[report.ID]string{} - for key, reportCfg := range cfg.Reports { - id, err := report.IDForConfigKey(key) - if err != nil { - return nil, fmt.Errorf("reports.%s: %w", key, err) - } - if previous, ok := seenReports[id]; ok { - return nil, fmt.Errorf("reports.%s duplicates report override %q", key, previous) - } - seenReports[id] = key - if !reportCfg.deterministicModulesSet { - continue - } - overrides[id] = moduleItemsFromConfig(reportCfg.DeterministicModules) - } - return overrides, nil + return traverseReportModules(&cfg, reportModuleTraversalOptions{ + normalizeOptions: true, + }) } func normalizeReportModules(cfg *Config) error { if cfg.Reports == nil { cfg.Reports = map[string]ReportConfig{} } - moduleRegistry, err := briefing.DefaultModuleRegistry() - if err != nil { - return fmt.Errorf("initialize module registry: %w", err) - } - reportRegistry := report.DefaultRegistry() - seenReports := map[report.ID]string{} - for key, reportCfg := range cfg.Reports { - reportID, err := report.IDForConfigKey(key) - if err != nil { - return fmt.Errorf("reports.%s: %w", key, err) - } - if previous, ok := seenReports[reportID]; ok { - return fmt.Errorf("reports.%s duplicates report override %q", key, previous) - } - seenReports[reportID] = key - if _, err := reportRegistry.Lookup(reportID); err != nil { - return fmt.Errorf("reports.%s: %w", key, err) - } - if !reportCfg.deterministicModulesSet { - continue - } - for i, rawItem := range reportCfg.DeterministicModules { - options, err := normalizeModuleOptions(moduleRegistry, rawItem.ID, rawItem.Options) - if err != nil { - return fmt.Errorf("reports.%s.deterministic_modules[%d]: %w", key, i, err) - } - reportCfg.DeterministicModules[i].Options = options - } - items := moduleItemsFromConfig(reportCfg.DeterministicModules) - if err := moduleRegistry.ValidateComposition(reportID, items); err != nil { - return fmt.Errorf("reports.%s.deterministic_modules: %w", key, err) - } - cfg.Reports[key] = reportCfg - } - return nil + _, err := traverseReportModules(cfg, reportModuleTraversalOptions{ + normalizeOptions: true, + updateConfig: true, + }) + return err } func validateReportModules(cfg Config) error { + _, err := traverseReportModules(&cfg, reportModuleTraversalOptions{ + normalizeOptions: true, + }) + return err +} + +type reportModuleTraversalOptions struct { + normalizeOptions bool + updateConfig bool +} + +func traverseReportModules(cfg *Config, opts reportModuleTraversalOptions) (map[report.ID][]module.ConfigItem, error) { + overrides := map[report.ID][]module.ConfigItem{} if cfg.Reports == nil { - return nil + return overrides, nil } moduleRegistry, err := briefing.DefaultModuleRegistry() if err != nil { - return fmt.Errorf("initialize module registry: %w", err) + return nil, fmt.Errorf("initialize module registry: %w", err) } reportRegistry := report.DefaultRegistry() seenReports := map[report.ID]string{} for key, reportCfg := range cfg.Reports { reportID, err := report.IDForConfigKey(key) if err != nil { - return fmt.Errorf("reports.%s: %w", key, err) + return nil, fmt.Errorf("reports.%s: %w", key, err) } if previous, ok := seenReports[reportID]; ok { - return fmt.Errorf("reports.%s duplicates report override %q", key, previous) + return nil, fmt.Errorf("reports.%s duplicates report override %q", key, previous) } seenReports[reportID] = key if _, err := reportRegistry.Lookup(reportID); err != nil { - return fmt.Errorf("reports.%s: %w", key, err) + return nil, fmt.Errorf("reports.%s: %w", key, err) } if !reportCfg.deterministicModulesSet { continue } - items := make([]module.ConfigItem, 0, len(reportCfg.DeterministicModules)) - for i, rawItem := range reportCfg.DeterministicModules { - options := rawItem.Options - if options != nil { - normalized, err := normalizeModuleOptions(moduleRegistry, rawItem.ID, options) - if err != nil { - return fmt.Errorf("reports.%s.deterministic_modules[%d]: %w", key, i, err) - } - options = normalized - } - items = append(items, module.ConfigItem{ID: rawItem.ID, Options: options}) + items, normalized, err := moduleItemsFromConfig(moduleRegistry, key, reportCfg.DeterministicModules, opts.normalizeOptions) + if err != nil { + return nil, err } if err := moduleRegistry.ValidateComposition(reportID, items); err != nil { - return fmt.Errorf("reports.%s.deterministic_modules: %w", key, err) + return nil, fmt.Errorf("reports.%s.deterministic_modules: %w", key, err) + } + overrides[reportID] = items + if opts.updateConfig { + reportCfg.DeterministicModules = normalized + cfg.Reports[key] = reportCfg } } - return nil + return overrides, nil } -func moduleItemsFromConfig(items []ModuleConfigItem) []module.ConfigItem { +func moduleItemsFromConfig(registry briefing.ModuleRegistry, reportKey string, items []ModuleConfigItem, normalizeOptions bool) ([]module.ConfigItem, []ModuleConfigItem, error) { out := make([]module.ConfigItem, 0, len(items)) - for _, item := range items { - out = append(out, module.ConfigItem{ID: item.ID, Options: item.Options}) + normalizedItems := append([]ModuleConfigItem(nil), items...) + for i, item := range items { + options := item.Options + if normalizeOptions { + var err error + options, err = normalizeModuleOptions(registry, item.ID, item.Options) + if err != nil { + return nil, nil, fmt.Errorf("reports.%s.deterministic_modules[%d]: %w", reportKey, i, err) + } + normalizedItems[i].Options = options + } + out = append(out, module.ConfigItem{ID: item.ID, Options: options}) } - return out + return out, normalizedItems, nil } func normalizeModuleOptions(registry briefing.ModuleRegistry, id module.ID, raw any) (any, error) {