Exclude nil Response from response body

[stoneman]

Fixes: https://github.com/migueleliasweb/go-github-mock/issues/6

[migueleliasweb]

Hi @stoneman ,

I think I've caught up with this issue. It took me a while to understand it once again.

I think you've nailed the problem and I also checked that creating this simplified error response does mitigate the current problem of using the errors.

Have said that, now that we know where the problem comes from, I've found suuuuper weird that the ErrorResponse in the Github SDK doesn have a JSON tag like json:"omitempty" link.

Is that possible that this is just an issue with the SDK itself?

With that struct the way it is right now, it's basically impossible (when just using the SDK) to create an error message that works correctly as the Response field will always be present. Am I right?

I'm happy to mitigate the problem with your fix but I reckon we should take this to the folks in the golang github sdk repo.

Any thoughts?

[stoneman]

There is no json: tag for Response so I don't think it's expected for the body to ever contain response data.

Presumably, other types of errors (none of which ever have Response properties) are serialized into the body by the GitHub API and ErrorResponse objects are only ever created on the client side where the SDK is the only thing that ever sets their Response property.

Maybe I should change the name of the struct from githubErrorResponse to gitHubApiError or something and change the comment to note that while it will be deserialized into an ErrorResponse those should not be created server side.

[migueleliasweb]

There is no json: tag for Response so I don't think it's expected for the body to ever contain response data.

That's exactly the point. Since the github.ErrorResponse is missing the json tag and there is a pointer to a struct as on the attributes, that attribute when not set by the server (go-github-mock) will be shown as nil after the marshalling process. That's what breaks the .Error() call in the SDK as it tries to access an attribute that was overwritten with a nil.

Presumably, other types of errors (none of which ever have Response properties) are serialized into the body by the GitHub API and ErrorResponse objects are only ever created on the client side where the SDK is the only thing that ever sets their Response property.

Exactly. Github itself never returns the attribute Response set. Note that this differs from the current implementation of github.ErrorResponse. From my point of view, this is an issue of the SDK when modeling the responses from the Github's API.

Maybe I should change the name of the struct from githubErrorResponse to gitHubApiError or something and change the comment to note that while it will be deserialized into an ErrorResponse those should not be created server side.

I still think the problem is in the SDK and go-githuib-mock is actually doing the right thing. The only missing piece of the puzzle seems to the json tag that will fix the Marshalling.

[migueleliasweb]

I've added a small PR to go-github and Glen already approved it. We're just waiting for another approval to merge it to master.

I reckon if that gets merged to master, it's a win and we should fix this issue.

https://github.com/google/go-github/pull/2641

If, for some reason we are unable to merge it to master, we can then mitigate the issue with this new struct. So, at least we unblock the .Error() calls from mocked responses.

[migueleliasweb]

Hi @stoneman , I just got the fix merged in go-github and it seems the issue is now fixed on go-github-mock as well.

Thanks for the insights about this issue!

See: https://github.com/migueleliasweb/go-github-mock/pull/45

[stoneman]

Ah, I see - thanks for getting this fixed 🙂