Skip to content

fix(stale_repositories): fix 8 logical bugs causing false positives #6610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Liu-meng-hao wants to merge 1 commit into
base: main
Choose a base branch
from
+102 −80
Open

fix(stale_repositories): fix 8 logical bugs causing false positives #6610

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
182 changes: 102 additions & 80 deletions stale_repositories_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ import (
"context"
"encoding/json"
"fmt"
"log"
"io"
"net/http"
"net/url"
"os"
"regexp"
"strings"
Expand All @@ -26,27 +27,22 @@ const issueTemplateContent = `

var issueTemplate = template.Must(template.New("issue").Parse(issueTemplateContent))

// FIXME: use official github client
var reGithubRepo = regexp.MustCompile("https://github.com/([a-zA-Z0-9-._]+)/([a-zA-Z0-9-._]+)$")
var githubGETREPO = "https://api.github.com/repos%s"
var githubGETCOMMITS = "https://api.github.com/repos%s/commits"
var reGithubRepo = regexp.MustCompile(`^https://github\.com/([-a-zA-Z0-9_.]+)/([-a-zA-Z0-9_.]+)/?$`)
var githubGETREPO = "https://api.github.com/repos/%s/%s"
var githubGETCOMMITS = "https://api.github.com/repos/%s/%s/commits"
var githubPOSTISSUES = "https://api.github.com/repos/avelino/awesome-go/issues"
var awesomeGoGETISSUES = "https://api.github.com/repos/avelino/awesome-go/issues"

// FIXME: use https
var awesomeGoGETISSUES = "http://api.github.com/repos/avelino/awesome-go/issues" //only returns open issues
// FIXME: variable has type Duration, but contains a number. we should use
//
// time.Hour * ... or change type of variable
var numberOfYears time.Duration = 1
var numberOfYears = 1
var timeNow = time.Now()
var issueTitle = fmt.Sprintf("Investigate repositories with more than 1 year without update - %s", timeNow.Format(time.DateOnly))

const deadLinkMessage = " this repository might no longer exist! (status code >= 400 returned)"
const deadLinkMessage = " this repository might no longer exist! (status code 404/410 returned)"
const movedPermanently = " status code 301 received"
const status302 = " status code 302 received"
const archived = " repository has been archived"
const noRecentCommits = " repository has not received any commits in over 1 year"

// LIMIT specifies the max number of repositories that are added in a single run of the script
var LIMIT = 10
var ctr = 0

Expand Down Expand Up @@ -77,30 +73,26 @@ func getRepositoriesFromBody(body string) []string {
link = strings.ReplaceAll(link, movedPermanently, "")
link = strings.ReplaceAll(link, status302, "")
link = strings.ReplaceAll(link, archived, "")
link = strings.ReplaceAll(link, noRecentCommits, "")
links[i] = link
}

return links
}

func generateIssueBody(t *testing.T, repositories []string) (string, error) {
t.Helper()

buf := bytes.NewBuffer(nil)
err := issueTemplate.Execute(buf, repositories)
requireNoErr(t, err, "Failed to generate template")

return buf.String(), nil
}

func createIssue(t *testing.T, staleRepos []string, client *http.Client) {
t.Helper()

if len(staleRepos) == 0 {
log.Print("NO STALE REPOSITORIES")
t.Log("NO STALE REPOSITORIES")
return
}

body, err := generateIssueBody(t, staleRepos)
requireNoErr(t, err, "failed to generate issue body")

Expand All @@ -114,21 +106,29 @@ func createIssue(t *testing.T, staleRepos []string, client *http.Client) {
req, err := http.NewRequest(http.MethodPost, githubPOSTISSUES, buf)
requireNoErr(t, err, "failed to create request")

_, roundTripErr := client.Do(req)
resp, roundTripErr := client.Do(req)
requireNoErr(t, roundTripErr, "failed to send request")
defer resp.Body.Close()

if resp.StatusCode < 200 || resp.StatusCode >= 300 {
respBody, _ := io.ReadAll(resp.Body)
t.Fatalf("create issue failed: status %d, body: %s", resp.StatusCode, string(respBody))
}
}

func getAllFlaggedRepositories(t *testing.T, client *http.Client) map[string]bool {
t.Helper()

req, err := http.NewRequest(http.MethodGet, awesomeGoGETISSUES, nil)
requireNoErr(t, err, "failed to create request")

res, err := client.Do(req)
requireNoErr(t, err, "failed to send request")

defer res.Body.Close()

if res.StatusCode == http.StatusForbidden || res.StatusCode == http.StatusUnauthorized {
t.Fatalf("getAllFlaggedRepositories: rate limit or auth failure (status %d)", res.StatusCode)
}

var issues []issue
requireNoErr(t, json.NewDecoder(res.Body).Decode(&issues), "failed to unmarshal response")

Expand All @@ -137,90 +137,92 @@ func getAllFlaggedRepositories(t *testing.T, client *http.Client) map[string]boo
if issue.Title != issueTitle {
continue
}

repos := getRepositoriesFromBody(issue.Body)
for _, repo := range repos {
addressedRepositories[repo] = true
}
}

return addressedRepositories
}

func checkRepoAvailability(toRun bool, href string, client *http.Client) ([]string, bool) {
func checkRepoAvailability(t *testing.T, toRun bool, href string, client *http.Client) (warnings []string, aborted bool) {
t.Helper()
if !toRun {
return nil, false
}

ownerRepo := strings.ReplaceAll(href, "https://github.com", "")
apiCall := fmt.Sprintf(githubGETREPO, ownerRepo)
matches := reGithubRepo.FindStringSubmatch(href)
if len(matches) < 3 {
t.Logf("Invalid github repo url format: %s", href)
return nil, false
}
owner, repo := matches[1], matches[2]
apiCall := fmt.Sprintf(githubGETREPO, owner, repo)

req, err := http.NewRequest(http.MethodGet, apiCall, nil)
if err != nil {
log.Printf("Failed at repository %s\n", href)
t.Logf("Failed to create request for repository %s\n", href)
return nil, false
}

resp, err := client.Do(req)
if err != nil {
log.Printf("Failed at repository %s\n", href)
t.Logf("Failed at repository %s\n", href)
return nil, false
}

defer resp.Body.Close()

if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusUnauthorized {
t.Logf("GitHub API rate limit exceeded or unauthorized (Status: %d). Signal abort.", resp.StatusCode)
return nil, true
}

var repoResp struct {
Archived bool `json:"archived"`
}

if err := json.NewDecoder(resp.Body).Decode(&repoResp); err != nil {
return nil, false
}

var isRepoAdded bool

var warnings []string
if resp.StatusCode == http.StatusMovedPermanently {
warnings = append(warnings, href+movedPermanently)
log.Printf("%s returned %d", href, resp.StatusCode)
isRepoAdded = true
}

if resp.StatusCode == http.StatusFound && !isRepoAdded {
t.Logf("%s returned %d", href, resp.StatusCode)
} else if resp.StatusCode == http.StatusFound {
warnings = append(warnings, href+status302)
log.Printf("%s returned %d", href, resp.StatusCode)
isRepoAdded = true
}

if resp.StatusCode >= http.StatusBadRequest && !isRepoAdded {
t.Logf("%s returned %d", href, resp.StatusCode)
} else if resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusGone {
warnings = append(warnings, href+deadLinkMessage)
log.Printf("%s might not exist!", href)
isRepoAdded = true
t.Logf("%s might not exist! (Status: %d)", href, resp.StatusCode)
}

if repoResp.Archived && !isRepoAdded {
if repoResp.Archived && len(warnings) == 0 {
warnings = append(warnings, href+archived)
log.Printf("%s is archived!", href)
isRepoAdded = true
t.Logf("%s is archived!", href)
}

// FIXME: expression `(len(warnings) > 0) == isRepoAdded` is always true.
return warnings, isRepoAdded
return warnings, false
}

func checkRepoCommitActivity(toRun bool, href string, client *http.Client) ([]string, bool) {
func checkRepoCommitActivity(t *testing.T, toRun bool, href string, client *http.Client) (warnings []string, aborted bool) {
t.Helper()
if !toRun {
return nil, false
}

ownerRepo := strings.ReplaceAll(href, "https://github.com", "")
apiCall := fmt.Sprintf(githubGETCOMMITS, ownerRepo)
matches := reGithubRepo.FindStringSubmatch(href)
if len(matches) < 3 {
return nil, false
}
owner, repo := matches[1], matches[2]
apiCall := fmt.Sprintf(githubGETCOMMITS, owner, repo)

req, err := http.NewRequest(http.MethodGet, apiCall, nil)
if err != nil {
log.Printf("Failed at repository %s\n", href)
t.Logf("Failed to create request for repository %s\n", href)
return nil, false
}

since := timeNow.Add(-1 * 365 * 24 * numberOfYears * time.Hour)
since := timeNow.Add(-1 * time.Duration(numberOfYears) * 365 * 24 * time.Hour)
sinceQuery := since.Format(time.RFC3339)

q := req.URL.Query()
Expand All @@ -229,29 +231,27 @@ func checkRepoCommitActivity(toRun bool, href string, client *http.Client) ([]st

resp, err := client.Do(req)
if err != nil {
log.Printf("Failed at repository %s\n", href)
t.Logf("Failed at repository %s\n", href)
return nil, false
}

defer resp.Body.Close()

if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusUnauthorized {
t.Logf("GitHub API rate limit exceeded or unauthorized (Status: %d). Signal abort.", resp.StatusCode)
return nil, true
}

var respObj []map[string]interface{}
// FIXME: handle error in all that cases
if err := json.NewDecoder(resp.Body).Decode(&respObj); err != nil {
return nil, false
}

var warnings []string
var isRepoAdded bool
isAged := len(respObj) == 0
if isAged {
log.Printf("%s has not had a commit in a while", href)
warnings = append(warnings, href)
isRepoAdded = true
if len(respObj) == 0 {
t.Logf("%s has not had a commit in a while", href)
warnings = append(warnings, href+noRecentCommits)
}

// FIXME: expression `(len(warnings) > 0) == isRepoAdded` is always true.
return warnings, isRepoAdded
return warnings, false
}

func TestStaleRepository(t *testing.T) {
Expand All @@ -263,15 +263,14 @@ func TestStaleRepository(t *testing.T) {
}

if oauth == "" {
log.Print("No oauth token found. Using unauthenticated client ...")
t.Log("No oauth token found. Using unauthenticated client ...")
} else {
tokenSource := &tokenSource{
AccessToken: oauth,
}
client = oauth2.NewClient(context.Background(), tokenSource)
}

// FIXME: return addressedRepositories, no need to pass
addressedRepositories := getAllFlaggedRepositories(t, client)

var staleRepos []string
Expand All @@ -280,32 +279,55 @@ func TestStaleRepository(t *testing.T) {
EachWithBreak(func(_ int, s *goquery.Selection) bool {
href, ok := s.Attr("href")
if !ok {
log.Println("expected to have href")
t.Log("expected to have href")
return true
}

if strings.HasPrefix(href, "#") {
return true
}

u, parseErr := url.Parse(href)
if parseErr == nil {
u.RawQuery = ""
u.Fragment = ""
href = u.String()
}

if ctr >= LIMIT && LIMIT != -1 {
log.Print("Max number of issues created")
t.Log("Max number of issues created")
return false
}

if _, issueExists := addressedRepositories[href]; issueExists {
log.Printf("issue already exists for %s\n", href)
t.Logf("issue already exists for %s\n", href)
return true
}

if !reGithubRepo.MatchString(href) {
log.Printf("%s non-github repo not currently handled", href)
t.Logf("%s non-github repo not currently handled", href)
return true
}

// FIXME: this is `or` expres24sion. Probably we need `and`?
warnings, isRepoAdded := checkRepoAvailability(true, href, client)
staleRepos = append(staleRepos, warnings...)
var aborted bool
var availWarnings []string
availWarnings, aborted = checkRepoAvailability(t, true, href, client)
if aborted {
t.Logf("Scan aborted due to API limits. Proceeding to create issue with %d records.", len(staleRepos))
return false
}
staleRepos = append(staleRepos, availWarnings...)

warnings, isRepoAdded = checkRepoCommitActivity(!isRepoAdded, href, client)
staleRepos = append(staleRepos, warnings...)
isRepoAdded := len(availWarnings) > 0
var commitWarnings []string
commitWarnings, aborted = checkRepoCommitActivity(t, !isRepoAdded, href, client)
if aborted {
t.Logf("Scan aborted due to API limits. Proceeding to create issue with %d records.", len(staleRepos))
return false
}
staleRepos = append(staleRepos, commitWarnings...)

if isRepoAdded {
if len(availWarnings)+len(commitWarnings) > 0 {
ctr++
}

Expand Down
Loading