From d522f2b7e87295229c491bd84404700406473937 Mon Sep 17 00:00:00 2001 From: Anton Telyshev Date: Wed, 13 Nov 2024 11:46:15 +0300 Subject: [PATCH] encoded-compare false positives fix (#199) * encoded-compare: require string or []byte type for json-like or yaml-like named vars * encoded-compare: strict match for yml (not yaml) * cosmetic: reorder funcs --- analyzer/analyzer_test.go | 8 ++ .../false_positive_test.go | 84 +++++++++++++++++++ .../var_naming_and_type_test.go | 45 ++++++++++ internal/checkers/encoded_compare.go | 2 +- internal/checkers/helpers_basic_type.go | 6 ++ internal/checkers/helpers_encoded.go | 24 +++++- 6 files changed, 164 insertions(+), 5 deletions(-) create mode 100644 analyzer/testdata/src/encoded-compare-issue196/false_positive_test.go create mode 100644 analyzer/testdata/src/encoded-compare-issue198/var_naming_and_type_test.go diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index 695c4e37..8427893f 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -60,6 +60,14 @@ func TestTestifyLint_NotDefaultCases(t *testing.T) { dir: "checkers-priority", flags: map[string]string{"enable-all": "true"}, }, + { + dir: "encoded-compare-issue196", + flags: map[string]string{"disable-all": "true", "enable": checkers.NewEncodedCompare().Name()}, + }, + { + dir: "encoded-compare-issue198", + flags: map[string]string{"disable-all": "true", "enable": checkers.NewEncodedCompare().Name()}, + }, { dir: "error-as-target", flags: map[string]string{"disable-all": "true", "enable": checkers.NewErrorIsAs().Name()}, diff --git a/analyzer/testdata/src/encoded-compare-issue196/false_positive_test.go b/analyzer/testdata/src/encoded-compare-issue196/false_positive_test.go new file mode 100644 index 00000000..c5af4c14 --- /dev/null +++ b/analyzer/testdata/src/encoded-compare-issue196/false_positive_test.go @@ -0,0 +1,84 @@ +package encodedcompareissue196 + +import ( + "bytes" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRepoFollowSymlink(t *testing.T) { + defer PrepareTestEnv(t)() + session := loginUser(t, "user2") + + assertCase := func(t *testing.T, url, expectedSymlinkURL string, shouldExist bool) { + t.Helper() + + req := NewRequest(t, "GET", url) + resp := session.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + symlinkURL, ok := htmlDoc.Find(".file-actions .button[data-kind='follow-symlink']").Attr("href") + if shouldExist { + assert.True(t, ok) + assert.Equal(t, expectedSymlinkURL, symlinkURL) + } else { + assert.False(t, ok) + } + } + + t.Run("Normal", func(t *testing.T) { + defer PrintCurrentTest(t)() + assertCase(t, "/user2/readme-test/src/branch/symlink/README.md?display=source", "/user2/readme-test/src/branch/symlink/some/other/path/awefulcake.txt", true) + }) +} + +func PrepareTestEnv(t *testing.T) func() { return func() {} } +func PrintCurrentTest(t *testing.T) func() { return func() {} } + +func loginUser(t *testing.T, uid string) *session { + return new(session) +} + +type session struct { +} + +func (s *session) MakeRequest(t *testing.T, req *http.Request, expStatusCode int) *Response { + return new(Response) +} + +func NewRequest(t *testing.T, method string, url string) *http.Request { + return new(http.Request) +} + +type Response struct { + Body *bytes.Buffer +} + +// HTMLDoc struct +type HTMLDoc struct { +} + +// NewHTMLParser parse html file +func NewHTMLParser(t testing.TB, body *bytes.Buffer) *HTMLDoc { + t.Helper() + return new(HTMLDoc) +} + +// Find gets the descendants of each element in the current set of +// matched elements, filtered by a selector. It returns a new Selection +// object containing these matched elements. +func (doc *HTMLDoc) Find(selector string) *Selection { + return new(Selection) +} + +type Selection struct { +} + +// Attr gets the specified attribute's value for the first element in the +// Selection. To get the value for each element individually, use a looping +// construct such as Each or Map method. +func (s *Selection) Attr(attrName string) (val string, exists bool) { + return "", false +} diff --git a/analyzer/testdata/src/encoded-compare-issue198/var_naming_and_type_test.go b/analyzer/testdata/src/encoded-compare-issue198/var_naming_and_type_test.go new file mode 100644 index 00000000..7581a4d5 --- /dev/null +++ b/analyzer/testdata/src/encoded-compare-issue198/var_naming_and_type_test.go @@ -0,0 +1,45 @@ +package encodedcompareissue198 + +import ( + "bytes" + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCommonResultFromFullResult(t *testing.T) { + jsonData := new(bytes.Buffer) + var cr *commonResult + + crFromJSON := new(commonResult) + err := json.Unmarshal(jsonData.Bytes(), crFromJSON) + require.NoError(t, err) + + assert.Equal(t, cr, crFromJSON) +} + +func TestCommonResultFromFullAndCompactJSON(t *testing.T) { + compactJSONData := new(bytes.Buffer) + fullJSONData := new(bytes.Buffer) + + crFromCompactJSON := new(commonResult) + crFromFullJSON := new(commonResult) + + err := json.NewDecoder(compactJSONData).Decode(crFromCompactJSON) + require.NoError(t, err) + + err = json.NewDecoder(fullJSONData).Decode(crFromCompactJSON) + require.NoError(t, err) + + assert.Equal(t, crFromCompactJSON, crFromFullJSON) + + var crFromYML *commonResult + assert.Equal(t, crFromYML, new(commonResult)) +} + +type commonResult struct { + Name string `json:"name"` + Size int64 `json:"size"` +} diff --git a/internal/checkers/encoded_compare.go b/internal/checkers/encoded_compare.go index 53c74ac4..1464fd64 100644 --- a/internal/checkers/encoded_compare.go +++ b/internal/checkers/encoded_compare.go @@ -48,7 +48,7 @@ func (checker EncodedCompare) Check(pass *analysis.Pass, call *CallMeta) *analys switch { case aIsExplicitJSON, bIsExplicitJSON, isJSONStyleExpr(pass, a), isJSONStyleExpr(pass, b): proposed = "JSONEq" - case isYAMLStyleExpr(a), isYAMLStyleExpr(b): + case isYAMLStyleExpr(pass, a), isYAMLStyleExpr(pass, b): proposed = "YAMLEq" } diff --git a/internal/checkers/helpers_basic_type.go b/internal/checkers/helpers_basic_type.go index 9b43e914..b4bb5632 100644 --- a/internal/checkers/helpers_basic_type.go +++ b/internal/checkers/helpers_basic_type.go @@ -166,6 +166,12 @@ func hasBytesType(pass *analysis.Pass, e ast.Expr) bool { return ok && el.Kind() == types.Uint8 } +// hasStringType returns true if the expression is of `string` type. +func hasStringType(pass *analysis.Pass, e ast.Expr) bool { + basicType, ok := pass.TypesInfo.TypeOf(e).(*types.Basic) + return ok && basicType.Kind() == types.String +} + // untype returns v from type(v) expression or v itself if there is no type conversion. func untype(e ast.Expr) ast.Expr { ce, ok := e.(*ast.CallExpr) diff --git a/internal/checkers/helpers_encoded.go b/internal/checkers/helpers_encoded.go index 35a497a7..c366f856 100644 --- a/internal/checkers/helpers_encoded.go +++ b/internal/checkers/helpers_encoded.go @@ -11,13 +11,15 @@ import ( ) var ( + wordsRe = regexp.MustCompile(`[A-Z]+(?:[a-z]*|$)|[a-z]+`) // NOTE(a.telyshev): ChatGPT. + jsonIdentRe = regexp.MustCompile(`json|JSON|Json`) - yamlIdentRe = regexp.MustCompile(`yaml|YAML|Yaml|yml|YML|Yml`) + yamlWordRe = regexp.MustCompile(`yaml|YAML|Yaml|^(yml|YML|Yml)$`) ) func isJSONStyleExpr(pass *analysis.Pass, e ast.Expr) bool { if isIdentNamedAfterPattern(jsonIdentRe, e) { - return true + return hasBytesType(pass, e) || hasStringType(pass, e) } if t, ok := pass.TypesInfo.Types[e]; ok && t.Value != nil { @@ -35,6 +37,20 @@ func isJSONStyleExpr(pass *analysis.Pass, e ast.Expr) bool { return false } -func isYAMLStyleExpr(e ast.Expr) bool { - return isIdentNamedAfterPattern(yamlIdentRe, e) +func isYAMLStyleExpr(pass *analysis.Pass, e ast.Expr) bool { + id, ok := e.(*ast.Ident) + return ok && (hasBytesType(pass, e) || hasStringType(pass, e)) && hasWordAfterPattern(id.Name, yamlWordRe) +} + +func hasWordAfterPattern(s string, re *regexp.Regexp) bool { + for _, w := range splitIntoWords(s) { + if re.MatchString(w) { + return true + } + } + return false +} + +func splitIntoWords(s string) []string { + return wordsRe.FindAllString(s, -1) }