Don't return InvalidArgument if error details are of unknown type (#94)

Author: alexshtin

LICENSE

@@ -1,6 +1,6 @@
 The MIT License
 
-Copyright (c) 2020 Temporal Technologies Inc.  All rights reserved.
+Copyright (c) 2022 Temporal Technologies Inc.  All rights reserved.
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal

go.mod

@@ -3,10 +3,11 @@ module go.temporal.io/api
 go 1.16
 
 require (
-	github.com/gogo/googleapis v1.4.1 // indirect
+	github.com/gogo/googleapis v1.4.1
 	github.com/gogo/protobuf v1.3.2
 	github.com/gogo/status v1.1.0
 	github.com/golang/mock v1.6.0
+	github.com/stretchr/testify v1.7.0
 	golang.org/x/net v0.0.0-20220531201128-c960675eff93 // indirect
 	google.golang.org/genproto v0.0.0-20220531173845-685668d2de03 // indirect
 	google.golang.org/grpc v1.46.2

go.sum

@@ -11,6 +11,7 @@ github.com/cncf/udpa/go v0.0.0-20210930031921-04548b0d99d4/go.mod h1:6pvJx4me5XP
 github.com/cncf/xds/go v0.0.0-20210922020428-25de7278fc84/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
 github.com/cncf/xds/go v0.0.0-20211001041855-01bcc9b48dfe/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
 github.com/cncf/xds/go v0.0.0-20211011173535-cb28da3451f1/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
+github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
 github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
@@ -57,11 +58,13 @@ github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+
 github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw=
 github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8=
 github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
+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/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA=
 github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
+github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
@@ -165,9 +168,11 @@ google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQ
 google.golang.org/protobuf v1.27.1/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc=
 google.golang.org/protobuf v1.28.0 h1:w43yiav+6bVFTBQFZX0r7ipe9JQ1QsbMgHwbBziscLw=
 google.golang.org/protobuf v1.28.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.3/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
+gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=

serviceerror/convert.go

@@ -63,7 +63,7 @@ func FromStatus(st *status.Status) error {
 		return nil
 	}
 
-	// Simple case. Code to serviceerror is one to one mapping and there are no error details.
+	// Simple case. `st.Code()` to `serviceerror` is one to one mapping and there are no error details.
 	switch st.Code() {
 	case codes.DataLoss:
 		return newDataLoss(st)
@@ -87,58 +87,52 @@ func FromStatus(st *status.Status) error {
 	}
 
 	errDetails := extractErrorDetails(st)
-
-	// If there was an error during details extraction, it will go to errDetails.
-	if err, ok := errDetails.(error); ok {
-		return NewInvalidArgument(err.Error())
-	}
+	// If there was an error during details extraction, for example unknown message type,
+	// which can happen when new error details are added and getting read by old clients,
+	// then errDetails will be of type `error` with corresponding error inside.
+	// This error is ignored and `serviceerror` is built using `st.Code()` only.
 
 	switch st.Code() {
 	case codes.Internal:
-		if errDetails == nil {
-			return newInternal(st)
-		}
 		switch errDetails := errDetails.(type) {
 		case *errordetails.SystemWorkflowFailure:
 			return newSystemWorkflow(st, errDetails)
+		default:
+			return newInternal(st)
 		}
 	case codes.NotFound:
-		if errDetails == nil {
-			return newNotFound(st, nil)
-		}
 		switch errDetails := errDetails.(type) {
 		case *errordetails.NotFoundFailure:
 			return newNotFound(st, errDetails)
 		case *errordetails.NamespaceNotFoundFailure:
 			return newNamespaceNotFound(st, errDetails)
+		default:
+			return newNotFound(st, nil)
 		}
 	case codes.InvalidArgument:
-		if errDetails == nil {
-			return newInvalidArgument(st)
-		}
 		switch errDetails.(type) {
 		case *errordetails.QueryFailedFailure:
 			return newQueryFailed(st)
+		default:
+			return newInvalidArgument(st)
 		}
 	case codes.ResourceExhausted:
-		if errDetails == nil {
-			return newResourceExhausted(st, nil)
-		}
 		switch errDetails := errDetails.(type) {
 		case *errordetails.ResourceExhaustedFailure:
 			return newResourceExhausted(st, errDetails)
+		default:
+			return newResourceExhausted(st, nil)
 		}
 	case codes.AlreadyExists:
-		if errDetails == nil {
-			return newAlreadyExists(st)
-		}
 		switch errDetails := errDetails.(type) {
 		case *errordetails.NamespaceAlreadyExistsFailure:
 			return newNamespaceAlreadyExists(st)
 		case *errordetails.WorkflowExecutionAlreadyStartedFailure:
 			return newWorkflowExecutionAlreadyStarted(st, errDetails)
 		case *errordetails.CancellationAlreadyRequestedFailure:
 			return newCancellationAlreadyRequested(st)
+		default:
+			return newAlreadyExists(st)
 		}
 	case codes.FailedPrecondition:
 		switch errDetails := errDetails.(type) {
@@ -152,17 +146,20 @@ func FromStatus(st *status.Status) error {
 			return newServerVersionNotSupported(st, errDetails)
 		case *errordetails.WorkflowNotReadyFailure:
 			return newWorkflowNotReady(st)
+		default:
+			return newFailedPrecondition(st)
 		}
 	case codes.PermissionDenied:
 		switch errDetails := errDetails.(type) {
 		case *errordetails.PermissionDeniedFailure:
 			return newPermissionDenied(st, errDetails)
+		default:
+			return newPermissionDenied(st, nil)
 		}
-		return newPermissionDenied(st, nil)
 	}
 
-	// st.Code() should have error details, but it didn't (or error details are of a wrong type).
-	// Then use standard gRPC error representation ("rpc error: code = %s desc = %s").
+	// `st.Code()` has unknown value (should never happen).
+	// Use standard gRPC error representation "rpc error: code = %s desc = %s".
 	return st.Err()
 }
 

serviceerror/convert_test.go

@@ -0,0 +1,92 @@
+// The MIT License
+//
+// Copyright (c) 2022 Temporal Technologies Inc.  All rights reserved.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package serviceerror_test
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/gogo/googleapis/google/rpc"
+	"github.com/gogo/protobuf/types"
+	"github.com/gogo/status"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc/codes"
+
+	"go.temporal.io/api/errordetails/v1"
+	"go.temporal.io/api/serviceerror"
+)
+
+func TestFromStatus_NotFound(t *testing.T) {
+	var err error
+	st1 := status.New(codes.NotFound, "Not found.")
+	err1 := serviceerror.FromStatus(st1)
+	require.IsType(t, &serviceerror.NotFound{}, err1)
+	require.Equal(t, codes.NotFound, err1.(*serviceerror.NotFound).Status().Code())
+	require.Equal(t, "Not found.", err1.(*serviceerror.NotFound).Message)
+
+	st2 := status.New(codes.NotFound, "Not found.")
+	st2, err = st1.WithDetails(&errordetails.NamespaceNotFoundFailure{
+		Namespace: "test-ns",
+	})
+	require.NoError(t, err)
+	err2 := serviceerror.FromStatus(st2)
+	require.IsType(t, &serviceerror.NamespaceNotFound{}, err2)
+	require.Equal(t, codes.NotFound, err2.(*serviceerror.NamespaceNotFound).Status().Code())
+	require.Equal(t, "Not found.", err2.(*serviceerror.NamespaceNotFound).Message)
+	require.Equal(t, "test-ns", err2.(*serviceerror.NamespaceNotFound).Namespace)
+}
+
+func TestFromStatus_UnknownErrorDetails(t *testing.T) {
+	st1 := status.FromProto(&rpc.Status{
+		Code:    int32(codes.NotFound),
+		Message: "Not found.",
+		Details: []*types.Any{{TypeUrl: "type.googleapis.com/some.unknown.Type"}},
+	})
+
+	err1 := serviceerror.FromStatus(st1)
+	require.IsType(t, &serviceerror.NotFound{}, err1)
+	require.Equal(t, codes.NotFound, err1.(*serviceerror.NotFound).Status().Code())
+	require.Equal(t, "Not found.", err1.(*serviceerror.NotFound).Message)
+}
+
+func TestToStatus_UnknownErrorDetails(t *testing.T) {
+	err1 := status.ErrorProto(&rpc.Status{
+		Code:    int32(codes.NotFound),
+		Message: "Not found.",
+		Details: []*types.Any{{TypeUrl: "type.googleapis.com/some.unknown.Type"}},
+	})
+
+	st1 := serviceerror.ToStatus(err1)
+	require.Equal(t, codes.NotFound, st1.Code())
+	require.Equal(t, "Not found.", st1.Message())
+	require.Len(t, st1.Details(), 1)
+	require.Equal(t, "type.googleapis.com/some.unknown.Type", st1.Proto().Details[0].TypeUrl)
+}
+
+func TestToStatus_NotServiceError(t *testing.T) {
+	err1 := errors.New("some error")
+	st1 := serviceerror.ToStatus(err1)
+	require.Equal(t, codes.Unknown, st1.Code())
+	require.Equal(t, "some error", st1.Message())
+	require.Len(t, st1.Details(), 0)
+}

serviceerror/failed_precondition.go

@@ -0,0 +1,63 @@
+// The MIT License
+//
+// Copyright (c) 2020 Temporal Technologies Inc.  All rights reserved.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package serviceerror
+
+import (
+	"github.com/gogo/status"
+	"google.golang.org/grpc/codes"
+)
+
+type (
+	// FailedPrecondition represents failed precondition error.
+	FailedPrecondition struct {
+		Message string
+		st      *status.Status
+	}
+)
+
+// NewFailedPrecondition returns new FailedPrecondition error.
+func NewFailedPrecondition(message string) error {
+	return &FailedPrecondition{
+		Message: message,
+	}
+}
+
+// Error returns string message.
+func (e *FailedPrecondition) Error() string {
+	return e.Message
+}
+
+func (e *FailedPrecondition) Status() *status.Status {
+	if e.st != nil {
+		return e.st
+	}
+
+	return status.New(codes.FailedPrecondition, e.Message)
+}
+
+func newFailedPrecondition(st *status.Status) error {
+	return &FailedPrecondition{
+		Message: st.Message(),
+		st:      st,
+	}
+}