Resolve 9 bugs in SMTP plugin and add comprehensive tests

cleanup_test.go

@@ -0,0 +1,136 @@
+package smtp
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"go.uber.org/zap"
+)
+
+func TestCleanupTempFiles(t *testing.T) {
+	tmpDir := t.TempDir()
+	log, _ := zap.NewDevelopment()
+
+	p := &Plugin{
+		cfg: &Config{
+			AttachmentStorage: AttachmentConfig{
+				Mode:         "tempfile",
+				TempDir:      tmpDir,
+				CleanupAfter: 50 * time.Millisecond,
+			},
+		},
+		log: log,
+	}
+
+	// Create old files (matching pattern)
+	oldFile := filepath.Join(tmpDir, "smtp-att-old-file.txt")
+	os.WriteFile(oldFile, []byte("old"), 0644)
+	// Backdate the file
+	past := time.Now().Add(-1 * time.Hour)
+	os.Chtimes(oldFile, past, past)
+
+	// Create recent file (matching pattern)
+	recentFile := filepath.Join(tmpDir, "smtp-att-recent-file.txt")
+	os.WriteFile(recentFile, []byte("recent"), 0644)
+
+	// Create non-matching file
+	otherFile := filepath.Join(tmpDir, "other-file.txt")
+	os.WriteFile(otherFile, []byte("other"), 0644)
+	os.Chtimes(otherFile, past, past)
+
+	p.cleanupTempFiles()
+
+	// Old matching file should be removed
+	if _, err := os.Stat(oldFile); !os.IsNotExist(err) {
+		t.Error("old smtp-att file should have been removed")
+	}
+
+	// Recent matching file should remain
+	if _, err := os.Stat(recentFile); err != nil {
+		t.Error("recent smtp-att file should NOT have been removed")
+	}
+
+	// Non-matching file should remain
+	if _, err := os.Stat(otherFile); err != nil {
+		t.Error("non-matching file should NOT have been removed")
+	}
+}
+
+func TestCleanupTempFiles_NonexistentDir(t *testing.T) {
+	log, _ := zap.NewDevelopment()
+	p := &Plugin{
+		cfg: &Config{
+			AttachmentStorage: AttachmentConfig{
+				Mode:         "tempfile",
+				TempDir:      "/nonexistent/path",
+				CleanupAfter: time.Hour,
+			},
+		},
+		log: log,
+	}
+
+	// Should not panic
+	p.cleanupTempFiles()
+}
+
+func TestStartCleanupRoutine_SkipsMemoryMode(t *testing.T) {
+	log, _ := zap.NewDevelopment()
+	p := &Plugin{
+		cfg: &Config{
+			AttachmentStorage: AttachmentConfig{
+				Mode: "memory",
+			},
+		},
+		log: log,
+	}
+
+	// Should not start goroutine or panic
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	p.startCleanupRoutine(ctx)
+}
+
+func TestStartCleanupRoutine_StopsOnCancel(t *testing.T) {
+	tmpDir := t.TempDir()
+	log, _ := zap.NewDevelopment()
+
+	p := &Plugin{
+		cfg: &Config{
+			AttachmentStorage: AttachmentConfig{
+				Mode:         "tempfile",
+				TempDir:      tmpDir,
+				CleanupAfter: 10 * time.Millisecond,
+			},
+		},
+		log: log,
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	p.startCleanupRoutine(ctx)
+
+	// Let it tick at least once
+	time.Sleep(30 * time.Millisecond)
+
+	// Cancel should stop the goroutine
+	cancel()
+
+	// Give goroutine time to exit
+	time.Sleep(20 * time.Millisecond)
+
+	// Create a file after cancel - it should NOT be cleaned up
+	testFile := filepath.Join(tmpDir, "smtp-att-after-cancel.txt")
+	os.WriteFile(testFile, []byte("test"), 0644)
+	past := time.Now().Add(-1 * time.Hour)
+	os.Chtimes(testFile, past, past)
+
+	// Wait more than the ticker interval
+	time.Sleep(30 * time.Millisecond)
+
+	// File should still exist since cleanup routine was cancelled
+	if _, err := os.Stat(testFile); os.IsNotExist(err) {
+		t.Error("file should still exist after cleanup routine was cancelled")
+	}
+}

config.go

@@ -50,8 +50,6 @@ func (c *Config) InitDefaults() error {
 		c.Hostname = "localhost"
 	}
 
-	c.IncludeRaw = true
-
 	if c.ReadTimeout == 0 {
 		c.ReadTimeout = 60 * time.Second
 	}

config_test.go

@@ -0,0 +1,141 @@
+package smtp
+
+import (
+	"testing"
+	"time"
+)
+
+func TestInitDefaults_SetsAllDefaults(t *testing.T) {
+	cfg := &Config{}
+	cfg.Jobs.Pipeline = "test" // required field
+
+	if err := cfg.InitDefaults(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if cfg.Addr != "127.0.0.1:1025" {
+		t.Errorf("expected default addr 127.0.0.1:1025, got %s", cfg.Addr)
+	}
+	if cfg.Hostname != "localhost" {
+		t.Errorf("expected default hostname localhost, got %s", cfg.Hostname)
+	}
+	if cfg.ReadTimeout != 60*time.Second {
+		t.Errorf("expected default read_timeout 60s, got %v", cfg.ReadTimeout)
+	}
+	if cfg.WriteTimeout != 10*time.Second {
+		t.Errorf("expected default write_timeout 10s, got %v", cfg.WriteTimeout)
+	}
+	if cfg.MaxMessageSize != 10*1024*1024 {
+		t.Errorf("expected default max_message_size 10MB, got %d", cfg.MaxMessageSize)
+	}
+	if cfg.AttachmentStorage.Mode != "memory" {
+		t.Errorf("expected default attachment mode memory, got %s", cfg.AttachmentStorage.Mode)
+	}
+	if cfg.AttachmentStorage.TempDir != "/tmp/smtp-attachments" {
+		t.Errorf("expected default temp_dir, got %s", cfg.AttachmentStorage.TempDir)
+	}
+	if cfg.AttachmentStorage.CleanupAfter != time.Hour {
+		t.Errorf("expected default cleanup_after 1h, got %v", cfg.AttachmentStorage.CleanupAfter)
+	}
+	if cfg.Jobs.Priority != 10 {
+		t.Errorf("expected default priority 10, got %d", cfg.Jobs.Priority)
+	}
+}
+
+func TestInitDefaults_DoesNotOverrideIncludeRaw(t *testing.T) {
+	cfg := &Config{
+		IncludeRaw: false,
+	}
+	cfg.Jobs.Pipeline = "test"
+
+	if err := cfg.InitDefaults(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if cfg.IncludeRaw != false {
+		t.Error("IncludeRaw should remain false when explicitly set")
+	}
+}
+
+func TestInitDefaults_PreservesUserValues(t *testing.T) {
+	cfg := &Config{
+		Addr:           "0.0.0.0:2525",
+		Hostname:       "mail.example.com",
+		ReadTimeout:    30 * time.Second,
+		WriteTimeout:   5 * time.Second,
+		MaxMessageSize: 5 * 1024 * 1024,
+		Jobs:           JobsConfig{Pipeline: "emails", Priority: 5},
+	}
+
+	if err := cfg.InitDefaults(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if cfg.Addr != "0.0.0.0:2525" {
+		t.Errorf("addr was overwritten: %s", cfg.Addr)
+	}
+	if cfg.Hostname != "mail.example.com" {
+		t.Errorf("hostname was overwritten: %s", cfg.Hostname)
+	}
+	if cfg.ReadTimeout != 30*time.Second {
+		t.Errorf("read_timeout was overwritten: %v", cfg.ReadTimeout)
+	}
+	if cfg.Jobs.Priority != 5 {
+		t.Errorf("priority was overwritten: %d", cfg.Jobs.Priority)
+	}
+}
+
+func TestValidate_MissingPipeline(t *testing.T) {
+	cfg := &Config{
+		Addr:              "127.0.0.1:1025",
+		AttachmentStorage: AttachmentConfig{Mode: "memory"},
+		Jobs:              JobsConfig{Pipeline: ""},
+	}
+
+	err := cfg.validate()
+	if err == nil {
+		t.Error("expected validation error for empty pipeline")
+	}
+}
+
+func TestValidate_NegativeMaxMessageSize(t *testing.T) {
+	cfg := &Config{
+		Addr:              "127.0.0.1:1025",
+		MaxMessageSize:    -1,
+		AttachmentStorage: AttachmentConfig{Mode: "memory"},
+		Jobs:              JobsConfig{Pipeline: "test"},
+	}
+
+	err := cfg.validate()
+	if err == nil {
+		t.Error("expected validation error for negative max_message_size")
+	}
+}
+
+func TestValidate_InvalidAttachmentMode(t *testing.T) {
+	cfg := &Config{
+		Addr:              "127.0.0.1:1025",
+		AttachmentStorage: AttachmentConfig{Mode: "invalid"},
+		Jobs:              JobsConfig{Pipeline: "test"},
+	}
+
+	err := cfg.validate()
+	if err == nil {
+		t.Error("expected validation error for invalid attachment mode")
+	}
+}
+
+func TestValidate_ValidConfig(t *testing.T) {
+	for _, mode := range []string{"memory", "tempfile"} {
+		t.Run(mode, func(t *testing.T) {
+			cfg := &Config{
+				Addr:              "127.0.0.1:1025",
+				AttachmentStorage: AttachmentConfig{Mode: mode},
+				Jobs:              JobsConfig{Pipeline: "test"},
+			}
+			if err := cfg.validate(); err != nil {
+				t.Errorf("unexpected validation error for mode %s: %v", mode, err)
+			}
+		})
+	}
+}

jobs_integration.go

@@ -3,6 +3,7 @@ package smtp
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 
 	"github.com/google/uuid"
 	"github.com/roadrunner-server/api/v4/plugins/v4/jobs"
@@ -112,8 +113,11 @@ func (j *Job) UpdatePriority(p int64) {
 }
 
 // emailToJobMessage converts EmailData to a jobs.Message for the Jobs plugin
-func emailToJobMessage(email *EmailData, cfg *JobsConfig) jobs.Message {
-	payload, _ := json.Marshal(email)
+func emailToJobMessage(email *EmailData, cfg *JobsConfig) (jobs.Message, error) {
+	payload, err := json.Marshal(email)
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal email data: %w", err)
+	}
 
 	// Generate a unique job ID
 	jobID := uuid.NewString()
@@ -132,5 +136,5 @@ func emailToJobMessage(email *EmailData, cfg *JobsConfig) jobs.Message {
 			Delay:    cfg.Delay,
 			AutoAck:  cfg.AutoAck,
 		},
-	}
+	}, nil
 }