Skip to content
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

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Conversation

gsora
Copy link
Collaborator

@gsora gsora commented Feb 7, 2024

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

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/#/.
@gsora gsora self-assigned this Feb 7, 2024
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5e962d4) 53.41% compared to head (8760b79) 53.49%.
Report is 3 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Comment on lines +252 to +254
if len(e.Methods) != 0 {
handler.Methods(e.Methods...)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(e.Methods) != 0 {
handler.Methods(e.Methods...)
}
handler.Methods(e.Methods...)

i wonder if this could work?

Copy link
Collaborator Author

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) {
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

@xenowits xenowits left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@pinebit pinebit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

sonarcloud bot commented Feb 8, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue

Measures
0 Security Hotspots
No data about Coverage
4.6% Duplication on New Code

See analysis details on SonarCloud

@@ -86,142 +86,172 @@ func NewRouter(ctx context.Context, h Handler, eth2Cl eth2wrap.Client) (*mux.Rou
Name string
Path string
Handler handlerFunc
Methods []string
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

@gsora gsora added the merge when ready Indicates bulldozer bot may merge when all checks pass label Feb 8, 2024
@obol-bulldozer obol-bulldozer bot merged commit 9bcb0b9 into main Feb 8, 2024
12 checks passed
@obol-bulldozer obol-bulldozer bot deleted the gsora/vapi-http-methods branch February 8, 2024 13:59
gsora added a commit that referenced this pull request Feb 8, 2024
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
obol-bulldozer bot pushed a commit that referenced this pull request Feb 9, 2024
Cherry-pick: 
 - #2866
 - #2868
 - #2871
 - #2779
 
category: misc
ticket: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants