Refactoring Patterns#

Lead Section#

Historical Context: PR #404 eliminated the internal/model/ re-export layer and moved the model to pkg/ for public consumption. Code examples referencing internal/model/ are preserved for historical/educational reference but no longer exist in the current codebase.

Refactoring patterns are systematic techniques for restructuring code without changing its external behavior. In the context of the opnDossier project, these patterns address common challenges in Go codebases including type alias removal, package renames, function migrations, and re-export layer updates. These patterns emerged from practical refactoring work on a multi-layered architecture that separates XML schema definitions from domain logic.

The patterns documented here are derived from real refactoring efforts that successfully reduced the model package from ~3,565 to ~537 non-test lines while maintaining full backward compatibility. They emphasize safety-first approaches including grep-based validation, type aliases for backward compatibility, and re-export layers to maintain stable API boundaries during package restructuring.

These refactoring patterns are particularly relevant for Go projects with complex package hierarchies, especially those implementing parser/converter architectures that transform external data formats (like XML configuration files) into internal domain models.

Core Refactoring Patterns#

Type Alias Migration Pattern#

Historical Note: This pattern was extensively used in opnDossier but the re-export layer has since been removed in PR #404. See the "Re-Export Layer Removal Pattern" section for how the layer was eventually eliminated.

Type aliases (type X = Y) enable backward-compatible package refactoring by maintaining stable API surfaces while reorganizing internal implementation. This pattern was demonstrated in PR #186, which split a monolithic model package into focused schema and enrichment packages.

Implementation Example (historical - from the now-removed internal/model/ re-export layer):

// Package model re-exports types from internal/schema for backward compatibility.
// New code should import internal/schema directly.
package model

import "github.com/EvilBit-Labs/opnDossier/internal/schema"

// BoolFlag provides custom XML marshaling for OPNsense boolean values.
// Type alias to schema.BoolFlag - all methods are inherited.
type BoolFlag = schema.BoolFlag

// ChangeMeta tracks creation and modification metadata for configuration items.
type ChangeMeta = schema.ChangeMeta

Safety Consideration: AGENTS.md documents the grep validation pattern required before removing type aliases:

Before removing, grep the entire codebase for external references (e.g., grep -r 'pkg.AliasName'). The internal/model/ re-export layer and cmd/ package frequently reference aliases from internal packages.

Results: This pattern enabled an 85% code reduction in the model package while maintaining zero breaking changes for consumers.

Re-Export Layer Pattern#

Historical Note: This pattern was used during the internal reorganization phase. The re-export layer (internal/model/) was later removed in PR #404 when the model was moved to pkg/ for public consumption. See the "Re-Export Layer Removal Pattern" section for the reverse pattern.

The re-export layer pattern creates a stable seam between internal implementation packages and external consumers. This pattern was implemented in internal/model/, which re-exported both types and constants (historical example):

package model

import "github.com/EvilBit-Labs/opnDossier/internal/model/common"

// CommonDevice is the platform-agnostic domain model for a firewall device.
type CommonDevice = common.CommonDevice

// DeviceType identifies the platform that produced a configuration.
type DeviceType = common.DeviceType

const (
    // DeviceTypeOPNsense represents an OPNsense device.
    DeviceTypeOPNsense = common.DeviceTypeOPNsense
    // DeviceTypePfSense represents a pfSense device.
    DeviceTypePfSense = common.DeviceTypePfSense
    // DeviceTypeUnknown represents an unrecognized device type.
    DeviceTypeUnknown = common.DeviceTypeUnknown
)

Architecture Principle: The re-export layer maintained backward compatibility while enabling internal reorganization. Consumers imported from internal/model/, not from implementation packages like internal/schema/ or internal/model/common/.

Package Rename Pattern#

Package renaming is performed systematically with re-export layers maintaining compatibility during migration. PR #258 demonstrates this by renaming internal/log to internal/logging across all imports.

Migration Strategy:

1. Create new package with desired name
2. Update all internal imports systematically
3. Update test files and documentation
4. Verify with grep that no old imports remain

PR #273 shows a more complex example where import paths changed from internal/model to internal/model/common as part of introducing device abstraction capabilities. Later, PR #404 moved these packages from internal/ to pkg/ for public consumption:

• internal/model/common/ → pkg/model/
• internal/model/opnsense/ → pkg/parser/opnsense/
• internal/model/factory.go → pkg/parser/factory.go
• internal/schema/ → pkg/schema/opnsense/

Function Migration Pattern#

Function migrations involve updating signatures, often to add context or logging parameters. PR #269 demonstrates refactoring audit options handling:

Before:

handleAuditMode(ctx, doc, opt, logger)

After:

handleAuditMode(ctx, device, auditOpts, opt, logger)

The migration introduced a dedicated audit.Options parameter for explicit configuration passing. The PR also removed the MustGetCommandContext() function, shifting from panic-based error handling to explicit nil-handling.

Systematic Approach:

1. Define new function signature with explicit parameters
2. Update all call sites to pass new parameters
3. Remove old helper functions that are no longer needed
4. Verify with tests that behavior is preserved

Logging Framework Migration Pattern#

The logging migration demonstrates multi-phase refactoring across several PRs. The project uses charmbracelet/log wrapped in internal/logging/ as an abstraction layer:

package logging

import "github.com/charmbracelet/log"

// Logger is the application logger instance.
type Logger struct {
    *log.Logger
}

// Config holds the logger configuration options.
type Config struct {
    Level string
    Format string // "text" or "json"
    Output io.Writer
    ReportCaller bool
    ReportTimestamp bool
}

Migration Evolution:

1. PR #257: Implemented dynamic logging levels mapping application logger to slog and charmbracelet/log
2. PR #269: Simplified logging level mapping by removing determineAuditLogLevels function
3. PR #277: Completed migration by removing unnecessary logger creation and passing application logger directly

Context Support: The logger includes placeholder for future context integration:

// WithContext returns a logger with the provided context.
// charmbracelet/log doesn't have built-in context support yet;
// this signature exists so callers can pass ctx for future extraction of
// trace IDs, request IDs, or cancellation-aware logging.
func (l *Logger) WithContext(ctx context.Context) *Logger {
    _ = ctx // TODO: extract trace/request IDs from ctx when logging middleware is added
    return l
}

Interface Injection Pattern for pkg/internal Boundary Enforcement#

When pkg/ packages need functionality from internal/ packages, Go's module-level internal/ access restriction prevents direct imports. The interface injection pattern solves this by defining an interface in pkg/ and injecting the concrete internal/ implementation at the cmd/ layer.

Problem: After moving packages from internal/ to pkg/ for public API exposure, some pkg/ code still needed internal/ functionality (e.g., cfgparser.XMLParser). Moving the entire dependency chain to pkg/ would cascade across the codebase.

Solution: Define interfaces in pkg/, implement in internal/, wire in cmd/.

Implementation Example:

// pkg/parser/factory.go - Define interface in public package
type OPNsenseXMLDecoder interface {
    Parse(ctx context.Context, r io.Reader) (*schema.OpnSenseDocument, error)
    ParseAndValidate(ctx context.Context, r io.Reader) (*schema.OpnSenseDocument, error)
}

func NewFactory(decoder OPNsenseXMLDecoder) *Factory {
    return &Factory{xmlDecoder: decoder}
}

Go's structural typing allows pkg/ sub-packages to define their own unexported interface that internal/ types satisfy without importing them:

// pkg/parser/opnsense/parser.go - Local unexported interface
type xmlDecoder interface {
    Parse(ctx context.Context, r io.Reader) (*schema.OpnSenseDocument, error)
    ParseAndValidate(ctx context.Context, r io.Reader) (*schema.OpnSenseDocument, error)
}

func NewParser(decoder xmlDecoder) *Parser {
    return &Parser{decoder: decoder}
}

Application code wires the concrete implementation:

// cmd/convert.go - Wire at application boundary
factory := parser.NewFactory(cfgparser.NewXMLParser())

Benefits:

• Maintains pkg/ purity (no internal/ imports in production code)
• Avoids cascading dependency chain moves to pkg/
• Allows flexible implementation swapping
• Supports testing with mock implementations

Boundary Verification:

# Check for pkg/ importing internal/ before committing
grep -rn 'internal/' --include='*.go' pkg/ | grep -v _test.go

Related: See docs/solutions/architecture-issues/pkg-internal-import-boundary.md for a real refactoring example that fixed pkg/ importing internal/ violations.

Unexporting Types Pattern#

When reducing API surface, making types unexported requires maintaining testability for external test packages. This pattern demonstrates adding convenience functions to preserve access while simplifying the public interface.

When to Use: A type should be unexported when it's an implementation detail, not a primary entry point for consumers.

Problem: After moving Converter from internal/model/opnsense/ to pkg/parser/opnsense/, the type was exported but Parser was the intended public entry point. External test packages needed access to conversion logic for testing.

Solution: Unexport the type and add a convenience function for direct access:

// pkg/parser/opnsense/converter.go
// converter is unexported - Parser is the intended entry point
type converter struct{}

// ConvertDocument converts an OPNsense schema document into a platform-agnostic CommonDevice.
// This is a convenience function for consumers who have a pre-parsed OpnSenseDocument.
func ConvertDocument(doc *schema.OpnSenseDocument) (*common.CommonDevice, []common.ConversionWarning, error) {
    c := &converter{}
    return c.toCommonDevice(doc)
}

Trade-offs:

• Simpler API surface: Consumers see fewer public types in pkg/parser/opnsense/
• Explicit control: No direct constructor access means conversions go through ConvertDocument() or Parser.Parse()
• Testability preserved: Test packages can still test conversion logic via the convenience function

Pattern Usage: Common when consolidating multiple entry points (e.g., Parser and Converter) into a single primary API (Parser), while maintaining backward compatibility or test access through functions.

File-Split Refactoring Pattern#

When splitting a large file (>800 lines) into domain-specific files within the same package, all functions remain in the package regardless of which file they're in. This pattern demonstrates safe file splitting while avoiding common pitfalls with pre-commit hooks and test breakage.

When to Use: Files exceeding the 800-line project maximum should be split by domain responsibility while keeping related functionality cohesive.

Pattern Guidelines:

• All functions remain in the same package — tests find them regardless of file
• Shared helpers used across domain files stay in the orchestrator file; domain-specific helpers move with their domain
• Naming convention: <base>_<domain>.go (e.g., validate_system.go, report_statistics.go)
• Precedents: PR #415 (report.go split), PR #417 (opnsense.go split), pkg/parser/opnsense/ (converter_*.go)

Implementation Example (from PR #417):

Gotchas and Prevention:

1. Pre-commit hook interference: gofumpt and linters may silently rearrange helpers after file splits

   • Prevention: Always re-verify file contents after pre-commit hooks run
   • Detection: Check line counts with wc -l and verify no duplicates with go build

2. Test breakage on signature changes: Unexported function signature changes break same-package test files that call directly

   • Prevention: Grep for all call sites in *_test.go before changing signatures: grep -rn 'functionName(' package/*_test.go
   • Fix: Update all test call sites to match the new signature

3. Pre-existing issues in moved code: Per project policy ("Zero tolerance for tech debt"), code review findings on moved code are in-scope and must be fixed in the same PR

Verification Steps:

# After completing a file split
wc -l internal/package/*.go # Verify line counts match intent
go build ./internal/package/ # Verify no duplicate declarations
just ci-check # Catch all issues in one pass

Related: See docs/solutions/architecture-issues/file-split-refactor-gotchas.md for detailed gotchas and the real refactoring example from PR #417.

Registry-Based Dispatch Pattern#

When multiple locations require format dispatch, validation, or metadata (file extensions, aliases), scattered switch statements create maintenance burden. The registry pattern centralizes these concerns into a single source of truth with pluggable handlers.

The opnDossier project implements this pattern in two places: FormatRegistry for output format handling and DeviceParserRegistry for parser dispatch.

FormatRegistry Example#

Problem: Before PR #434, format dispatch was scattered across 8+ locations with switch statements, making it difficult to add new formats. Adding a new format required updating constants in cmd/convert.go, validation logic in internal/config/validation.go, switch blocks in internal/converter/hybrid_generator.go, completion functions in cmd/shared_flags.go, and file extension logic in multiple locations.

Solution: Introduce a FormatRegistry as the single source of truth for format names, aliases, file extensions, validation, and handler dispatch.

Implementation:

// internal/converter/registry.go

// FormatHandler defines the interface for format-specific generation logic.
type FormatHandler interface {
    FileExtension() string
    Aliases() []string
    Generate(g *HybridGenerator, data *common.CommonDevice, opts Options) (string, error)
    GenerateToWriter(g *HybridGenerator, w io.Writer, data *common.CommonDevice, opts Options) error
}

// FormatRegistry centralises format dispatch by mapping canonical format names
// and aliases to FormatHandler implementations.
type FormatRegistry struct {
    handlers map[string]FormatHandler
    aliases map[string]string
}

// Register adds a handler for the given canonical format name.
// Panics on duplicate registration (database/sql driver pattern).
func (r *FormatRegistry) Register(format string, handler FormatHandler) {
    // ... registration logic with alias handling
}

// Get returns the handler for the given format name (canonical or alias).
func (r *FormatRegistry) Get(format string) (FormatHandler, error) {
    // ... resolution logic
}

The DefaultRegistry singleton registers all built-in handlers:

// DefaultRegistry is the package-level registry pre-populated with all built-in
// format handlers. It is the single source of truth for supported formats.
var DefaultRegistry = newDefaultRegistry()

func newDefaultRegistry() *FormatRegistry {
    r := NewFormatRegistry()
    r.Register("markdown", &markdownHandler{})
    r.Register("json", &jsonHandler{})
    r.Register("yaml", &yamlHandler{})
    r.Register("text", &textHandler{})
    r.Register("html", &htmlHandler{})
    return r
}

Each handler delegates to the generator's private format-specific methods:

// markdownHandler handles markdown format output.
type markdownHandler struct{}

func (h *markdownHandler) FileExtension() string { return ".md" }
func (h *markdownHandler) Aliases() []string { return []string{"md"} }

func (h *markdownHandler) Generate(g *HybridGenerator, data *common.CommonDevice, opts Options) (string, error) {
    return g.generateMarkdown(data, opts)
}

Benefits:

• Centralized format metadata: File extensions, aliases, and format names live in one place
• Automatic validation: Format.Validate() and config.ValidFormats delegate to registry
• Shell completion: cmd.ValidFormats() derives from DefaultRegistry.ValidFormats() (alphabetically sorted)
• Eliminates switch statements: Both Generate() and GenerateToWriter() dispatch via handlerForFormat()
• Extensibility: Adding a new format requires only registering a handler in newDefaultRegistry()

Handler Coupling: FormatHandler.Generate() accepts *HybridGenerator (concrete type) because this is a dispatch table, not a strategy pattern. All handlers are internal to the converter package. This design is acceptable for the current scope; decouple to interface/closure if external format registration is needed.

Related: See AGENTS.md §5.9b for detailed implementation guidance on the FormatRegistry pattern.

DeviceParserRegistry Example#

Problem: Before PR #437, the Factory used hardcoded switch statements to dispatch parser creation based on device type. Adding a new device parser required modifying pkg/parser/factory.go, and there was no runtime discovery mechanism for enumerating supported device types.

Solution: Introduce a thread-safe DeviceParserRegistry with self-registration via init() functions in parser packages, following the database/sql driver pattern.

Implementation:

// pkg/parser/registry.go

// ConstructorFunc is the factory function signature for creating DeviceParser instances.
type ConstructorFunc = func(OPNsenseXMLDecoder) DeviceParser

type DeviceParserRegistry struct {
    mu sync.RWMutex
    parsers map[string]ConstructorFunc
}

func NewDeviceParserRegistry() *DeviceParserRegistry {
    return &DeviceParserRegistry{parsers: make(map[string]ConstructorFunc)}
}

func DefaultRegistry() *DeviceParserRegistry {
    defaultRegistryOnce.Do(func() {
        defaultRegistry = NewDeviceParserRegistry()
    })
    return defaultRegistry
}

// Register adds a constructor for the given device type name.
// Panics on duplicate registration (database/sql driver pattern).
func (r *DeviceParserRegistry) Register(deviceType string, fn ConstructorFunc) {
    // ... registration logic with case-insensitive matching
}

// Get returns the constructor for the given device type.
func (r *DeviceParserRegistry) Get(deviceType string) (ConstructorFunc, bool) {
    // ... resolution logic
}

// List returns a sorted slice of all registered device type names.
func (r *DeviceParserRegistry) List() []string {
    // ... sorted list generation
}

// Package-level convenience wrapper
func Register(deviceType string, fn ConstructorFunc) {
    DefaultRegistry().Register(deviceType, fn)
}

Self-Registration Pattern:

Each parser package registers itself during initialization:

// pkg/parser/opnsense/parser.go

func NewParserFactory(decoder parser.OPNsenseXMLDecoder) parser.DeviceParser {
    return NewParser(decoder)
}

func init() {
    parser.Register("opnsense", NewParserFactory)
}

Factory Integration:

The Factory consults the registry via (fn, ok) lookups, eliminating switch statements:

// pkg/parser/factory.go

type Factory struct {
    xmlDecoder OPNsenseXMLDecoder
    registry *DeviceParserRegistry
}

func NewFactory(decoder OPNsenseXMLDecoder) *Factory {
    return &Factory{xmlDecoder: decoder, registry: DefaultRegistry()}
}

func NewFactoryWithRegistry(decoder OPNsenseXMLDecoder, reg *DeviceParserRegistry) *Factory {
    return &Factory{xmlDecoder: decoder, registry: reg}
}

func (f *Factory) createWithOverride(ctx context.Context, r io.Reader, override string, validateMode bool) {
    fn, ok := f.registry.Get(override)
    if !ok {
        return nil, nil, fmt.Errorf(
            "unsupported device type override: %s; supported: %s",
            override, strings.Join(f.registry.List(), ", "),
        )
    }
    return parseDevice(ctx, fn(f.xmlDecoder), r, validateMode)
}

Benefits:

• Self-registration: Parser packages register via init() without modifying factory code
• Dynamic discovery: registry.List() enables CLI completions and dynamic error messages
• Thread-safe: All operations protected by sync.RWMutex
• Test isolation: NewFactoryWithRegistry() + NewDeviceParserRegistry() prevent global pollution
• Fail-fast: Duplicate registrations panic immediately during initialization
• Extensibility: External Go projects can register custom parsers at compile time

Blank Import Requirement:

Since factory.go no longer imports parser packages directly, registration requires blank imports:

// cmd/root.go
import _ "github.com/EvilBit-Labs/opnDossier/pkg/parser/opnsense"

Without the blank import, the parser's init() function never runs and the registry remains empty. Test files using parser.NewFactory() must include their own blank imports.

Interface Injection: The ConstructorFunc signature uses parser.OPNsenseXMLDecoder, following the same Interface Injection Pattern used throughout the parser layer. This allows internal/cfgparser.XMLParser to be injected at the cmd/ boundary without pkg/ importing internal/.

Related: See docs/solutions/architecture-issues/pluggable-deviceparser-registry-pattern.md for complete implementation details, testing strategy, and gotchas.

Re-Export Layer Removal Pattern#

This pattern demonstrates how to safely remove a re-export compatibility layer after migrating consumers to direct imports. PR #404 shows the complete removal of internal/model/ (17 files, ~90 type aliases) when moving to public pkg/ packages.

Prerequisites for Safe Removal:

1. All consumers have migrated to direct imports
2. No external references to the re-export layer remain
3. Documentation updated to reflect new import paths
4. Public package structure is stable

Implementation Strategy:

1. Move implementation packages to public location:

   ⧉

   git mv internal/model/common/ pkg/model/
   git mv internal/model/opnsense/ pkg/parser/opnsense/
   git mv internal/schema/ pkg/schema/opnsense/
   

2. Update all imports systematically (104 files in PR #404):

   ⧉

   // Before
   "github.com/EvilBit-Labs/opnDossier/internal/model/common"
   "github.com/EvilBit-Labs/opnDossier/internal/model"
   
   // After
   common "github.com/EvilBit-Labs/opnDossier/pkg/model"
   "github.com/EvilBit-Labs/opnDossier/pkg/parser"
   

3. Rename types to follow Go naming conventions:

   • ParserFactory → Factory (avoid package name stuttering: parser.Factory not parser.ParserFactory)
   • Update constructor: NewParserFactory() → NewFactory(decoder)

4. Delete the entire re-export layer:

   ⧉

   rm -rf internal/model/
   

5. Update linter configuration:

   ⧉

   # .golangci.yml
   - path: pkg/model/ # was internal/model/common/
   

Verification:

# No remaining references to old paths
grep -r "internal/model" internal/ cmd/ pkg/
grep -r "internal/schema" internal/ cmd/ pkg/

# All tests pass
just ci-check

Design Decisions:

• Import aliases preserved (common, schema) to minimize code changes
• pkg/ packages never import internal/ packages (enforced by Go compiler)
• Only one new file created: pkg/model/warning.go (moved from embedded re-export location)

Impact: PR #404 changed 173 files (+599 / -1,303) - net deletion because the re-export layer was entirely removed.

Multi-Layer Architecture Context#

Package Structure#

The refactoring patterns are applied within a multi-layer architecture that separates concerns:

1. pkg/schema/opnsense/ - XML DTO layer with OPNsense-specific structs carrying xml tags
2. pkg/parser/opnsense/ - Parser and converter that transforms schema to CommonDevice
3. pkg/model/ - Device-agnostic domain model with no XML tags (previously internal/model/common/)
4. pkg/parser/ - Factory interface providing device type detection and parser delegation
5. internal/converter/ - Format registry and generators that transform CommonDevice to output formats

Historical Note: Before PR #404, there was a Layer 2 (internal/model/) that served as a re-export compatibility layer. This layer was eliminated when the packages moved from internal/ to pkg/ for public consumption.

Parser/Converter Pattern#

The pkg/parser/opnsense/ package demonstrates the conversion pattern:

// Parser implements the DeviceParser interface for OPNsense configuration
// files. The XML decoder is injected at construction to keep this package
// free of internal/ imports.
type Parser struct {
    decoder xmlDecoder
}

// NewParser returns a new OPNsense Parser backed by the given XML decoder.
// The decoder is typically cfgparser.NewXMLParser(), wired at the application layer.
func NewParser(decoder xmlDecoder) *Parser {
    return &Parser{decoder: decoder}
}

// Parse reads an OPNsense XML configuration from r and returns a 
// platform-agnostic CommonDevice.
func (p *Parser) Parse(ctx context.Context, r io.Reader) (*common.CommonDevice, []common.ConversionWarning, error) {
    doc, err := p.decoder.Parse(ctx, r)
    if err != nil {
        return nil, nil, fmt.Errorf("opnsense parser: %w", err)
    }
    return toCommonDevice(doc)
}

The converter transforms schema documents to CommonDevice:

// Converter transforms a schema.OpnSenseDocument into a common.CommonDevice.
type Converter struct{}

// ToCommonDevice converts an OPNsense schema document into a platform-agnostic CommonDevice.
func (c *Converter) ToCommonDevice(doc *schema.OpnSenseDocument) (*common.CommonDevice, []common.ConversionWarning, error) {
    if doc == nil {
        return nil, nil, fmt.Errorf("ToCommonDevice: %w", ErrNilDocument)
    }

    device := &common.CommonDevice{
        DeviceType: common.DeviceTypeOPNsense,
        Version: doc.Version,
        Theme: doc.Theme,
        System: c.convertSystem(doc),
        Interfaces: c.convertInterfaces(doc),
        // ... 30+ fields converted
    }

    return device, warnings, nil
}

Isolation Principle: Only pkg/parser/opnsense/ imports pkg/schema/opnsense/. All downstream code operates on common.CommonDevice.

Refactoring Gotchas and Best Practices#

Type Alias Removal Hazards#

Gotcha: Removing a type alias breaks ALL consumers that reference it, even if they import from the re-export layer.

Best Practice: Use grep to find all references before removal:

grep -r 'pkg.AliasName' internal/ cmd/

The internal/model/ re-export layer and cmd/ package frequently reference aliases from internal packages.

Re-Export Layer Consistency#

Historical Note: This gotcha applied when the internal/model/ re-export layer existed. After PR #404, consumers import directly from pkg/ packages.

Gotcha: Changing constants in source packages requires updating all consumers.

Best Practice: When modifying constants in public packages like pkg/schema/opnsense/ or pkg/model/, ensure all consumers are updated. Previously, the re-export layer had to remain stable for consumers, but after PR #404, consumers import directly from pkg/ packages.

XML Field Semantics#

Gotcha: OPNsense uses two boolean patterns - presence-based and value-based. Choosing the wrong type silently breaks semantics.

Best Practice:

• Presence-based (isset() in PHP): Use BoolFlag. Examples: <disabled/>, <log/>, <not/>, <quick/>
• Value-based (== "1" in PHP): Use string. Examples: <enable>1</enable>, <blockpriv>1</blockpriv>

Context Underutilization#

Gotcha: Many functions accept context but don't use it.

Best Practice: When refactoring, add context checks to long-running operations:

select {
case <-ctx.Done():
    return ctx.Err()
default:
}

The factory demonstrates context cancellation handling:

// newCtxReader wraps an io.Reader so that each Read call checks ctx for
// cancellation before delegating.
func newCtxReader(ctx context.Context, r io.Reader) io.Reader {
    return readerFunc(func(p []byte) (int, error) {
        if err := ctx.Err(); err != nil {
            return 0, err
        }
        return r.Read(p)
    })
}

Global State Dependencies#

Gotcha: Plugin registry and CLI globals can complicate testing.

Best Practice: Use dependency injection where possible:

// Avoid: GlobalRegistry = NewPluginRegistry()
// Prefer: registry := NewPluginRegistry(); pass to consumers

Error Handling During Refactoring#

Best Practice: Follow Go's standard error wrapping pattern with context:

// Always wrap errors with context
func parseXMLConfig(filename string) (*Config, error) {
    data, err := os.ReadFile(filename)
    if err != nil {
        return nil, fmt.Errorf("failed to read config file %s: %w", filename, err)
    }
    // ...
}

Usage Examples#

Example 1: Type Alias Migration#

Migrating from a flat model package to layered architecture:

Step 1: Create focused packages

mkdir -p internal/schema internal/model/common
mv internal/model/*_dto.go internal/schema/
mv internal/model/*_domain.go internal/model/common/

Step 2: Add type aliases in re-export layer (internal/model/types.go)

package model

import (
    "github.com/yourorg/yourproject/internal/schema"
    "github.com/yourorg/yourproject/internal/model/common"
)

// Re-export schema types for backward compatibility
type OpnSenseDocument = schema.OpnSenseDocument
type InterfaceGroups = schema.InterfaceGroups

// Re-export domain types
type CommonDevice = common.CommonDevice
type DeviceType = common.DeviceType

Step 3: Verify no external breakage

grep -r 'model\.' cmd/ internal/converter/ internal/processor/
go test ./...

Step 4: Add deprecation comments

// OpnSenseDocument represents OPNsense configuration.
// Deprecated: Import from internal/schema directly.
type OpnSenseDocument = schema.OpnSenseDocument

Step 5: Gradually migrate consumers to direct imports

// Old: import "github.com/yourorg/yourproject/internal/model"
// New: import "github.com/yourorg/yourproject/internal/schema"

Example 2: Function Migration with Logging#

Migrating from implicit logger to explicit parameter:

Before:

func ProcessConfig(ctx context.Context, filename string) error {
    log.Info("Processing", "file", filename)
    // ...
}

After:

func ProcessConfig(ctx context.Context, filename string, logger *logging.Logger) error {
    logger.Info("Processing", "file", filename)
    // ...
}

Migration Steps:

1. Add logger parameter to function signature
2. Replace all log calls with logger method calls
3. Update all call sites to pass logger
4. Update tests to inject test logger

Example 3: Package Rename with Re-Export#

Renaming internal/log to internal/logging:

Step 1: Create compatibility bridge (internal/log/bridge.go)

// Package log is deprecated. Use internal/logging instead.
package log

import "github.com/yourorg/yourproject/internal/logging"

// Logger is deprecated. Use logging.Logger.
type Logger = logging.Logger

// New is deprecated. Use logging.New.
func New(cfg Config) (*Logger, error) {
    return logging.New(cfg)
}

Step 2: Update imports systematically

# Find all imports
grep -r 'internal/log"' internal/ cmd/

# Update one package at a time, verify tests pass
sed -i 's|internal/log"|internal/logging"|g' internal/converter/*.go
go test ./internal/converter/

Step 3: Remove bridge after all consumers migrated

rm internal/log/bridge.go

Testing During Refactoring#

Use table-driven tests with t.Run() subtests to verify refactored code:

func TestRefactoredFunction(t *testing.T) {
    tests := []struct {
        name string
        input string
        expected string
        wantErr bool
    }{
        {"valid case", "input1", "expected1", false},
        {"error case", "bad", "", true},
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            result, err := RefactoredFunction(tt.input)
            if (err != nil) != tt.wantErr {
                t.Errorf("error = %v, wantErr %v", err, tt.wantErr)
            }
            if result != tt.expected {
                t.Errorf("got %v, want %v", result, tt.expected)
            }
        })
    }
}

Pre-Commit Checks#

Run these before committing refactored code:

just format # Run gofmt
just lint # Run golangci-lint
just check # Run all pre-commit checks
just test # Verify >80% test coverage maintained

Commit Standards#

Conventional Commits#

Use Conventional Commits format:

<type>(<scope>): <description>

Examples:
refactor(model): consolidate type aliases in re-export layer
refactor(parser): migrate logging from slog to charmbracelet/log
refactor(converter): unify markdown, json, yaml converters

Types for refactoring work:

• refactor - Code refactoring without changing behavior
• style - Code formatting (use gofmt)
• fix - Bug fixes during refactoring

Migration Checklist#

When performing major refactoring:

• Identify all consumers of the API being changed (use grep)
• Create backward-compatibility aliases in re-export layer if needed
• Update consumer code systematically, one package at a time
• Maintain >80% test coverage throughout migration
• Run just ci-check to verify all quality gates pass
• Use meaningful commit messages following Conventional Commits
• Update documentation if API changes are user-facing
• Test with XML round-trip tests if modifying schema/parser
• Verify error handling preserves context with %w wrapper
• Check for global state dependencies that complicate testing
• Remove deprecated code after migration period

Tools and Commands#

Development commands for quality assurance during refactoring:

just test # Run all tests
just lint # Check code quality
just format # Format code with gofmt
just check # Run pre-commit checks
just ci-check # Run comprehensive CI checks
just coverage # Check test coverage
go test -race ./... # Detect race conditions

Grep Patterns for Refactoring#

Find type alias usage:

grep -r 'pkg\.TypeName' internal/ cmd/

Find function call sites:

grep -r 'FunctionName(' internal/ cmd/

Find import statements:

grep -r 'import.*"pkg/path"' .

Find constant references:

grep -r 'pkg\.ConstantName' internal/ cmd/

Relevant Code Files#

Related Topics#

Architectural Patterns#

• Multi-Layer Architecture - Separation of XML DTOs, domain models, and parsers
• Parser/Converter Pattern - Transform external formats to internal domain models
• Factory Pattern - DeviceParser interface and Factory for device type detection (renamed from ParserFactory in PR #404)
• Interface Injection Pattern - Define interfaces in pkg/ to avoid internal/ imports
• Registry-Based Dispatch - Centralized format handling via FormatRegistry (PR #434)

Go-Specific Patterns#

• Type Aliases for Backward Compatibility - Using type X = Y for refactoring
• Interface-Based Abstraction - DeviceParser as stable boundary
• Error Wrapping with Context - Using fmt.Errorf with %w verb
• Unexporting Types - Reducing API surface while maintaining testability

Refactoring Techniques#

• Deprecation Strategy - Gradual migration with deprecation comments
• Re-Export Layers - Stable API boundaries during internal restructuring
• Dependency Injection - Replacing global state with explicit parameters
• File-Split Refactoring - Domain-based file organization for large packages

Testing and Quality#

• Table-Driven Tests - Using t.Run() for refactoring verification
• Test Coverage Maintenance - Keeping >80% coverage during refactoring
• Pre-Commit Validation - Using just commands for quality gates

See Also#

• architecture documentation - Multi-layer architecture design
• api documentation - Re-export layer patterns (historical context)
• standards documentation - Go development standards
• pkg-internal-import-boundary.md - Interface injection pattern in practice
• file-split-refactor-gotchas.md - File-split refactoring gotchas and solutions
• pluggable-deviceparser-registry-pattern.md - DeviceParserRegistry implementation details
• PR #186 - Model package split refactoring
• PR #273 - Multi-device model layer introduction
• PR #269 - Logging and audit refactoring
• PR #404 - Re-export layer removal and public package promotion
• PR #415 - Report package file split
• PR #417 - Validator package file split
• PR #434 - FormatRegistry introduction for centralized format dispatch
• PR #437 - DeviceParserRegistry with self-registration pattern