Add multi-inbox support to backend #10
Conversation
This commit implements support for fetching DMARC reports from multiple IMAP inboxes concurrently while maintaining full backward compatibility with single inbox configurations. Key changes: Backend (internal/config/config.go): - Add IMAPConfigs []IMAPConfig field to support multiple inboxes - Add GetIMAPConfigs() method to normalize single/multi config access - Support IMAP_CONFIGS environment variable for JSON array of configs - Fix env.Parse bug that was overwriting array values with envDefaults - Apply defaults correctly to both single and multiple configs - Maintain backward compatibility with existing single IMAP config Fetching logic (cmd/parse-dmarc/main.go): - Update fetchReports() to iterate through all IMAP configs - Use goroutines and sync.WaitGroup for concurrent fetching - Add per-inbox error handling (one failing inbox doesn't stop others) - Aggregate total processed reports across all inboxes - Enhanced logging with inbox progress indicators [Inbox 1/3] - Return error only if all inboxes fail Documentation: - Add comprehensive multi-inbox section to README.md - Include configuration file and environment variable examples - Add feature description and benefits - Create config.multi-inbox.example.json with 3-inbox example - Update features list to highlight multi-inbox support Benefits: - Concurrent fetching improves performance for multiple inboxes - Automatic deduplication via existing report_id constraint - No UI changes required - completely transparent to frontend - Backward compatible - single inbox configs work unchanged - Flexible configuration via JSON file or environment variables Testing: - Verified single inbox backward compatibility - Verified multi-inbox configuration loading - Tested mailbox name preservation (no envDefault overwrites) - Confirmed successful build with no errors
There was a problem hiding this comment.
Pull Request Overview
This PR adds multi-inbox support to the DMARC report parser, enabling concurrent fetching from multiple IMAP accounts and aggregating reports into a single dashboard.
- Adds support for multiple IMAP configurations via
imap_configsarray in config file andIMAP_CONFIGSenvironment variable - Implements concurrent fetching from multiple inboxes using goroutines and proper synchronization
- Maintains backward compatibility with single inbox configuration
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/config/config.go | Adds IMAPConfigs array field, GetIMAPConfigs() helper method, and environment variable parsing logic for multi-inbox configuration |
| cmd/parse-dmarc/main.go | Refactors fetchReports() to process multiple inboxes concurrently with proper error handling and progress logging |
| config.multi-inbox.example.json | Provides example configuration file for multi-inbox setup |
| README.md | Documents multi-inbox feature with configuration examples and capabilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| allConfigs := cfg.GetIMAPConfigs() | ||
| for i := range allConfigs { | ||
| if allConfigs[i].Port == 0 { | ||
| allConfigs[i].Port = 993 | ||
| } | ||
| if allConfigs[i].Mailbox == "" { | ||
| allConfigs[i].Mailbox = "INBOX" | ||
| } | ||
| } |
There was a problem hiding this comment.
The defaults are being applied to a copy of the slice returned by GetIMAPConfigs(), not to the actual config values. This means that if cfg.IMAPConfigs is populated, the changes to allConfigs won't affect cfg.IMAPConfigs, and if cfg.IMAP is used, changes won't affect cfg.IMAP. The issue is partially addressed by lines 126-130, but those lines won't work correctly when a single IMAP config exists because cfg.IMAP.Host != \"\" will be true but allConfigs will contain modified copies, not the original. Consider directly iterating over cfg.IMAPConfigs when it's populated, and handling cfg.IMAP separately.
| // Fetch from each IMAP inbox concurrently | ||
| for i := range imapConfigs { | ||
| wg.Add(1) | ||
| go func(imapCfg *config.IMAPConfig, index int) { |
There was a problem hiding this comment.
The goroutine receives a pointer to the slice element (&imapConfigs[i]), which is safe, but it's better practice to pass by value for the config struct to avoid potential issues if the slice is modified elsewhere. Consider changing the parameter to imapCfg config.IMAPConfig and passing imapConfigs[i] instead of &imapConfigs[i] at line 249.
|
|
||
| **Features:** | ||
| - Concurrent fetching from all configured inboxes | ||
| - Automatic deduplication of reports across inboxes |
There was a problem hiding this comment.
The documentation claims 'Automatic deduplication of reports across inboxes', but this feature relies on the existing INSERT OR IGNORE behavior in SaveReport() which uses report_id as a unique constraint. This is not new functionality added by this PR. Consider clarifying that deduplication is handled by the existing database unique constraint on report_id, or remove this bullet if it's not specific to the multi-inbox feature.
| - Automatic deduplication of reports across inboxes | |
| - Deduplication of reports across inboxes is handled by the database's unique constraint on `report_id` |
| // Support IMAP_CONFIGS environment variable for multiple configs | ||
| imapConfigsJSON := os.Getenv("IMAP_CONFIGS") | ||
| if imapConfigsJSON != "" { | ||
| var imapConfigs []IMAPConfig | ||
| if err := json.Unmarshal([]byte(imapConfigsJSON), &imapConfigs); err != nil { | ||
| return nil, err | ||
| } | ||
| cfg.IMAPConfigs = imapConfigs | ||
| } |
There was a problem hiding this comment.
The IMAP_CONFIGS environment variable overrides any imap_configs loaded from the config file without warning or merging. This could lead to confusion if a user has both a config file with imap_configs and the IMAP_CONFIGS environment variable set. Consider documenting this precedence order or logging a warning when the environment variable overrides the file configuration.
| } else if cfg.IMAP.Host != "" { | ||
| cfg.IMAP = allConfigs[0] |
There was a problem hiding this comment.
When using single IMAP config, if allConfigs is empty (which can only happen if cfg.IMAP.Host is empty based on GetIMAPConfigs() logic), this code will panic with an index out of bounds error when accessing allConfigs[0]. Add a length check before accessing the first element or restructure the logic to ensure allConfigs is non-empty.
This commit implements support for fetching DMARC reports from multiple IMAP inboxes concurrently while maintaining full backward compatibility with single inbox configurations.
Key changes:
Backend (internal/config/config.go):
Fetching logic (cmd/parse-dmarc/main.go):
Documentation:
Benefits:
Testing: