lib.NewRequestHandler removes boilerplate from api package #1682
Conversation
b5
changed the title
| // switch on HTTP verbs. I think we should use tricks like this that leverage | ||
| // the gorilla/mux package until we get a better sense of how our API uses | ||
| // HTTP verbs | ||
| func handleRefRouteMethods(m *mux.Router, ae lib.APIEndpoint, f http.HandlerFunc, methods ...string) { |
api/api_test.go
Outdated
| @@ -251,7 +251,7 @@ func TestServerReadOnlyRoutes(t *testing.T) { | |||
| {"POST", "/registry/profile/new", 403}, | |||
| {"POST", "/registry/profile/prove", 403}, | |||
| {"GET", "/checkout/", 403}, | |||
| {"GET", "/status/", 403}, | |||
| // {"GET", "/status/", 403}, | |||
There was a problem hiding this comment.
This should work with {"GET", "/status", 403}, ie remove the trailing / as gorilla is strict about slashes by default.
| // UnmarshalParams deserialzes a lib req params stuct pointer from an HTTP | ||
| // request | ||
| // | ||
| // Deprecated: This is only exported for one-off handlers in the API package |
lib/http.go
Outdated
| } | ||
|
|
||
| if err := UnmarshalParams(r, p); err != nil { | ||
| log.Debugw("unmarshal dataset save error", "err", err) |
There was a problem hiding this comment.
This seems like an odd message, maybe unmarshal request params error
| @@ -107,7 +107,7 @@ const ( | |||
| // fsi endpoints | |||
|
|
|||
| // AEStatus returns the filesystem dataset status | |||
| AEStatus = APIEndpoint("/status/{path:.*}") | |||
| AEStatus = APIEndpoint("/status") | |||
There was a problem hiding this comment.
Does this mean /status cannot be used with a reference? That seems to contradict the unit test just above in api/fsi_test.go. Also, though that's how it works currently, it's actually a bug that should be fixed, see https://github.com/qri-io/qri/blob/master/cmd/status.go#L70 and #1246.
There was a problem hiding this comment.
It can, but the route expansion is handled by handleRefRouteMethods in api/api.go where it registers all the variants, this basically just represents the "route prefix" as we used to carry around.
So for now it works pretty much as it used to, still contradicting #1246 and is a fix for another day/PR I guess.
Leveraging our new patterns to have lib begin to generate it's own request handlers
b5
force-pushed
the
b5/dispatch_request_handler
branch
from
cf03f29
to
5ef1821
Compare
rework link params processing & add method-specific-and-ref-specific handler function
b5
force-pushed
the
b5/dispatch_request_handler
branch
from
5ef1821
to
27c187c
Compare
building on ideas from #1650, this adds the first method where the HTTP handler is generated by package lib, but mapped to an endpoint by package api.
This PR is mainly to establish the patten & talk through repercussions. Right now this only works for formats where the response format is well known / singular.