Skip to content

use http.Error instead of separately writing error code and message #1564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions api/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,9 +377,7 @@ func lockMiddleware(
func rejectMiddleware(handler http.Handler, ctx *snow.ConsensusContext) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // If chain isn't done bootstrapping, ignore API calls
if ctx.State.Get().State != snow.NormalOp {
w.WriteHeader(http.StatusServiceUnavailable)
// Doesn't matter if there's an error while writing. They'll get the StatusServiceUnavailable code.
_, _ = w.Write([]byte("API call rejected because chain is not done bootstrapping"))
http.Error(w, "API call rejected because chain is not done bootstrapping", http.StatusServiceUnavailable)
} else {
handler.ServeHTTP(w, r)
}
Expand Down
72 changes: 72 additions & 0 deletions api/server/server_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Copyright (C) 2019-2023, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package server

import (
"net/http"
"net/http/httptest"
"testing"

"github.com/stretchr/testify/require"

"github.com/ava-labs/avalanchego/snow"
)

func TestRejectMiddleware(t *testing.T) {
type test struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(No action required) Code golf:

// Using an anonymous struct allows defining type and tests in one statement
tests := []struct {
  name string
  ...
// This bracket style avoids unnecessary indentation
}{{
  name: foo,
  ...
}, {
  name: bar,
  ...
}}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - we aren't consistent in the repo about if we use anonymous structs for testing or named structs. I don't feel strongly

name string
handlerFunc func(*require.Assertions) http.Handler
state snow.State
expectedStatusCode int
}

tests := []test{
{
name: "chain is state syncing",
handlerFunc: func(require *require.Assertions) http.Handler {
return http.HandlerFunc(func(http.ResponseWriter, *http.Request) {
require.Fail("shouldn't have called handler")
})
},
state: snow.StateSyncing,
expectedStatusCode: http.StatusServiceUnavailable,
},
{
name: "chain is bootstrapping",
handlerFunc: func(require *require.Assertions) http.Handler {
return http.HandlerFunc(func(http.ResponseWriter, *http.Request) {
require.Fail("shouldn't have called handler")
})
},
state: snow.Bootstrapping,
expectedStatusCode: http.StatusServiceUnavailable,
},
{
name: "chain is done bootstrapping",
handlerFunc: func(*require.Assertions) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusTeapot)
})
},
state: snow.NormalOp,
expectedStatusCode: http.StatusTeapot,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
require := require.New(t)

ctx := &snow.ConsensusContext{}
ctx.State.Set(snow.EngineState{
State: tt.state,
})

middleware := rejectMiddleware(tt.handlerFunc(require), ctx)
w := httptest.NewRecorder()
middleware.ServeHTTP(w, nil)
require.Equal(tt.expectedStatusCode, w.Code)
})
}
}