Enable stricter linting with golangci-lint (#316)

* update golangci-lint

* add golangci config file w/ more linters

* correct issues flagged by stricter linters

* add more generous timeout for golangci-lint

* add some style + formatting guidelines

* move timeout to config file

* go fmt

.drone.yml

@@ -13,7 +13,7 @@ steps:
 # We use golangci-lint for linting.
 # See: https://golangci-lint.run/
 - name: lint
-  image: golangci/golangci-lint:v1.42.1
+  image: golangci/golangci-lint:v1.43.0
   volumes:
   - name: go-build-cache
     path: /root/.cache/go-build
@@ -22,7 +22,7 @@ steps:
   - name: go-src
     path: /go
   commands:
-  - golangci-lint run --timeout 5m0s --tests=false --verbose
+  - golangci-lint run
   when:
     event:
       include:
@@ -115,6 +115,6 @@ trigger:
 
 ---
 kind: signature
-hmac: aa44c4655421fb0ed3141b8d7255a9a240dd4312244081d9e95929c4a196fd92
+hmac: 07d6ed18510f9591c6b347d6768cbe8e613561b3759f1a8dda8721d1d231a522
 
 ...

.golangci.yml

@@ -0,0 +1,26 @@
+# Configuration file for golangci-lint linter.
+# This will be automatically picked up when golangci-lint is invoked.
+# For all config options, see https://golangci-lint.run/usage/configuration/#config-file
+#
+# For GoToSocial we mostly take the default linters, but we add a few to catch style issues as well.
+
+# options for analysis running
+run:
+  # include test files or not, default is true
+  tests: false
+  # timeout for analysis, e.g. 30s, 5m, default is 1m
+  timeout: 5m
+
+linters:
+  # enable some extra linters, see here for the list: https://golangci-lint.run/usage/linters/
+  enable:
+    - contextcheck
+    - forcetypeassert
+    - goconst
+    - gocritic
+    - gofmt
+    - gosec
+    - ifshort
+    - nilerr
+    - revive
+    - wastedassign

CONTRIBUTING.md

@@ -21,7 +21,9 @@ Check the [issues](https://github.com/superseriousbusiness/gotosocial/issues) to
     - [SQLite](#sqlite)
     - [Postgres](#postgres)
     - [Both](#both)
-- [Linting](#linting)
+- [Project Structure](#project-structure)
+- [Style](#style)
+- [Linting and Formatting](#linting-and-formatting)
 - [Updating Swagger docs](#updating-swagger-docs)
 - [CI/CD configuration](#cicd-configuration)
 - [Building releases and Docker containers](#building-releases-and-docker-containers)
@@ -179,33 +181,48 @@ Finally, to run tests against both database types one after the other, use:
 ./scripts/test.sh
 ```
 
-## Linting
+## Project Structure
 
-We use [golangci-lint](https://golangci-lint.run/) for linting. To run this locally, first install the linter following the instructions [here](https://golangci-lint.run/usage/install/#local-installation).
+For project structure, GoToSocial follows a standard and widely-accepted project layout [defined here](https://github.com/golang-standards/project-layout). As the author writes:
 
-Then, you can run the linter with:
+> This is a basic layout for Go application projects. It's not an official standard defined by the core Go dev team; however, it is a set of common historical and emerging project layout patterns in the Go ecosystem.
 
-```bash
-golangci-lint run --tests=false
-```
+Most of the crucial business logic of the application is found inside the various packages and subpackages of the `internal` directory.
 
-Note that this linter also runs as a job on the Github repo, so if you make a PR that doesn't pass the linter, it will be rejected. As such, it's good practice to run the linter locally before pushing or opening a PR.
+Where possible, we prefer more files and packages of shorter length that very clearly pertain to definable chunks of application logic, rather than fewer but longer files: if one `.go` file is pushing 1,000 lines of code, it's probably too long.
 
-Another useful linter is [golint](https://pkg.go.dev/github.com/360EntSecGroup-Skylar/goreporter/linters/golint), which catches some style issues that golangci-lint does not.
+## Style
 
-To install golint, run:
+It is a good idea to read the short official [Effective Go](https://golang.org/doc/effective_go) page before submitting code: this document is the foundation of many a style guide, for good reason, and GoToSocial more or less follows its advice.
 
-```bash
-go get -u github.com/golang/lint/golint
-```
+Another useful style guide that we try to follow: [this one](https://github.com/bahlo/go-styleguide).
+
+In addition, here are some specific highlights from Uber's Go style guide which agree with what we try to do in GtS:
+
+- [Group Similar Declarations](https://github.com/uber-go/guide/blob/master/style.md#group-similar-declarations).
+- [Reduce Nesting](https://github.com/uber-go/guide/blob/master/style.md#reduce-nesting).
+- [Unnecessary Else](https://github.com/uber-go/guide/blob/master/style.md#unnecessary-else).
+- [Local Variable Declarations](https://github.com/uber-go/guide/blob/master/style.md#local-variable-declarations).
+- [Reduce Scope of Variables](https://github.com/uber-go/guide/blob/master/style.md#reduce-scope-of-variables).
+- [Initializing Structs](https://github.com/uber-go/guide/blob/master/style.md#initializing-structs).
 
-To run the linter, use:
+## Linting and Formatting
+
+Before you submit any code, make sure to run `go fmt ./...` to update whitespace and other opinionated formatting.
+
+We use [golangci-lint](https://golangci-lint.run/) for linting, which allows us to catch style inconsistencies and potential bugs or security issues, using static code analysis.
+
+If you make a PR that doesn't pass the linter, it will be rejected. As such, it's good practice to run the linter locally before pushing or opening a PR.
+
+To do this, first install the linter following the instructions [here](https://golangci-lint.run/usage/install/#local-installation).
+
+Then, you can run the linter with:
 
 ```bash
-golint ./internal/...
+golangci-lint run
 ```
 
-Then make sure to run `go fmt ./...` to update whitespace and other opinionated formatting.
+If there's no output, great! It passed :)
 
 ## Updating Swagger docs
 

cmd/gotosocial/main.go

@@ -49,8 +49,7 @@ func main() {
 		Commands: getCommands(),
 	}
 
-	err := app.Run(os.Args)
-	if err != nil {
+	if err := app.Run(os.Args); err != nil {
 		logrus.Fatal(err)
 	}
 }

internal/ap/activitystreams.go

@@ -67,6 +67,6 @@ const (
 	ObjectRelationship   = "Relationship"   // ActivityStreamsRelationship https://www.w3.org/TR/activitystreams-vocabulary/#dfn-relationship
 	ObjectTombstone      = "Tombstone"      // ActivityStreamsTombstone https://www.w3.org/TR/activitystreams-vocabulary/#dfn-tombstone
 	ObjectVideo          = "Video"          // ActivityStreamsVideo https://www.w3.org/TR/activitystreams-vocabulary/#dfn-video
-	ObjectCollection     = "Collection"     //ActivityStreamsCollection https://www.w3.org/TR/activitystreams-vocabulary/#dfn-collection
+	ObjectCollection     = "Collection"     // ActivityStreamsCollection https://www.w3.org/TR/activitystreams-vocabulary/#dfn-collection
 	ObjectCollectionPage = "CollectionPage" // ActivityStreamsCollectionPage https://www.w3.org/TR/activitystreams-vocabulary/#dfn-collectionpage
 )

internal/api/client/app/appcreate.go

@@ -20,14 +20,22 @@ package app
 
 import (
 	"fmt"
-	"github.com/sirupsen/logrus"
 	"net/http"
 
+	"github.com/sirupsen/logrus"
+
 	"github.com/gin-gonic/gin"
 	"github.com/superseriousbusiness/gotosocial/internal/api/model"
 	"github.com/superseriousbusiness/gotosocial/internal/oauth"
 )
 
+const (
+	// permitted length for most fields
+	formFieldLen = 64
+	// redirect can be a bit bigger because we probably need to encode data in the redirect uri
+	formRedirectLen = 512
+)
+
 // AppsPOSTHandler swagger:operation POST /api/v1/apps appCreate
 //
 // Register a new application on this instance.
@@ -79,11 +87,6 @@ func (m *Module) AppsPOSTHandler(c *gin.Context) {
 		return
 	}
 
-	// permitted length for most fields
-	formFieldLen := 64
-	// redirect can be a bit bigger because we probably need to encode data in the redirect uri
-	formRedirectLen := 512
-
 	// check lengths of fields before proceeding so the user can't spam huge entries into the database
 	if len(form.ClientName) > formFieldLen {
 		c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("client_name must be less than %d bytes", formFieldLen)})

internal/api/client/auth/auth.go

@@ -29,6 +29,7 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/router"
 )
 
+/* #nosec G101 */
 const (
 	// AuthSignInPath is the API path for users to sign in through
 	AuthSignInPath = "/auth/sign_in"

internal/api/client/auth/callback.go

@@ -182,15 +182,15 @@ func (m *Module) parseUserFromClaims(ctx context.Context, claims *oidc.Claims, i
 	//
 	// note that for the first iteration, iString is still "" when the check is made, so our first choice
 	// is still the raw username with no integer stuck on the end
-	for i := 1; !found; i = i + 1 {
+	for i := 1; !found; i++ {
 		usernameAvailable, err := m.db.IsUsernameAvailable(ctx, username+iString)
 		if err != nil {
 			return nil, err
 		}
 		if usernameAvailable {
 			// no error so we've found a username that works
 			found = true
-			username = username + iString
+			username += iString
 			continue
 		}
 		iString = strconv.Itoa(i)

internal/api/client/status/status.go

@@ -19,10 +19,11 @@
 package status
 
 import (
-	"github.com/sirupsen/logrus"
 	"net/http"
 	"strings"
 
+	"github.com/sirupsen/logrus"
+
 	"github.com/gin-gonic/gin"
 	"github.com/superseriousbusiness/gotosocial/internal/api"
 	"github.com/superseriousbusiness/gotosocial/internal/config"
@@ -110,13 +111,13 @@ func (m *Module) muxHandler(c *gin.Context) {
 	logrus.Debug("entering mux handler")
 	ru := c.Request.RequestURI
 
-	switch c.Request.Method {
-	case http.MethodGet:
-		if strings.HasPrefix(ru, ContextPath) {
+	if c.Request.Method == http.MethodGet {
+		switch {
+		case strings.HasPrefix(ru, ContextPath):
 			// TODO
-		} else if strings.HasPrefix(ru, FavouritedPath) {
+		case strings.HasPrefix(ru, FavouritedPath):
 			m.StatusFavedByGETHandler(c)
-		} else {
+		default:
 			m.StatusGETHandler(c)
 		}
 	}

internal/api/s2s/user/outboxget.go

@@ -90,9 +90,8 @@ func (m *Module) OutboxGETHandler(c *gin.Context) {
 		return
 	}
 
-	page := false
-	pageString := c.Query(PageKey)
-	if pageString != "" {
+	var page bool
+	if pageString := c.Query(PageKey); pageString != "" {
 		i, err := strconv.ParseBool(pageString)
 		if err != nil {
 			l.Debugf("error parsing page string: %s", err)

internal/api/s2s/user/repliesget.go

@@ -102,9 +102,8 @@ func (m *Module) StatusRepliesGETHandler(c *gin.Context) {
 		return
 	}
 
-	page := false
-	pageString := c.Query(PageKey)
-	if pageString != "" {
+	var page bool
+	if pageString := c.Query(PageKey); pageString != "" {
 		i, err := strconv.ParseBool(pageString)
 		if err != nil {
 			l.Debugf("error parsing page string: %s", err)

internal/api/security/useragentblock.go

@@ -31,8 +31,7 @@ func (m *Module) UserAgentBlock(c *gin.Context) {
 		"func": "UserAgentBlock",
 	})
 
-	ua := c.Request.UserAgent()
-	if ua == "" {
+	if ua := c.Request.UserAgent(); ua == "" {
 		l.Debug("aborting request because there's no user-agent set")
 		c.AbortWithStatus(http.StatusTeapot)
 		return

internal/cache/account.go

@@ -4,6 +4,7 @@ import (
 	"sync"
 
 	"github.com/ReneKroon/ttlcache"
+	"github.com/sirupsen/logrus"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 )
 
@@ -26,7 +27,10 @@ func NewAccountCache() *AccountCache {
 
 	// Set callback to purge lookup maps on expiration
 	c.cache.SetExpirationCallback(func(key string, value interface{}) {
-		account := value.(*gtsmodel.Account)
+		account, ok := value.(*gtsmodel.Account)
+		if !ok {
+			logrus.Panicf("AccountCache could not assert entry with key %s to *gtsmodel.Account", key)
+		}
 
 		c.mutex.Lock()
 		delete(c.urls, account.URL)

internal/cache/status.go

@@ -4,6 +4,7 @@ import (
 	"sync"
 
 	"github.com/ReneKroon/ttlcache"
+	"github.com/sirupsen/logrus"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 )
 
@@ -26,7 +27,10 @@ func NewStatusCache() *StatusCache {
 
 	// Set callback to purge lookup maps on expiration
 	c.cache.SetExpirationCallback(func(key string, value interface{}) {
-		status := value.(*gtsmodel.Status)
+		status, ok := value.(*gtsmodel.Status)
+		if !ok {
+			logrus.Panicf("StatusCache could not assert entry with key %s to *gtsmodel.Status", key)
+		}
 
 		c.mutex.Lock()
 		delete(c.urls, status.URL)

internal/config/db.go

@@ -46,4 +46,4 @@ var DBTLSModeEnable DBTLSMode = "enable"
 var DBTLSModeRequire DBTLSMode = "require"
 
 // DBTLSModeUnset means that the TLS mode has not been set.
-var DBTLSModeUnset DBTLSMode = ""
+var DBTLSModeUnset DBTLSMode

internal/config/default.go

@@ -180,8 +180,8 @@ func GetDefaults() Defaults {
 		AccountsRequireApproval:  true,
 		AccountsReasonRequired:   true,
 
-		MediaMaxImageSize:        2097152,  //2mb
-		MediaMaxVideoSize:        10485760, //10mb
+		MediaMaxImageSize:        2097152,  // 2mb
+		MediaMaxVideoSize:        10485760, // 10mb
 		MediaMinDescriptionChars: 0,
 		MediaMaxDescriptionChars: 500,
 
@@ -244,8 +244,8 @@ func GetTestDefaults() Defaults {
 		AccountsRequireApproval:  true,
 		AccountsReasonRequired:   true,
 
-		MediaMaxImageSize:        1048576, //1mb
-		MediaMaxVideoSize:        5242880, //5mb
+		MediaMaxImageSize:        1048576, // 1mb
+		MediaMaxVideoSize:        5242880, // 5mb
 		MediaMinDescriptionChars: 0,
 		MediaMaxDescriptionChars: 500,
 

internal/db/bundb/account.go

@@ -137,8 +137,7 @@ func (a *accountDB) GetInstanceAccount(ctx context.Context, domain string) (*gts
 			WhereGroup(" AND ", whereEmptyOrNull("domain"))
 	}
 
-	err := q.Scan(ctx)
-	if err != nil {
+	if err := q.Scan(ctx); err != nil {
 		return nil, a.conn.ProcessError(err)
 	}
 	return account, nil
@@ -155,8 +154,7 @@ func (a *accountDB) GetAccountLastPosted(ctx context.Context, accountID string)
 		Where("account_id = ?", accountID).
 		Column("created_at")
 
-	err := q.Scan(ctx)
-	if err != nil {
+	if err := q.Scan(ctx); err != nil {
 		return time.Time{}, a.conn.ProcessError(err)
 	}
 	return status.CreatedAt, nil
@@ -168,11 +166,12 @@ func (a *accountDB) SetAccountHeaderOrAvatar(ctx context.Context, mediaAttachmen
 	}
 
 	var headerOrAVI string
-	if mediaAttachment.Avatar {
+	switch {
+	case mediaAttachment.Avatar:
 		headerOrAVI = "avatar"
-	} else if mediaAttachment.Header {
+	case mediaAttachment.Header:
 		headerOrAVI = "header"
-	} else {
+	default:
 		return errors.New("given media attachment was neither a header nor an avatar")
 	}
 
@@ -202,8 +201,7 @@ func (a *accountDB) GetLocalAccountByUsername(ctx context.Context, username stri
 		Where("username = ?", username).
 		WhereGroup(" AND ", whereEmptyOrNull("domain"))
 
-	err := q.Scan(ctx)
-	if err != nil {
+	if err := q.Scan(ctx); err != nil {
 		return nil, a.conn.ProcessError(err)
 	}
 	return account, nil
@@ -308,8 +306,7 @@ func (a *accountDB) GetAccountBlocks(ctx context.Context, accountID string, maxI
 		fq = fq.Limit(limit)
 	}
 
-	err := fq.Scan(ctx)
-	if err != nil {
+	if err := fq.Scan(ctx); err != nil {
 		return nil, "", "", a.conn.ProcessError(err)
 	}