Consistently use either pointer or value receivers

Subtle bugs can arise when mixing the two. A type is more coherent when
it behaves as only one of (1) a value or (2) a reference.

A new linter identifies this situation but does not yet account for
unmarshal methods on value types.

See: https://go.dev/wiki/CodeReviewComments#receiver-type
See: https://go.dev/wiki/MethodSets

.golangci.yaml

@@ -85,5 +85,10 @@ linters-settings:
     no-unaliased: true
 
 issues:
-  exclude-dirs:
-    - pkg/generated
+  exclude-generated: strict
+  exclude-rules:
+    # These value types have unmarshal methods.
+    # https://github.com/raeperd/recvcheck/issues/7
+    - linters: [recvcheck]
+      path: internal/pki/pki.go
+      text: 'methods of "(Certificate|PrivateKey)"'

internal/kubeapi/patch.go

@@ -18,12 +18,10 @@ var escapeJSONPointer = strings.NewReplacer(
 	"/", "~1",
 ).Replace
 
-// JSON6902 represents a JSON Patch according to RFC 6902; the same as
-// k8s.io/apimachinery/pkg/types.JSONPatchType.
-type JSON6902 []interface{}
+// JSON6902 represents a JSON Patch according to RFC 6902; the same as [types.JSONPatchType].
+type JSON6902 []any
 
-// NewJSONPatch creates a new JSON Patch according to RFC 6902; the same as
-// k8s.io/apimachinery/pkg/types.JSONPatchType.
+// NewJSONPatch creates a new JSON Patch according to RFC 6902; the same as [types.JSONPatchType].
 func NewJSONPatch() *JSON6902 { return &JSON6902{} }
 
 func (*JSON6902) pointer(tokens ...string) string {
@@ -50,10 +48,10 @@ func (*JSON6902) pointer(tokens ...string) string {
 // >
 // > o  If the target location specifies an object member that does exist,
 // >    that member's value is replaced.
-func (patch *JSON6902) Add(path ...string) func(value interface{}) *JSON6902 {
+func (patch *JSON6902) Add(path ...string) func(value any) *JSON6902 {
 	i := len(*patch)
-	f := func(value interface{}) *JSON6902 {
-		(*patch)[i] = map[string]interface{}{
+	f := func(value any) *JSON6902 {
+		(*patch)[i] = map[string]any{
 			"op":    "add",
 			"path":  patch.pointer(path...),
 			"value": value,
@@ -72,7 +70,7 @@ func (patch *JSON6902) Add(path ...string) func(value interface{}) *JSON6902 {
 // >
 // > The target location MUST exist for the operation to be successful.
 func (patch *JSON6902) Remove(path ...string) *JSON6902 {
-	*patch = append(*patch, map[string]interface{}{
+	*patch = append(*patch, map[string]any{
 		"op":   "remove",
 		"path": patch.pointer(path...),
 	})
@@ -86,10 +84,10 @@ func (patch *JSON6902) Remove(path ...string) *JSON6902 {
 // > with a new value.
 // >
 // > The target location MUST exist for the operation to be successful.
-func (patch *JSON6902) Replace(path ...string) func(value interface{}) *JSON6902 {
+func (patch *JSON6902) Replace(path ...string) func(value any) *JSON6902 {
 	i := len(*patch)
-	f := func(value interface{}) *JSON6902 {
-		(*patch)[i] = map[string]interface{}{
+	f := func(value any) *JSON6902 {
+		(*patch)[i] = map[string]any{
 			"op":    "replace",
 			"path":  patch.pointer(path...),
 			"value": value,
@@ -103,23 +101,21 @@ func (patch *JSON6902) Replace(path ...string) func(value interface{}) *JSON6902
 }
 
 // Bytes returns the JSON representation of patch.
-func (patch JSON6902) Bytes() ([]byte, error) { return patch.Data(nil) }
+func (patch *JSON6902) Bytes() ([]byte, error) { return patch.Data(nil) }
 
 // Data returns the JSON representation of patch.
-func (patch JSON6902) Data(client.Object) ([]byte, error) { return json.Marshal(patch) }
+func (patch *JSON6902) Data(client.Object) ([]byte, error) { return json.Marshal(*patch) }
 
 // IsEmpty returns true when patch has no operations.
-func (patch JSON6902) IsEmpty() bool { return len(patch) == 0 }
+func (patch *JSON6902) IsEmpty() bool { return len(*patch) == 0 }
 
-// Type returns k8s.io/apimachinery/pkg/types.JSONPatchType.
-func (patch JSON6902) Type() types.PatchType { return types.JSONPatchType }
+// Type returns [types.JSONPatchType].
+func (patch *JSON6902) Type() types.PatchType { return types.JSONPatchType }
 
-// Merge7386 represents a JSON Merge Patch according to RFC 7386; the same as
-// k8s.io/apimachinery/pkg/types.MergePatchType.
-type Merge7386 map[string]interface{}
+// Merge7386 represents a JSON Merge Patch according to RFC 7386; the same as [types.MergePatchType].
+type Merge7386 map[string]any
 
-// NewMergePatch creates a new JSON Merge Patch according to RFC 7386; the same
-// as k8s.io/apimachinery/pkg/types.MergePatchType.
+// NewMergePatch creates a new JSON Merge Patch according to RFC 7386; the same as [types.MergePatchType].
 func NewMergePatch() *Merge7386 { return &Merge7386{} }
 
 // Add modifies patch to indicate that the member at path should be added or
@@ -130,7 +126,7 @@ func NewMergePatch() *Merge7386 { return &Merge7386{} }
 // > contain the member, the value is replaced.  Null values in the merge
 // > patch are given special meaning to indicate the removal of existing
 // > values in the target.
-func (patch *Merge7386) Add(path ...string) func(value interface{}) *Merge7386 {
+func (patch *Merge7386) Add(path ...string) func(value any) *Merge7386 {
 	position := *patch
 
 	for len(path) > 1 {
@@ -145,10 +141,10 @@ func (patch *Merge7386) Add(path ...string) func(value interface{}) *Merge7386 {
 	}
 
 	if len(path) < 1 {
-		return func(interface{}) *Merge7386 { return patch }
+		return func(any) *Merge7386 { return patch }
 	}
 
-	f := func(value interface{}) *Merge7386 {
+	f := func(value any) *Merge7386 {
 		position[path[0]] = value
 		return patch
 	}
@@ -165,13 +161,13 @@ func (patch *Merge7386) Remove(path ...string) *Merge7386 {
 }
 
 // Bytes returns the JSON representation of patch.
-func (patch Merge7386) Bytes() ([]byte, error) { return patch.Data(nil) }
+func (patch *Merge7386) Bytes() ([]byte, error) { return patch.Data(nil) }
 
 // Data returns the JSON representation of patch.
-func (patch Merge7386) Data(client.Object) ([]byte, error) { return json.Marshal(patch) }
+func (patch *Merge7386) Data(client.Object) ([]byte, error) { return json.Marshal(*patch) }
 
 // IsEmpty returns true when patch has no modifications.
-func (patch Merge7386) IsEmpty() bool { return len(patch) == 0 }
+func (patch *Merge7386) IsEmpty() bool { return len(*patch) == 0 }
 
-// Type returns k8s.io/apimachinery/pkg/types.MergePatchType.
-func (patch Merge7386) Type() types.PatchType { return types.MergePatchType }
+// Type returns [types.MergePatchType].
+func (patch *Merge7386) Type() types.PatchType { return types.MergePatchType }

internal/logging/logr.go

@@ -51,20 +51,20 @@ type sink struct {
 	depth     int
 	verbosity int
 	names     []string
-	values    []interface{}
+	values    []any
 
 	// TODO(cbandy): add names or frame to the functions below.
 
-	fnError func(error, string, ...interface{})
-	fnInfo  func(int, string, ...interface{})
+	fnError func(error, string, ...any)
+	fnInfo  func(int, string, ...any)
 }
 
 var _ logr.LogSink = (*sink)(nil)
 
 func (s *sink) Enabled(level int) bool     { return level <= s.verbosity }
 func (s *sink) Init(info logr.RuntimeInfo) { s.depth = info.CallDepth }
 
-func (s sink) combineValues(kv ...interface{}) []interface{} {
+func (s *sink) combineValues(kv ...any) []any {
 	if len(kv) == 0 {
 		return s.values
 	}
@@ -74,11 +74,11 @@ func (s sink) combineValues(kv ...interface{}) []interface{} {
 	return kv
 }
 
-func (s *sink) Error(err error, msg string, kv ...interface{}) {
+func (s *sink) Error(err error, msg string, kv ...any) {
 	s.fnError(err, msg, s.combineValues(kv...)...)
 }
 
-func (s *sink) Info(level int, msg string, kv ...interface{}) {
+func (s *sink) Info(level int, msg string, kv ...any) {
 	s.fnInfo(level, msg, s.combineValues(kv...)...)
 }
 
@@ -89,7 +89,7 @@ func (s *sink) WithName(name string) logr.LogSink {
 	return &out
 }
 
-func (s *sink) WithValues(kv ...interface{}) logr.LogSink {
+func (s *sink) WithValues(kv ...any) logr.LogSink {
 	n := len(s.values)
 	out := *s
 	out.values = append(out.values[:n:n], kv...)

internal/patroni/config_test.go

@@ -462,8 +462,8 @@ func TestDynamicConfiguration(t *testing.T) {
 				},
 			},
 			hbas: postgres.HBAs{
-				Default: []postgres.HostBasedAuthentication{
-					*postgres.NewHBA().Local().Method("peer"),
+				Default: []*postgres.HostBasedAuthentication{
+					postgres.NewHBA().Local().Method("peer"),
 				},
 			},
 			expected: map[string]any{
@@ -487,8 +487,8 @@ func TestDynamicConfiguration(t *testing.T) {
 				},
 			},
 			hbas: postgres.HBAs{
-				Default: []postgres.HostBasedAuthentication{
-					*postgres.NewHBA().Local().Method("peer"),
+				Default: []*postgres.HostBasedAuthentication{
+					postgres.NewHBA().Local().Method("peer"),
 				},
 			},
 			expected: map[string]any{
@@ -512,8 +512,8 @@ func TestDynamicConfiguration(t *testing.T) {
 				},
 			},
 			hbas: postgres.HBAs{
-				Mandatory: []postgres.HostBasedAuthentication{
-					*postgres.NewHBA().Local().Method("peer"),
+				Mandatory: []*postgres.HostBasedAuthentication{
+					postgres.NewHBA().Local().Method("peer"),
 				},
 			},
 			expected: map[string]any{
@@ -538,8 +538,8 @@ func TestDynamicConfiguration(t *testing.T) {
 				},
 			},
 			hbas: postgres.HBAs{
-				Mandatory: []postgres.HostBasedAuthentication{
-					*postgres.NewHBA().Local().Method("peer"),
+				Mandatory: []*postgres.HostBasedAuthentication{
+					postgres.NewHBA().Local().Method("peer"),
 				},
 			},
 			expected: map[string]any{

internal/pgadmin/config.go

@@ -160,7 +160,7 @@ if os.path.isfile('` + ldapPasswordAbsolutePath + `'):
 
 // systemSettings returns pgAdmin settings as a value that can be marshaled to JSON.
 func systemSettings(spec *v1beta1.PGAdminPodSpec) map[string]interface{} {
-	settings := *spec.Config.Settings.DeepCopy()
+	settings := spec.Config.Settings.DeepCopy()
 	if settings == nil {
 		settings = make(map[string]interface{})
 	}

internal/pgbouncer/postgres.go

@@ -210,14 +210,14 @@ func generatePassword() (plaintext, verifier string, err error) {
 	return
 }
 
-func postgresqlHBAs() []postgres.HostBasedAuthentication {
+func postgresqlHBAs() []*postgres.HostBasedAuthentication {
 	// PgBouncer must connect over TLS using a SCRAM password. Other network
 	// connections are forbidden.
 	// - https://www.postgresql.org/docs/current/auth-pg-hba-conf.html
 	// - https://www.postgresql.org/docs/current/auth-password.html
 
-	return []postgres.HostBasedAuthentication{
-		*postgres.NewHBA().User(postgresqlUser).TLS().Method("scram-sha-256"),
-		*postgres.NewHBA().User(postgresqlUser).TCP().Method("reject"),
+	return []*postgres.HostBasedAuthentication{
+		postgres.NewHBA().User(postgresqlUser).TLS().Method("scram-sha-256"),
+		postgres.NewHBA().User(postgresqlUser).TCP().Method("reject"),
 	}
 }

internal/pgbouncer/reconcile_test.go

@@ -491,6 +491,6 @@ func TestPostgreSQL(t *testing.T) {
 				Mandatory: postgresqlHBAs(),
 			},
 			// postgres.HostBasedAuthentication has unexported fields. Call String() to compare.
-			gocmp.Transformer("", postgres.HostBasedAuthentication.String))
+			gocmp.Transformer("", (*postgres.HostBasedAuthentication).String))
 	})
 }

internal/pgmonitor/postgres.go

@@ -26,9 +26,9 @@ func PostgreSQLHBAs(inCluster *v1beta1.PostgresCluster, outHBAs *postgres.HBAs)
 	if ExporterEnabled(inCluster) {
 		// Limit the monitoring user to local connections using SCRAM.
 		outHBAs.Mandatory = append(outHBAs.Mandatory,
-			*postgres.NewHBA().TCP().User(MonitoringUser).Method("scram-sha-256").Network("127.0.0.0/8"),
-			*postgres.NewHBA().TCP().User(MonitoringUser).Method("scram-sha-256").Network("::1/128"),
-			*postgres.NewHBA().TCP().User(MonitoringUser).Method("reject"))
+			postgres.NewHBA().TCP().User(MonitoringUser).Method("scram-sha-256").Network("127.0.0.0/8"),
+			postgres.NewHBA().TCP().User(MonitoringUser).Method("scram-sha-256").Network("::1/128"),
+			postgres.NewHBA().TCP().User(MonitoringUser).Method("reject"))
 	}
 }
 

internal/postgres/hba.go

@@ -12,31 +12,31 @@ import (
 // NewHBAs returns HostBasedAuthentication records required by this package.
 func NewHBAs() HBAs {
 	return HBAs{
-		Mandatory: []HostBasedAuthentication{
+		Mandatory: []*HostBasedAuthentication{
 			// The "postgres" superuser must always be able to connect locally.
-			*NewHBA().Local().User("postgres").Method("peer"),
+			NewHBA().Local().User("postgres").Method("peer"),
 
 			// The replication user must always connect over TLS using certificate
 			// authentication. Patroni also connects to the "postgres" database
 			// when calling `pg_rewind`.
 			// - https://www.postgresql.org/docs/current/warm-standby.html#STREAMING-REPLICATION-AUTHENTICATION
-			*NewHBA().TLS().User(ReplicationUser).Method("cert").Replication(),
-			*NewHBA().TLS().User(ReplicationUser).Method("cert").Database("postgres"),
-			*NewHBA().TCP().User(ReplicationUser).Method("reject"),
+			NewHBA().TLS().User(ReplicationUser).Method("cert").Replication(),
+			NewHBA().TLS().User(ReplicationUser).Method("cert").Database("postgres"),
+			NewHBA().TCP().User(ReplicationUser).Method("reject"),
 		},
 
-		Default: []HostBasedAuthentication{
+		Default: []*HostBasedAuthentication{
 			// Allow TLS connections to any database using passwords. The "md5"
 			// authentication method automatically verifies passwords encrypted
 			// using either MD5 or SCRAM-SHA-256.
 			// - https://www.postgresql.org/docs/current/auth-password.html
-			*NewHBA().TLS().Method("md5"),
+			NewHBA().TLS().Method("md5"),
 		},
 	}
 }
 
 // HBAs is a pairing of HostBasedAuthentication records.
-type HBAs struct{ Mandatory, Default []HostBasedAuthentication }
+type HBAs struct{ Mandatory, Default []*HostBasedAuthentication }
 
 // HostBasedAuthentication represents a single record for pg_hba.conf.
 // - https://www.postgresql.org/docs/current/auth-pg-hba-conf.html
@@ -49,7 +49,7 @@ func NewHBA() *HostBasedAuthentication {
 	return new(HostBasedAuthentication).AllDatabases().AllNetworks().AllUsers()
 }
 
-func (HostBasedAuthentication) quote(value string) string {
+func (*HostBasedAuthentication) quote(value string) string {
 	return `"` + strings.ReplaceAll(value, `"`, `""`) + `"`
 }
 
@@ -148,7 +148,7 @@ func (hba *HostBasedAuthentication) User(name string) *HostBasedAuthentication {
 }
 
 // String returns hba formatted for the pg_hba.conf file without a newline.
-func (hba HostBasedAuthentication) String() string {
+func (hba *HostBasedAuthentication) String() string {
 	if hba.origin == "local" {
 		return strings.TrimSpace(fmt.Sprintf("local %s %s %s %s",
 			hba.database, hba.user, hba.method, hba.options))

internal/postgres/hba_test.go

@@ -14,7 +14,7 @@ import (
 )
 
 func TestNewHBAs(t *testing.T) {
-	matches := func(actual []HostBasedAuthentication, expected string) cmp.Comparison {
+	matches := func(actual []*HostBasedAuthentication, expected string) cmp.Comparison {
 		printed := make([]string, len(actual))
 		for i := range actual {
 			printed[i] = actual[i].String()

internal/postgres/parameters.go

@@ -65,7 +65,7 @@ func NewParameterSet() *ParameterSet {
 }
 
 // AsMap returns a copy of ps as a map.
-func (ps ParameterSet) AsMap() map[string]string {
+func (ps *ParameterSet) AsMap() map[string]string {
 	out := make(map[string]string, len(ps.values))
 	for name, value := range ps.values {
 		out[name] = value
@@ -102,25 +102,25 @@ func (ps *ParameterSet) AppendToList(name string, value ...string) {
 }
 
 // Get returns the value of parameter name and whether or not it was present in ps.
-func (ps ParameterSet) Get(name string) (string, bool) {
+func (ps *ParameterSet) Get(name string) (string, bool) {
 	value, ok := ps.values[ps.normalize(name)]
 	return value, ok
 }
 
 // Has returns whether or not parameter name is present in ps.
-func (ps ParameterSet) Has(name string) bool {
+func (ps *ParameterSet) Has(name string) bool {
 	_, ok := ps.Get(name)
 	return ok
 }
 
-func (ParameterSet) normalize(name string) string {
+func (*ParameterSet) normalize(name string) string {
 	// All parameter names are case-insensitive.
 	// -- https://www.postgresql.org/docs/current/config-setting.html
 	return strings.ToLower(name)
 }
 
 // Value returns empty string or the value of parameter name if it is present in ps.
-func (ps ParameterSet) Value(name string) string {
+func (ps *ParameterSet) Value(name string) string {
 	value, _ := ps.Get(name)
 	return value
 }