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

Ignore response body during HEAD request error handling #1919

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vntw
Copy link
Contributor

@vntw vntw commented Apr 16, 2024

Fixes #1047

This improves the error handling of a HEAD request response. It avoids trying to parse the body entirely. To handle the error, you can still check for a *gitlab.ErrorResponse:

_, _, err := gitlabClient.RepositoryFiles.GetFileMetaData(1, "not-there.txt", &gitlab.GetFileMetaDataOptions{Ref: gitlab.Ptr("main")})
if err != nil {
	var errResp *gitlab.ErrorResponse
	if errors.As(err, &errResp) {
		log.Println(errResp.Response.StatusCode)
	} else {
		log.Println(err)
	}
}

gitlab.go Outdated

if r.Request.Method == http.MethodHead {
return errorResponse
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fail to see how this fixes #1047? This will still return an error, the only difference being that the additional parsing error message is not added.

The main suggestion in #1047 was either not returning an error at all, of returning a specific error which could be found with things like errors.Is().

Any additional feedback or insights on that?

Copy link
Contributor Author

@vntw vntw Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm maybe I misunderstood but this returns exactly what the GitLab API returns but without the futile parsing attempt and the misleading error message (which in my eyes was the main issue).

I don't see why a HEAD request should behave differently for this use case/status code or why there's even a need for a new error type (what's the benefit?) – and alternatively, if this returns nil for both values, how do I know what happened?

The aim of this PR is to allow for the same predictable error handling that other resources share (see my code snippet).

Sorry if I don't get it but that's my reasoning for it, the proposed solutions just didn't make sense to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the commit I just pushed. I think that is a (somewhat) similar solution which does give users to distinguish between a 404 error and a functional/logical error.

The updated check to decide to parse the bode or not should fix issues with "empty" bodies that do not contain readable characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yea thanks for your time and the example, I got what you meant with the custom error, I just don't see a reason to introduce this when everything that's needed to handle these cases is already in the library.

But I don't want to take up more of your time with basically the same argument. If you feel that the custom type approach is better, please do go ahead and use it (or close this PR and see if someone has a better approach in the future, totally fine) 🙂

You're a little bit more familiar with the library and what does/doesn't fit in the picture 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetFileMetaData errors on missing file
2 participants