influxdata / influxdb

Scalable datastore for metrics, events, and real-time analytics
https://influxdata.com
Apache License 2.0
28.71k stars 3.54k forks source link

Refactor APIHandler to use router instead of a bunch of conditionals #13320

Closed desa closed 4 years ago

desa commented 5 years ago

Currently the http.APIHandler does route checking with conditionals that do prefix matching on the route

https://github.com/influxdata/influxdb/blob/f56c42794b7bf6c51052b86d30a8daa8826fae9f/http/api_handler.go#L209-L222

This pattern forces http handlers to push all routes that start with a common prefix to be defined together like so https://github.com/influxdata/influxdb/blob/f56c42794b7bf6c51052b86d30a8daa8826fae9f/http/bucket_service.go#L61-L69 https://github.com/influxdata/influxdb/blob/f56c42794b7bf6c51052b86d30a8daa8826fae9f/http/bucket_service.go#L86-L122

Ideally we'd be able to separate out the endpoints that handle buckets directly with the endpoints that deal with labels/members/etc. This way we could separate out the concerns of each handler.

I propose that we require that http handlers expose the HTTP verbs and routes that they expose. This way we can do something like

// NewAPIHandler constructs all api handlers beneath it and returns an APIHandler
func NewAPIHandler(b *APIBackend) *APIHandler {
    h := &APIHandler{router: httprouter.New()}
        ...
    bucketBackend := NewBucketBackend(b)
    bucketBackend.BucketService = authorizer.NewBucketService(b.BucketService)
    h.BucketHandler = NewBucketHandler(bucketBackend)
        h.Register(h.BucketHandler.Routes())
        ...
        h.Register(h.BucketLabelsHandler.Routes())
        ...
}

This way all of the routes for the buckets will point to the bucket handler and all the bucket label routes will point to the bucket label handler.

Additionally, since we added a router to the api handler we'll no longer need to implement ServeHTTP.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 5 years ago

This issue has been automatically closed because it has not had recent activity. Please reopen if this issue is still important to you. Thank you for your contributions.

imogenkinsman commented 5 years ago

Reopening. I like this idea a lot, and it won't take significant effort to implement.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 4 years ago

This issue has been automatically closed because it has not had recent activity. Please reopen if this issue is still important to you. Thank you for your contributions.