Support OPTIONS requests #3885
Support OPTIONS requests #3885
Conversation
- register endpoints with supported methods - support OPTIONS requests, indicating supported methods - extract method validation (error 405) from individual endpoints - on 405 where multiple methods are allowed, create a single Allow header with comma-separated values, not multiple Allow headers.
agent/http.go
Outdated
| if endpoints == nil { | ||
| endpoints = make(map[string]unboundEndpoint) | ||
| } | ||
| if endpoints[pattern] != nil { | ||
| if allowedMethods == nil { |
There was a problem hiding this comment.
Move creating the map to the init function, rather than a lazy initialization of this map here
There was a problem hiding this comment.
I moved this here. Did I understand you correctly? Should endpoints move too?
agent/http_test.go
Outdated
| } else if optionsStr != "OPTIONS,GET,POST" { | ||
| t.Fatalf("options method should set 'Allow' header correctly") | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you change this and TestMethodNotAllowed to loop through all the endpoints in allowedMethods and check their response codes/headers (instead of just the query/kv ones)?
There was a problem hiding this comment.
I realised from your comments that existing tests existed in http_oss_test.go so I moved my testing there.
This exposed an issue with specific prepared queries, since OPTIONS should return different methods if the path ends /execute or /explain. I got around that by allowing endpoints to register with no methods if they want to handle OPTIONS/NotFoundErrors themselves (this commit). I'm open to other solutions if this isn't the cleanest way.
|
Thanks, I don't have much go experience so I'm grateful for all pointers. I'll update with these suggestions in the next couple of days. |
agent/http_oss_test.go
Outdated
| a.Agent.LogWriter = logger.NewLogWriter(512) | ||
| defer a.Shutdown() | ||
|
|
||
| for _, tt := range expectedEndpoints { |
There was a problem hiding this comment.
Given that allowedMethods is package local and visible to this test there's no need to create another map. instead, you could loop over the keys and values in allowedMethods making the same OPTIONS call and verifying the list of options returned matches the values in allowedMethods.
There was a problem hiding this comment.
The problem with that is that
{"OPTIONS,GET", "/v1/query/xxx/execute"},
{"OPTIONS,GET", "/v1/query/xxx/explain"},
won't be tested, and it won't actually check that allowedMethods was correctly defined. Is that OK?
There was a problem hiding this comment.
Good point. However this test method also relies on expectedEndpoints being comprehensively defined, so won't catch things that we forget to add here as we add more end points.
I suggest explicitly adding test cases for the special cases like /execute/ and /explain in this method after looping through allowedMethods. Also add a comment line for it explaining why the special case lines exist.
Any future end points (not special cased) added to the http server will be tested automatically without us having to maintain two lists.
|
Hi all, any more requested changes? |
|
thanks @eddsteel |
header with comma-separated values, not multiple Allow headers.
This is based on #1957 and fixes #865 (and anything else that expects OPTIONS to work).
I made the allowed methods a parameter of
wrapso that they wouldn't need to be looked up in the map for every call ofwrap, but it does change the signature and makes some tests a bit uglier. LMK if you'd preferwrapto look it up instead, or any other improvements.