-
Notifications
You must be signed in to change notification settings - Fork 90
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
core/validatorapi: handle HTTP methods #2866
Conversation
Instead of handling all HTTP methods for all validator API endpoints, only support a subset of them for each. Selected the appropriate HTTP method by matching them from the Beacon Node API spec: https://ethereum.github.io/beacon-APIs/#/.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2866 +/- ##
==========================================
+ Coverage 53.41% 53.49% +0.08%
==========================================
Files 200 200
Lines 28059 28088 +29
==========================================
+ Hits 14989 15027 +38
+ Misses 11203 11201 -2
+ Partials 1867 1860 -7 ☔ View full report in Codecov by Sentry. |
if len(e.Methods) != 0 { | ||
handler.Methods(e.Methods...) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if len(e.Methods) != 0 { | |
handler.Methods(e.Methods...) | |
} | |
handler.Methods(e.Methods...) |
i wonder if this could work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly no:
package main
import (
"net/http"
"github.com/gorilla/mux"
)
func main() {
r := mux.NewRouter()
r.HandleFunc("/test", func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(r.Method))
}).Methods()
http.ListenAndServe(":9999", r)
}
This demo simulates the case in which e.Methods
is empty.
If you curl -v http://localhost:9999/test
you'll see that the answer is status 405.
@@ -576,6 +576,36 @@ func TestRouter(t *testing.T) { | |||
"dependent_root": dependentRoot, | |||
} | |||
|
|||
t.Run("wrong http method", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any endpoint that doesn't have a list of accepted methods? In that case, I think a test case for such an endpoint could make sense
But, I believe you have added either GET or a POST to all the endpoints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, we must always specify the list of supported methods for each VAPI endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Quality Gate passedIssues Measures |
@@ -86,142 +86,172 @@ func NewRouter(ctx context.Context, h Handler, eth2Cl eth2wrap.Client) (*mux.Rou | |||
Name string | |||
Path string | |||
Handler handlerFunc | |||
Methods []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a slice rather than a string? In this case, we will need to write a single handler which will handle all the http statuses. But if specify just one method per endpoint then we can separate handlers for each method which might be more clean than the current approach. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally when designing APIs like this I found that one might want to handle the same endpoint in different HTTP methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, gorilla/mux
does this as well, using a variadic argument for handler.Methods
.
Instead of handling all HTTP methods for all validator API endpoints, only support a subset of them for each. Selected the appropriate HTTP method by matching them from the Beacon Node API spec: https://ethereum.github.io/beacon-APIs/#/. category: bug ticket: none
Instead of handling all HTTP methods for all validator API endpoints, only support a subset of them for each.
Selected the appropriate HTTP method by matching them from the Beacon Node API spec: https://ethereum.github.io/beacon-APIs/#/.
category: bug
ticket: #2835