Skip to content

Commit 96132b8

Browse files
cbandycbandy
committed
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
1 parent 82097e7 commit 96132b8

File tree

14 files changed

+89
-97
lines changed

14 files changed

+89
-97
lines changed

‎.golangci.yaml‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,5 +85,10 @@ linters-settings:
8585
no-unaliased: true
8686

8787
issues:
88-
exclude-dirs:
89-
- pkg/generated
88+
exclude-generated: strict
89+
exclude-rules:
90+
# These value types have unmarshal methods.
91+
# https://github.com/raeperd/recvcheck/issues/7
92+
- linters: [recvcheck]
93+
path: internal/pki/pki.go
94+
text: 'methods of "(Certificate|PrivateKey)"'

‎internal/kubeapi/patch.go‎

Lines changed: 26 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,10 @@ var escapeJSONPointer = strings.NewReplacer(
1818
"/", "~1",
1919
).Replace
2020

21-
// JSON6902 represents a JSON Patch according to RFC 6902; the same as
22-
// k8s.io/apimachinery/pkg/types.JSONPatchType.
23-
type JSON6902 []interface{}
21+
// JSON6902 represents a JSON Patch according to RFC 6902; the same as [types.JSONPatchType].
22+
type JSON6902 []any
2423

25-
// NewJSONPatch creates a new JSON Patch according to RFC 6902; the same as
26-
// k8s.io/apimachinery/pkg/types.JSONPatchType.
24+
// NewJSONPatch creates a new JSON Patch according to RFC 6902; the same as [types.JSONPatchType].
2725
func NewJSONPatch() *JSON6902 { return &JSON6902{} }
2826

2927
func (*JSON6902) pointer(tokens ...string) string {
@@ -50,10 +48,10 @@ func (*JSON6902) pointer(tokens ...string) string {
5048
// >
5149
// > o If the target location specifies an object member that does exist,
5250
// > that member's value is replaced.
53-
func (patch *JSON6902) Add(path ...string) func(value interface{}) *JSON6902 {
51+
func (patch *JSON6902) Add(path ...string) func(value any) *JSON6902 {
5452
i := len(*patch)
55-
f := func(value interface{}) *JSON6902 {
56-
(*patch)[i] = map[string]interface{}{
53+
f := func(value any) *JSON6902 {
54+
(*patch)[i] = map[string]any{
5755
"op": "add",
5856
"path": patch.pointer(path...),
5957
"value": value,
@@ -72,7 +70,7 @@ func (patch *JSON6902) Add(path ...string) func(value interface{}) *JSON6902 {
7270
// >
7371
// > The target location MUST exist for the operation to be successful.
7472
func (patch *JSON6902) Remove(path ...string) *JSON6902 {
75-
*patch = append(*patch, map[string]interface{}{
73+
*patch = append(*patch, map[string]any{
7674
"op": "remove",
7775
"path": patch.pointer(path...),
7876
})
@@ -86,10 +84,10 @@ func (patch *JSON6902) Remove(path ...string) *JSON6902 {
8684
// > with a new value.
8785
// >
8886
// > The target location MUST exist for the operation to be successful.
89-
func (patch *JSON6902) Replace(path ...string) func(value interface{}) *JSON6902 {
87+
func (patch *JSON6902) Replace(path ...string) func(value any) *JSON6902 {
9088
i := len(*patch)
91-
f := func(value interface{}) *JSON6902 {
92-
(*patch)[i] = map[string]interface{}{
89+
f := func(value any) *JSON6902 {
90+
(*patch)[i] = map[string]any{
9391
"op": "replace",
9492
"path": patch.pointer(path...),
9593
"value": value,
@@ -103,23 +101,21 @@ func (patch *JSON6902) Replace(path ...string) func(value interface{}) *JSON6902
103101
}
104102

105103
// Bytes returns the JSON representation of patch.
106-
func (patch JSON6902) Bytes() ([]byte, error) { return patch.Data(nil) }
104+
func (patch *JSON6902) Bytes() ([]byte, error) { return patch.Data(nil) }
107105

108106
// Data returns the JSON representation of patch.
109-
func (patch JSON6902) Data(client.Object) ([]byte, error) { return json.Marshal(patch) }
107+
func (patch *JSON6902) Data(client.Object) ([]byte, error) { return json.Marshal(*patch) }
110108

111109
// IsEmpty returns true when patch has no operations.
112-
func (patch JSON6902) IsEmpty() bool { return len(patch) == 0 }
110+
func (patch *JSON6902) IsEmpty() bool { return len(*patch) == 0 }
113111

114-
// Type returns k8s.io/apimachinery/pkg/types.JSONPatchType.
115-
func (patch JSON6902) Type() types.PatchType { return types.JSONPatchType }
112+
// Type returns [types.JSONPatchType].
113+
func (patch *JSON6902) Type() types.PatchType { return types.JSONPatchType }
116114

117-
// Merge7386 represents a JSON Merge Patch according to RFC 7386; the same as
118-
// k8s.io/apimachinery/pkg/types.MergePatchType.
119-
type Merge7386 map[string]interface{}
115+
// Merge7386 represents a JSON Merge Patch according to RFC 7386; the same as [types.MergePatchType].
116+
type Merge7386 map[string]any
120117

121-
// NewMergePatch creates a new JSON Merge Patch according to RFC 7386; the same
122-
// as k8s.io/apimachinery/pkg/types.MergePatchType.
118+
// NewMergePatch creates a new JSON Merge Patch according to RFC 7386; the same as [types.MergePatchType].
123119
func NewMergePatch() *Merge7386 { return &Merge7386{} }
124120

125121
// Add modifies patch to indicate that the member at path should be added or
@@ -130,7 +126,7 @@ func NewMergePatch() *Merge7386 { return &Merge7386{} }
130126
// > contain the member, the value is replaced. Null values in the merge
131127
// > patch are given special meaning to indicate the removal of existing
132128
// > values in the target.
133-
func (patch *Merge7386) Add(path ...string) func(value interface{}) *Merge7386 {
129+
func (patch *Merge7386) Add(path ...string) func(value any) *Merge7386 {
134130
position := *patch
135131

136132
for len(path) > 1 {
@@ -145,10 +141,10 @@ func (patch *Merge7386) Add(path ...string) func(value interface{}) *Merge7386 {
145141
}
146142

147143
if len(path) < 1 {
148-
return func(interface{}) *Merge7386 { return patch }
144+
return func(any) *Merge7386 { return patch }
149145
}
150146

151-
f := func(value interface{}) *Merge7386 {
147+
f := func(value any) *Merge7386 {
152148
position[path[0]] = value
153149
return patch
154150
}
@@ -165,13 +161,13 @@ func (patch *Merge7386) Remove(path ...string) *Merge7386 {
165161
}
166162

167163
// Bytes returns the JSON representation of patch.
168-
func (patch Merge7386) Bytes() ([]byte, error) { return patch.Data(nil) }
164+
func (patch *Merge7386) Bytes() ([]byte, error) { return patch.Data(nil) }
169165

170166
// Data returns the JSON representation of patch.
171-
func (patch Merge7386) Data(client.Object) ([]byte, error) { return json.Marshal(patch) }
167+
func (patch *Merge7386) Data(client.Object) ([]byte, error) { return json.Marshal(*patch) }
172168

173169
// IsEmpty returns true when patch has no modifications.
174-
func (patch Merge7386) IsEmpty() bool { return len(patch) == 0 }
170+
func (patch *Merge7386) IsEmpty() bool { return len(*patch) == 0 }
175171

176-
// Type returns k8s.io/apimachinery/pkg/types.MergePatchType.
177-
func (patch Merge7386) Type() types.PatchType { return types.MergePatchType }
172+
// Type returns [types.MergePatchType].
173+
func (patch *Merge7386) Type() types.PatchType { return types.MergePatchType }

‎internal/logging/logr.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,20 +51,20 @@ type sink struct {
5151
depth int
5252
verbosity int
5353
names []string
54-
values []interface{}
54+
values []any
5555

5656
// TODO(cbandy): add names or frame to the functions below.
5757

58-
fnError func(error, string, ...interface{})
59-
fnInfo func(int, string, ...interface{})
58+
fnError func(error, string, ...any)
59+
fnInfo func(int, string, ...any)
6060
}
6161

6262
var _ logr.LogSink = (*sink)(nil)
6363

6464
func (s *sink) Enabled(level int) bool { return level <= s.verbosity }
6565
func (s *sink) Init(info logr.RuntimeInfo) { s.depth = info.CallDepth }
6666

67-
func (s sink) combineValues(kv ...interface{}) []interface{} {
67+
func (s *sink) combineValues(kv ...any) []any {
6868
if len(kv) == 0 {
6969
return s.values
7070
}
@@ -74,11 +74,11 @@ func (s sink) combineValues(kv ...interface{}) []interface{} {
7474
return kv
7575
}
7676

77-
func (s *sink) Error(err error, msg string, kv ...interface{}) {
77+
func (s *sink) Error(err error, msg string, kv ...any) {
7878
s.fnError(err, msg, s.combineValues(kv...)...)
7979
}
8080

81-
func (s *sink) Info(level int, msg string, kv ...interface{}) {
81+
func (s *sink) Info(level int, msg string, kv ...any) {
8282
s.fnInfo(level, msg, s.combineValues(kv...)...)
8383
}
8484

@@ -89,7 +89,7 @@ func (s *sink) WithName(name string) logr.LogSink {
8989
return &out
9090
}
9191

92-
func (s *sink) WithValues(kv ...interface{}) logr.LogSink {
92+
func (s *sink) WithValues(kv ...any) logr.LogSink {
9393
n := len(s.values)
9494
out := *s
9595
out.values = append(out.values[:n:n], kv...)

‎internal/patroni/config_test.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -462,8 +462,8 @@ func TestDynamicConfiguration(t *testing.T) {
462462
},
463463
},
464464
hbas: postgres.HBAs{
465-
Default: []postgres.HostBasedAuthentication{
466-
*postgres.NewHBA().Local().Method("peer"),
465+
Default: []*postgres.HostBasedAuthentication{
466+
postgres.NewHBA().Local().Method("peer"),
467467
},
468468
},
469469
expected: map[string]any{
@@ -487,8 +487,8 @@ func TestDynamicConfiguration(t *testing.T) {
487487
},
488488
},
489489
hbas: postgres.HBAs{
490-
Default: []postgres.HostBasedAuthentication{
491-
*postgres.NewHBA().Local().Method("peer"),
490+
Default: []*postgres.HostBasedAuthentication{
491+
postgres.NewHBA().Local().Method("peer"),
492492
},
493493
},
494494
expected: map[string]any{
@@ -512,8 +512,8 @@ func TestDynamicConfiguration(t *testing.T) {
512512
},
513513
},
514514
hbas: postgres.HBAs{
515-
Mandatory: []postgres.HostBasedAuthentication{
516-
*postgres.NewHBA().Local().Method("peer"),
515+
Mandatory: []*postgres.HostBasedAuthentication{
516+
postgres.NewHBA().Local().Method("peer"),
517517
},
518518
},
519519
expected: map[string]any{
@@ -538,8 +538,8 @@ func TestDynamicConfiguration(t *testing.T) {
538538
},
539539
},
540540
hbas: postgres.HBAs{
541-
Mandatory: []postgres.HostBasedAuthentication{
542-
*postgres.NewHBA().Local().Method("peer"),
541+
Mandatory: []*postgres.HostBasedAuthentication{
542+
postgres.NewHBA().Local().Method("peer"),
543543
},
544544
},
545545
expected: map[string]any{

‎internal/pgadmin/config.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ if os.path.isfile('` + ldapPasswordAbsolutePath + `'):
160160

161161
// systemSettings returns pgAdmin settings as a value that can be marshaled to JSON.
162162
func systemSettings(spec *v1beta1.PGAdminPodSpec) map[string]interface{} {
163-
settings := *spec.Config.Settings.DeepCopy()
163+
settings := spec.Config.Settings.DeepCopy()
164164
if settings == nil {
165165
settings = make(map[string]interface{})
166166
}

‎internal/pgbouncer/postgres.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -210,14 +210,14 @@ func generatePassword() (plaintext, verifier string, err error) {
210210
return
211211
}
212212

213-
func postgresqlHBAs() []postgres.HostBasedAuthentication {
213+
func postgresqlHBAs() []*postgres.HostBasedAuthentication {
214214
// PgBouncer must connect over TLS using a SCRAM password. Other network
215215
// connections are forbidden.
216216
// - https://www.postgresql.org/docs/current/auth-pg-hba-conf.html
217217
// - https://www.postgresql.org/docs/current/auth-password.html
218218

219-
return []postgres.HostBasedAuthentication{
220-
*postgres.NewHBA().User(postgresqlUser).TLS().Method("scram-sha-256"),
221-
*postgres.NewHBA().User(postgresqlUser).TCP().Method("reject"),
219+
return []*postgres.HostBasedAuthentication{
220+
postgres.NewHBA().User(postgresqlUser).TLS().Method("scram-sha-256"),
221+
postgres.NewHBA().User(postgresqlUser).TCP().Method("reject"),
222222
}
223223
}

‎internal/pgbouncer/reconcile_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,6 @@ func TestPostgreSQL(t *testing.T) {
491491
Mandatory: postgresqlHBAs(),
492492
},
493493
// postgres.HostBasedAuthentication has unexported fields. Call String() to compare.
494-
gocmp.Transformer("", postgres.HostBasedAuthentication.String))
494+
gocmp.Transformer("", (*postgres.HostBasedAuthentication).String))
495495
})
496496
}

‎internal/pgmonitor/postgres.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,9 @@ func PostgreSQLHBAs(inCluster *v1beta1.PostgresCluster, outHBAs *postgres.HBAs)
2626
if ExporterEnabled(inCluster) {
2727
// Limit the monitoring user to local connections using SCRAM.
2828
outHBAs.Mandatory = append(outHBAs.Mandatory,
29-
*postgres.NewHBA().TCP().User(MonitoringUser).Method("scram-sha-256").Network("127.0.0.0/8"),
30-
*postgres.NewHBA().TCP().User(MonitoringUser).Method("scram-sha-256").Network("::1/128"),
31-
*postgres.NewHBA().TCP().User(MonitoringUser).Method("reject"))
29+
postgres.NewHBA().TCP().User(MonitoringUser).Method("scram-sha-256").Network("127.0.0.0/8"),
30+
postgres.NewHBA().TCP().User(MonitoringUser).Method("scram-sha-256").Network("::1/128"),
31+
postgres.NewHBA().TCP().User(MonitoringUser).Method("reject"))
3232
}
3333
}
3434

‎internal/postgres/hba.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,31 +12,31 @@ import (
1212
// NewHBAs returns HostBasedAuthentication records required by this package.
1313
func NewHBAs() HBAs {
1414
return HBAs{
15-
Mandatory: []HostBasedAuthentication{
15+
Mandatory: []*HostBasedAuthentication{
1616
// The "postgres" superuser must always be able to connect locally.
17-
*NewHBA().Local().User("postgres").Method("peer"),
17+
NewHBA().Local().User("postgres").Method("peer"),
1818

1919
// The replication user must always connect over TLS using certificate
2020
// authentication. Patroni also connects to the "postgres" database
2121
// when calling `pg_rewind`.
2222
// - https://www.postgresql.org/docs/current/warm-standby.html#STREAMING-REPLICATION-AUTHENTICATION
23-
*NewHBA().TLS().User(ReplicationUser).Method("cert").Replication(),
24-
*NewHBA().TLS().User(ReplicationUser).Method("cert").Database("postgres"),
25-
*NewHBA().TCP().User(ReplicationUser).Method("reject"),
23+
NewHBA().TLS().User(ReplicationUser).Method("cert").Replication(),
24+
NewHBA().TLS().User(ReplicationUser).Method("cert").Database("postgres"),
25+
NewHBA().TCP().User(ReplicationUser).Method("reject"),
2626
},
2727

28-
Default: []HostBasedAuthentication{
28+
Default: []*HostBasedAuthentication{
2929
// Allow TLS connections to any database using passwords. The "md5"
3030
// authentication method automatically verifies passwords encrypted
3131
// using either MD5 or SCRAM-SHA-256.
3232
// - https://www.postgresql.org/docs/current/auth-password.html
33-
*NewHBA().TLS().Method("md5"),
33+
NewHBA().TLS().Method("md5"),
3434
},
3535
}
3636
}
3737

3838
// HBAs is a pairing of HostBasedAuthentication records.
39-
type HBAs struct{ Mandatory, Default []HostBasedAuthentication }
39+
type HBAs struct{ Mandatory, Default []*HostBasedAuthentication }
4040

4141
// HostBasedAuthentication represents a single record for pg_hba.conf.
4242
// - https://www.postgresql.org/docs/current/auth-pg-hba-conf.html
@@ -49,7 +49,7 @@ func NewHBA() *HostBasedAuthentication {
4949
return new(HostBasedAuthentication).AllDatabases().AllNetworks().AllUsers()
5050
}
5151

52-
func (HostBasedAuthentication) quote(value string) string {
52+
func (*HostBasedAuthentication) quote(value string) string {
5353
return `"` + strings.ReplaceAll(value, `"`, `""`) + `"`
5454
}
5555

@@ -148,7 +148,7 @@ func (hba *HostBasedAuthentication) User(name string) *HostBasedAuthentication {
148148
}
149149

150150
// String returns hba formatted for the pg_hba.conf file without a newline.
151-
func (hba HostBasedAuthentication) String() string {
151+
func (hba *HostBasedAuthentication) String() string {
152152
if hba.origin == "local" {
153153
return strings.TrimSpace(fmt.Sprintf("local %s %s %s %s",
154154
hba.database, hba.user, hba.method, hba.options))

‎internal/postgres/hba_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import (
1414
)
1515

1616
func TestNewHBAs(t *testing.T) {
17-
matches := func(actual []HostBasedAuthentication, expected string) cmp.Comparison {
17+
matches := func(actual []*HostBasedAuthentication, expected string) cmp.Comparison {
1818
printed := make([]string, len(actual))
1919
for i := range actual {
2020
printed[i] = actual[i].String()

0 commit comments

Comments
 (0)