Refactor: Extract shared log aggregation logic into generic helper (#3465)

Author: Copilot

pkg/cli/access_log.go

@@ -35,6 +35,35 @@ type DomainAnalysis struct {
 	DeniedCount    int
 }
 
+// GetAllowedDomains returns the list of allowed domains
+func (d *DomainAnalysis) GetAllowedDomains() []string {
+	return d.AllowedDomains
+}
+
+// GetDeniedDomains returns the list of denied domains
+func (d *DomainAnalysis) GetDeniedDomains() []string {
+	return d.DeniedDomains
+}
+
+// SetAllowedDomains sets the list of allowed domains
+func (d *DomainAnalysis) SetAllowedDomains(domains []string) {
+	d.AllowedDomains = domains
+}
+
+// SetDeniedDomains sets the list of denied domains
+func (d *DomainAnalysis) SetDeniedDomains(domains []string) {
+	d.DeniedDomains = domains
+}
+
+// AddMetrics adds metrics from another analysis
+func (d *DomainAnalysis) AddMetrics(other LogAnalysis) {
+	if otherDomain, ok := other.(*DomainAnalysis); ok {
+		d.TotalRequests += otherDomain.TotalRequests
+		d.AllowedCount += otherDomain.AllowedCount
+		d.DeniedCount += otherDomain.DeniedCount
+	}
+}
+
 // parseSquidAccessLog parses a squid access log file and extracts domain information
 func parseSquidAccessLog(logPath string, verbose bool) (*DomainAnalysis, error) {
 	file, err := os.Open(logPath)
@@ -175,70 +204,18 @@ func analyzeAccessLogs(runDir string, verbose bool) (*DomainAnalysis, error) {
 
 // analyzeMultipleAccessLogs analyzes multiple separate access log files
 func analyzeMultipleAccessLogs(accessLogsDir string, verbose bool) (*DomainAnalysis, error) {
-	files, err := filepath.Glob(filepath.Join(accessLogsDir, "access-*.log"))
-	if err != nil {
-		return nil, fmt.Errorf("failed to find access log files: %w", err)
-	}
-
-	if len(files) == 0 {
-		if verbose {
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("No access log files found in %s", accessLogsDir)))
-		}
-		return nil, nil
-	}
-
-	if verbose {
-		fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Analyzing %d access log files from %s", len(files), accessLogsDir)))
-	}
-
-	// Aggregate analysis from all files
-	aggregatedAnalysis := &DomainAnalysis{
-		AllowedDomains: []string{},
-		DeniedDomains:  []string{},
-	}
-
-	allAllowedDomains := make(map[string]bool)
-	allDeniedDomains := make(map[string]bool)
-
-	for _, file := range files {
-		if verbose {
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Parsing %s", filepath.Base(file))))
-		}
-
-		analysis, err := parseSquidAccessLog(file, verbose)
-		if err != nil {
-			if verbose {
-				fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to parse %s: %v", filepath.Base(file), err)))
+	return aggregateLogFiles(
+		accessLogsDir,
+		"access-*.log",
+		verbose,
+		parseSquidAccessLog,
+		func() *DomainAnalysis {
+			return &DomainAnalysis{
+				AllowedDomains: []string{},
+				DeniedDomains:  []string{},
 			}
-			continue
-		}
-
-		// Aggregate the metrics
-		aggregatedAnalysis.TotalRequests += analysis.TotalRequests
-		aggregatedAnalysis.AllowedCount += analysis.AllowedCount
-		aggregatedAnalysis.DeniedCount += analysis.DeniedCount
-
-		// Collect unique domains
-		for _, domain := range analysis.AllowedDomains {
-			allAllowedDomains[domain] = true
-		}
-		for _, domain := range analysis.DeniedDomains {
-			allDeniedDomains[domain] = true
-		}
-	}
-
-	// Convert maps to sorted slices
-	for domain := range allAllowedDomains {
-		aggregatedAnalysis.AllowedDomains = append(aggregatedAnalysis.AllowedDomains, domain)
-	}
-	for domain := range allDeniedDomains {
-		aggregatedAnalysis.DeniedDomains = append(aggregatedAnalysis.DeniedDomains, domain)
-	}
-
-	sort.Strings(aggregatedAnalysis.AllowedDomains)
-	sort.Strings(aggregatedAnalysis.DeniedDomains)
-
-	return aggregatedAnalysis, nil
+		},
+	)
 }
 
 // formatDomainWithEcosystem formats a domain with its ecosystem identifier if found

pkg/cli/firewall_log.go

@@ -122,6 +122,43 @@ type FirewallAnalysis struct {
 	RequestsByDomain map[string]DomainRequestStats
 }
 
+// GetAllowedDomains returns the list of allowed domains
+func (f *FirewallAnalysis) GetAllowedDomains() []string {
+	return f.AllowedDomains
+}
+
+// GetDeniedDomains returns the list of denied domains
+func (f *FirewallAnalysis) GetDeniedDomains() []string {
+	return f.DeniedDomains
+}
+
+// SetAllowedDomains sets the list of allowed domains
+func (f *FirewallAnalysis) SetAllowedDomains(domains []string) {
+	f.AllowedDomains = domains
+}
+
+// SetDeniedDomains sets the list of denied domains
+func (f *FirewallAnalysis) SetDeniedDomains(domains []string) {
+	f.DeniedDomains = domains
+}
+
+// AddMetrics adds metrics from another analysis
+func (f *FirewallAnalysis) AddMetrics(other LogAnalysis) {
+	if otherFirewall, ok := other.(*FirewallAnalysis); ok {
+		f.TotalRequests += otherFirewall.TotalRequests
+		f.AllowedRequests += otherFirewall.AllowedRequests
+		f.DeniedRequests += otherFirewall.DeniedRequests
+
+		// Merge request stats by domain
+		for domain, stats := range otherFirewall.RequestsByDomain {
+			existing := f.RequestsByDomain[domain]
+			existing.Allowed += stats.Allowed
+			existing.Denied += stats.Denied
+			f.RequestsByDomain[domain] = existing
+		}
+	}
+}
+
 // DomainRequestStats tracks request statistics per domain
 type DomainRequestStats struct {
 	Allowed int
@@ -374,77 +411,17 @@ func analyzeFirewallLogs(runDir string, verbose bool) (*FirewallAnalysis, error)
 
 // analyzeMultipleFirewallLogs analyzes multiple firewall log files in a directory
 func analyzeMultipleFirewallLogs(logsDir string, verbose bool) (*FirewallAnalysis, error) {
-	files, err := filepath.Glob(filepath.Join(logsDir, "*.log"))
-	if err != nil {
-		return nil, fmt.Errorf("failed to find firewall log files: %w", err)
-	}
-
-	if len(files) == 0 {
-		if verbose {
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("No firewall log files found in %s", logsDir)))
-		}
-		return nil, nil
-	}
-
-	if verbose {
-		fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Analyzing %d firewall log files from %s", len(files), logsDir)))
-	}
-
-	// Aggregate analysis from all files
-	aggregated := &FirewallAnalysis{
-		AllowedDomains:   []string{},
-		DeniedDomains:    []string{},
-		RequestsByDomain: make(map[string]DomainRequestStats),
-	}
-
-	allAllowedDomains := make(map[string]bool)
-	allDeniedDomains := make(map[string]bool)
-
-	for _, file := range files {
-		if verbose {
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Parsing %s", filepath.Base(file))))
-		}
-
-		analysis, err := parseFirewallLog(file, verbose)
-		if err != nil {
-			if verbose {
-				fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to parse %s: %v", filepath.Base(file), err)))
+	return aggregateLogFiles(
+		logsDir,
+		"*.log",
+		verbose,
+		parseFirewallLog,
+		func() *FirewallAnalysis {
+			return &FirewallAnalysis{
+				AllowedDomains:   []string{},
+				DeniedDomains:    []string{},
+				RequestsByDomain: make(map[string]DomainRequestStats),
 			}
-			continue
-		}
-
-		// Aggregate metrics
-		aggregated.TotalRequests += analysis.TotalRequests
-		aggregated.AllowedRequests += analysis.AllowedRequests
-		aggregated.DeniedRequests += analysis.DeniedRequests
-
-		// Collect unique domains
-		for _, domain := range analysis.AllowedDomains {
-			allAllowedDomains[domain] = true
-		}
-		for _, domain := range analysis.DeniedDomains {
-			allDeniedDomains[domain] = true
-		}
-
-		// Merge request stats by domain
-		for domain, stats := range analysis.RequestsByDomain {
-			existing := aggregated.RequestsByDomain[domain]
-			existing.Allowed += stats.Allowed
-			existing.Denied += stats.Denied
-			aggregated.RequestsByDomain[domain] = existing
-		}
-	}
-
-	// Convert sets to sorted slices
-	for domain := range allAllowedDomains {
-		aggregated.AllowedDomains = append(aggregated.AllowedDomains, domain)
-	}
-	for domain := range allDeniedDomains {
-		aggregated.DeniedDomains = append(aggregated.DeniedDomains, domain)
-	}
-
-	sort.Strings(aggregated.AllowedDomains)
-	sort.Strings(aggregated.DeniedDomains)
-
-	return aggregated, nil
+		},
+	)
 }

pkg/cli/log_aggregation.go

@@ -0,0 +1,108 @@
+package cli
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"sort"
+
+	"github.com/githubnext/gh-aw/pkg/console"
+)
+
+// LogAnalysis is an interface that both DomainAnalysis and FirewallAnalysis implement
+type LogAnalysis interface {
+	// GetAllowedDomains returns the list of allowed domains
+	GetAllowedDomains() []string
+	// GetDeniedDomains returns the list of denied domains
+	GetDeniedDomains() []string
+	// SetAllowedDomains sets the list of allowed domains
+	SetAllowedDomains(domains []string)
+	// SetDeniedDomains sets the list of denied domains
+	SetDeniedDomains(domains []string)
+	// AddMetrics adds metrics from another analysis
+	AddMetrics(other LogAnalysis)
+}
+
+// LogParser is a function type that parses a single log file
+type LogParser[T LogAnalysis] func(logPath string, verbose bool) (T, error)
+
+// aggregateLogFiles is a generic helper that aggregates multiple log files
+// It handles file discovery, parsing, domain deduplication, and sorting
+func aggregateLogFiles[T LogAnalysis](
+	logsDir string,
+	globPattern string,
+	verbose bool,
+	parser LogParser[T],
+	newAnalysis func() T,
+) (T, error) {
+	var zero T
+
+	// Find log files matching the pattern
+	files, err := filepath.Glob(filepath.Join(logsDir, globPattern))
+	if err != nil {
+		return zero, fmt.Errorf("failed to find log files: %w", err)
+	}
+
+	if len(files) == 0 {
+		if verbose {
+			fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("No log files found in %s", logsDir)))
+		}
+		return zero, nil
+	}
+
+	if verbose {
+		fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Analyzing %d log files from %s", len(files), logsDir)))
+	}
+
+	// Initialize aggregated analysis
+	aggregated := newAnalysis()
+
+	// Track unique domains across all files
+	allAllowedDomains := make(map[string]bool)
+	allDeniedDomains := make(map[string]bool)
+
+	// Parse each file and aggregate results
+	for _, file := range files {
+		if verbose {
+			fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Parsing %s", filepath.Base(file))))
+		}
+
+		analysis, err := parser(file, verbose)
+		if err != nil {
+			if verbose {
+				fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to parse %s: %v", filepath.Base(file), err)))
+			}
+			continue
+		}
+
+		// Aggregate metrics
+		aggregated.AddMetrics(analysis)
+
+		// Collect unique domains
+		for _, domain := range analysis.GetAllowedDomains() {
+			allAllowedDomains[domain] = true
+		}
+		for _, domain := range analysis.GetDeniedDomains() {
+			allDeniedDomains[domain] = true
+		}
+	}
+
+	// Convert maps to sorted slices
+	allowedDomains := make([]string, 0, len(allAllowedDomains))
+	for domain := range allAllowedDomains {
+		allowedDomains = append(allowedDomains, domain)
+	}
+	sort.Strings(allowedDomains)
+
+	deniedDomains := make([]string, 0, len(allDeniedDomains))
+	for domain := range allDeniedDomains {
+		deniedDomains = append(deniedDomains, domain)
+	}
+	sort.Strings(deniedDomains)
+
+	// Set the sorted domain lists
+	aggregated.SetAllowedDomains(allowedDomains)
+	aggregated.SetDeniedDomains(deniedDomains)
+
+	return aggregated, nil
+}