From e517704ccb36b0a3b48907977c29506327d80f12 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 17 Nov 2021 08:46:32 -0800 Subject: [PATCH] cleanup minio-go code, add gocritic linter (#1586) --- .golangci.yml | 11 +++++++ api-get-options.go | 2 +- api-list.go | 2 +- api-put-bucket.go | 2 +- api-s3-datatypes.go | 4 +-- api.go | 6 ++-- bucket-cache.go | 2 +- constants.go | 2 +- core_test.go | 9 +++--- functional_tests.go | 10 +++++-- pkg/credentials/sts_ldap_identity.go | 2 +- pkg/lifecycle/lifecycle.go | 1 + pkg/replication/replication.go | 3 ++ pkg/replication/replication_test.go | 44 ++++++++++++++++------------ pkg/s3utils/utils.go | 2 +- pkg/signer/request-signature-v2.go | 11 +------ pkg/signer/request-signature-v4.go | 36 +++++++++++------------ post-policy.go | 4 +-- retry-continous.go | 2 +- retry.go | 2 +- utils.go | 2 +- 21 files changed, 88 insertions(+), 71 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index b60d07fc30..dfc0c2d537 100644 --- a/.golangci.yml +++ b/.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:" diff --git a/api-get-options.go b/api-get-options.go index 9e0cb21479..0be858d191 100644 --- a/api-get-options.go +++ b/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 diff --git a/api-list.go b/api-list.go index a8d7e71377..85209b5d27 100644 --- a/api-list.go +++ b/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) diff --git a/api-put-bucket.go b/api-put-bucket.go index 441c922307..1a6db3e10d 100644 --- a/api-put-bucket.go +++ b/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 { diff --git a/api-s3-datatypes.go b/api-s3-datatypes.go index 37ed97b725..948f8a7408 100644 --- a/api-s3-datatypes.go +++ b/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", diff --git a/api.go b/api.go index 2652ad2720..b5eaa690bb 100644 --- a/api.go +++ b/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) } } } diff --git a/bucket-cache.go b/bucket-cache.go index 760af77515..19dabd25b7 100644 --- a/bucket-cache.go +++ b/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 { diff --git a/constants.go b/constants.go index 7caa42d9f6..dee83b870c 100644 --- a/constants.go +++ b/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. diff --git a/core_test.go b/core_test.go index 77e01d5e13..cb512fdc9e 100644 --- a/core_test.go +++ b/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{}) diff --git a/functional_tests.go b/functional_tests.go index ffe1063e46..b8950dd27b 100644 --- a/functional_tests.go +++ b/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 } diff --git a/pkg/credentials/sts_ldap_identity.go b/pkg/credentials/sts_ldap_identity.go index 0fa5b55f1f..bdde1fa3ce 100644 --- a/pkg/credentials/sts_ldap_identity.go +++ b/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}, diff --git a/pkg/lifecycle/lifecycle.go b/pkg/lifecycle/lifecycle.go index c343e67143..96f1101ca7 100644 --- a/pkg/lifecycle/lifecycle.go +++ b/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 diff --git a/pkg/replication/replication.go b/pkg/replication/replication.go index 3a29fa330f..97c1492b88 100644 --- a/pkg/replication/replication.go +++ b/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"` diff --git a/pkg/replication/replication_test.go b/pkg/replication/replication_test.go index 24ee2ad319..e289a229b3 100644 --- a/pkg/replication/replication_test.go +++ b/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{{ diff --git a/pkg/s3utils/utils.go b/pkg/s3utils/utils.go index 05e8b7ece7..4494546455 100644 --- a/pkg/s3utils/utils.go +++ b/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 diff --git a/pkg/signer/request-signature-v2.go b/pkg/signer/request-signature-v2.go index 71821a26a7..b6ea78f7d9 100644 --- a/pkg/signer/request-signature-v2.go +++ b/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') } diff --git a/pkg/signer/request-signature-v4.go b/pkg/signer/request-signature-v4.go index b518cc526f..ce64c37d11 100644 --- a/pkg/signer/request-signature-v4.go +++ b/pkg/signer/request-signature-v4.go @@ -42,22 +42,22 @@ const ( ServiceTypeSTS = "sts" ) -/// -/// Excerpts from @lsegal - -/// https://github.com/aws/aws-sdk-js/issues/659#issuecomment-120477258. -/// -/// User-Agent: -/// -/// This is ignored from signing because signing this causes -/// problems with generating pre-signed URLs (that are executed -/// by other agents) or when customers pass requests through -/// proxies, which may modify the user-agent. -/// -/// -/// Authorization: -/// -/// Is skipped for obvious reasons -/// +// +// Excerpts from @lsegal - +// https:/github.com/aws/aws-sdk-js/issues/659#issuecomment-120477258. +// +// User-Agent: +// +// This is ignored from signing because signing this causes +// problems with generating pre-signed URLs (that are executed +// by other agents) or when customers pass requests through +// proxies, which may modify the user-agent. +// +// +// Authorization: +// +// Is skipped for obvious reasons +// var v4IgnoredHeaders = map[string]bool{ "Authorization": true, "User-Agent": true, @@ -183,7 +183,7 @@ func getSignedHeaders(req http.Request, ignoredHeaders map[string]bool) string { // \n // func getCanonicalRequest(req http.Request, ignoredHeaders map[string]bool, hashedPayload string) string { - req.URL.RawQuery = strings.Replace(req.URL.Query().Encode(), "+", "%20", -1) + req.URL.RawQuery = strings.ReplaceAll(req.URL.Query().Encode(), "+", "%20") canonicalRequest := strings.Join([]string{ req.Method, s3utils.EncodePath(req.URL.Path), @@ -199,7 +199,7 @@ func getCanonicalRequest(req http.Request, ignoredHeaders map[string]bool, hashe func getStringToSignV4(t time.Time, location, canonicalRequest, serviceType string) string { stringToSign := signV4Algorithm + "\n" + t.Format(iso8601DateFormat) + "\n" stringToSign = stringToSign + getScope(location, t, serviceType) + "\n" - stringToSign = stringToSign + hex.EncodeToString(sum256([]byte(canonicalRequest))) + stringToSign += hex.EncodeToString(sum256([]byte(canonicalRequest))) return stringToSign } diff --git a/post-policy.go b/post-policy.go index 31a7308ccf..7aa96e0d69 100644 --- a/post-policy.go +++ b/post-policy.go @@ -316,8 +316,8 @@ func (p PostPolicy) marshalJSON() []byte { } retStr := "{" retStr = retStr + expirationStr + "," - retStr = retStr + conditionsStr - retStr = retStr + "}" + retStr += conditionsStr + retStr += "}" return []byte(retStr) } diff --git a/retry-continous.go b/retry-continous.go index 12b81d91ad..b54081d0d4 100644 --- a/retry-continous.go +++ b/retry-continous.go @@ -39,7 +39,7 @@ func (c *Client) newRetryTimerContinous(unit time.Duration, cap time.Duration, j if attempt > maxAttempt { attempt = maxAttempt } - //sleep = random_between(0, min(cap, base * 2 ** attempt)) + // sleep = random_between(0, min(cap, base * 2 ** attempt)) sleep := unit * time.Duration(1< cap { sleep = cap diff --git a/retry.go b/retry.go index a449d55dde..5611770d3a 100644 --- a/retry.go +++ b/retry.go @@ -56,7 +56,7 @@ func (c *Client) newRetryTimer(ctx context.Context, maxRetry int, unit time.Dura jitter = MaxJitter } - //sleep = random_between(0, min(cap, base * 2 ** attempt)) + // sleep = random_between(0, min(cap, base * 2 ** attempt)) sleep := unit * time.Duration(1< cap { sleep = cap diff --git a/utils.go b/utils.go index ed388d7131..297df7bf64 100644 --- a/utils.go +++ b/utils.go @@ -436,7 +436,7 @@ func redactSignature(origAuth string) string { return "AWS **REDACTED**:**REDACTED**" } - /// Signature V4 authorization header. + // Signature V4 authorization header. // Strip out accessKeyID from: // Credential=////aws4_request