From ee2710ab824fe74be9a92b7bc22afe8e7c74868b Mon Sep 17 00:00:00 2001 From: git-hulk Date: Wed, 13 Jul 2022 11:48:39 +0800 Subject: [PATCH 1/6] Check whether the tag name is duplicate or not --- rule/struct-tag.go | 27 ++++++++++++++++++++++++--- testdata/struct-tag.go | 17 +++++++++-------- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/rule/struct-tag.go b/rule/struct-tag.go index 876104d79..aeece3d29 100644 --- a/rule/struct-tag.go +++ b/rule/struct-tag.go @@ -34,8 +34,9 @@ func (*StructTagRule) Name() string { } type lintStructTagRule struct { - onFailure func(lint.Failure) - usedTagNbr map[string]bool // list of used tag numbers + onFailure func(lint.Failure) + usedTagNbr map[string]bool // list of used tag numbers + usedTagName map[string]bool // list of used tag keys } func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor { @@ -44,7 +45,8 @@ func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor { if n.Fields == nil || n.Fields.NumFields() < 1 { return nil // skip empty structs } - w.usedTagNbr = map[string]bool{} // init + w.usedTagNbr = map[string]bool{} // init + w.usedTagName = map[string]bool{} // init for _, f := range n.Fields.List { if f.Tag != nil { w.checkTaggedField(f) @@ -55,6 +57,21 @@ func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor { return w } +func (w lintStructTagRule) checkTagNameIfNeed(tag *structtag.Tag) (string, bool) { + if tag.Key != "bson" && tag.Key != "json" && tag.Key != "xml" && + tag.Key != "yaml" && tag.Key != "protobuf" { + return "", true + } + if tag.Name == "" || tag.Name == "-" { + return "", true + } + if _, ok := w.usedTagName[tag.Name]; ok { + return fmt.Sprintf("duplicate tag name: '%s'", tag.Name), false + } + w.usedTagName[tag.Name] = true + return "", true +} + // checkTaggedField checks the tag of the given field. // precondition: the field has a tag func (w lintStructTagRule) checkTaggedField(f *ast.Field) { @@ -69,6 +86,10 @@ func (w lintStructTagRule) checkTaggedField(f *ast.Field) { } for _, tag := range tags.Tags() { + if msg, ok := w.checkTagNameIfNeed(tag); !ok { + w.addFailure(f.Tag, msg) + continue + } switch key := tag.Key; key { case "asn1": msg, ok := w.checkASN1Tag(f.Type, tag) diff --git a/testdata/struct-tag.go b/testdata/struct-tag.go index e4aa07446..f4ce89ffd 100644 --- a/testdata/struct-tag.go +++ b/testdata/struct-tag.go @@ -7,22 +7,23 @@ type decodeAndValidateRequest struct { URLParam string `json:"-" path:"url_param" validate:"numeric"` Text string `json:"text" validate:"max=10"` DefaultInt int `json:"defaultInt" default:"10.0"` // MATCH /field's type and default value's type mismatch/ - DefaultInt2 int `json:"defaultInt" default:"10"` + DefaultInt2 int `json:"defaultInt2" default:"10"` + DefaultInt3 int `json:"defaultInt2" default:"11"` // MATCH /duplicate tag name: 'defaultInt2'/ DefaultString string `json:"defaultString" default:"foo"` DefaultBool bool `json:"defaultBool" default:"trues"` // MATCH /field's type and default value's type mismatch/ - DefaultBool2 bool `json:"defaultBool" default:"true"` - DefaultBool3 bool `json:"defaultBool" default:"false"` + DefaultBool2 bool `json:"defaultBool2" default:"true"` + DefaultBool3 bool `json:"defaultBool3" default:"false"` DefaultFloat float64 `json:"defaultFloat" default:"f10.0"` // MATCH /field's type and default value's type mismatch/ - DefaultFloat2 float64 `json:"defaultFloat" default:"10.0"` + DefaultFloat2 float64 `json:"defaultFloat2" default:"10.0"` MandatoryStruct mandatoryStruct `json:"mandatoryStruct" required:"trues"` // MATCH /required should be 'true' or 'false'/ - MandatoryStruct2 mandatoryStruct `json:"mandatoryStruct" required:"true"` - MandatoryStruct4 mandatoryStruct `json:"mandatoryStruct" required:"false"` + MandatoryStruct2 mandatoryStruct `json:"mandatoryStruct2" required:"true"` + MandatoryStruct4 mandatoryStruct `json:"mandatoryStruct4" required:"false"` OptionalStruct *optionalStruct `json:"optionalStruct,omitempty"` OptionalQuery string `json:"-" querystring:"queryfoo"` optionalQuery string `json:"-" querystring:"queryfoo"` // MATCH /tag on not-exported field optionalQuery/ // No-reg test for bug https://github.com/mgechev/revive/issues/208 - Tiret string `json:"-,"` - BadTiret string `json:"other,"` // MATCH /option can not be empty in JSON tag/ + Tiret string `json:"-,"` + BadTiret string `json:"other,"` // MATCH /option can not be empty in JSON tag/ } type RangeAllocation struct { From 35097c57b52edc7729b5d15b87c0c47b2b2075ba Mon Sep 17 00:00:00 2001 From: chavacava Date: Wed, 13 Jul 2022 11:19:40 +0000 Subject: [PATCH 2/6] - minor refactoring - continues checking tag even if name is repeated --- rule/struct-tag.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/rule/struct-tag.go b/rule/struct-tag.go index aeece3d29..dbe511232 100644 --- a/rule/struct-tag.go +++ b/rule/struct-tag.go @@ -58,17 +58,27 @@ func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor { } func (w lintStructTagRule) checkTagNameIfNeed(tag *structtag.Tag) (string, bool) { - if tag.Key != "bson" && tag.Key != "json" && tag.Key != "xml" && - tag.Key != "yaml" && tag.Key != "protobuf" { + isUnnamedTag := tag.Name == "" || tag.Name == "-" + if isUnnamedTag { return "", true } - if tag.Name == "" || tag.Name == "-" { + + needsToCheckTagName := tag.Key == "bson" || + tag.Key == "json" || + tag.Key == "xml" || + tag.Key == "yaml" || + tag.Key == "protobuf" + + if !needsToCheckTagName { return "", true } + if _, ok := w.usedTagName[tag.Name]; ok { return fmt.Sprintf("duplicate tag name: '%s'", tag.Name), false } + w.usedTagName[tag.Name] = true + return "", true } @@ -88,8 +98,8 @@ func (w lintStructTagRule) checkTaggedField(f *ast.Field) { for _, tag := range tags.Tags() { if msg, ok := w.checkTagNameIfNeed(tag); !ok { w.addFailure(f.Tag, msg) - continue } + switch key := tag.Key; key { case "asn1": msg, ok := w.checkASN1Tag(f.Type, tag) From 6a73405e87afa74685d35e883f1d0577615bec10 Mon Sep 17 00:00:00 2001 From: chavacava Date: Wed, 13 Jul 2022 11:20:23 +0000 Subject: [PATCH 3/6] adds test cases for duplicated tag names --- testdata/struct-tag.go | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/testdata/struct-tag.go b/testdata/struct-tag.go index f4ce89ffd..ea354aa76 100644 --- a/testdata/struct-tag.go +++ b/testdata/struct-tag.go @@ -4,11 +4,12 @@ import "time" type decodeAndValidateRequest struct { // BEAWRE : the flag of URLParam should match the const string URLParam - URLParam string `json:"-" path:"url_param" validate:"numeric"` - Text string `json:"text" validate:"max=10"` - DefaultInt int `json:"defaultInt" default:"10.0"` // MATCH /field's type and default value's type mismatch/ - DefaultInt2 int `json:"defaultInt2" default:"10"` - DefaultInt3 int `json:"defaultInt2" default:"11"` // MATCH /duplicate tag name: 'defaultInt2'/ + URLParam string `json:"-" path:"url_param" validate:"numeric"` + Text string `json:"text" validate:"max=10"` + DefaultInt int `json:"defaultInt" default:"10.0"` // MATCH /field's type and default value's type mismatch/ + DefaultInt2 int `json:"defaultInt2" default:"10"` + // MATCH:12 /unknown option 'inline' in JSON tag/ + DefaultInt3 int `json:"defaultInt2,inline" default:"11"` // MATCH /duplicate tag name: 'defaultInt2'/ DefaultString string `json:"defaultString" default:"foo"` DefaultBool bool `json:"defaultBool" default:"trues"` // MATCH /field's type and default value's type mismatch/ DefaultBool2 bool `json:"defaultBool2" default:"true"` @@ -58,3 +59,27 @@ type VirtualMachineRelocateSpecDiskLocator struct { DiskBackingInfo BaseVirtualDeviceBackingInfo `xml:"diskBackingInfo,omitempty,any"` Profile []BaseVirtualMachineProfileSpec `xml:"profile,omitempty,other"` // MATCH /unknown option 'other' in XML tag/ } + +type TestDuplicatedXMLTags struct { + A int `xml:"a"` + B int `xml:"a"` // MATCH /duplicate tag name: 'a'/ + C int `xml:"c"` +} + +type TestDuplicatedBSONTags struct { + A int `bson:"b"` + B int `bson:"b"` // MATCH /duplicate tag name: 'b'/ + C int `bson:"c"` +} + +type TestDuplicatedYAMLTags struct { + A int `yaml:"b"` + B int `yaml:"c"` + C int `yaml:"c"` // MATCH /duplicate tag name: 'c'/ +} + +type TestDuplicatedProtobufTags struct { + A int `protobuf:"b"` + B int `protobuf:"c"` + C int `protobuf:"c"` // MATCH /duplicate tag name: 'c'/ +} From 6ec6f06d351597829afd9d810871f21f4879d8e5 Mon Sep 17 00:00:00 2001 From: chavacava Date: Wed, 13 Jul 2022 11:25:37 +0000 Subject: [PATCH 4/6] adds test case with two tag types (json & yaml) --- testdata/struct-tag.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/testdata/struct-tag.go b/testdata/struct-tag.go index ea354aa76..54ac91705 100644 --- a/testdata/struct-tag.go +++ b/testdata/struct-tag.go @@ -83,3 +83,22 @@ type TestDuplicatedProtobufTags struct { B int `protobuf:"c"` C int `protobuf:"c"` // MATCH /duplicate tag name: 'c'/ } + +// test case from +// sigs.k8s.io/kustomize/api/types/helmchartargs.go + +type HelmChartArgs struct { + ChartName string `json:"chartName,omitempty" yaml:"chartName,omitempty"` + ChartVersion string `json:"chartVersion,omitempty" yaml:"chartVersion,omitempty"` + ChartRepoURL string `json:"chartRepoUrl,omitempty" yaml:"chartRepoUrl,omitempty"` + ChartHome string `json:"chartHome,omitempty" yaml:"chartHome,omitempty"` + ChartRepoName string `json:"chartRepoName,omitempty" yaml:"chartRepoName,omitempty"` + HelmBin string `json:"helmBin,omitempty" yaml:"helmBin,omitempty"` + HelmHome string `json:"helmHome,omitempty" yaml:"helmHome,omitempty"` + Values string `json:"values,omitempty" yaml:"values,omitempty"` + ValuesLocal map[string]interface{} `json:"valuesLocal,omitempty" yaml:"valuesLocal,omitempty"` + ValuesMerge string `json:"valuesMerge,omitempty" yaml:"valuesMerge,omitempty"` + ReleaseName string `json:"releaseName,omitempty" yaml:"releaseName,omitempty"` + ReleaseNamespace string `json:"releaseNamespace,omitempty" yaml:"releaseNamespace,omitempty"` + ExtraArgs []string `json:"extraArgs,omitempty" yaml:"extraArgs,omitempty"` +} From ed6300511ba9500dc4b25b1c25533f02c97c6779 Mon Sep 17 00:00:00 2001 From: git-hulk Date: Wed, 13 Jul 2022 23:28:43 +0800 Subject: [PATCH 5/6] Fix allow the same tag name in different tag key --- rule/struct-tag.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rule/struct-tag.go b/rule/struct-tag.go index dbe511232..066407d29 100644 --- a/rule/struct-tag.go +++ b/rule/struct-tag.go @@ -73,11 +73,14 @@ func (w lintStructTagRule) checkTagNameIfNeed(tag *structtag.Tag) (string, bool) return "", true } - if _, ok := w.usedTagName[tag.Name]; ok { + // We concat the key and name as the mapping key here + // to allow the same tag name in different tag type. + key := tag.Key + ":" + tag.Name + if _, ok := w.usedTagName[key]; ok { return fmt.Sprintf("duplicate tag name: '%s'", tag.Name), false } - w.usedTagName[tag.Name] = true + w.usedTagName[key] = true return "", true } From 682ef71389b6435747afc89b19bbd540db1e1b70 Mon Sep 17 00:00:00 2001 From: chavacava Date: Wed, 13 Jul 2022 18:23:16 +0000 Subject: [PATCH 6/6] fix checks on protobuf tag names --- rule/struct-tag.go | 19 +++++++++++++++++-- testdata/struct-tag.go | 6 +++--- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/rule/struct-tag.go b/rule/struct-tag.go index 066407d29..6e3e00b5b 100644 --- a/rule/struct-tag.go +++ b/rule/struct-tag.go @@ -73,11 +73,12 @@ func (w lintStructTagRule) checkTagNameIfNeed(tag *structtag.Tag) (string, bool) return "", true } + tagName := w.getTagName(tag) // We concat the key and name as the mapping key here // to allow the same tag name in different tag type. - key := tag.Key + ":" + tag.Name + key := tag.Key + ":" + tagName if _, ok := w.usedTagName[key]; ok { - return fmt.Sprintf("duplicate tag name: '%s'", tag.Name), false + return fmt.Sprintf("duplicate tag name: '%s'", tagName), false } w.usedTagName[key] = true @@ -85,6 +86,20 @@ func (w lintStructTagRule) checkTagNameIfNeed(tag *structtag.Tag) (string, bool) return "", true } +func (lintStructTagRule) getTagName(tag *structtag.Tag) string { + switch tag.Key { + case "protobuf": + for _, option := range tag.Options { + if strings.HasPrefix(option, "name=") { + return strings.TrimLeft(option, "name=") + } + } + return "protobuf tag lacks name" + default: + return tag.Name + } +} + // checkTaggedField checks the tag of the given field. // precondition: the field has a tag func (w lintStructTagRule) checkTaggedField(f *ast.Field) { diff --git a/testdata/struct-tag.go b/testdata/struct-tag.go index 54ac91705..600895a23 100644 --- a/testdata/struct-tag.go +++ b/testdata/struct-tag.go @@ -79,9 +79,9 @@ type TestDuplicatedYAMLTags struct { } type TestDuplicatedProtobufTags struct { - A int `protobuf:"b"` - B int `protobuf:"c"` - C int `protobuf:"c"` // MATCH /duplicate tag name: 'c'/ + A int `protobuf:"varint,name=b"` + B int `protobuf:"varint,name=c"` + C int `protobuf:"varint,name=c"` // MATCH /duplicate tag name: 'c'/ } // test case from