Skip to content

Commit aa07291

Browse files
committed
lint query params
1 parent ad918f5 commit aa07291

8 files changed

Lines changed: 194 additions & 66 deletions

File tree

.custom-gcl.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@ plugins:
66
path: ./tools/fmtpercentv
77
- module: "github.com/google/go-github/v88/tools/redundantptr"
88
path: ./tools/redundantptr
9-
- module: "github.com/google/go-github/v88/tools/requestbody"
10-
path: ./tools/requestbody
9+
- module: "github.com/google/go-github/v88/tools/paramcheck"
10+
path: ./tools/paramcheck
1111
- module: "github.com/google/go-github/v88/tools/sliceofpointers"
1212
path: ./tools/sliceofpointers
1313
- module: "github.com/google/go-github/v88/tools/structfield"

.golangci.yml

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ linters:
2828
- paralleltest
2929
- perfsprint
3030
- redundantptr
31-
- requestbody
31+
- paramcheck
3232
- revive
3333
- sliceofpointers
3434
- staticcheck
@@ -195,18 +195,14 @@ linters:
195195
type: module
196196
description: Reports usage of %d or %s in format strings.
197197
original-url: github.com/google/go-github/v88/tools/fmtpercentv
198-
redundantptr:
199-
type: module
200-
description: Reports github.Ptr(x) calls that can be replaced with &x.
201-
original-url: github.com/google/go-github/v88/tools/redundantptr
202-
requestbody:
198+
paramcheck:
203199
type: module
204-
description: Reports miscellaneous issues for request body parameters.
205-
original-url: github.com/google/go-github/v88/tools/requestbody
200+
description: Reports parameter naming and type convention issues.
201+
original-url: github.com/google/go-github/v88/tools/paramcheck
206202
settings:
207203
# Body type names exempt from the "pass by value, not by pointer" rule.
208204
# TODO: fix and remove these exceptions.
209-
allowed-pointer-types:
205+
body-allowed-pointer-types:
210206
- ActionsVariable
211207
- AddProjectItemOptions
212208
- AuthorizationRequest
@@ -294,7 +290,7 @@ linters:
294290
- WorkflowsPermissionsOpt
295291
# Body type names exempt from the "Options" suffix rule.
296292
# TODO: fix and remove these exceptions.
297-
allowed-wrong-names:
293+
body-allowed-wrong-names:
298294
- AddProjectItemOptions
299295
- AutolinkOptions
300296
- CheckSuitePreferenceOptions
@@ -327,6 +323,10 @@ linters:
327323
- UpdateDefaultSetupConfigurationOptions
328324
- UpdateProjectItemOptions
329325
- UserSuspendOptions
326+
redundantptr:
327+
type: module
328+
description: Reports github.Ptr(x) calls that can be replaced with &x.
329+
original-url: github.com/google/go-github/v88/tools/redundantptr
330330
sliceofpointers:
331331
type: module
332332
description: Reports usage of []*string and slices of structs without pointers.
@@ -565,7 +565,7 @@ linters:
565565
issues:
566566
max-issues-per-linter: 0
567567
max-same-issues: 0
568-
# requestbody reports multiple distinct issues on the same parameter line.
568+
# paramcheck reports multiple distinct issues on the same parameter line.
569569
uniq-by-line: false
570570
formatters:
571571
enable:
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
module tools/requestbody
1+
module tools/paramcheck
22

33
go 1.25.0
44

Lines changed: 121 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,18 @@
33
// Use of this source code is governed by a BSD-style
44
// license that can be found in the LICENSE file.
55

6-
// Package requestbody is a custom linter for client.NewRequest body parameters.
6+
// Package paramcheck is a custom linter for parameter naming and type conventions.
77
//
8-
// For client.NewRequest calls using the PATCH, POST, or PUT methods it:
8+
// For body parameters it:
99
// - suggests renaming a body parameter to "body"
1010
// - reports body parameters passed by pointer, which should be passed by value
1111
// - reports body parameter types with an "Options" suffix, which should use a "Request" suffix instead.
12-
package requestbody
12+
//
13+
// For query parameters it:
14+
// - suggests renaming the options parameter to "opts"
15+
// - reports options parameters passed by value, which should be passed by pointer
16+
// - reports options parameter types without an "Options" suffix.
17+
package paramcheck
1318

1419
import (
1520
"fmt"
@@ -22,68 +27,68 @@ import (
2227
)
2328

2429
func init() {
25-
register.Plugin("requestbody", New)
30+
register.Plugin("paramcheck", New)
2631
}
2732

28-
// RequestBodyPlugin is a custom linter plugin for golangci-lint.
29-
type RequestBodyPlugin struct {
30-
// allowedPointerTypes are body type names exempt from the by-value rule.
31-
allowedPointerTypes map[string]bool
32-
// allowedWrongNames are body type names exempt from the "Options" suffix rule.
33-
allowedWrongNames map[string]bool
33+
// ParamCheckPlugin is a custom linter plugin for golangci-lint.
34+
type ParamCheckPlugin struct {
35+
// bodyAllowedPointerTypes are body type names exempt from the by-value rule.
36+
bodyAllowedPointerTypes map[string]bool
37+
// bodyAllowedWrongNames are body type names exempt from the "Options" suffix rule.
38+
bodyAllowedWrongNames map[string]bool
3439
}
3540

3641
// New returns an analysis.Analyzer to use with golangci-lint.
3742
func New(cfg any) (register.LinterPlugin, error) {
38-
allowedPointerTypes := map[string]bool{}
39-
allowedWrongNames := map[string]bool{}
43+
bodyAllowedPointerTypes := map[string]bool{}
44+
bodyAllowedWrongNames := map[string]bool{}
4045

4146
if cfg != nil {
4247
if settingsMap, ok := cfg.(map[string]any); ok {
43-
if exceptionsRaw, ok := settingsMap["allowed-pointer-types"]; ok {
48+
if exceptionsRaw, ok := settingsMap["body-allowed-pointer-types"]; ok {
4449
if exceptionsList, ok := exceptionsRaw.([]any); ok {
4550
for _, item := range exceptionsList {
4651
if exception, ok := item.(string); ok {
47-
allowedPointerTypes[exception] = true
52+
bodyAllowedPointerTypes[exception] = true
4853
}
4954
}
5055
}
5156
}
5257

53-
if exceptionsRaw, ok := settingsMap["allowed-wrong-names"]; ok {
58+
if exceptionsRaw, ok := settingsMap["body-allowed-wrong-names"]; ok {
5459
if exceptionsList, ok := exceptionsRaw.([]any); ok {
5560
for _, item := range exceptionsList {
5661
if exception, ok := item.(string); ok {
57-
allowedWrongNames[exception] = true
62+
bodyAllowedWrongNames[exception] = true
5863
}
5964
}
6065
}
6166
}
6267
}
6368
}
64-
return &RequestBodyPlugin{
65-
allowedPointerTypes: allowedPointerTypes,
66-
allowedWrongNames: allowedWrongNames,
69+
return &ParamCheckPlugin{
70+
bodyAllowedPointerTypes: bodyAllowedPointerTypes,
71+
bodyAllowedWrongNames: bodyAllowedWrongNames,
6772
}, nil
6873
}
6974

70-
// BuildAnalyzers builds the analyzers for the RequestBodyPlugin.
71-
func (p *RequestBodyPlugin) BuildAnalyzers() ([]*analysis.Analyzer, error) {
75+
// BuildAnalyzers builds the analyzers for the ParamCheckPlugin.
76+
func (p *ParamCheckPlugin) BuildAnalyzers() ([]*analysis.Analyzer, error) {
7277
return []*analysis.Analyzer{
7378
{
74-
Name: "requestbody",
75-
Doc: "Reports issues with request body parameters in client.NewRequest PATCH/POST/PUT calls.",
79+
Name: "paramcheck",
80+
Doc: "Reports parameter naming and type convention issues in endpoint method calls.",
7681
Run: p.run,
7782
},
7883
}, nil
7984
}
8085

81-
// GetLoadMode returns the load mode for the RequestBodyPlugin.
82-
func (p *RequestBodyPlugin) GetLoadMode() string {
86+
// GetLoadMode returns the load mode for the ParamCheckPlugin.
87+
func (p *ParamCheckPlugin) GetLoadMode() string {
8388
return register.LoadModeSyntax
8489
}
8590

86-
func (p *RequestBodyPlugin) run(pass *analysis.Pass) (any, error) {
91+
func (p *ParamCheckPlugin) run(pass *analysis.Pass) (any, error) {
8792
for _, file := range pass.Files {
8893
for _, decl := range file.Decls {
8994
fn, ok := decl.(*ast.FuncDecl)
@@ -96,31 +101,46 @@ func (p *RequestBodyPlugin) run(pass *analysis.Pass) (any, error) {
96101
return nil, nil
97102
}
98103

99-
func (p *RequestBodyPlugin) analyzeFunc(pass *analysis.Pass, fn *ast.FuncDecl) {
104+
func (p *ParamCheckPlugin) analyzeFunc(pass *analysis.Pass, fn *ast.FuncDecl) {
100105
ast.Inspect(fn.Body, func(n ast.Node) bool {
101106
call, ok := n.(*ast.CallExpr)
102-
if !ok || !isClientNewRequest(call) || !isMutatingMethod(call) {
107+
if !ok {
103108
return true
104109
}
105110

106-
bodyIdent, ok := call.Args[3].(*ast.Ident)
107-
if !ok {
108-
return true
111+
if isClientNewRequest(call) && isMutatingMethod(call) {
112+
bodyIdent, ok := call.Args[3].(*ast.Ident)
113+
if !ok {
114+
return true
115+
}
116+
field, name := findParam(fn, bodyIdent.Name)
117+
if field == nil {
118+
return true
119+
}
120+
bodyReportName(pass, fn, name)
121+
p.bodyReportPass(pass, field, name)
122+
p.bodyReportSuffix(pass, field)
109123
}
110124

111-
field, name := findParam(fn, bodyIdent.Name)
112-
if field == nil {
113-
return true
125+
if isAddOptions(call) {
126+
optsIdent, ok := call.Args[1].(*ast.Ident)
127+
if !ok {
128+
return true
129+
}
130+
field, name := findParam(fn, optsIdent.Name)
131+
if field == nil {
132+
return true
133+
}
134+
queryReportName(pass, fn, name)
135+
p.queryReportPass(pass, field, name)
136+
p.queryReportSuffix(pass, field)
114137
}
115138

116-
reportRename(pass, fn, name)
117-
p.reportByValue(pass, field, name)
118-
p.reportTypeSuffix(pass, field)
119139
return true
120140
})
121141
}
122142

123-
func reportRename(pass *analysis.Pass, fn *ast.FuncDecl, name *ast.Ident) {
143+
func bodyReportName(pass *analysis.Pass, fn *ast.FuncDecl, name *ast.Ident) {
124144
if name.Name == "body" {
125145
return
126146
}
@@ -130,7 +150,7 @@ func reportRename(pass *analysis.Pass, fn *ast.FuncDecl, name *ast.Ident) {
130150
End: name.End(),
131151
Message: fmt.Sprintf("rename request body parameter %q to \"body\"", name.Name),
132152
}
133-
if edits := renameEdits(fn, name.Name); edits != nil {
153+
if edits := renameEdits(fn, name.Name, "body"); edits != nil {
134154
diag.SuggestedFixes = []analysis.SuggestedFix{
135155
{
136156
Message: `Rename to "body"`,
@@ -141,11 +161,32 @@ func reportRename(pass *analysis.Pass, fn *ast.FuncDecl, name *ast.Ident) {
141161
pass.Report(diag)
142162
}
143163

144-
func (p *RequestBodyPlugin) reportByValue(pass *analysis.Pass, field *ast.Field, name *ast.Ident) {
164+
func queryReportName(pass *analysis.Pass, fn *ast.FuncDecl, name *ast.Ident) {
165+
if name.Name == "opts" {
166+
return
167+
}
168+
169+
diag := analysis.Diagnostic{
170+
Pos: name.Pos(),
171+
End: name.End(),
172+
Message: fmt.Sprintf("rename addOptions parameter %q to \"opts\"", name.Name),
173+
}
174+
if edits := renameEdits(fn, name.Name, "opts"); edits != nil {
175+
diag.SuggestedFixes = []analysis.SuggestedFix{
176+
{
177+
Message: `Rename to "opts"`,
178+
TextEdits: edits,
179+
},
180+
}
181+
}
182+
pass.Report(diag)
183+
}
184+
185+
func (p *ParamCheckPlugin) bodyReportPass(pass *analysis.Pass, field *ast.Field, name *ast.Ident) {
145186
if _, ok := field.Type.(*ast.StarExpr); !ok {
146187
return
147188
}
148-
if ident := typeNameIdent(field.Type); ident != nil && p.allowedPointerTypes[ident.Name] {
189+
if ident := typeNameIdent(field.Type); ident != nil && p.bodyAllowedPointerTypes[ident.Name] {
149190
return
150191
}
151192
pass.Report(analysis.Diagnostic{
@@ -155,15 +196,38 @@ func (p *RequestBodyPlugin) reportByValue(pass *analysis.Pass, field *ast.Field,
155196
})
156197
}
157198

158-
// reportTypeSuffix reports body parameter types whose name ends with "Options", which should use a "Request" suffix instead
199+
func (p *ParamCheckPlugin) queryReportPass(pass *analysis.Pass, field *ast.Field, name *ast.Ident) {
200+
if _, ok := field.Type.(*ast.StarExpr); ok {
201+
return
202+
}
203+
pass.Report(analysis.Diagnostic{
204+
Pos: name.Pos(),
205+
End: name.End(),
206+
Message: fmt.Sprintf("pass query parameter %q by pointer, not by value", name.Name),
207+
})
208+
}
209+
210+
func (p *ParamCheckPlugin) queryReportSuffix(pass *analysis.Pass, field *ast.Field) {
211+
ident := typeNameIdent(field.Type)
212+
if ident == nil || strings.HasSuffix(ident.Name, "Options") {
213+
return
214+
}
215+
pass.Report(analysis.Diagnostic{
216+
Pos: ident.Pos(),
217+
End: ident.End(),
218+
Message: fmt.Sprintf("query parameter type %q should use an \"Options\" suffix", ident.Name),
219+
})
220+
}
221+
222+
// bodyReportSuffix reports body parameter types whose name ends with "Options", which should use a "Request" suffix instead
159223
// (e.g. UserSuspendOptions -> UserSuspendRequest).
160224
// It is report-only because renaming a type affects its declaration and every use across the codebase.
161-
func (p *RequestBodyPlugin) reportTypeSuffix(pass *analysis.Pass, field *ast.Field) {
225+
func (p *ParamCheckPlugin) bodyReportSuffix(pass *analysis.Pass, field *ast.Field) {
162226
ident := typeNameIdent(field.Type)
163227
if ident == nil || !strings.HasSuffix(ident.Name, "Options") {
164228
return
165229
}
166-
if p.allowedWrongNames[ident.Name] {
230+
if p.bodyAllowedWrongNames[ident.Name] {
167231
return
168232
}
169233
pass.Report(analysis.Diagnostic{
@@ -173,6 +237,16 @@ func (p *RequestBodyPlugin) reportTypeSuffix(pass *analysis.Pass, field *ast.Fie
173237
})
174238
}
175239

240+
// isAddOptions reports whether call is of the form addOptions(...) with at least two arguments.
241+
// addOptions is used in endpoint methods to add query parameters, so the second argument is expected to be the options struct.
242+
func isAddOptions(call *ast.CallExpr) bool {
243+
if len(call.Args) < 2 {
244+
return false
245+
}
246+
ident, ok := call.Fun.(*ast.Ident)
247+
return ok && ident.Name == "addOptions"
248+
}
249+
176250
// isClientNewRequest reports whether call is of the form x.client.NewRequest(...) or client.NewRequest(...).
177251
func isClientNewRequest(call *ast.CallExpr) bool {
178252
sel, ok := call.Fun.(*ast.SelectorExpr)
@@ -234,12 +308,10 @@ func typeNameIdent(expr ast.Expr) *ast.Ident {
234308
}
235309
}
236310

237-
// renameEdits builds the text edits that rename every reference to the old parameter within fn to "body".
238-
// It returns nil (no auto-fix) when "body" is already used as an identifier in fn or when old is shadowed by a local declaration,
311+
// renameEdits builds the text edits that rename every reference to the old parameter within fn to newName.
312+
// It returns nil (no auto-fix) when newName is already used as an identifier in fn or when old is shadowed by a local declaration,
239313
// since either case could make the rename incorrect.
240-
func renameEdits(fn *ast.FuncDecl, old string) []analysis.TextEdit {
241-
const newName = "body"
242-
314+
func renameEdits(fn *ast.FuncDecl, old, newName string) []analysis.TextEdit {
243315
// Idents that are the selector field (x.old) must not be renamed.
244316
skip := map[*ast.Ident]bool{}
245317
ast.Inspect(fn, func(n ast.Node) bool {
Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
// Use of this source code is governed by a BSD-style
44
// license that can be found in the LICENSE file.
55

6-
package requestbody
6+
package paramcheck
77

88
import (
99
"testing"
@@ -15,8 +15,8 @@ func TestRun(t *testing.T) {
1515
t.Parallel()
1616
testdata := analysistest.TestData()
1717
plugin, _ := New(map[string]any{
18-
"allowed-pointer-types": []any{"AllowedPtr"},
19-
"allowed-wrong-names": []any{"AllowedOptions"},
18+
"body-allowed-pointer-types": []any{"AllowedPtr"},
19+
"body-allowed-wrong-names": []any{"AllowedOptions"},
2020
})
2121
analyzers, _ := plugin.BuildAnalyzers()
2222
analysistest.Run(t, testdata, analyzers[0], "has-warnings", "no-warnings")

0 commit comments

Comments
 (0)