Skip to content

Conversation

gsora
Copy link
Contributor

@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 requested review from pinebit, dB2510 and xenowits February 7, 2024 16:59
@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
Contributor 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.

"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
Contributor 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
Collaborator

@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

@gsora gsora requested review from pinebit and xenowits February 8, 2024 09:11
Copy link

sonarqubecloud 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

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
Contributor 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
Contributor 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
@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