config: Tidy default handling and comparisons in tests

This patch tidies how defaults are handled in the config, using a new
logic to allow "overriding" one config (the default) with another (the
user supplied).

It also improves how the comparisons are done in the tests, using the
more convenient "github.com/google/go-cmp/cmp" package, which also
prints nice diffs on errors.

This is in preparation for a future path where the override mechanism
will be reused.

go.mod

@@ -8,6 +8,7 @@ require (
 	blitiri.com.ar/go/systemd v0.0.0-20171003041308-cdc4fd023aa4
 	github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815
 	github.com/golang/protobuf v1.4.0
+	github.com/google/go-cmp v0.4.0
 	golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59
 	golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e
 	golang.org/x/text v0.3.2

internal/config/config.go

@@ -12,24 +12,47 @@ import (
 	"blitiri.com.ar/go/log"
 
 	"google.golang.org/protobuf/encoding/prototext"
+	"google.golang.org/protobuf/proto"
 )
 
+var defaultConfig = &Config{
+	MaxDataSizeMb: 50,
+
+	SmtpAddress:              []string{"systemd"},
+	SubmissionAddress:        []string{"systemd"},
+	SubmissionOverTlsAddress: []string{"systemd"},
+
+	MailDeliveryAgentBin:  "maildrop",
+	MailDeliveryAgentArgs: []string{"-f", "%from%", "-d", "%to_user%"},
+
+	DataDir: "/var/lib/chasquid",
+
+	SuffixSeparators: "+",
+	DropCharacters:   ".",
+
+	MailLogPath: "<syslog>",
+}
+
 // Load the config from the given file.
 func Load(path string) (*Config, error) {
-	c := &Config{}
+	// Start with a copy of the default config.
+	c := proto.Clone(defaultConfig).(*Config)
 
+	// Load from the path.
 	buf, err := ioutil.ReadFile(path)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read config at %q: %v", path, err)
 	}
 
-	err = prototext.Unmarshal(buf, c)
+	fromFile := &Config{}
+	err = prototext.Unmarshal(buf, fromFile)
 	if err != nil {
 		return nil, fmt.Errorf("parsing config: %v", err)
 	}
+	override(c, fromFile)
 
-	// Fill in defaults for anything that's missing.
+	// Handle hostname separate, because if it is set, we don't need to call
-
+	// os.Hostname which can fail.
 	if c.Hostname == "" {
 		c.Hostname, err = os.Hostname()
 		if err != nil {
@@ -37,47 +60,63 @@ func Load(path string) (*Config, error) {
 		}
 	}
 
-	if c.MaxDataSizeMb == 0 {
-		c.MaxDataSizeMb = 50
-	}
-
-	if len(c.SmtpAddress) == 0 {
-		c.SmtpAddress = append(c.SmtpAddress, "systemd")
-	}
-	if len(c.SubmissionAddress) == 0 {
-		c.SubmissionAddress = append(c.SubmissionAddress, "systemd")
-	}
-	if len(c.SubmissionOverTlsAddress) == 0 {
-		c.SubmissionOverTlsAddress = append(c.SubmissionOverTlsAddress, "systemd")
-	}
-
-	if c.MailDeliveryAgentBin == "" {
-		c.MailDeliveryAgentBin = "maildrop"
-	}
-	if len(c.MailDeliveryAgentArgs) == 0 {
-		c.MailDeliveryAgentArgs = append(c.MailDeliveryAgentArgs,
-			"-f", "%from%", "-d", "%to_user%")
-	}
-
-	if c.DataDir == "" {
-		c.DataDir = "/var/lib/chasquid"
-	}
-
-	if c.SuffixSeparators == "" {
-		c.SuffixSeparators = "+"
-	}
-
-	if c.DropCharacters == "" {
-		c.DropCharacters = "."
-	}
-
-	if c.MailLogPath == "" {
-		c.MailLogPath = "<syslog>"
-	}
-
 	return c, nil
 }
 
+// Override fields in `c` that are set in `o`. We can't use proto.Merge
+// because the semantics would not be convenient for overriding.
+func override(c, o *Config) {
+	if o.Hostname != "" {
+		c.Hostname = o.Hostname
+	}
+	if o.MaxDataSizeMb > 0 {
+		c.MaxDataSizeMb = o.MaxDataSizeMb
+	}
+	if len(o.SmtpAddress) > 0 {
+		c.SmtpAddress = o.SmtpAddress
+	}
+	if len(o.SubmissionAddress) > 0 {
+		c.SubmissionAddress = o.SubmissionAddress
+	}
+	if len(o.SubmissionOverTlsAddress) > 0 {
+		c.SubmissionOverTlsAddress = o.SubmissionOverTlsAddress
+	}
+	if o.MonitoringAddress != "" {
+		c.MonitoringAddress = o.MonitoringAddress
+	}
+
+	if o.MailDeliveryAgentBin != "" {
+		c.MailDeliveryAgentBin = o.MailDeliveryAgentBin
+	}
+	if len(o.MailDeliveryAgentArgs) > 0 {
+		c.MailDeliveryAgentArgs = o.MailDeliveryAgentArgs
+	}
+
+	if o.DataDir != "" {
+		c.DataDir = o.DataDir
+	}
+
+	if o.SuffixSeparators != "" {
+		c.SuffixSeparators = o.SuffixSeparators
+	}
+	if o.DropCharacters != "" {
+		c.DropCharacters = o.DropCharacters
+	}
+	if o.MailLogPath != "" {
+		c.MailLogPath = o.MailLogPath
+	}
+
+	if o.DovecotAuth {
+		c.DovecotAuth = true
+	}
+	if o.DovecotUserdbPath != "" {
+		c.DovecotUserdbPath = o.DovecotUserdbPath
+	}
+	if o.DovecotClientPath != "" {
+		c.DovecotClientPath = o.DovecotClientPath
+	}
+}
+
 // LogConfig logs the given configuration, in a human-friendly way.
 func LogConfig(c *Config) {
 	log.Infof("Configuration:")

internal/config/config_test.go

@@ -8,6 +8,9 @@ import (
 
 	"blitiri.com.ar/go/chasquid/internal/testlib"
 	"blitiri.com.ar/go/log"
+	"github.com/google/go-cmp/cmp"
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/testing/protocmp"
 )
 
 func mustCreateConfig(t *testing.T, contents string) (string, string) {
@@ -30,26 +33,13 @@ func TestEmptyConfig(t *testing.T) {
 	}
 
 	// Test the default values are set.
-
+	defaults := proto.Clone(defaultConfig).(*Config)
 	hostname, _ := os.Hostname()
-	if c.Hostname == "" || c.Hostname != hostname {
+	defaults.Hostname = hostname
-		t.Errorf("invalid hostname %q, should be: %q", c.Hostname, hostname)
-	}
 
-	if c.MaxDataSizeMb != 50 {
+	diff := cmp.Diff(defaults, c, protocmp.Transform())
-		t.Errorf("max data size != 50: %d", c.MaxDataSizeMb)
+	if diff != "" {
-	}
+		t.Errorf("Load() mismatch (-want +got):\n%s", diff)
-
-	if len(c.SmtpAddress) != 1 || c.SmtpAddress[0] != "systemd" {
-		t.Errorf("unexpected address default: %v", c.SmtpAddress)
-	}
-
-	if len(c.SubmissionAddress) != 1 || c.SubmissionAddress[0] != "systemd" {
-		t.Errorf("unexpected address default: %v", c.SubmissionAddress)
-	}
-
-	if c.MonitoringAddress != "" {
-		t.Errorf("monitoring address is set: %v", c.MonitoringAddress)
 	}
 
 	testLogConfig(c)
@@ -60,6 +50,8 @@ func TestFullConfig(t *testing.T) {
 		hostname: "joust"
 		smtp_address: ":1234"
 		smtp_address: ":5678"
+		submission_address: ":10001"
+		submission_address: ":10002"
 		monitoring_address: ":1111"
 		max_data_size_mb: 26
 	`
@@ -67,26 +59,34 @@ func TestFullConfig(t *testing.T) {
 	tmpDir, path := mustCreateConfig(t, confStr)
 	defer testlib.RemoveIfOk(t, tmpDir)
 
+	expected := &Config{
+		Hostname:      "joust",
+		MaxDataSizeMb: 26,
+
+		SmtpAddress:              []string{":1234", ":5678"},
+		SubmissionAddress:        []string{":10001", ":10002"},
+		SubmissionOverTlsAddress: []string{"systemd"},
+		MonitoringAddress:        ":1111",
+
+		MailDeliveryAgentBin:  "maildrop",
+		MailDeliveryAgentArgs: []string{"-f", "%from%", "-d", "%to_user%"},
+
+		DataDir: "/var/lib/chasquid",
+
+		SuffixSeparators: "+",
+		DropCharacters:   ".",
+
+		MailLogPath: "<syslog>",
+	}
+
 	c, err := Load(path)
 	if err != nil {
 		t.Fatalf("error loading non-existent config: %v", err)
 	}
 
-	if c.Hostname != "joust" {
+	diff := cmp.Diff(expected, c, protocmp.Transform())
-		t.Errorf("hostname %q != 'joust'", c.Hostname)
+	if diff != "" {
-	}
+		t.Errorf("Load() mismatch (-want +got):\n%s", diff)
-
-	if c.MaxDataSizeMb != 26 {
-		t.Errorf("max data size != 26: %d", c.MaxDataSizeMb)
-	}
-
-	if len(c.SmtpAddress) != 2 ||
-		c.SmtpAddress[0] != ":1234" || c.SmtpAddress[1] != ":5678" {
-		t.Errorf("different address: %v", c.SmtpAddress)
-	}
-
-	if c.MonitoringAddress != ":1111" {
-		t.Errorf("monitoring address %q != ':1111;", c.MonitoringAddress)
 	}
 
 	testLogConfig(c)