chore:more strict linting (#206)

* bumped the staticcheck version we are tested against

* migrated from `staticcheck` to `golangci-lint` (includes `staticcheck` and other linters)

* renamed the workflow file from `staticcheck` to `ci`

* migrated to go 1.21

* fixed linting not running

* fixed defects the new logger turned up

* fixed more issues turned up by the linter

* fixed more issues turned up by the linter

* fixed another bug

* Apply suggestions from code review

.github/workflows/ci.yml

@@ -0,0 +1,29 @@
+name: CI
+on:
+  push:
+    branches: [ main ]
+  pull_request:
+
+permissions:
+  contents: read
+
+jobs:
+  ci:
+    name: lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-go@v4
+        with:
+          go-version: '1.21'
+          cache: false
+      - name: golangci-lint-server
+        uses: golangci/golangci-lint-action@v3
+        with:
+          version: v1.54
+          working-directory: server
+      - name: golangci-lint-client
+        uses: golangci/golangci-lint-action@v3
+        with:
+          version: v1.54
+          working-directory: client

client/go.mod

@@ -1,6 +1,6 @@
 module github.com/TUM-Dev/Campus-Backend/client
 
-go 1.18
+go 1.21
 
 require (
 	github.com/TUM-Dev/Campus-Backend/api v0.0.0-20221212204029-68b05b451617

client/go.sum

@@ -12,10 +12,13 @@ github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg
 github.com/golang/protobuf v1.5.3/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY=
 github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
+github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/grpc-ecosystem/grpc-gateway/v2 v2.11.3 h1:lLT7ZLSzGLI08vc9cpd+tYmNWjdKDqyr/2L+f6U12Fk=
 github.com/grpc-ecosystem/grpc-gateway/v2 v2.11.3/go.mod h1:o//XUCC/F+yRGJoPO/VU0GSB0f8Nhgmxx0VIRUvaC0w=
 github.com/kr/pretty v0.1.0 h1:L/CwN0zerZDmRFUapSPitk6f+Q3+0za1rQkzVuMiMFI=
+github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
+github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/sirupsen/logrus v1.9.3 h1:dueUQJ1C2q9oE3F7wvmSGAaVtTmUizReu6fjN8uqzbQ=
@@ -48,6 +51,7 @@ google.golang.org/protobuf v1.31.0 h1:g0LDEJHgrBl9N9r17Ru3sqWhkIx2NB67okBHPwC7hs
 google.golang.org/protobuf v1.31.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY=
+gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
 gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

server/Dockerfile

@@ -1,4 +1,4 @@
-FROM golang:1.19.3-alpine3.15 as builder
+FROM golang:1.21-alpine3.18 as builder
 
 # Install git + SSL ca certificates.
 # Git is required for fetching the dependencies.

server/backend/cafeteriaRatingDBInitializer.go

@@ -103,21 +103,22 @@ func updateNameTagOptions(db *gorm.DB) {
 
 func addNotIncluded(parentId int32, db *gorm.DB, v nameTag) {
 	var count int64
-	for _, u := range v.NotIncluded {
-		errorLoadingIncluded := db.Model(&model.DishNameTagOptionExcluded{}).
-			Where("expression LIKE ? AND NameTagID = ?", u, parentId).
+	for _, expression := range v.NotIncluded {
+		fields := log.Fields{"expression": expression, "parentId": parentId}
+		err := db.Model(&model.DishNameTagOptionExcluded{}).
+			Where("expression LIKE ? AND NameTagID = ?", expression, parentId).
 			Select("DishNameTagOptionExcluded").
 			Count(&count).Error
-		if errorLoadingIncluded != nil {
-			log.WithError(errorLoadingIncluded).Errorf("Unable to load can be excluded tag with expression %s and parentId %s", u, parentId)
+		if err != nil {
+			log.WithError(err).WithFields(fields).Error("Unable to load can be excluded tag")
 		} else {
 			if count == 0 {
-				createError := db.Model(&model.DishNameTagOptionExcluded{}).
+				err := db.Model(&model.DishNameTagOptionExcluded{}).
 					Create(&model.DishNameTagOptionExcluded{
-						Expression: u,
+						Expression: expression,
 						NameTagID:  parentId}).Error
-				if createError != nil {
-					log.WithError(errorLoadingIncluded).Error("Unable to create new can be excluded tag with expression {} and parentId {} ", u, parentId)
+				if err != nil {
+					log.WithError(err).WithFields(fields).Error("Unable to create new can be excluded tag")
 				}
 			}
 		}
@@ -126,22 +127,23 @@ func addNotIncluded(parentId int32, db *gorm.DB, v nameTag) {
 
 func addCanBeIncluded(parentId int32, db *gorm.DB, v nameTag) {
 	var count int64
-	for _, u := range v.CanBeIncluded {
-		errorLoadingIncluded := db.Model(&model.DishNameTagOptionIncluded{}).
-			Where("expression LIKE ? AND NameTagID = ?", u, parentId).
+	for _, expression := range v.CanBeIncluded {
+		fields := log.Fields{"expression": expression, "parentId": parentId}
+		err := db.Model(&model.DishNameTagOptionIncluded{}).
+			Where("expression LIKE ? AND NameTagID = ?", expression, parentId).
 			Select("DishNameTagOptionIncluded").
 			Count(&count).Error
-		if errorLoadingIncluded != nil {
-			log.WithError(errorLoadingIncluded).Errorf("Unable to load can be included tag with expression %s and parentId %s", u, parentId)
+		if err != nil {
+			log.WithError(err).WithFields(fields).Error("Unable to load can be included tag")
 		} else {
 			if count == 0 {
-				createError := db.Model(&model.DishNameTagOptionIncluded{}).
+				err := db.Model(&model.DishNameTagOptionIncluded{}).
 					Create(&model.DishNameTagOptionIncluded{
-						Expression: u,
+						Expression: expression,
 						NameTagID:  parentId,
 					}).Error
-				if createError != nil {
-					log.WithError(errorLoadingIncluded).Errorf("Unable to create new can be excluded tag with expression %s and parentId %s", u, parentId)
+				if err != nil {
+					log.WithError(err).WithFields(fields).Error("Unable to create new can be excluded tag")
 				}
 			}
 		}

server/backend/cafeteriaService.go

@@ -155,7 +155,8 @@ func (s *CampusServer) GetDishRatings(_ context.Context, input *pb.DishRatingReq
 		First(&result)
 
 	if err.Error != nil {
-		log.WithError(err.Error).Errorf("Error while querying the average ratings for the dish %s in the cafeteria %s.", dishID, cafeteriaID)
+		fields := log.Fields{"dishID": dishID, "cafeteriaID": cafeteriaID}
+		log.WithError(err.Error).WithFields(fields).Error("Error while querying the average ratings")
 		return nil, status.Errorf(codes.Internal, "This dish has not yet been rated.")
 	}
 

server/backend/cron/canteenHeadCount.go

@@ -143,8 +143,12 @@ func (c *CronService) canteenHeadCountCron() error {
 		}
 
 		count := sumApCounts(aps)
-		updateDb(&canteen, count, c.db)
-		log.Debug("Canteen head count stats (", count, ") updated for: ", canteen.CanteenId)
+		fields := log.Fields{"count": count, "CanteenId": canteen.CanteenId}
+		if err := updateDb(&canteen, count, c.db); err != nil {
+			log.WithFields(fields).WithError(err).Error("Failed to update Canteen head count stats")
+		} else {
+			log.WithFields(fields).Debug("Canteen head count stats updated")
+		}
 	}
 	log.Info("Canteen head count stats updated.")
 	return nil

server/backend/cron/cronjobs.go

@@ -84,10 +84,13 @@ func (c *CronService) Run() error {
 			cronjob.LastRun = int32(time.Now().Unix()) + offset
 			c.db.Save(&cronjob)
 
-			// Run each job in a separate goroutine so we can parallelize them
+			// Run each job in a separate goroutine, so we can parallelize them
 			switch cronjob.Type.String {
 			case NewsType:
-				g.Go(func() error { return c.newsCron(&cronjob) })
+				// if this is not copied here, this may not be threads save due to go's guarantees
+				// loop variable cronjob captured by func literal (govet)
+				copyCronjob := cronjob
+				g.Go(func() error { return c.newsCron(&copyCronjob) })
 			case FileDownloadType:
 				g.Go(func() error { return c.fileDownloadCron() })
 			case DishNameDownload:
@@ -122,9 +125,8 @@ func (c *CronService) Run() error {
 			}
 		}
 
-		err := g.Wait()
-		if err != nil {
-			log.Println("Couldn't run all cron jobs: %v", err)
+		if err := g.Wait(); err != nil {
+			log.WithError(err).Println("Couldn't run all cron jobs")
 		}
 		log.Trace("Cron: sleeping for 60 seconds")
 		time.Sleep(60 * time.Second)

server/backend/cron/dishNameDownload.go

@@ -202,14 +202,15 @@ func addDishTagsToMapping(dishID int32, dishName string, db *gorm.DB) {
 		}
 	}
 
-	for _, a := range includedTags {
-		if a != -1 {
+	for _, nametagID := range includedTags {
+		if nametagID != -1 {
 			err := db.Model(&model.DishToDishNameTag{}).Create(&model.DishToDishNameTag{
 				DishID:    dishID,
-				NameTagID: a,
+				NameTagID: nametagID,
 			}).Error
 			if err != nil {
-				log.WithError(err).Errorf("Error while creating a new entry with dish %s and nametag %s", dishID, a)
+				fields := log.Fields{"dishID": dishID, "nametagID": nametagID}
+				log.WithError(err).WithFields(fields).Error("creating a new entry")
 			}
 		}
 	}

server/backend/cron/news.go

@@ -3,7 +3,6 @@ package cron
 import (
 	"crypto/md5"
 	"database/sql"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"github.com/TUM-Dev/Campus-Backend/server/model"
@@ -31,8 +30,14 @@ var ImageContentTypeRegex, _ = regexp.Compile("image/[a-z.]+")
 func (c *CronService) newsCron(cronjob *model.Crontab) error {
 	//check if source id provided for news job is not null
 	if !cronjob.ID.Valid {
-		cronjobJson, _ := json.Marshal(cronjob)
-		log.Println("skipping news job, id of source is null, cronjob: %s", string(cronjobJson))
+		fields := log.Fields{
+			"Cron":     cronjob.Cron,
+			"Interval": cronjob.Interval,
+			"LastRun":  cronjob.LastRun,
+			"Type":     cronjob.Type,
+			"ID":       cronjob.ID,
+		}
+		log.WithFields(fields).Warn("skipping news job, id of source is null")
 		return nil
 	}
 	// get news source for cronjob

server/backend/ios_notifications/ios_scheduling/iosSchedulingService.go

@@ -97,14 +97,13 @@ func (service *Service) handleDevices(devices []model.IOSDeviceLastUpdated) {
 
 func (service *Service) handleDevicesChunk(devices []model.IOSDeviceLastUpdated) {
 	for _, device := range devices {
-		err := service.APNs.RequestGradeUpdateForDevice(device.DeviceID)
-
-		if err != nil {
-			log.Errorf("Error while handling device: %s", err)
+		if err := service.APNs.RequestGradeUpdateForDevice(device.DeviceID); err != nil {
+			log.WithError(err).Error("could not RequestGradeUpdateForDevice")
 			continue
 		}
-
-		service.LogScheduledUpdate(device.DeviceID)
+		if err := service.LogScheduledUpdate(device.DeviceID); err != nil {
+			log.WithError(err).WithField("deviceID", device.DeviceID).Error("could not log scheduled update")
+		}
 	}
 }
 

server/backend/rpcserver.go

@@ -153,7 +153,7 @@ func (s *CampusServer) GetTopNews(ctx context.Context, _ *emptypb.Empty) (*pb.Ge
 	var res *model.NewsAlert
 	err := s.db.Joins("Company").Where("NOW() between `from` and `to`").Limit(1).First(&res).Error
 	if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) {
-		log.Errorf("Failed to fetch top news: %w", err)
+		log.WithError(err).Errorf("Failed to fetch top news")
 	} else if res != nil {
 		return &pb.GetTopNewsReply{
 			//ImageUrl: res.Name,

server/go.mod

@@ -1,6 +1,6 @@
 module github.com/TUM-Dev/Campus-Backend/server
 
-go 1.18
+go 1.21
 
 require (
 	github.com/disintegration/imaging v1.6.2