Add unit tests and refactor to improve coverage (#7617)

cmd/admin-handlers.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -476,7 +477,6 @@ func newLockEntry(l lockRequesterInfo, resource, server string) *madmin.LockEntr
 }
 
 func topLockEntries(peerLocks []*PeerLocks) madmin.LockEntries {
-	const listCount int = 10
 	entryMap := make(map[string]*madmin.LockEntry)
 	for _, peerLock := range peerLocks {
 		if peerLock == nil {
@@ -497,6 +497,7 @@ func topLockEntries(peerLocks []*PeerLocks) madmin.LockEntries {
 		lockEntries = append(lockEntries, *v)
 	}
 	sort.Sort(lockEntries)
+	const listCount int = 10
 	if len(lockEntries) > listCount {
 		lockEntries = lockEntries[:listCount]
 	}
@@ -654,45 +655,60 @@ func (a adminAPIHandlers) DownloadProfilingHandler(w http.ResponseWriter, r *htt
 	}
 }
 
+type healInitParams struct {
+	bucket, objPrefix     string
+	hs                    madmin.HealOpts
+	clientToken           string
+	forceStart, forceStop bool
+}
+
 // extractHealInitParams - Validates params for heal init API.
-func extractHealInitParams(r *http.Request) (bucket, objPrefix string,
+func extractHealInitParams(vars map[string]string, qParms url.Values, r io.Reader) (hip healInitParams, err APIErrorCode) {
-	hs madmin.HealOpts, clientToken string, forceStart bool, forceStop bool,
+	hip.bucket = vars[string(mgmtBucket)]
-	err APIErrorCode) {
+	hip.objPrefix = vars[string(mgmtPrefix)]
 
-	vars := mux.Vars(r)
+	if hip.bucket == "" {
-	bucket = vars[string(mgmtBucket)]
+		if hip.objPrefix != "" {
-	objPrefix = vars[string(mgmtPrefix)]
-
-	if bucket == "" {
-		if objPrefix != "" {
 			// Bucket is required if object-prefix is given
 			err = ErrHealMissingBucket
 			return
 		}
-	} else if isReservedOrInvalidBucket(bucket, false) {
+	} else if isReservedOrInvalidBucket(hip.bucket, false) {
 		err = ErrInvalidBucketName
 		return
 	}
 
 	// empty prefix is valid.
-	if !IsValidObjectPrefix(objPrefix) {
+	if !IsValidObjectPrefix(hip.objPrefix) {
 		err = ErrInvalidObjectName
 		return
 	}
 
-	qParms := r.URL.Query()
 	if len(qParms[string(mgmtClientToken)]) > 0 {
-		clientToken = qParms[string(mgmtClientToken)][0]
+		hip.clientToken = qParms[string(mgmtClientToken)][0]
 	}
 	if _, ok := qParms[string(mgmtForceStart)]; ok {
-		forceStart = true
+		hip.forceStart = true
 	}
 	if _, ok := qParms[string(mgmtForceStop)]; ok {
-		forceStop = true
+		hip.forceStop = true
 	}
+
+	// Invalid request conditions:
+	//
+	//   Cannot have both forceStart and forceStop in the same
+	//   request; If clientToken is provided, request can only be
+	//   to continue receiving logs, so it cannot be start or
+	//   stop;
+	if (hip.forceStart && hip.forceStop) ||
+		(hip.clientToken != "" && (hip.forceStart || hip.forceStop)) {
+		err = ErrInvalidRequest
+		return
+	}
+
 	// ignore body if clientToken is provided
-	if clientToken == "" {
+	if hip.clientToken == "" {
-		jerr := json.NewDecoder(r.Body).Decode(&hs)
+		jerr := json.NewDecoder(r).Decode(&hip.hs)
 		if jerr != nil {
 			logger.LogIf(context.Background(), jerr)
 			err = ErrRequestBodyParse
@@ -731,7 +747,7 @@ func (a adminAPIHandlers) HealHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	bucket, objPrefix, hs, clientToken, forceStart, forceStop, errCode := extractHealInitParams(r)
+	hip, errCode := extractHealInitParams(mux.Vars(r), r.URL.Query(), r.Body)
 	if errCode != ErrNone {
 		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(errCode), r.URL)
 		return
@@ -805,8 +821,8 @@ func (a adminAPIHandlers) HealHandler(w http.ResponseWriter, r *http.Request) {
 	info := objectAPI.StorageInfo(ctx)
 	numDisks := info.Backend.OfflineDisks + info.Backend.OnlineDisks
 
-	healPath := pathJoin(bucket, objPrefix)
+	healPath := pathJoin(hip.bucket, hip.objPrefix)
-	if clientToken == "" && !forceStart && !forceStop {
+	if hip.clientToken == "" && !hip.forceStart && !hip.forceStop {
 		nh, exists := globalAllHealState.getHealSequence(healPath)
 		if exists && !nh.hasEnded() && len(nh.currentStatus.Items) > 0 {
 			b, err := json.Marshal(madmin.HealStartSuccess{
@@ -825,11 +841,11 @@ func (a adminAPIHandlers) HealHandler(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	if clientToken != "" && !forceStart && !forceStop {
+	if hip.clientToken != "" && !hip.forceStart && !hip.forceStop {
 		// Since clientToken is given, fetch heal status from running
 		// heal sequence.
 		respBytes, errCode := globalAllHealState.PopHealStatusJSON(
-			healPath, clientToken)
+			healPath, hip.clientToken)
 		if errCode != ErrNone {
 			writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(errCode), r.URL)
 		} else {
@@ -840,14 +856,14 @@ func (a adminAPIHandlers) HealHandler(w http.ResponseWriter, r *http.Request) {
 
 	respCh := make(chan healResp)
 	switch {
-	case forceStop:
+	case hip.forceStop:
 		go func() {
 			respBytes, apiErr := globalAllHealState.stopHealSequence(healPath)
 			hr := healResp{respBytes: respBytes, apiErr: apiErr}
 			respCh <- hr
 		}()
-	case clientToken == "":
+	case hip.clientToken == "":
-		nh := newHealSequence(bucket, objPrefix, handlers.GetSourceIP(r), numDisks, hs, forceStart)
+		nh := newHealSequence(hip.bucket, hip.objPrefix, handlers.GetSourceIP(r), numDisks, hip.hs, hip.forceStart)
 		go func() {
 			respBytes, apiErr, errMsg := globalAllHealState.LaunchNewHealSequence(nh)
 			hr := healResp{respBytes, apiErr, errMsg}
@@ -1609,7 +1625,10 @@ func convertValueType(elem []byte, jsonType gjson.Type) (interface{}, error) {
 	case gjson.False, gjson.True:
 		return strconv.ParseBool(str)
 	case gjson.JSON:
-		return gjson.Parse(str).Value(), nil
+		if gjson.Valid(str) {
+			return gjson.Parse(str).Value(), nil
+		}
+		return nil, errors.New("invalid json")
 	case gjson.String:
 		return str, nil
 	case gjson.Number:

cmd/admin-handlers_test.go

@@ -25,13 +25,16 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"reflect"
 	"strings"
 	"sync"
 	"testing"
+	"time"
 
 	"github.com/gorilla/mux"
 	"github.com/minio/minio/pkg/auth"
 	"github.com/minio/minio/pkg/madmin"
+	"github.com/tidwall/gjson"
 )
 
 var (
@@ -650,3 +653,149 @@ func TestToAdminAPIErrCode(t *testing.T) {
 		}
 	}
 }
+
+func TestTopLockEntries(t *testing.T) {
+	t1 := UTCNow()
+	t2 := UTCNow().Add(10 * time.Second)
+	peerLocks := []*PeerLocks{
+		{
+			Addr: "1",
+			Locks: map[string][]lockRequesterInfo{
+				"1": {
+					{false, "node2", "ep2", "2", t2, t2, ""},
+					{true, "node1", "ep1", "1", t1, t1, ""},
+				},
+				"2": {
+					{false, "node2", "ep2", "2", t2, t2, ""},
+					{true, "node1", "ep1", "1", t1, t1, ""},
+				},
+			},
+		},
+		{
+			Addr: "2",
+			Locks: map[string][]lockRequesterInfo{
+				"1": {
+					{false, "node2", "ep2", "2", t2, t2, ""},
+					{true, "node1", "ep1", "1", t1, t1, ""},
+				},
+				"2": {
+					{false, "node2", "ep2", "2", t2, t2, ""},
+					{true, "node1", "ep1", "1", t1, t1, ""},
+				},
+			},
+		},
+	}
+	les := topLockEntries(peerLocks)
+	if len(les) != 2 {
+		t.Fatalf("Did not get 2 results")
+	}
+	if les[0].Timestamp.After(les[1].Timestamp) {
+		t.Fatalf("Got wrong sorted value")
+	}
+}
+
+func TestExtractHealInitParams(t *testing.T) {
+	mkParams := func(clientToken string, forceStart, forceStop bool) url.Values {
+		v := url.Values{}
+		if clientToken != "" {
+			v.Add(string(mgmtClientToken), clientToken)
+		}
+		if forceStart {
+			v.Add(string(mgmtForceStart), "")
+		}
+		if forceStop {
+			v.Add(string(mgmtForceStop), "")
+		}
+		return v
+	}
+	qParmsArr := []url.Values{
+		// Invalid cases
+		mkParams("", true, true),
+		mkParams("111", true, true),
+		mkParams("111", true, false),
+		mkParams("111", false, true),
+		// Valid cases follow
+		mkParams("", true, false),
+		mkParams("", false, true),
+		mkParams("", false, false),
+		mkParams("111", false, false),
+	}
+	varsArr := []map[string]string{
+		// Invalid cases
+		{string(mgmtPrefix): "objprefix"},
+		// Valid cases
+		{},
+		{string(mgmtBucket): "bucket"},
+		{string(mgmtBucket): "bucket", string(mgmtPrefix): "objprefix"},
+	}
+
+	// Body is always valid - we do not test JSON decoding.
+	body := `{"recursive": false, "dryRun": true, "remove": false, "scanMode": 0}`
+
+	// Test all combinations!
+	for pIdx, parms := range qParmsArr {
+		for vIdx, vars := range varsArr {
+			_, err := extractHealInitParams(vars, parms, bytes.NewBuffer([]byte(body)))
+			isErrCase := false
+			if pIdx < 4 || vIdx < 1 {
+				isErrCase = true
+			}
+
+			if err != ErrNone && !isErrCase {
+				t.Errorf("Got unexpected error: %v %v %v", pIdx, vIdx, err)
+			} else if err == ErrNone && isErrCase {
+				t.Errorf("Got no error but expected one: %v %v", pIdx, vIdx)
+			}
+		}
+	}
+
+}
+
+func TestNormalizeJSONKey(t *testing.T) {
+	cases := []struct {
+		input         string
+		correctResult string
+	}{
+		{"notify.webhook.1", "notify.webhook.:1"},
+		{"notify.webhook.ok", "notify.webhook.ok"},
+		{"notify.webhook.1.2", "notify.webhook.:1.:2"},
+		{"abc", "abc"},
+	}
+	for i, tcase := range cases {
+		res := normalizeJSONKey(tcase.input)
+		if res != tcase.correctResult {
+			t.Errorf("Case %d: failed to match", i)
+		}
+	}
+}
+
+func TestConvertValueType(t *testing.T) {
+	cases := []struct {
+		elem          []byte
+		jt            gjson.Type
+		correctResult interface{}
+		isError       bool
+	}{
+		{[]byte(""), gjson.Null, nil, false},
+		{[]byte("t"), gjson.False, true, false},
+		{[]byte("f"), gjson.False, false, false},
+		{[]byte(`{"a": 1}`), gjson.JSON, map[string]interface{}{"a": float64(1)}, false},
+		{[]byte(`abc`), gjson.String, "abc", false},
+		{[]byte(`1`), gjson.Number, float64(1), false},
+		{[]byte(`a1`), gjson.Number, nil, true},
+		{[]byte(`{"A": `), gjson.JSON, nil, true},
+		{[]byte(`2`), gjson.False, nil, true},
+	}
+	for i, tc := range cases {
+		res, err := convertValueType(tc.elem, tc.jt)
+		if err != nil {
+			if !tc.isError {
+				t.Errorf("Case %d: got an error when none was expected", i)
+			}
+		} else if err == nil && tc.isError {
+			t.Errorf("Case %d: got no error though we expected one", i)
+		} else if !reflect.DeepEqual(res, tc.correctResult) {
+			t.Errorf("Case %d: result mismatch - got %#v", i, res)
+		}
+	}
+}