fix: revert to old put/get handler behavior, and fix tests

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))

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)
+		})
+	}
 }