GOCBC-700: Improve management API error handling

Motivation ---------- Management operations which do not return status 200 will generate a rather generic error. This should be updated such that they generate more contextual error information. Changes ------- Updated management APIs to try to parse response bodies for non 200 status codes. If possible then return a known typed error, if not then return an unknown typed error with the response body. If the response body cannot be read then return a generic error. Change-Id: Ide1c6339fec0eb5103e508899f5ed89c5cd17a85 Reviewed-on: http://review.couchbase.org/122775 Reviewed-by: Brett Lawson <brett19@gmail.com> Tested-by: Charles Dixon <chvckd@gmail.com>
couchbase · Mar 2, 2020 · a9fb739 · a9fb739
1 parent ab32adf
commit a9fb739
Show file tree

Hide file tree

Showing 16 changed files with 839 additions and 176 deletions.
diff --git a/Makefile b/Makefile
@@ -46,6 +46,7 @@ updatemocks:
 	mockery -name kvProvider -output . -testonly -inpkg
 	mockery -name httpProvider -output . -testonly -inpkg
 	mockery -name diagnosticsProvider -output . -testonly -inpkg
+	mockery -name mgmtProvider -output . -testonly -inpkg
 	# pendingOp is manually mocked
 
 .PHONY: all test devsetup fasttest lint cover checkerrs checkfmt checkvet checkiea checkspell check bench updatetestcases updatemocks
diff --git a/bucket_collectionsmgr.go b/bucket_collectionsmgr.go
@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/google/uuid"
+	"github.com/pkg/errors"
 
 	"github.com/couchbase/gocbcore/v8"
 )
@@ -49,6 +50,30 @@ type CollectionManager struct {
 	tracer               requestTracer
 }
 
+func (cm *CollectionManager) tryParseErrorMessage(req *gocbcore.HTTPRequest, resp *gocbcore.HTTPResponse) error {
+	errBody := tryReadHTTPBody(resp)
+	if errBody == "" {
+		return nil
+	}
+	errText := strings.ToLower(errBody)
+
+	if resp.StatusCode == 404 {
+		if strings.Contains(errText, "not found") && strings.Contains(errText, "scope") {
+			return makeGenericHTTPError(ErrScopeNotFound, req, resp)
+		} else if strings.Contains(errText, "not found") && strings.Contains(errText, "scope") {
+			return makeGenericHTTPError(ErrScopeNotFound, req, resp)
+		}
+	}
+
+	if strings.Contains(errText, "already exists") && strings.Contains(errText, "collection") {
+		return makeGenericHTTPError(ErrCollectionExists, req, resp)
+	} else if strings.Contains(errText, "already exists") && strings.Contains(errText, "scope") {
+		return makeGenericHTTPError(ErrScopeExists, req, resp)
+	}
+
+	return makeGenericHTTPError(errors.New(errText), req, resp)
+}
+
 // GetAllScopesOptions is the set of options available to the GetAllScopes operation.
 type GetAllScopesOptions struct {
 	Timeout       time.Duration
@@ -89,7 +114,11 @@ func (cm *CollectionManager) GetAllScopes(opts *GetAllScopesOptions) ([]ScopeSpe
 	resp, err := cm.httpClient.DoHTTPRequest(req)
 	dspan.Finish()
 	if err != nil {
-		return nil, makeGenericHTTPError(err, req, resp)
+		colErr := cm.tryParseErrorMessage(req, resp)
+		if colErr != nil {
+			return nil, colErr
+		}
+		return nil, makeHTTPBadStatusError("failed to get users", req, resp)
 	}
 
 	defer func() {
@@ -204,18 +233,11 @@ func (cm *CollectionManager) CreateCollection(spec CollectionSpec, opts *CreateC
 	}
 
 	if resp.StatusCode != 200 {
-		errBody := tryReadHTTPBody(resp)
-		errText := strings.ToLower(errBody)
-
-		if strings.Contains(errText, "already exists") && strings.Contains(errText, "collection") {
-			return makeGenericHTTPError(ErrCollectionExists, req, resp)
+		colErr := cm.tryParseErrorMessage(req, resp)
+		if colErr != nil {
+			return colErr
 		}
-
-		if strings.Contains(errText, "not found") && strings.Contains(errText, "scope") {
-			return makeGenericHTTPError(ErrScopeNotFound, req, resp)
-		}
-
-		return makeHTTPBadStatusError("failed to create collection", req, resp)
+		return makeHTTPBadStatusError("failed to get users", req, resp)
 	}
 
 	err = resp.Body.Close()
@@ -277,14 +299,11 @@ func (cm *CollectionManager) DropCollection(spec CollectionSpec, opts *DropColle
 	}
 
 	if resp.StatusCode != 200 {
-		errBody := tryReadHTTPBody(resp)
-		errText := strings.ToLower(errBody)
-
-		if strings.Contains(errText, "not found") && strings.Contains(errText, "collection") {
-			return makeGenericHTTPError(ErrCollectionNotFound, req, resp)
+		colErr := cm.tryParseErrorMessage(req, resp)
+		if colErr != nil {
+			return colErr
 		}
-
-		return makeHTTPBadStatusError("failed to drop collection", req, resp)
+		return makeHTTPBadStatusError("failed to get users", req, resp)
 	}
 
 	err = resp.Body.Close()
@@ -347,14 +366,11 @@ func (cm *CollectionManager) CreateScope(scopeName string, opts *CreateScopeOpti
 	}
 
 	if resp.StatusCode != 200 {
-		errBody := tryReadHTTPBody(resp)
-		errText := strings.ToLower(errBody)
-
-		if strings.Contains(errText, "already exists") && strings.Contains(errText, "scope") {
-			return makeGenericHTTPError(ErrScopeExists, req, resp)
+		colErr := cm.tryParseErrorMessage(req, resp)
+		if colErr != nil {
+			return colErr
 		}
-
-		return makeHTTPBadStatusError("failed to create scope", req, resp)
+		return makeHTTPBadStatusError("failed to get users", req, resp)
 	}
 
 	err = resp.Body.Close()
@@ -408,14 +424,11 @@ func (cm *CollectionManager) DropScope(scopeName string, opts *DropScopeOptions)
 	}
 
 	if resp.StatusCode != 200 {
-		errBody := tryReadHTTPBody(resp)
-		errText := strings.ToLower(errBody)
-
-		if strings.Contains(errText, "not found") && strings.Contains(errText, "scope") {
-			return makeGenericHTTPError(ErrScopeNotFound, req, resp)
+		colErr := cm.tryParseErrorMessage(req, resp)
+		if colErr != nil {
+			return colErr
 		}
-
-		return makeHTTPBadStatusError("failed to drop scope", req, resp)
+		return makeHTTPBadStatusError("failed to get users", req, resp)
 	}
 
 	err = resp.Body.Close()

diff --git a/bucket_ping_test.go b/bucket_ping_test.go
@@ -96,19 +96,7 @@ func (suite *UnitTestSuite) TestPingAll() {
 	cli.On("getKvProvider").Return(kvProvider, nil)
 	cli.On("getHTTPProvider").Return(httpProvider, nil)
 
-	b := &Bucket{
-		sb: stateBlock{
-			clientStateBlock: clientStateBlock{
-				BucketName: "mock",
-			},
-
-			KvTimeout:        1000 * time.Second,
-			AnalyticsTimeout: 1000 * time.Second,
-			QueryTimeout:     1000 * time.Second,
-			SearchTimeout:    1000 * time.Second,
-			cachedClient:     cli,
-		},
-	}
+	b := suite.bucket("mock", suite.defaultTimeoutConfig(), cli)
 
 	report, err := b.Ping(nil)
 	if err != nil {

diff --git a/bucket_viewindexes.go b/bucket_viewindexes.go
@@ -3,8 +3,11 @@ package gocb
 import (
 	"encoding/json"
 	"fmt"
+	"io/ioutil"
 	"strings"
 	"time"
+
+	"github.com/pkg/errors"
 )
 
 // DesignDocumentNamespace represents which namespace a design document resides in.
@@ -100,6 +103,44 @@ type ViewIndexManager struct {
 	tracer requestTracer
 }
 
+func (vm *ViewIndexManager) tryParseErrorMessage(req mgmtRequest, resp *mgmtResponse) error {
+	b, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		logDebugf("Failed to read view index manager response body: %s", err)
+		return nil
+	}
+
+	if resp.StatusCode == 404 {
+		if strings.Contains(strings.ToLower(string(b)), "not_found") {
+			return makeGenericMgmtError(ErrDesignDocumentNotFound, &req, resp)
+		}
+
+		return makeGenericMgmtError(errors.New(string(b)), &req, resp)
+	}
+
+	var mgrErr bucketMgrErrorResp
+	err = json.Unmarshal(b, &mgrErr)
+	if err != nil {
+		logDebugf("Failed to unmarshal error body: %s", err)
+		return makeGenericMgmtError(errors.New(string(b)), &req, resp)
+	}
+
+	var bodyErr error
+	var firstErr string
+	for _, err := range mgrErr.Errors {
+		firstErr = strings.ToLower(err)
+		break
+	}
+
+	if strings.Contains(firstErr, "bucket with given name already exists") {
+		bodyErr = ErrBucketExists
+	} else {
+		bodyErr = errors.New(firstErr)
+	}
+
+	return makeGenericMgmtError(bodyErr, &req, resp)
+}
+
 func (vm *ViewIndexManager) doMgmtRequest(req mgmtRequest) (*mgmtResponse, error) {
 	resp, err := vm.mgmtProvider.executeMgmtRequest(req)
 	if err != nil {
@@ -160,11 +201,12 @@ func (vm *ViewIndexManager) getDesignDocument(tracectx requestSpanContext, name
 	}
 
 	if resp.StatusCode != 200 {
-		if resp.StatusCode == 404 {
-			return nil, makeGenericMgmtError(ErrDesignDocumentNotFound, &req, resp)
+		vwErr := vm.tryParseErrorMessage(req, resp)
+		if vwErr != nil {
+			return nil, vwErr
 		}
 
-		return nil, makeMgmtBadStatusError("failed to get design document", &req, resp)
+		return nil, makeGenericMgmtError(errors.New("failed to get design document"), &req, resp)
 	}
 
 	var ddocData jsonDesignDocument
@@ -219,7 +261,12 @@ func (vm *ViewIndexManager) GetAllDesignDocuments(namespace DesignDocumentNamesp
 	}
 
 	if resp.StatusCode != 200 {
-		return nil, makeMgmtBadStatusError("failed to get all design documents", &req, resp)
+		vwErr := vm.tryParseErrorMessage(req, resp)
+		if vwErr != nil {
+			return nil, vwErr
+		}
+
+		return nil, makeGenericMgmtError(errors.New("failed to get design documents"), &req, resp)
 	}
 
 	var ddocsResp struct {
@@ -310,7 +357,12 @@ func (vm *ViewIndexManager) upsertDesignDocument(
 	}
 
 	if resp.StatusCode != 201 {
-		return makeMgmtBadStatusError("failed to upsert design document", &req, resp)
+		vwErr := vm.tryParseErrorMessage(req, resp)
+		if vwErr != nil {
+			return vwErr
+		}
+
+		return makeGenericMgmtError(errors.New("failed to upsert design document"), &req, resp)
 	}
 
 	return nil
@@ -352,11 +404,12 @@ func (vm *ViewIndexManager) dropDesignDocument(tracectx requestSpanContext, name
 	}
 
 	if resp.StatusCode != 200 {
-		if resp.StatusCode == 404 {
-			return makeGenericMgmtError(ErrDesignDocumentNotFound, &req, resp)
+		vwErr := vm.tryParseErrorMessage(req, resp)
+		if vwErr != nil {
+			return vwErr
 		}
 
-		return makeMgmtBadStatusError("failed to drop design document", &req, resp)
+		return makeGenericMgmtError(errors.New("failed to drop design document"), &req, resp)
 	}
 
 	return nil