fix: address review feedback

filecoin-project · Oct 19, 2023 · acb6424 · acb6424
1 parent 703ddbc
commit acb6424
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 37 deletions.
diff --git a/cmd/booster-http/e2e_test.go b/cmd/booster-http/e2e_test.go
@@ -133,23 +133,20 @@ func TestE2E(t *testing.T) {
 		{"/foo/bar/fizz.mov", rootEnt.Children[0].Children[0].Children[2].Content, "video/quicktime"},
 		{"/qux.html", rootEnt.Children[1].Content, "text/html"},
 	} {
-		byts, ct, code, err := curl(fmt.Sprintf("http://0.0.0.0:%d/ipfs/%s%s", bifrostPort, rootEnt.Root.String(), fetch.path))
-		req.NoError(err)
+		byts, ct, code := requireHttpResponse(t, fmt.Sprintf("http://0.0.0.0:%d/ipfs/%s%s", bifrostPort, rootEnt.Root.String(), fetch.path))
 		req.Equal(http.StatusOK, code)
 		req.Equal(fetch.expect, byts)
 		req.Equal(fetch.expectType, ct)
 	}
 
 	t.Log("Perform some curl requests to bifrost-gateway that should fail")
 
-	byts, ct, code, err := curl(fmt.Sprintf("http://0.0.0.0:%d/ipfs/%s/nope", bifrostPort, rootEnt.Root.String()))
-	req.NoError(err)
+	byts, ct, code := requireHttpResponse(t, fmt.Sprintf("http://0.0.0.0:%d/ipfs/%s/nope", bifrostPort, rootEnt.Root.String()))
 	req.Equal(http.StatusNotFound, code)
 	req.Contains(string(byts), "failed to resolve")
 	req.Equal("text/plain; charset=utf-8", ct)
 
-	byts, ct, code, err = curl(fmt.Sprintf("http://0.0.0.0:%d/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi", bifrostPort))
-	req.NoError(err)
+	byts, ct, code = requireHttpResponse(t, fmt.Sprintf("http://0.0.0.0:%d/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi", bifrostPort))
 	req.Equal(http.StatusBadGateway, code)
 	req.Contains(string(byts), "failed to resolve")
 	req.Equal("text/plain; charset=utf-8", ct)
@@ -196,15 +193,11 @@ func verifyCarContains(t *testing.T, carFilepath string, wantCids []cid.Cid) {
 }
 
 // simulate a curl to the url and return the body bytes, content type and status code
-func curl(to string) ([]byte, string, int, error) {
+func requireHttpResponse(t *testing.T, to string) ([]byte, string, int) {
 	req, err := http.Get(to)
-	if err != nil {
-		return nil, "", 0, err
-	}
+	require.NoError(t, err)
 	defer req.Body.Close()
 	byts, err := io.ReadAll(req.Body)
-	if err != nil {
-		return nil, "", 0, err
-	}
-	return byts, req.Header.Get("Content-Type"), req.StatusCode, nil
+	require.NoError(t, err)
+	return byts, req.Header.Get("Content-Type"), req.StatusCode
 }
diff --git a/cmd/booster-http/http_test.go b/cmd/booster-http/http_test.go
@@ -53,7 +53,7 @@ func TestNewHttpServer(t *testing.T) {
 
 		t.Logf("%s %s %s %s %d %s %d %s %s %s", ts.Format(time.RFC3339), remoteAddr, method, url.String(), status, duration, bytes, compressionRatio, userAgent, msg)
 		requestCount++
-		req.Equal("GET", method)
+		req.Equal(http.MethodGet, method)
 		req.Equal("-", compressionRatio)
 
 		switch requestCount {
@@ -87,7 +87,7 @@ func TestNewHttpServer(t *testing.T) {
 	req.Equal(200, resp.StatusCode)
 
 	// Create a request with Cors header
-	request, err := http.NewRequest("GET", fmt.Sprintf("http://localhost:%d/", port), nil)
+	request, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://localhost:%d/", port), nil)
 	req.NoError(err)
 	request.Header.Add("Origin", "test")
 	client := new(http.Client)
@@ -98,7 +98,7 @@ func TestNewHttpServer(t *testing.T) {
 	req.Equal("*", response.Header.Get("Access-Control-Allow-Origin"))
 
 	// Test an error condition
-	request, err = http.NewRequest("GET", fmt.Sprintf("http://localhost:%d/piece/bafynotacid!", port), nil)
+	request, err = http.NewRequest(http.MethodGet, fmt.Sprintf("http://localhost:%d/piece/bafynotacid!", port), nil)
 	req.NoError(err)
 	response, err = client.Do(request)
 	req.NoError(err)
@@ -219,7 +219,7 @@ func TestHttpGzipResponse(t *testing.T) {
 
 			logHandler := func(ts time.Time, remoteAddr, method string, url url.URL, status int, duration time.Duration, bytes int, compressionRatio, userAgent, msg string) {
 				t.Logf("%s %s %s %s %d %s %d %s %s %s", ts.Format(time.RFC3339), remoteAddr, method, url.String(), status, duration, bytes, compressionRatio, userAgent, msg)
-				req.Equal("GET", method)
+				req.Equal(http.MethodGet, method)
 				if url.Path == "/" { // waitServerUp
 					return
 				}
@@ -258,7 +258,7 @@ func TestHttpGzipResponse(t *testing.T) {
 			}()
 
 			{ // test /piece retrieval
-				request, err := http.NewRequest("GET", fmt.Sprintf("http://localhost:%d/piece/%s", port, testPieceCid), nil)
+				request, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://localhost:%d/piece/%s", port, testPieceCid), nil)
 				request.Header.Set("Accept", "application/vnd.ipld.car")
 				if tc.acceptGzip {
 					request.Header.Set("Accept-Encoding", "gzip")
@@ -301,7 +301,7 @@ func TestHttpGzipResponse(t *testing.T) {
 			}
 
 			{ // test /ipfs CAR retrieval
-				request, err := http.NewRequest("GET", fmt.Sprintf("http://localhost:%d/ipfs/%s", port, rootEnt.Root.String()), nil)
+				request, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://localhost:%d/ipfs/%s", port, rootEnt.Root.String()), nil)
 				request.Header.Set("Accept", "application/vnd.ipld.car")
 				if tc.acceptGzip {
 					request.Header.Set("Accept-Encoding", "gzip")

diff --git a/cmd/booster-http/multiminer_retrieval_test.go b/cmd/booster-http/multiminer_retrieval_test.go
@@ -34,7 +34,7 @@ func TestMultiMinerHttpRetrieval(t *testing.T) {
 
 		runAndWaitForBoosterHttp(ctx, t, []string{miner1ApiInfo, miner2ApiInfo}, fullNode2ApiInfo, port)
 
-		req, err := http.NewRequest("GET", fmt.Sprintf("http://localhost:%d/ipfs/%s", port, rt.RootCid.String()), nil)
+		req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://localhost:%d/ipfs/%s", port, rt.RootCid.String()), nil)
 		require.NoError(t, err)
 		req.Header.Set("Accept", "application/vnd.ipld.car")
 		resp, err := http.DefaultClient.Do(req)
@@ -100,7 +100,8 @@ func waitForHttp(ctx context.Context, t *testing.T, port int, waitForCode int, w
 }
 
 func runBoosterHttp(ctx context.Context, t *testing.T, minerApiInfo []string, fullNodeApiInfo string, lidApiInfo string, args ...string) error {
-	args = append([]string{"booster-http",
+	args = append([]string{
+		"booster-http",
 		"--repo=" + t.TempDir(),
 		"--vv",
 		"run",

diff --git a/cmd/booster-http/piecehandler.go b/cmd/booster-http/piecehandler.go
@@ -126,7 +126,7 @@ func serveContent(res http.ResponseWriter, req *http.Request, content io.ReadSee
 	// Note that the last modified time is a constant value because the data
 	// in a piece identified by a cid will never change.
 
-	if req.Method == "HEAD" {
+	if req.Method == http.MethodHead {
 		// For an HTTP HEAD request ServeContent doesn't send any data (just headers)
 		http.ServeContent(res, req, "", time.Time{}, content)
 		return
@@ -140,16 +140,15 @@ func serveContent(res http.ResponseWriter, req *http.Request, content io.ReadSee
 // Unfortunately we can't always use errors.Is() because the error might
 // have crossed an RPC boundary.
 func isNotFoundError(err error) bool {
-	if errors.Is(err, ErrNotFound) {
+	switch {
+	case errors.Is(err, ErrNotFound),
+		errors.Is(err, datastore.ErrNotFound),
+		errors.Is(err, retrievalmarket.ErrNotFound),
+		strings.Contains(strings.ToLower(err.Error()), "not found"):
 		return true
+	default:
+		return false
 	}
-	if errors.Is(err, datastore.ErrNotFound) {
-		return true
-	}
-	if errors.Is(err, retrievalmarket.ErrNotFound) {
-		return true
-	}
-	return strings.Contains(strings.ToLower(err.Error()), "not found")
 }
 
 func writeError(w http.ResponseWriter, r *http.Request, status int, msg error) {
@@ -193,7 +192,6 @@ func (s *HttpServer) unsealedDeal(ctx context.Context, pieceCid cid.Cid, pieceDe
 		} else {
 			dealSectors = append(dealSectors, fmt.Sprintf("Deal %d: Sector %d", di.ChainDealID, di.SectorID))
 		}
-
 	}
 
 	if allErr == nil {

diff --git a/cmd/booster-http/run.go b/cmd/booster-http/run.go
@@ -90,7 +90,9 @@ var runCmd = &cli.Command{
 			Usage:  "(removed option)",
 			Hidden: true,
 			Action: func(ctx *cli.Context, _ bool) error {
-				fmt.Fprintf(ctx.App.ErrWriter, "--serve-blocks is no longer supported.\n\n%s\n\n", trustlessMessage)
+				if _, err := fmt.Fprintf(ctx.App.ErrWriter, "--serve-blocks is no longer supported.\n\n%s\n\n", trustlessMessage); err != nil {
+					return err
+				}
 				return errors.New("--serve-blocks is no longer supported, use bifrost-gateway instead")
 			},
 		},
@@ -104,7 +106,9 @@ var runCmd = &cli.Command{
 			Usage:  "(removed option)",
 			Hidden: true,
 			Action: func(ctx *cli.Context, _ bool) error {
-				fmt.Fprintf(ctx.App.ErrWriter, "--serve-files is no longer supported.\n\n%s\n\n", trustlessMessage)
+				if _, err := fmt.Fprintf(ctx.App.ErrWriter, "--serve-files is no longer supported.\n\n%s\n\n", trustlessMessage); err != nil {
+					return err
+				}
 				return errors.New("--serve-files is no longer supported, use bifrost-gateway instead")
 			},
 		},
@@ -254,7 +258,7 @@ var runCmd = &cli.Command{
 		case "-":
 			opts.LogWriter = cctx.App.Writer
 		default:
-			opts.LogWriter, err = os.OpenFile(cctx.String("log-file"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+			opts.LogWriter, err = os.OpenFile(cctx.String("log-file"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644)
 			if err != nil {
 				return err
 			}
@@ -324,7 +328,7 @@ func createRepoDir(repoDir string) (string, error) {
 	if repoDir == "" {
 		return "", fmt.Errorf("%s is a required flag", FlagRepo.Name)
 	}
-	return repoDir, os.MkdirAll(repoDir, 0744)
+	return repoDir, os.MkdirAll(repoDir, 0o744)
 }
 
 type serverApi struct {

diff --git a/cmd/booster-http/trustless_gateway_test.go b/cmd/booster-http/trustless_gateway_test.go
@@ -82,7 +82,6 @@ func TestTrustlessGateway(t *testing.T) {
 			}
 		})
 	}
-
 }
 
 // dealTestCarInParts creates deals for the given CAR file in 7M chunks since we