API SSZ content negotiation #10760
API SSZ content negotiation #10760
Conversation
| if !ok { | ||
| return false | ||
| func sszRequested(req *http.Request) (bool, error) { | ||
| acceptableTypes := []contenttype.MediaType{ |
There was a problem hiding this comment.
Could we accomplish this without a new dependency? It doesn't seem like this library is doing much heavy lifting for us.
There was a problem hiding this comment.
+1 , less dependencies the better.
| @@ -193,17 +201,53 @@ func handlePostSSZ( | |||
| return true | |||
| } | |||
|
|
|||
| func sszRequested(req *http.Request) bool { | |||
| func sszRequested(req *http.Request) (bool, error) { | |||
| accept, ok := req.Header["Accept"] | |||
There was a problem hiding this comment.
nitpick: would be slightly more concise if you used https://pkg.go.dev/net/http#Header.Values
accept := req.Header.Values("Accept")
if len(accept) == 0 {
return false, nil
}
| return true | ||
| types := strings.Split(accept[0], ",") | ||
| // match a number with optional decimals | ||
| regex, err := regexp.Compile("q=\\d(\\.\\d)?") |
There was a problem hiding this comment.
This only needs to be compiled once for the life of the program - you can avoid recompiling on every call by declaring a variable above the function in package scope, using https://pkg.go.dev/regexp#MustCompile
| params := values[1] | ||
| match := regex.Find([]byte(params)) | ||
| if match != nil { | ||
| priority, err := strconv.ParseFloat(strings.Split(string(match), "=")[1], 32) |
There was a problem hiding this comment.
If you're going to take on the overhead of using a regex I would suggest simplifying things by using the String regexp methods and using a capture group to extract the desired value, via https://pkg.go.dev/regexp#Regexp.FindAllStringSubmatch
prioRx := regexp.MustCompile(`q=(\d+(?:\.\d+)?)`) // note that I added a `+` to the `\d` shorthands, otherwise you'll only capture the first character
pg := prioRx.FindAllStringSubmatch(params, 1)
if len(pg) != 1 {
continue
}
priority, err := strconv.ParseFloat(pg[0][1])
...
more self-contained example: https://go.dev/play/p/NRswRIwv2Tr
| if err != nil { | ||
| return false, err | ||
| } | ||
| currentType, currentPriority := "", 1.0 |
There was a problem hiding this comment.
Won't this cause the initial empty value to take precedence over a string where all values include a priority? I think this causes a bug; if you had an accept string like application/octet-stream;q=0.95,application/json;q=0.9", the function would return false.
I think initializing currentPriority to 0 is going to simplify things. It would fix the bug just mentioned. You also won't need to check if currentType == "" { on line 227, because the priority comparison will implicitly handle that.
| assert.Equal(t, true, result) | ||
| }) | ||
|
|
||
| t.Run("other_content_type_preferred", func(t *testing.T) { | ||
| request := httptest.NewRequest("GET", "http://foo.example", nil) | ||
| request.Header["Accept"] = []string{fmt.Sprintf("%s,%s;q=0.9", jsonMediaType, octetStreamMediaType)} |
There was a problem hiding this comment.
Please add a test case like application/octet-stream;q=0.95,application/json;q=0.9
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
The way we currently check if a request expects an SSZ response is insufficient. The
AcceptHTTP header can be formed in many ways and we should support the syntax from RFC 7231.Which issues(s) does this PR fix?
Fixes #10743