cleanup minio-go code, add gocritic linter (#1586)

.golangci.yml

@@ -14,3 +14,14 @@ linters:
     - gosimple
     - deadcode
     - structcheck
+    - gocritic
+
+issues:
+  exclude-use-default: false
+  exclude:
+      # todo fix these when we get enough time.
+      - "singleCaseSwitch: should rewrite switch statement to if statement"
+      - "unlambda: replace"
+      - "captLocal:"
+      - "ifElseChain:"
+      - "elseif:"

api-get-options.go

@@ -25,7 +25,7 @@ import (
 	"github.com/minio/minio-go/v7/pkg/encrypt"
 )
 
-//AdvancedGetOptions for internal use by MinIO server - not intended for client use.
+// AdvancedGetOptions for internal use by MinIO server - not intended for client use.
 type AdvancedGetOptions struct {
 	ReplicationDeleteMarker bool
 	ReplicationProxyRequest string

api-list.go

@@ -56,7 +56,7 @@ func (c *Client) ListBuckets(ctx context.Context) ([]BucketInfo, error) {
 	return listAllMyBucketsResult.Buckets.Bucket, nil
 }
 
-/// Bucket List Operations.
+// Bucket List Operations.
 func (c *Client) listObjectsV2(ctx context.Context, bucketName string, opts ListObjectsOptions) <-chan ObjectInfo {
 	// Allocate new list objects channel.
 	objectStatCh := make(chan ObjectInfo, 1)

api-put-bucket.go

@@ -26,7 +26,7 @@ import (
 	"github.com/minio/minio-go/v7/pkg/s3utils"
 )
 
-/// Bucket operations
+// Bucket operations
 func (c *Client) makeBucket(ctx context.Context, bucketName string, opts MakeBucketOptions) (err error) {
 	// Validate the input arguments.
 	if err := s3utils.CheckValidBucketNameStrict(bucketName); err != nil {

api-s3-datatypes.go

@@ -121,8 +121,8 @@ func (l *ListVersionsResult) UnmarshalXML(d *xml.Decoder, start xml.StartElement
 			return err
 		}
 
-		switch se := t.(type) {
-		case xml.StartElement:
+		se, ok := t.(xml.StartElement)
+		if ok {
 			tagName := se.Name.Local
 			switch tagName {
 			case "Name", "Prefix",

api.go

@@ -46,7 +46,7 @@ import (
 
 // Client implements Amazon S3 compatible methods.
 type Client struct {
-	///  Standard options.
+	//  Standard options.
 
 	// Parsed endpoint url provided by the user.
 	endpointURL *url.URL
@@ -947,13 +947,13 @@ func (c *Client) makeTargetURL(bucketName, objectName, bucketLocation string, is
 		if isVirtualHostStyle {
 			urlStr = scheme + "://" + bucketName + "." + host + "/"
 			if objectName != "" {
-				urlStr = urlStr + s3utils.EncodePath(objectName)
+				urlStr += s3utils.EncodePath(objectName)
 			}
 		} else {
 			// If not fall back to using path style.
 			urlStr = urlStr + bucketName + "/"
 			if objectName != "" {
-				urlStr = urlStr + s3utils.EncodePath(objectName)
+				urlStr += s3utils.EncodePath(objectName)
 			}
 		}
 	}

bucket-cache.go

@@ -188,7 +188,7 @@ func (c *Client) getBucketLocationRequest(ctx context.Context, bucketName string
 
 	var urlStr string
 
-	//only support Aliyun OSS for virtual hosted path,  compatible  Amazon & Google Endpoint
+	// only support Aliyun OSS for virtual hosted path,  compatible  Amazon & Google Endpoint
 	if isVirtualHost && s3utils.IsAliyunOSSEndpoint(targetURL) {
 		urlStr = c.endpointURL.Scheme + "://" + bucketName + "." + targetURL.Host + "/?location"
 	} else {

constants.go

@@ -17,7 +17,7 @@
 
 package minio
 
-/// Multipart upload defaults.
+// Multipart upload defaults.
 
 // absMinPartSize - absolute minimum part size (5 MiB) below which
 // a part in a multipart upload may not be uploaded.

core_test.go

@@ -20,7 +20,6 @@ package minio
 import (
 	"bytes"
 	"context"
-	"log"
 	"net/http"
 	"os"
 	"strconv"
@@ -777,17 +776,17 @@ func TestCoreGetObjectMetadata(t *testing.T) {
 	_, err = core.PutObject(context.Background(), bucketName, "my-objectname",
 		bytes.NewReader([]byte("hello")), 5, "", "", putopts)
 	if err != nil {
-		log.Fatalln(err)
+		t.Fatal(err)
 	}
 
 	reader, objInfo, _, err := core.GetObject(context.Background(), bucketName, "my-objectname", GetObjectOptions{})
 	if err != nil {
-		log.Fatalln(err)
+		t.Fatal(err)
 	}
-	defer reader.Close()
+	reader.Close()
 
 	if objInfo.Metadata.Get("X-Amz-Meta-Key-1") != "Val-1" {
-		log.Fatalln("Expected metadata to be available but wasn't")
+		t.Fatal("Expected metadata to be available but wasn't")
 	}
 
 	err = core.RemoveObject(context.Background(), bucketName, "my-objectname", RemoveObjectOptions{})

functional_tests.go

@@ -11869,6 +11869,7 @@ func testRemoveObjects() {
 	_, err = c.PutObject(context.Background(), bucketName, objectName, reader, int64(bufSize), minio.PutObjectOptions{})
 	if err != nil {
 		logError(testName, function, args, startTime, "", "Error uploading object", err)
+		return
 	}
 
 	// Replace with smaller...
@@ -11890,7 +11891,8 @@ func testRemoveObjects() {
 	}
 	err = c.PutObjectRetention(context.Background(), bucketName, objectName, opts)
 	if err != nil {
-		log.Fatalln(err)
+		logError(testName, function, args, startTime, "", "Error setting retention", err)
+		return
 	}
 
 	objectsCh := make(chan minio.ObjectInfo)
@@ -11900,7 +11902,8 @@ func testRemoveObjects() {
 		// List all objects from a bucket-name with a matching prefix.
 		for object := range c.ListObjects(context.Background(), bucketName, minio.ListObjectsOptions{UseV1: true, Recursive: true}) {
 			if object.Err != nil {
-				log.Fatalln(object.Err)
+				logError(testName, function, args, startTime, "", "Error listing objects", object.Err)
+				return
 			}
 			objectsCh <- object
 		}
@@ -11923,7 +11926,8 @@ func testRemoveObjects() {
 		// List all objects from a bucket-name with a matching prefix.
 		for object := range c.ListObjects(context.Background(), bucketName, minio.ListObjectsOptions{UseV1: true, Recursive: true}) {
 			if object.Err != nil {
-				log.Fatalln(object.Err)
+				logError(testName, function, args, startTime, "", "Error listing objects", object.Err)
+				return
 			}
 			objectsCh1 <- object
 		}

pkg/credentials/sts_ldap_identity.go

@@ -124,7 +124,7 @@ func stripPassword(err error) error {
 // LDAP Identity with a specified session policy. The `policy` parameter must be
 // a JSON string specifying the policy document.
 //
-// DEPRECATED: Use the `LDAPIdentityPolicyOpt` with `NewLDAPIdentity` instead.
+// Deprecated: Use the `LDAPIdentityPolicyOpt` with `NewLDAPIdentity` instead.
 func NewLDAPIdentityWithSessionPolicy(stsEndpoint, ldapUsername, ldapPassword, policy string) (*Credentials, error) {
 	return New(&LDAPIdentity{
 		Client:       &http.Client{Transport: http.DefaultTransport},

pkg/lifecycle/lifecycle.go

@@ -99,6 +99,7 @@ func (n NoncurrentVersionTransition) isNull() bool {
 	return n.StorageClass == ""
 }
 
+// UnmarshalJSON implements NoncurrentVersionTransition JSONify
 func (n *NoncurrentVersionTransition) UnmarshalJSON(b []byte) error {
 	type noncurrentVersionTransition NoncurrentVersionTransition
 	var nt noncurrentVersionTransition

pkg/replication/replication.go

@@ -719,9 +719,12 @@ type Metrics struct {
 	FailedCount uint64 `json:"failedReplicationCount"`
 }
 
+// ResyncTargetsInfo provides replication target information to resync replicated data.
 type ResyncTargetsInfo struct {
 	Targets []ResyncTarget `json:"target,omitempty"`
 }
+
+// ResyncTarget provides the replica resources and resetID to initiate resync replication.
 type ResyncTarget struct {
 	Arn     string `json:"arn"`
 	ResetID string `json:"resetid"`

pkg/replication/replication_test.go

@@ -28,7 +28,7 @@ func TestAddReplicationRule(t *testing.T) {
 		opts        Options
 		expectedErr string
 	}{
-		{ //test case :1
+		{ // test case :1
 			cfg: Config{},
 			opts: Options{
 				ID:           "xyz.id",
@@ -41,7 +41,7 @@ func TestAddReplicationRule(t *testing.T) {
 			},
 			expectedErr: "",
 		},
-		{ //test case :2
+		{ // test case :2
 			cfg: Config{},
 			opts: Options{
 				ID:           "",
@@ -54,7 +54,7 @@ func TestAddReplicationRule(t *testing.T) {
 			},
 			expectedErr: "rule state should be either [enable|disable]",
 		},
-		{ //test case :3
+		{ // test case :3
 			cfg: Config{Rules: []Rule{{Priority: 1}}},
 			opts: Options{
 				ID:           "xyz.id",
@@ -67,7 +67,7 @@ func TestAddReplicationRule(t *testing.T) {
 			},
 			expectedErr: "priority must be unique. Replication configuration already has a rule with this priority",
 		},
-		{ //test case :4
+		{ // test case :4
 			cfg: Config{},
 			opts: Options{
 				ID:           "xyz.id",
@@ -80,8 +80,7 @@ func TestAddReplicationRule(t *testing.T) {
 			},
 			expectedErr: "destination bucket needs to be in Arn format",
 		},
-		{ //test case :5
-
+		{ // test case :5
 			cfg: Config{},
 			opts: Options{
 				ID:           "xyz.id",
@@ -94,7 +93,7 @@ func TestAddReplicationRule(t *testing.T) {
 			},
 			expectedErr: "destination bucket needs to be in Arn format",
 		},
-		{ //test case :6
+		{ // test case :6
 			cfg: Config{Role: "arn:minio:replication:eu-west-1:c5acb6ac-9918-4dc6-8534-6244ed1a611a:targetbucket"},
 			opts: Options{
 				ID:           "xyz.id",
@@ -107,7 +106,7 @@ func TestAddReplicationRule(t *testing.T) {
 			},
 			expectedErr: "",
 		},
-		{ //test case :7
+		{ // test case :7
 			cfg: Config{},
 			opts: Options{
 				ID:           "xyz.id",
@@ -120,8 +119,17 @@ func TestAddReplicationRule(t *testing.T) {
 			},
 			expectedErr: "",
 		},
-		{ //test case :8
-			cfg: Config{Rules: []Rule{{ID: "xyz.id", Destination: Destination{Bucket: "arn:aws:s3:::destbucket"}}}},
+		{ // test case :8
+			cfg: Config{
+				Rules: []Rule{
+					{
+						ID: "xyz.id",
+						Destination: Destination{
+							Bucket: "arn:aws:s3:::destbucket",
+						},
+					},
+				},
+			},
 			opts: Options{
 				ID:           "xyz.id",
 				Prefix:       "abc/",
@@ -153,7 +161,7 @@ func TestEditReplicationRule(t *testing.T) {
 		opts        Options
 		expectedErr string
 	}{
-		{ //test case :1 edit a rule in older config with remote ARN in destination bucket
+		{ // test case :1 edit a rule in older config with remote ARN in destination bucket
 			cfg: Config{
 				Role: "arn:minio:replication:eu-west-1:c5acb6ac-9918-4dc6-8534-6244ed1a611a:destbucket",
 				Rules: []Rule{{
@@ -173,7 +181,7 @@ func TestEditReplicationRule(t *testing.T) {
 			},
 			expectedErr: "invalid destination bucket for this rule",
 		},
-		{ //test case :2 mismatched rule id
+		{ // test case :2 mismatched rule id
 			cfg: Config{
 				Role: "arn:minio:replication:eu-west-1:c5acb6ac-9918-4dc6-8534-6244ed1a611a:destbucket",
 				Rules: []Rule{{
@@ -193,7 +201,7 @@ func TestEditReplicationRule(t *testing.T) {
 			},
 			expectedErr: "rule with ID xyz.id not found in replication configuration",
 		},
-		{ //test case :3 missing rule id
+		{ // test case :3 missing rule id
 			cfg: Config{
 				Role: "arn:minio:replication:eu-west-1:c5acb6ac-9918-4dc6-8534-6244ed1a611a:destbucket",
 				Rules: []Rule{{
@@ -212,7 +220,7 @@ func TestEditReplicationRule(t *testing.T) {
 			},
 			expectedErr: "rule ID missing",
 		},
-		{ //test case :4 different destination bucket
+		{ // test case :4 different destination bucket
 			cfg: Config{
 				Role: "",
 				Rules: []Rule{{
@@ -232,7 +240,7 @@ func TestEditReplicationRule(t *testing.T) {
 			},
 			expectedErr: "invalid destination bucket for this rule",
 		},
-		{ //test case :5 invalid destination bucket arn format
+		{ // test case :5 invalid destination bucket arn format
 			cfg: Config{
 				Role: "arn:minio:replication:eu-west-1:c5acb6ac-9918-4dc6-8534-6244ed1a611a:destbucket",
 				Rules: []Rule{{
@@ -253,7 +261,7 @@ func TestEditReplicationRule(t *testing.T) {
 			expectedErr: "destination bucket needs to be in Arn format",
 		},
 
-		{ //test case :6 invalid rule status
+		{ // test case :6 invalid rule status
 			cfg: Config{
 				Rules: []Rule{{
 					ID:          "xyz.id",
@@ -272,7 +280,7 @@ func TestEditReplicationRule(t *testing.T) {
 			},
 			expectedErr: "rule state should be either [enable|disable]",
 		},
-		{ //test case :7 another rule has same priority
+		{ // test case :7 another rule has same priority
 			cfg: Config{
 				Rules: []Rule{{
 					ID:          "xyz.id",
@@ -297,7 +305,7 @@ func TestEditReplicationRule(t *testing.T) {
 			},
 			expectedErr: "priority must be unique. Replication configuration already has a rule with this priority",
 		},
-		{ //test case :8 ; edit a rule in older config
+		{ // test case :8 ; edit a rule in older config
 			cfg: Config{
 				Role: "arn:minio:replication:eu-west-1:c5acb6ac-9918-4dc6-8534-6244ed1a611a:destbucket",
 				Rules: []Rule{{

pkg/s3utils/utils.go

@@ -212,7 +212,7 @@ func IsGoogleEndpoint(endpointURL url.URL) bool {
 
 // Expects ascii encoded strings - from output of urlEncodePath
 func percentEncodeSlash(s string) string {
-	return strings.Replace(s, "/", "%2F", -1)
+	return strings.ReplaceAll(s, "/", "%2F")
 }
 
 // QueryEncode - encodes query values in their URL encoded form. In

pkg/signer/request-signature-v2.go

@@ -233,16 +233,7 @@ func writeCanonicalizedHeaders(buf *bytes.Buffer, req http.Request) {
 			if idx > 0 {
 				buf.WriteByte(',')
 			}
-			if strings.Contains(v, "\n") {
-				// TODO: "Unfold" long headers that
-				// span multiple lines (as allowed by
-				// RFC 2616, section 4.2) by replacing
-				// the folding white-space (including
-				// new-line) by a single space.
-				buf.WriteString(v)
-			} else {
-				buf.WriteString(v)
-			}
+			buf.WriteString(v)
 		}
 		buf.WriteByte('\n')
 	}