Regression: 404 responses are not handled again

[papkos]

Handling of 404 Not found responses from CommerceTools has a regression: It was not handled properly earlier, fixed at some point, and now it isn't again.

Commit introducing proper handling: 2a3c377
Released in v1.3.0

Commit removing it again: 2463f65#diff-d37bd1a31dfd77757a89090c321d69489ef49983e0270f0a1b82627f3494f854L95 (deep link to a big diff, sorry)

Can you please re-introduce it?

---

Also, would you consider not including the entire request response in the GenericRequestError? It is potentially leaking resources (open body (IO reader) and maybe more?), and I think the content already contains enough information (can be parsed as other types of errors).

result := GenericRequestError{
	StatusCode: resp.StatusCode,
	Content:    content,
	Response:   resp,
}

Edit: I meant response, not request, sorry

[demeyerthom]

Hi @papkos I will look into this tomorrow!

[demeyerthom]

Hi @papkos took some searching, but found that we had generated v1.3.0 with a local copy of the codegen tool where we had made the changes. These however had not been merged back. Did that now, the not found error is restored again: https://github.com/labd/commercetools-go-sdk/releases/tag/v1.5.1

With regards to your second comment: i am not entirely clear what you mean with including the Request? It should be available under GenericRequestError.Response.Request? Or am I missing something:

[papkos]

Hi, firstly, thanks for the quick fix&release! 🎉

As for this:

With regards to your second comment: i am not entirely clear what you mean with including the Request? It should be available under GenericRequestError.Response.Request? Or am I missing something:

I meant response, not request, sorry.
And I meant not including it in the error struct, because it's quite a heavy object.
Also, it is not an easily serializable struct, because it has functions, IO streams, etc. We discovered this because our error logging failed (we do nest/wrap errors, then we use mapstructure to marshal it into a simple map then JSON for structured logging. The response field made this fail)

Why is it needed/useful? The body/content and the status code is already there, is there some usecase for having the entire response?