Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(errors): flatten sdk problem chains to reduce hash complexity #218

Merged
merged 1 commit into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/base_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ func (service *BaseService) Request(req *http.Request, result interface{}) (deta
var sdkErr *SDKProblem
if errors.As(authenticateError, &authErr) {
detailedResponse = authErr.Response
err = SDKErrorf(authErr.HTTPProblem, fmt.Sprintf(ERRORMSG_AUTHENTICATE_ERROR, authErr.Error()), "auth-failed", getComponentInfo())
err = SDKErrorf(authErr.HTTPProblem, fmt.Sprintf(ERRORMSG_AUTHENTICATE_ERROR, authErr.Error()), "auth-request-failed", getComponentInfo())
} else if errors.As(authenticateError, &sdkErr) {
sdkErr := RepurposeSDKProblem(authenticateError, "auth-failed")
// For compatibility.
Expand Down
28 changes: 22 additions & 6 deletions core/http_problem.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,16 +150,32 @@ func EnrichHTTPProblem(err error, operationID string, component *ProblemComponen
// If the problem originated from an HTTP error response, populate the
// HTTPProblem instance with details from the SDK that weren't available
// in the core at problem creation time.
var httpProp *HTTPProblem
if errors.As(err, &httpProp) {
enrichHTTPProblem(httpProp, operationID, component)
var httpProb *HTTPProblem

// In the case of an SDKProblem instance originating in the core,
// it will not track an HTTPProblem instance in its "caused by"
// chain, but we still want to be able to enrich it. It will be
// stored in the private "httpProblem" field.
var sdkProb *SDKProblem

if errors.As(err, &httpProb) {
enrichHTTPProblem(httpProb, operationID, component)
} else if errors.As(err, &sdkProb) && sdkProb.httpProblem != nil {
enrichHTTPProblem(sdkProb.httpProblem, operationID, component)
}
}

// enrichHTTPProblem takes an HTTPProblem instance alongside information about the request
// and adds the extra info to the instance. It also loosely deserializes the response
// in order to set additional information, like the error code.
func enrichHTTPProblem(httpProp *HTTPProblem, operationID string, component *ProblemComponent) {
httpProp.Component = component
httpProp.OperationID = operationID
func enrichHTTPProblem(httpProb *HTTPProblem, operationID string, component *ProblemComponent) {
// If this problem is already populated with service-level information,
// we should not enrich it any further. Most likely, this is an authentication
// error passed from the core to the SDK.
if httpProb.Component.Name != "" {
return
}

httpProb.Component = component
httpProb.OperationID = operationID
}
30 changes: 30 additions & 0 deletions core/http_problem_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,23 @@ func TestPublicEnrichHTTPProblemWithinSDKProblem(t *testing.T) {
assert.Equal(t, "delete_resource", httpProb.OperationID)
}

func TestPublicEnrichHTTPProblemWithinCoreProblem(t *testing.T) {
httpProb := httpErrorf("Bad request", &DetailedResponse{})
assert.Empty(t, httpProb.Component)
assert.Empty(t, httpProb.OperationID)

sdkProb := SDKErrorf(httpProb, "Wrong!", "disc", getComponentInfo())
assert.Nil(t, sdkProb.causedBy)
assert.NotNil(t, sdkProb.httpProblem)

EnrichHTTPProblem(sdkProb, "delete_resource", NewProblemComponent("test", "v2"))

assert.NotEmpty(t, httpProb.Component)
assert.Equal(t, "test", httpProb.Component.Name)
assert.Equal(t, "v2", httpProb.Component.Version)
assert.Equal(t, "delete_resource", httpProb.OperationID)
}

func TestPrivateEnrichHTTPProblem(t *testing.T) {
httpProb := httpErrorf("Bad request", &DetailedResponse{})
assert.Empty(t, httpProb.Component)
Expand All @@ -286,6 +303,19 @@ func TestPrivateEnrichHTTPProblem(t *testing.T) {
assert.Equal(t, "delete_resource", httpProb.OperationID)
}

func TestPrivateEnrichHTTPProblemWithPopulatedProblem(t *testing.T) {
httpProb := httpErrorf("Bad request", &DetailedResponse{})
httpProb.Component = NewProblemComponent("some-api", "v3")
httpProb.OperationID = "get_resource"
assert.NotEmpty(t, httpProb.Component)
assert.NotEmpty(t, httpProb.OperationID)

enrichHTTPProblem(httpProb, "delete_resource", NewProblemComponent("test", "v2"))
assert.Equal(t, "some-api", httpProb.Component.Name)
assert.Equal(t, "v3", httpProb.Component.Version)
assert.Equal(t, "get_resource", httpProb.OperationID)
}

func getPopulatedHTTPProblem() *HTTPProblem {
return &HTTPProblem{
IBMProblem: &IBMProblem{
Expand Down
59 changes: 57 additions & 2 deletions core/sdk_problem.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,19 @@ type SDKProblem struct {
// function names, files, and line numbers invoked
// leading up to the origination of the problem.
stack []sdkStackFrame

// If the problem instance originated in the core, we
// want to keep track of the information for debugging
// purposes (even though we aren't using the problem
// as a "caused by" problem).
coreProblem *sparseSDKProblem

// If the problem instance originated in the core and
// was caused by an HTTP request, we don't use the
// HTTPProblem instance as a "caused by" but we need
// to store the instance to pass it to a downstream
// SDKProblem instance.
httpProblem *HTTPProblem
}

// GetConsoleMessage returns all public fields of
Expand Down Expand Up @@ -84,6 +97,10 @@ func (e *SDKProblem) GetDebugOrderedMaps() *OrderedMaps {

orderedMaps.Add("stack", e.stack)

if e.coreProblem != nil {
orderedMaps.Add("core_problem", e.coreProblem)
}

var orderableCausedBy OrderableProblem
if errors.As(e.GetCausedBy(), &orderableCausedBy) {
orderedMaps.Add("caused_by", orderableCausedBy.GetDebugOrderedMaps().GetMaps())
Expand All @@ -97,11 +114,49 @@ func SDKErrorf(err error, summary, discriminator string, component *ProblemCompo
function := computeFunctionName(component.Name)
stack := getStackInfo(component.Name)

return &SDKProblem{
IBMProblem: IBMErrorf(err, component, summary, discriminator),
ibmProb := IBMErrorf(err, component, summary, discriminator)
newSDKProb := &SDKProblem{
IBMProblem: ibmProb,
Function: function,
stack: stack,
}

// Flatten chains of SDKProblem instances in order to present a single,
// unique error scenario for the SDK context. Multiple Golang components
// can invoke each other, but we only want to track "caused by" problems
// through context boundaries (like API, SDK, Terraform, etc.). This
// eliminates unnecessary granularity of problem scenarios for the SDK
// context. If the problem originated in this library (the Go SDK Core),
// we still want to track that info for debugging purposes.
var sdkCausedBy *SDKProblem
if errors.As(err, &sdkCausedBy) {
// Not a "native" caused by but allows us to maintain compatibility through "Unwrap".
newSDKProb.nativeCausedBy = sdkCausedBy
newSDKProb.causedBy = nil

if isCoreProblem(sdkCausedBy) {
newSDKProb.coreProblem = newSparseSDKProblem(sdkCausedBy)

// If we stored an HTTPProblem instance in the core, we'll want to use
// it as the actual "caused by" problem for the new SDK problem.
if sdkCausedBy.httpProblem != nil {
newSDKProb.causedBy = sdkCausedBy.httpProblem
}
}
}

// We can't use HTTPProblem instances as "caused by" problems for Go SDK Core
// problems because 1) it prevents us from enumerating hashes in the core and
// 2) core problems will almost never be the instances that users interact with
// and the HTTPProblem will need to be used as the "caused by" of the problems
// coming from actual service SDK libraries.
var httpCausedBy *HTTPProblem
if errors.As(err, &httpCausedBy) && isCoreProblem(newSDKProb) {
newSDKProb.httpProblem = httpCausedBy
newSDKProb.causedBy = nil
}

return newSDKProb
}

// RepurposeSDKProblem provides a convenient way to take a problem from
Expand Down
87 changes: 86 additions & 1 deletion core/sdk_problem_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,27 @@ caused_by:
assert.Equal(t, expected, message)
}

func TestSDKProblemGetDebugMessageWithCoreProblem(t *testing.T) {
coreProb := SDKErrorf(nil, "", "", getComponentInfo())
sdkProb := SDKErrorf(coreProb, "Wrong!", "disc", NewProblemComponent("a", "b"))
message := sdkProb.GetDebugMessage()
expected := `---
id: sdk-1518356c
summary: Wrong!
severity: error
function: github.com/IBM/go-sdk-core/v5/core.TestSDKProblemGetDebugMessageWithCoreProblem
component:
name: a
version: b
stack: []
core_problem:
id: sdk-fd790a1d
function: core.TestSDKProblemGetDebugMessageWithCoreProblem
---
`
assert.Equal(t, expected, message)
}

func TestSDKProblemGetID(t *testing.T) {
sdkProb := getPopulatedSDKProblem()
assert.Equal(t, "sdk-32d4ac5e", sdkProb.GetID())
Expand Down Expand Up @@ -170,7 +191,7 @@ func TestSDKErrorf(t *testing.T) {
assert.Equal(t, "github.com/IBM/go-sdk-core/v5/core.TestSDKErrorf", stack[0].Function)
assert.Contains(t, stack[0].File, "core/sdk_problem_test.go")
// This might be too fragile. If it becomes an issue, we can remove it.
assert.Equal(t, 158, stack[0].Line)
assert.Equal(t, 179, stack[0].Line)
}

func TestSDKErrorfNoCausedBy(t *testing.T) {
Expand Down Expand Up @@ -217,6 +238,51 @@ func TestSDKErrorfNoSummary(t *testing.T) {
assert.Contains(t, stack[0].File, "core/sdk_problem_test.go")
}

func TestSDKErrorfDoesntUseSDKCausedBy(t *testing.T) {
sdkProb := getPopulatedSDKProblem();
newSDKProb := SDKErrorf(sdkProb, "", "", NewProblemComponent("a", "b"))
assert.Nil(t, newSDKProb.causedBy)
assert.NotNil(t, newSDKProb.nativeCausedBy)
assert.Equal(t, sdkProb, newSDKProb.nativeCausedBy)
assert.Nil(t, newSDKProb.coreProblem)
}

func TestSDKErrorfStoreCoreProblem(t *testing.T) {
coreProb := SDKErrorf(nil, "", "", getComponentInfo())
sdkProb := SDKErrorf(coreProb, "", "", NewProblemComponent("a", "b"))
assert.Nil(t, sdkProb.causedBy)
assert.NotNil(t, sdkProb.nativeCausedBy)
assert.Equal(t, coreProb, sdkProb.nativeCausedBy)
assert.NotNil(t, sdkProb.coreProblem)
assert.Equal(t, coreProb.GetID(), sdkProb.coreProblem.ID)
assert.Equal(t, coreProb.Function, sdkProb.coreProblem.Function)
}

func TestSDKErrorfHTTPCausedByNotSetForCoreProblem(t *testing.T) {
httpProb := getPopulatedHTTPProblem()
coreProb := SDKErrorf(httpProb, "", "", getComponentInfo())
assert.Nil(t, coreProb.causedBy)
assert.Nil(t, coreProb.nativeCausedBy)
assert.NotNil(t, coreProb.httpProblem)
assert.Equal(t, httpProb, coreProb.httpProblem)
}

func TestSDKErrorfGetHTTPCausedByFromCoreProblem(t *testing.T) {
httpProb := getPopulatedHTTPProblem()

coreProb := SDKErrorf(httpProb, "", "", getComponentInfo())
assert.Nil(t, coreProb.causedBy)
assert.NotNil(t, coreProb.httpProblem)
assert.Equal(t, httpProb, coreProb.httpProblem)

sdkProb := SDKErrorf(coreProb, "", "", NewProblemComponent("a", "b"))
assert.NotNil(t, sdkProb.causedBy)
assert.Equal(t, httpProb, sdkProb.causedBy)

assert.NotNil(t, sdkProb.nativeCausedBy)
assert.Equal(t, coreProb, sdkProb.nativeCausedBy)
}

func TestRepurposeSDKProblem(t *testing.T) {
sdkProb := getPopulatedSDKProblem()
assert.Equal(t, "some-issue", sdkProb.discriminator)
Expand Down Expand Up @@ -254,6 +320,25 @@ func TestSDKProblemIsWithNative(t *testing.T) {
assert.True(t, errors.Is(secondProb, context.Canceled))
}

func TestSDKProblemIsWithCoreProblem(t *testing.T) {
firstProb := SDKErrorf(nil, "msg", "disc", getComponentInfo())
secondProb := SDKErrorf(firstProb, "", "other-disc", NewProblemComponent("a", "b"))
assert.False(t, errors.Is(firstProb, secondProb))
assert.True(t, errors.Is(secondProb, firstProb))
}

func TestNewSparseSDKProblem(t *testing.T) {
sparse := newSparseSDKProblem(getPopulatedSDKProblem())
assert.NotNil(t, sparse)
assert.Equal(t, "sdk-32d4ac5e", sparse.ID)
assert.Equal(t, "mysdk.(*MySdkV1).GetResource", sparse.Function)
}

func TestIsCoreProblem(t *testing.T) {
assert.False(t, isCoreProblem(getPopulatedSDKProblem()))
assert.True(t, isCoreProblem(SDKErrorf(nil, "", "", getComponentInfo())))
}

func getPopulatedSDKProblem() *SDKProblem {
return &SDKProblem{
IBMProblem: &IBMProblem{
Expand Down
16 changes: 16 additions & 0 deletions core/sdk_problem_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,19 @@ func formatFrames(pcs []uintptr, componentName string) []sdkStackFrame {

return result
}

type sparseSDKProblem struct {
ID string
Function string
}

func newSparseSDKProblem(prob *SDKProblem) *sparseSDKProblem {
return &sparseSDKProblem{
ID: prob.GetID(),
Function: prob.Function,
}
}

func isCoreProblem(prob *SDKProblem) bool {
return prob.Component != nil && prob.Component.Name == MODULE_NAME
}