Skip to content

Commit 8dd3945

Browse files
git-hulkchavacava
git-hulk
and
chavacava
authored
Check whether the tag name is duplicate or not (#706)
* Check whether the tag name is duplicate or not * - minor refactoring - continues checking tag even if name is repeated * adds test cases for duplicated tag names * adds test case with two tag types (json & yaml) * Fix allow the same tag name in different tag key * fix checks on protobuf tag names Co-authored-by: chavacava <[email protected]>
1 parent 23ed063 commit 8dd3945

File tree

2 files changed

+108
-14
lines changed

2 files changed

+108
-14
lines changed

‎rule/struct-tag.go

Lines changed: 52 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,9 @@ func (*StructTagRule) Name() string {
3434
}
3535

3636
type lintStructTagRule struct {
37-
onFailure func(lint.Failure)
38-
usedTagNbr map[string]bool // list of used tag numbers
37+
onFailure func(lint.Failure)
38+
usedTagNbr map[string]bool // list of used tag numbers
39+
usedTagName map[string]bool // list of used tag keys
3940
}
4041

4142
func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor {
@@ -44,7 +45,8 @@ func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor {
4445
if n.Fields == nil || n.Fields.NumFields() < 1 {
4546
return nil // skip empty structs
4647
}
47-
w.usedTagNbr = map[string]bool{} // init
48+
w.usedTagNbr = map[string]bool{} // init
49+
w.usedTagName = map[string]bool{} // init
4850
for _, f := range n.Fields.List {
4951
if f.Tag != nil {
5052
w.checkTaggedField(f)
@@ -55,6 +57,49 @@ func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor {
5557
return w
5658
}
5759

60+
func (w lintStructTagRule) checkTagNameIfNeed(tag *structtag.Tag) (string, bool) {
61+
isUnnamedTag := tag.Name == "" || tag.Name == "-"
62+
if isUnnamedTag {
63+
return "", true
64+
}
65+
66+
needsToCheckTagName := tag.Key == "bson" ||
67+
tag.Key == "json" ||
68+
tag.Key == "xml" ||
69+
tag.Key == "yaml" ||
70+
tag.Key == "protobuf"
71+
72+
if !needsToCheckTagName {
73+
return "", true
74+
}
75+
76+
tagName := w.getTagName(tag)
77+
// We concat the key and name as the mapping key here
78+
// to allow the same tag name in different tag type.
79+
key := tag.Key + ":" + tagName
80+
if _, ok := w.usedTagName[key]; ok {
81+
return fmt.Sprintf("duplicate tag name: '%s'", tagName), false
82+
}
83+
84+
w.usedTagName[key] = true
85+
86+
return "", true
87+
}
88+
89+
func (lintStructTagRule) getTagName(tag *structtag.Tag) string {
90+
switch tag.Key {
91+
case "protobuf":
92+
for _, option := range tag.Options {
93+
if strings.HasPrefix(option, "name=") {
94+
return strings.TrimLeft(option, "name=")
95+
}
96+
}
97+
return "protobuf tag lacks name"
98+
default:
99+
return tag.Name
100+
}
101+
}
102+
58103
// checkTaggedField checks the tag of the given field.
59104
// precondition: the field has a tag
60105
func (w lintStructTagRule) checkTaggedField(f *ast.Field) {
@@ -69,6 +114,10 @@ func (w lintStructTagRule) checkTaggedField(f *ast.Field) {
69114
}
70115

71116
for _, tag := range tags.Tags() {
117+
if msg, ok := w.checkTagNameIfNeed(tag); !ok {
118+
w.addFailure(f.Tag, msg)
119+
}
120+
72121
switch key := tag.Key; key {
73122
case "asn1":
74123
msg, ok := w.checkASN1Tag(f.Type, tag)

‎testdata/struct-tag.go

Lines changed: 56 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,25 +4,27 @@ import "time"
44

55
type decodeAndValidateRequest struct {
66
// BEAWRE : the flag of URLParam should match the const string URLParam
7-
URLParam string `json:"-" path:"url_param" validate:"numeric"`
8-
Text string `json:"text" validate:"max=10"`
9-
DefaultInt int `json:"defaultInt" default:"10.0"` // MATCH /field's type and default value's type mismatch/
10-
DefaultInt2 int `json:"defaultInt" default:"10"`
7+
URLParam string `json:"-" path:"url_param" validate:"numeric"`
8+
Text string `json:"text" validate:"max=10"`
9+
DefaultInt int `json:"defaultInt" default:"10.0"` // MATCH /field's type and default value's type mismatch/
10+
DefaultInt2 int `json:"defaultInt2" default:"10"`
11+
// MATCH:12 /unknown option 'inline' in JSON tag/
12+
DefaultInt3 int `json:"defaultInt2,inline" default:"11"` // MATCH /duplicate tag name: 'defaultInt2'/
1113
DefaultString string `json:"defaultString" default:"foo"`
1214
DefaultBool bool `json:"defaultBool" default:"trues"` // MATCH /field's type and default value's type mismatch/
13-
DefaultBool2 bool `json:"defaultBool" default:"true"`
14-
DefaultBool3 bool `json:"defaultBool" default:"false"`
15+
DefaultBool2 bool `json:"defaultBool2" default:"true"`
16+
DefaultBool3 bool `json:"defaultBool3" default:"false"`
1517
DefaultFloat float64 `json:"defaultFloat" default:"f10.0"` // MATCH /field's type and default value's type mismatch/
16-
DefaultFloat2 float64 `json:"defaultFloat" default:"10.0"`
18+
DefaultFloat2 float64 `json:"defaultFloat2" default:"10.0"`
1719
MandatoryStruct mandatoryStruct `json:"mandatoryStruct" required:"trues"` // MATCH /required should be 'true' or 'false'/
18-
MandatoryStruct2 mandatoryStruct `json:"mandatoryStruct" required:"true"`
19-
MandatoryStruct4 mandatoryStruct `json:"mandatoryStruct" required:"false"`
20+
MandatoryStruct2 mandatoryStruct `json:"mandatoryStruct2" required:"true"`
21+
MandatoryStruct4 mandatoryStruct `json:"mandatoryStruct4" required:"false"`
2022
OptionalStruct *optionalStruct `json:"optionalStruct,omitempty"`
2123
OptionalQuery string `json:"-" querystring:"queryfoo"`
2224
optionalQuery string `json:"-" querystring:"queryfoo"` // MATCH /tag on not-exported field optionalQuery/
2325
// No-reg test for bug https://github.com/mgechev/revive/issues/208
24-
Tiret string `json:"-,"`
25-
BadTiret string `json:"other,"` // MATCH /option can not be empty in JSON tag/
26+
Tiret string `json:"-,"`
27+
BadTiret string `json:"other,"` // MATCH /option can not be empty in JSON tag/
2628
}
2729

2830
type RangeAllocation struct {
@@ -57,3 +59,46 @@ type VirtualMachineRelocateSpecDiskLocator struct {
5759
DiskBackingInfo BaseVirtualDeviceBackingInfo `xml:"diskBackingInfo,omitempty,any"`
5860
Profile []BaseVirtualMachineProfileSpec `xml:"profile,omitempty,other"` // MATCH /unknown option 'other' in XML tag/
5961
}
62+
63+
type TestDuplicatedXMLTags struct {
64+
A int `xml:"a"`
65+
B int `xml:"a"` // MATCH /duplicate tag name: 'a'/
66+
C int `xml:"c"`
67+
}
68+
69+
type TestDuplicatedBSONTags struct {
70+
A int `bson:"b"`
71+
B int `bson:"b"` // MATCH /duplicate tag name: 'b'/
72+
C int `bson:"c"`
73+
}
74+
75+
type TestDuplicatedYAMLTags struct {
76+
A int `yaml:"b"`
77+
B int `yaml:"c"`
78+
C int `yaml:"c"` // MATCH /duplicate tag name: 'c'/
79+
}
80+
81+
type TestDuplicatedProtobufTags struct {
82+
A int `protobuf:"varint,name=b"`
83+
B int `protobuf:"varint,name=c"`
84+
C int `protobuf:"varint,name=c"` // MATCH /duplicate tag name: 'c'/
85+
}
86+
87+
// test case from
88+
// sigs.k8s.io/kustomize/api/types/helmchartargs.go
89+
90+
type HelmChartArgs struct {
91+
ChartName string `json:"chartName,omitempty" yaml:"chartName,omitempty"`
92+
ChartVersion string `json:"chartVersion,omitempty" yaml:"chartVersion,omitempty"`
93+
ChartRepoURL string `json:"chartRepoUrl,omitempty" yaml:"chartRepoUrl,omitempty"`
94+
ChartHome string `json:"chartHome,omitempty" yaml:"chartHome,omitempty"`
95+
ChartRepoName string `json:"chartRepoName,omitempty" yaml:"chartRepoName,omitempty"`
96+
HelmBin string `json:"helmBin,omitempty" yaml:"helmBin,omitempty"`
97+
HelmHome string `json:"helmHome,omitempty" yaml:"helmHome,omitempty"`
98+
Values string `json:"values,omitempty" yaml:"values,omitempty"`
99+
ValuesLocal map[string]interface{} `json:"valuesLocal,omitempty" yaml:"valuesLocal,omitempty"`
100+
ValuesMerge string `json:"valuesMerge,omitempty" yaml:"valuesMerge,omitempty"`
101+
ReleaseName string `json:"releaseName,omitempty" yaml:"releaseName,omitempty"`
102+
ReleaseNamespace string `json:"releaseNamespace,omitempty" yaml:"releaseNamespace,omitempty"`
103+
ExtraArgs []string `json:"extraArgs,omitempty" yaml:"extraArgs,omitempty"`
104+
}

0 commit comments

Comments
 (0)