From acb6424a5b8439cad55cc155aff17c876d6885a1 Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Wed, 11 Oct 2023 17:32:29 +1100 Subject: [PATCH] fix: address review feedback --- cmd/booster-http/e2e_test.go | 21 +++++++------------ cmd/booster-http/http_test.go | 12 +++++------ cmd/booster-http/multiminer_retrieval_test.go | 5 +++-- cmd/booster-http/piecehandler.go | 18 +++++++--------- cmd/booster-http/run.go | 12 +++++++---- cmd/booster-http/trustless_gateway_test.go | 1 - 6 files changed, 32 insertions(+), 37 deletions(-) diff --git a/cmd/booster-http/e2e_test.go b/cmd/booster-http/e2e_test.go index 4e5be6dab..4ee6ff24a 100644 --- a/cmd/booster-http/e2e_test.go +++ b/cmd/booster-http/e2e_test.go @@ -133,8 +133,7 @@ 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) @@ -142,14 +141,12 @@ func TestE2E(t *testing.T) { 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 index 687581802..89d811477 100644 --- 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 index 2610163b4..4b4e7325e 100644 --- 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 index 3f203be0a..005c20551 100644 --- 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 index 582870cb3..d6b162adb 100644 --- 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 index de8eb574c..e2866dfcf 100644 --- 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