Skip to content

Commit

Permalink
plumbing: Do not swallow http message coming from VCS providers.
Browse files Browse the repository at this point in the history
For diagnostics reasons we want to surface error messages coming from VCS providers.
That's why we introduce the reason field to Err struct in http package.
This field can be used by an end user of the library in order to better understand failures.
  • Loading branch information
matejrisek committed Sep 5, 2023
1 parent cd3a21c commit 5ad72db
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 4 deletions.
18 changes: 16 additions & 2 deletions plumbing/transport/http/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -406,14 +406,28 @@ func (a *TokenAuth) String() string {
// Err is a dedicated error to return errors based on status code
type Err struct {
Response *http.Response
Reason string
}

// NewErr returns a new Err based on a http response
// NewErr returns a new Err based on a http response and closes response body
// if needed
func NewErr(r *http.Response) error {
if r.StatusCode >= http.StatusOK && r.StatusCode < http.StatusMultipleChoices {
return nil
}

var reason string

// If a response message is present, add it to error
var messageBuffer bytes.Buffer
if r.Body != nil {
messageLength, _ := messageBuffer.ReadFrom(r.Body)
if messageLength > 0 {
reason = messageBuffer.String()
}
_ = r.Body.Close()
}

switch r.StatusCode {
case http.StatusUnauthorized:
return transport.ErrAuthenticationRequired
Expand All @@ -423,7 +437,7 @@ func NewErr(r *http.Response) error {
return transport.ErrRepositoryNotFound
}

return plumbing.NewUnexpectedError(&Err{r})
return plumbing.NewUnexpectedError(&Err{r, reason})
}

// StatusCode returns the status code of the response
Expand Down
19 changes: 19 additions & 0 deletions plumbing/transport/http/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package http
import (
"crypto/tls"
"fmt"
"io"
"log"
"net"
"net/http"
Expand All @@ -14,6 +15,7 @@ import (
"strings"
"testing"

"github.com/go-git/go-git/v5/plumbing"
"github.com/go-git/go-git/v5/plumbing/transport"

fixtures "github.com/go-git/go-git-fixtures/v4"
Expand Down Expand Up @@ -90,6 +92,23 @@ func (s *ClientSuite) TestNewHTTPError40x(c *C) {
"unexpected client error.*")
}

func (s *ClientSuite) TestNewUnexpectedError(c *C) {
res := &http.Response{
StatusCode: 500,
Body: io.NopCloser(strings.NewReader("Unexpected error")),
}

err := NewErr(res)
c.Assert(err, NotNil)
c.Assert(err, FitsTypeOf, &plumbing.UnexpectedError{})

unexpectedError, _ := err.(*plumbing.UnexpectedError)
c.Assert(unexpectedError.Err, FitsTypeOf, &Err{})

httpError, _ := unexpectedError.Err.(*Err)
c.Assert(httpError.Reason, Equals, "Unexpected error")
}

func (s *ClientSuite) Test_newSession(c *C) {
cl := NewClientWithOptions(nil, &ClientOptions{
CacheMaxEntries: 2,
Expand Down
1 change: 0 additions & 1 deletion plumbing/transport/http/receive_pack.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ func (s *rpSession) doRequest(
}

if err := NewErr(res); err != nil {
_ = res.Body.Close()
return nil, err
}

Expand Down
1 change: 0 additions & 1 deletion plumbing/transport/http/upload_pack.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ func (s *upSession) doRequest(
}

if err := NewErr(res); err != nil {
_ = res.Body.Close()
return nil, err
}

Expand Down

0 comments on commit 5ad72db

Please sign in to comment.