Response headers are not added to returned error

[NerdJeremia]

Related to: #81

I noticed that the request headers are only being added to the returned error if the function returns prematurely.
Would be nice if someone could take a look at it.

kiota-http-go/nethttp_request_adapter.go

Lines 811 to 813 in b162cb6

	err = errValue.(error)
	spanForAttributes.RecordError(err)
	return err

[baywet]

Hi @NerdJeremia
Thanks for using kiota and for reaching out.
I'm not sure I'm following what the issue here is, would you mind expanding please?

[NerdJeremia]

Hey @baywet,

first of all thanks for your fast response.
Here is the debugger output for some more context:

As you can see the headers are not set in the err which I think is not the wanted behavior and we intended to use them in our application which uses the kiota generated client.

[baywet]

Thanks for the additional information.
I'm not sure why the headers are hoisted at the same level as the error.
Can you share the context of the Get/post/patch/put method please? As well as the error definition?

[NerdJeremia]

Sure. This is the definition of the error

type CmpErrorResponse struct {
    i2ae4187f7daee263371cb1c977df639813ab50ffa529013b7437480d1ec0158f.ApiError
    code *CmpErrorCode
    description *string
    details *string
    stacktrace *string
    typeEscaped *CmpErrorType
}

// ApiError is the parent type for errors thrown by the client when receiving failed responses to its requests
type ApiError struct {
	Message            string
	ResponseStatusCode int
	ResponseHeaders    *ResponseHeaders
}

In my understanding the ResponseHeaders should be in the field ResponseHeaders of the ApiError.
But it seems to me that they are not correctly added in the code I referenced at the top.

[baywet]

Thanks for sharing the additional information.
The fact the ResponseStatusCode is also intriguing. I'm not able to reproduce the behaviour locally.
The best thing is probably to:

1. clone the kiota repositories locally
2. use local references for all of them (add replace in all kiota repos to the abstractions one, add replace in your repo to the kiota ones)
3. debug and step through the code to better understand what's going on.
4. share the findings here.

[NerdJeremia]

Thanks for your reply. I will try it.

[NerdJeremia]

Hey @baywet,

Sorry for the late reply but these are the things I found:

In the throwIfFailedResponse method where the errValue gets set:
errValue, err := rootNode.GetObjectValue(errorCtor)

TheCreateCmpErrorResponseFromDiscriminatorValue function of our generated code gets called which then calls the NewCmpErrorResponse function and that calls the NewApiError function of the https://github.com/microsoft/kiota-abstractions-go package. This creates a new API error with empty ResponseHeaders.

package models

import (
    i2ae4187f7daee263371cb1c977df639813ab50ffa529013b7437480d1ec0158f "github.com/microsoft/kiota-abstractions-go"
    i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91 "github.com/microsoft/kiota-abstractions-go/serialization"
)

type CmpErrorResponse struct {
    i2ae4187f7daee263371cb1c977df639813ab50ffa529013b7437480d1ec0158f.ApiError
    code *CmpErrorCode
    description *string
    details *string
    stacktrace *string
    typeEscaped *CmpErrorType
}
func NewCmpErrorResponse()(*CmpErrorResponse) {
    m := &CmpErrorResponse{
        ApiError: *i2ae4187f7daee263371cb1c977df639813ab50ffa529013b7437480d1ec0158f.NewApiError(),
    }
    return m
}
func CreateCmpErrorResponseFromDiscriminatorValue(parseNode i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseNode)(i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.Parsable, error) {
    return NewCmpErrorResponse(), nil
}
func (m *CmpErrorResponse) Error()(string) {
    return m.ApiError.Error()
}
func (m *CmpErrorResponse) GetCode()(*CmpErrorCode) {
    return m.code
}
func (m *CmpErrorResponse) GetDescription()(*string) {
    return m.description
}
func (m *CmpErrorResponse) GetDetails()(*string) {
    return m.details
}
func (m *CmpErrorResponse) GetFieldDeserializers()(map[string]func(i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseNode)(error)) {
    res := make(map[string]func(i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseNode)(error))
    res["code"] = func (n i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseNode) error {
        val, err := n.GetEnumValue(ParseCmpErrorCode)
        if err != nil {
            return err
        }
        if val != nil {
            m.SetCode(val.(*CmpErrorCode))
        }
        return nil
    }
    res["description"] = func (n i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseNode) error {
        val, err := n.GetStringValue()
        if err != nil {
            return err
        }
        if val != nil {
            m.SetDescription(val)
        }
        return nil
    }
    res["details"] = func (n i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseNode) error {
        val, err := n.GetStringValue()
        if err != nil {
            return err
        }
        if val != nil {
            m.SetDetails(val)
        }
        return nil
    }
    res["stacktrace"] = func (n i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseNode) error {
        val, err := n.GetStringValue()
        if err != nil {
            return err
        }
        if val != nil {
            m.SetStacktrace(val)
        }
        return nil
    }
    res["type"] = func (n i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.ParseNode) error {
        val, err := n.GetEnumValue(ParseCmpErrorType)
        if err != nil {
            return err
        }
        if val != nil {
            m.SetTypeEscaped(val.(*CmpErrorType))
        }
        return nil
    }
    return res
}
func (m *CmpErrorResponse) GetStacktrace()(*string) {
    return m.stacktrace
}
func (m *CmpErrorResponse) GetTypeEscaped()(*CmpErrorType) {
    return m.typeEscaped
}
func (m *CmpErrorResponse) Serialize(writer i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.SerializationWriter)(error) {
    if m.GetCode() != nil {
        cast := (*m.GetCode()).String()
        err := writer.WriteStringValue("code", &cast)
        if err != nil {
            return err
        }
    }
    {
        err := writer.WriteStringValue("description", m.GetDescription())
        if err != nil {
            return err
        }
    }
    {
        err := writer.WriteStringValue("details", m.GetDetails())
        if err != nil {
            return err
        }
    }
    {
        err := writer.WriteStringValue("stacktrace", m.GetStacktrace())
        if err != nil {
            return err
        }
    }
    if m.GetTypeEscaped() != nil {
        cast := (*m.GetTypeEscaped()).String()
        err := writer.WriteStringValue("type", &cast)
        if err != nil {
            return err
        }
    }
    return nil
}
func (m *CmpErrorResponse) SetCode(value *CmpErrorCode)() {
    m.code = value
}
func (m *CmpErrorResponse) SetDescription(value *string)() {
    m.description = value
}
func (m *CmpErrorResponse) SetDetails(value *string)() {
    m.details = value
}
func (m *CmpErrorResponse) SetStacktrace(value *string)() {
    m.stacktrace = value
}
func (m *CmpErrorResponse) SetTypeEscaped(value *CmpErrorType)() {
    m.typeEscaped = value
}
// CmpErrorResponseable 
type CmpErrorResponseable interface {
    i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.Parsable
    GetCode()(*CmpErrorCode)
    GetDescription()(*string)
    GetDetails()(*string)
    GetStacktrace()(*string)
    GetTypeEscaped()(*CmpErrorType)
    SetCode(value *CmpErrorCode)()
    SetDescription(value *string)()
    SetDetails(value *string)()
    SetStacktrace(value *string)()
    SetTypeEscaped(value *CmpErrorType)()
}

These then never get set to hold the values of the actual response headers in the throwIfFailedResponse method (at least in our case).
I was able to set them afterwards using reflection like this:

err = errValue.(error)
spanForAttributes.RecordError(err)
errRef := setApiErrorResponse(err, response, responseHeaders)
if errRef != nil {
		// Handle the error
		return err
}
return err

func setApiErrorResponse(errValue error, response *nethttp.Response, responseHeaders *abs.ResponseHeaders) error {
	val := reflect.ValueOf(errValue).Elem() // Elem returns the value that the pointer 'errValue' points to

	// Check if the struct has the field 'ApiError'
	apiErrorField := val.FieldByName("ApiError")
	if !apiErrorField.IsValid() {
		return &abs.ApiError{
			Message:            "ApiError field could not be set",
			ResponseStatusCode: response.StatusCode,
			ResponseHeaders:    responseHeaders,
		}
	}

	// Construct the ApiError we want to set
	apiError := abs.ApiError{
		ResponseStatusCode: response.StatusCode,
		ResponseHeaders:    responseHeaders,
	}

	// Set the field - this requires that the field is settable (exported)
	if apiErrorField.CanSet() {
		apiErrorField.Set(reflect.ValueOf(apiError))
	} else {
		return &abs.ApiError{
			Message:            "ApiError field could not be set",
			ResponseStatusCode: response.StatusCode,
			ResponseHeaders:    responseHeaders,
		}
	}

	return nil
}

I don’t think it is ideal but I was able to set the proper response headers that way.

[baywet]

Thanks for the additional information @NerdJeremia
You are correct, the request adapter should be setting the headers here like it does at line 800

kiota-http-go/nethttp_request_adapter.go

Line 811 in 243b44c

err = errValue.(error)

I don't think there's a need for reflection, a type assertion should be enough like we're doing above.

Would you be willing to submit a pull request to correct the issue?

[NerdJeremia]

Thanks for the reply.
Yes I can fix the issue but I don't think using type assertion will work since the error returned at the end is of a custom type from the generated code which will be different for everyone I guess.

I think we can either use reflection or pass the response headers through all the functions to where the new ApiError gets created.

[baywet]

Something you might not have noticed is that all custom errors inherit from ApiError. So type assertions should work (pointer to pointer)

[NerdJeremia]

Am I missing something?
This is what I tried:

err = errValue.(error)
test := err.(*abs.ApiError)
test.ResponseHeaders = responseHeaders

[baywet]

Right, because it's actually one of the member types that's an API error.
What if we:

1. introduced an ApiErrorable interface at the abstractions level
2. had two methods (SetResponseHeaders, SetStatusCode) defined in the interface
3. had ApiError implement those methods
4. use that interface for the type assertion here
5. (because custom error types derive from API error, they should match that interface)

Thoughts?

[NerdJeremia]

Sounds like a plan I will take a look

[NerdJeremia]

Hi @baywet,

I tested it locally and it worked! Thanks for the suggestion.
I opened a PR in the abstractions repo: microsoft/kiota-abstractions-go#123
After that I will create a PR in this repo to fix the issue.