diff --git a/server/server.go b/server/server.go index 50886cd..3df7164 100644 --- a/server/server.go +++ b/server/server.go @@ -146,19 +146,23 @@ func (svr *Server) Health(w http.ResponseWriter, _ *http.Request) error { return nil } +// HandleGet handles the GET request for commitments. +// Note: even when an error is returned, the commitment meta is still returned, +// because it is needed for metrics (see the WithMetrics middleware). +// TODO: we should change this behavior and instead use a custom error that contains the commitment meta. func (svr *Server) HandleGet(w http.ResponseWriter, r *http.Request) (commitments.CommitmentMeta, error) { meta, err := ReadCommitmentMeta(r) if err != nil { err = fmt.Errorf("invalid commitment mode: %w", err) svr.WriteBadRequest(w, err) - return commitments.CommitmentMeta{}, err + return meta, err } key := path.Base(r.URL.Path) comm, err := commitments.StringToDecodedCommitment(key, meta.Mode) if err != nil { err = fmt.Errorf("failed to decode commitment from key %v (commitment mode %v): %w", key, meta.Mode, err) svr.WriteBadRequest(w, err) - return commitments.CommitmentMeta{}, err + return meta, err } input, err := svr.router.Get(r.Context(), comm, meta.Mode) @@ -169,26 +173,30 @@ func (svr *Server) HandleGet(w http.ResponseWriter, r *http.Request) (commitment } else { svr.WriteInternalError(w, err) } - return commitments.CommitmentMeta{}, err + return meta, err } svr.WriteResponse(w, input) return meta, nil } +// HandlePut handles the PUT request for commitments. +// Note: even when an error is returned, the commitment meta is still returned, +// because it is needed for metrics (see the WithMetrics middleware). +// TODO: we should change this behavior and instead use a custom error that contains the commitment meta. func (svr *Server) HandlePut(w http.ResponseWriter, r *http.Request) (commitments.CommitmentMeta, error) { meta, err := ReadCommitmentMeta(r) if err != nil { err = fmt.Errorf("invalid commitment mode: %w", err) svr.WriteBadRequest(w, err) - return commitments.CommitmentMeta{}, err + return meta, err } input, err := io.ReadAll(r.Body) if err != nil { err = fmt.Errorf("failed to read request body: %w", err) svr.WriteBadRequest(w, err) - return commitments.CommitmentMeta{}, err + return meta, err } key := path.Base(r.URL.Path) @@ -199,7 +207,7 @@ func (svr *Server) HandlePut(w http.ResponseWriter, r *http.Request) (commitment if err != nil { err = fmt.Errorf("failed to decode commitment from key %v (commitment mode %v): %w", key, meta.Mode, err) svr.WriteBadRequest(w, err) - return commitments.CommitmentMeta{}, err + return meta, err } } @@ -214,7 +222,7 @@ func (svr *Server) HandlePut(w http.ResponseWriter, r *http.Request) (commitment if err != nil { err = fmt.Errorf("failed to encode commitment %v (commitment mode %v): %w", commitment, meta.Mode, err) svr.WriteInternalError(w, err) - return commitments.CommitmentMeta{}, err + return meta, err } svr.log.Info(fmt.Sprintf("write commitment: %x\n", comm)) diff --git a/server/server_test.go b/server/server_test.go index 14e8fec..863375a 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -85,7 +85,7 @@ func TestGetHandler(t *testing.T) { expectedCode: http.StatusInternalServerError, expectedBody: "", expectError: true, - expectedCommitmentMeta: commitments.CommitmentMeta{}, + expectedCommitmentMeta: commitments.CommitmentMeta{Mode: commitments.OptimismGeneric, CertVersion: 0}, }, { name: "Success - OP Keccak256", @@ -107,7 +107,7 @@ func TestGetHandler(t *testing.T) { expectedCode: http.StatusInternalServerError, expectedBody: "", expectError: true, - expectedCommitmentMeta: commitments.CommitmentMeta{}, + expectedCommitmentMeta: commitments.CommitmentMeta{Mode: commitments.OptimismAltDA, CertVersion: 0}, }, { name: "Success - OP Alt-DA", @@ -129,13 +129,6 @@ func TestGetHandler(t *testing.T) { req := httptest.NewRequest(http.MethodGet, tt.url, nil) rec := httptest.NewRecorder() - t.Run("CheckMiddlewaresNoPanic", func(_ *testing.T) { - // we also run the request through the middlewares, to make sure no panic occurs - // could happen if there's a problem with the metrics. For eg, in the past we saw - // panic: inconsistent label cardinality: expected 3 label values but got 1 in []string{"GET"} - handler := WithLogging(WithMetrics(server.HandleGet, server.m), server.log) - handler(rec, req) - }) meta, err := server.HandleGet(rec, req) if tt.expectError { require.Error(t, err) @@ -146,6 +139,20 @@ func TestGetHandler(t *testing.T) { require.Equal(t, tt.expectedCode, rec.Code) require.Equal(t, tt.expectedCommitmentMeta, meta) require.Equal(t, tt.expectedBody, rec.Body.String()) + + }) + } + for _, tt := range tests { + t.Run(tt.name+"/CheckMiddlewaresNoPanic", func(_ *testing.T) { + tt.mockBehavior() + + req := httptest.NewRequest(http.MethodGet, tt.url, nil) + rec := httptest.NewRecorder() + // we also run the request through the middlewares, to make sure no panic occurs + // could happen if there's a problem with the metrics. For eg, in the past we saw + // panic: inconsistent label cardinality: expected 3 label values but got 1 in []string{"GET"} + handler := WithLogging(WithMetrics(server.HandleGet, server.m), server.log) + handler(rec, req) }) } } @@ -261,14 +268,6 @@ func TestPutHandler(t *testing.T) { req := httptest.NewRequest(http.MethodPut, tt.url, bytes.NewReader(tt.body)) rec := httptest.NewRecorder() - t.Run("CheckMiddlewaresNoPanic", func(_ *testing.T) { - // we also run the request through the middlewares, to make sure no panic occurs - // could happen if there's a problem with the metrics. For eg, in the past we saw - // panic: inconsistent label cardinality: expected 3 label values but got 1 in []string{"GET"} - handler := WithLogging(WithMetrics(server.HandleGet, server.m), server.log) - handler(rec, req) - }) - meta, err := server.HandlePut(rec, req) if tt.expectError { require.Error(t, err) @@ -282,4 +281,18 @@ func TestPutHandler(t *testing.T) { require.Equal(t, tt.expectedCommitmentMeta, meta) }) } + + for _, tt := range tests { + t.Run(tt.name+"/CheckMiddlewaresNoPanic", func(_ *testing.T) { + tt.mockBehavior() + + req := httptest.NewRequest(http.MethodGet, tt.url, nil) + rec := httptest.NewRecorder() + // we also run the request through the middlewares, to make sure no panic occurs + // could happen if there's a problem with the metrics. For eg, in the past we saw + // panic: inconsistent label cardinality: expected 3 label values but got 1 in []string{"GET"} + handler := WithLogging(WithMetrics(server.HandlePut, server.m), server.log) + handler(rec, req) + }) + } }