Refactor lfs requests (#26783)

- Refactor lfs request code - The original code uses `performRequest` function to create the request, uses a callback to modify the request, and then send the request. - Now it's replaced with `createRequest` that only creates request and `performRequest` that only sends the request. - Reuse `createRequest` and `performRequest` in `http_client.go` and `transferadapter.go` --------- Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
go-gitea · Sep 18, 2023 · 9631958 · 9631958
1 parent a50d9af
commit 9631958
Show file tree

Hide file tree

Showing 6 changed files with 124 additions and 135 deletions.
diff --git a/modules/lfs/filesystem_client.go b/modules/lfs/filesystem_client.go
@@ -15,7 +15,7 @@ import (
 
 // FilesystemClient is used to read LFS data from a filesystem path
 type FilesystemClient struct {
-	lfsdir string
+	lfsDir string
 }
 
 // BatchSize returns the preferred size of batchs to process
@@ -25,16 +25,12 @@ func (c *FilesystemClient) BatchSize() int {
 
 func newFilesystemClient(endpoint *url.URL) *FilesystemClient {
 	path, _ := util.FileURLToPath(endpoint)
-
-	lfsdir := filepath.Join(path, "lfs", "objects")
-
-	client := &FilesystemClient{lfsdir}
-
-	return client
+	lfsDir := filepath.Join(path, "lfs", "objects")
+	return &FilesystemClient{lfsDir}
 }
 
 func (c *FilesystemClient) objectPath(oid string) string {
-	return filepath.Join(c.lfsdir, oid[0:2], oid[2:4], oid)
+	return filepath.Join(c.lfsDir, oid[0:2], oid[2:4], oid)
 }
 
 // Download reads the specific LFS object from the target path

diff --git a/modules/lfs/http_client.go b/modules/lfs/http_client.go
@@ -8,6 +8,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"net/http"
 	"net/url"
 	"strings"
@@ -17,7 +18,7 @@ import (
 	"code.gitea.io/gitea/modules/proxy"
 )
 
-const batchSize = 20
+const httpBatchSize = 20
 
 // HTTPClient is used to communicate with the LFS server
 // https://github.com/git-lfs/git-lfs/blob/main/docs/api/batch.md
@@ -29,7 +30,7 @@ type HTTPClient struct {
 
 // BatchSize returns the preferred size of batchs to process
 func (c *HTTPClient) BatchSize() int {
-	return batchSize
+	return httpBatchSize
 }
 
 func newHTTPClient(endpoint *url.URL, httpTransport *http.Transport) *HTTPClient {
@@ -43,28 +44,25 @@ func newHTTPClient(endpoint *url.URL, httpTransport *http.Transport) *HTTPClient
 		Transport: httpTransport,
 	}
 
+	basic := &BasicTransferAdapter{hc}
 	client := &HTTPClient{
-		client:    hc,
-		endpoint:  strings.TrimSuffix(endpoint.String(), "/"),
-		transfers: make(map[string]TransferAdapter),
+		client:   hc,
+		endpoint: strings.TrimSuffix(endpoint.String(), "/"),
+		transfers: map[string]TransferAdapter{
+			basic.Name(): basic,
+		},
 	}
 
-	basic := &BasicTransferAdapter{hc}
-
-	client.transfers[basic.Name()] = basic
-
 	return client
 }
 
 func (c *HTTPClient) transferNames() []string {
 	keys := make([]string, len(c.transfers))
-
 	i := 0
 	for k := range c.transfers {
 		keys[i] = k
 		i++
 	}
-
 	return keys
 }
 
@@ -74,40 +72,24 @@ func (c *HTTPClient) batch(ctx context.Context, operation string, objects []Poin
 	url := fmt.Sprintf("%s/objects/batch", c.endpoint)
 
 	request := &BatchRequest{operation, c.transferNames(), nil, objects}
-
 	payload := new(bytes.Buffer)
 	err := json.NewEncoder(payload).Encode(request)
 	if err != nil {
 		log.Error("Error encoding json: %v", err)
 		return nil, err
 	}
 
-	log.Trace("Calling: %s", url)
-
-	req, err := http.NewRequestWithContext(ctx, "POST", url, payload)
+	req, err := createRequest(ctx, http.MethodPost, url, map[string]string{"Content-Type": MediaType}, payload)
 	if err != nil {
-		log.Error("Error creating request: %v", err)
 		return nil, err
 	}
-	req.Header.Set("Content-type", MediaType)
-	req.Header.Set("Accept", MediaType)
 
-	res, err := c.client.Do(req)
+	res, err := performRequest(ctx, c.client, req)
 	if err != nil {
-		select {
-		case <-ctx.Done():
-			return nil, ctx.Err()
-		default:
-		}
-		log.Error("Error while processing request: %v", err)
 		return nil, err
 	}
 	defer res.Body.Close()
 
-	if res.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("Unexpected server response: %s", res.Status)
-	}
-
 	var response BatchResponse
 	err = json.NewDecoder(res.Body).Decode(&response)
 	if err != nil {
@@ -177,7 +159,7 @@ func (c *HTTPClient) performOperation(ctx context.Context, objects []Pointer, dc
 			link, ok := object.Actions["upload"]
 			if !ok {
 				log.Debug("%+v", object)
-				return errors.New("Missing action 'upload'")
+				return errors.New("missing action 'upload'")
 			}
 
 			content, err := uc(object.Pointer, nil)
@@ -187,8 +169,6 @@ func (c *HTTPClient) performOperation(ctx context.Context, objects []Pointer, dc
 
 			err = transferAdapter.Upload(ctx, link, object.Pointer, content)
 
-			content.Close()
-
 			if err != nil {
 				return err
 			}
@@ -203,7 +183,7 @@ func (c *HTTPClient) performOperation(ctx context.Context, objects []Pointer, dc
 			link, ok := object.Actions["download"]
 			if !ok {
 				log.Debug("%+v", object)
-				return errors.New("Missing action 'download'")
+				return errors.New("missing action 'download'")
 			}
 
 			content, err := transferAdapter.Download(ctx, link)
@@ -219,3 +199,59 @@ func (c *HTTPClient) performOperation(ctx context.Context, objects []Pointer, dc
 
 	return nil
 }
+
+// createRequest creates a new request, and sets the headers.
+func createRequest(ctx context.Context, method, url string, headers map[string]string, body io.Reader) (*http.Request, error) {
+	log.Trace("createRequest: %s", url)
+	req, err := http.NewRequestWithContext(ctx, method, url, body)
+	if err != nil {
+		log.Error("Error creating request: %v", err)
+		return nil, err
+	}
+
+	for key, value := range headers {
+		req.Header.Set(key, value)
+	}
+	req.Header.Set("Accept", MediaType)
+
+	return req, nil
+}
+
+// performRequest sends a request, optionally performs a callback on the request and returns the response.
+// If the status code is 200, the response is returned, and it will contain a non-nil Body.
+// Otherwise, it will return an error, and the Body will be nil or closed.
+func performRequest(ctx context.Context, client *http.Client, req *http.Request) (*http.Response, error) {
+	log.Trace("performRequest: %s", req.URL)
+	res, err := client.Do(req)
+	if err != nil {
+		select {
+		case <-ctx.Done():
+			return res, ctx.Err()
+		default:
+		}
+		log.Error("Error while processing request: %v", err)
+		return res, err
+	}
+
+	if res.StatusCode != http.StatusOK {
+		defer res.Body.Close()
+		return res, handleErrorResponse(res)
+	}
+
+	return res, nil
+}
+
+func handleErrorResponse(resp *http.Response) error {
+	var er ErrorResponse
+	err := json.NewDecoder(resp.Body).Decode(&er)
+	if err != nil {
+		if err == io.EOF {
+			return io.ErrUnexpectedEOF
+		}
+		log.Error("Error decoding json: %v", err)
+		return err
+	}
+
+	log.Trace("ErrorResponse: %v", er)
+	return errors.New(er.Message)
+}
diff --git a/modules/lfs/http_client_test.go b/modules/lfs/http_client_test.go
@@ -177,7 +177,7 @@ func TestHTTPClientDownload(t *testing.T) {
 		// case 0
 		{
 			endpoint:      "https://status-not-ok.io",
-			expectederror: "Unexpected server response: ",
+			expectederror: io.ErrUnexpectedEOF.Error(),
 		},
 		// case 1
 		{
@@ -207,7 +207,7 @@ func TestHTTPClientDownload(t *testing.T) {
 		// case 6
 		{
 			endpoint:      "https://empty-actions-map.io",
-			expectederror: "Missing action 'download'",
+			expectederror: "missing action 'download'",
 		},
 		// case 7
 		{
@@ -217,27 +217,28 @@ func TestHTTPClientDownload(t *testing.T) {
 		// case 8
 		{
 			endpoint:      "https://upload-actions-map.io",
-			expectederror: "Missing action 'download'",
+			expectederror: "missing action 'download'",
 		},
 		// case 9
 		{
 			endpoint:      "https://verify-actions-map.io",
-			expectederror: "Missing action 'download'",
+			expectederror: "missing action 'download'",
 		},
 		// case 10
 		{
 			endpoint:      "https://unknown-actions-map.io",
-			expectederror: "Missing action 'download'",
+			expectederror: "missing action 'download'",
 		},
 	}
 
 	for n, c := range cases {
 		client := &HTTPClient{
-			client:    hc,
-			endpoint:  c.endpoint,
-			transfers: make(map[string]TransferAdapter),
+			client:   hc,
+			endpoint: c.endpoint,
+			transfers: map[string]TransferAdapter{
+				"dummy": dummy,
+			},
 		}
-		client.transfers["dummy"] = dummy
 
 		err := client.Download(context.Background(), []Pointer{p}, func(p Pointer, content io.ReadCloser, objectError error) error {
 			if objectError != nil {
@@ -284,7 +285,7 @@ func TestHTTPClientUpload(t *testing.T) {
 		// case 0
 		{
 			endpoint:      "https://status-not-ok.io",
-			expectederror: "Unexpected server response: ",
+			expectederror: io.ErrUnexpectedEOF.Error(),
 		},
 		// case 1
 		{
@@ -319,7 +320,7 @@ func TestHTTPClientUpload(t *testing.T) {
 		// case 7
 		{
 			endpoint:      "https://download-actions-map.io",
-			expectederror: "Missing action 'upload'",
+			expectederror: "missing action 'upload'",
 		},
 		// case 8
 		{
@@ -329,22 +330,23 @@ func TestHTTPClientUpload(t *testing.T) {
 		// case 9
 		{
 			endpoint:      "https://verify-actions-map.io",
-			expectederror: "Missing action 'upload'",
+			expectederror: "missing action 'upload'",
 		},
 		// case 10
 		{
 			endpoint:      "https://unknown-actions-map.io",
-			expectederror: "Missing action 'upload'",
+			expectederror: "missing action 'upload'",
 		},
 	}
 
 	for n, c := range cases {
 		client := &HTTPClient{
-			client:    hc,
-			endpoint:  c.endpoint,
-			transfers: make(map[string]TransferAdapter),
+			client:   hc,
+			endpoint: c.endpoint,
+			transfers: map[string]TransferAdapter{
+				"dummy": dummy,
+			},
 		}
-		client.transfers["dummy"] = dummy
 
 		err := client.Upload(context.Background(), []Pointer{p}, func(p Pointer, objectError error) (io.ReadCloser, error) {
 			return io.NopCloser(new(bytes.Buffer)), objectError

diff --git a/modules/lfs/pointer.go b/modules/lfs/pointer.go
@@ -29,10 +29,10 @@ const (
 
 var (
 	// ErrMissingPrefix occurs if the content lacks the LFS prefix
-	ErrMissingPrefix = errors.New("Content lacks the LFS prefix")
+	ErrMissingPrefix = errors.New("content lacks the LFS prefix")
 
 	// ErrInvalidStructure occurs if the content has an invalid structure
-	ErrInvalidStructure = errors.New("Content has an invalid structure")
+	ErrInvalidStructure = errors.New("content has an invalid structure")
 
 	// ErrInvalidOIDFormat occurs if the oid has an invalid format
 	ErrInvalidOIDFormat = errors.New("OID has an invalid format")