suborbital / vektor

Opinionated production-grade HTTP server framework for Go
Apache License 2.0
82 stars 8 forks source link

feat(vektor): Rework http server structure to allow middleware wrapping #101

Closed javorszky closed 2 years ago

javorszky commented 2 years ago

Closes #79

This is the version that touches the least amount of code (yes, even with this amount of changes) in a way that still achieves our need to wrap middlewares, while also not painting ourselves in a corner.

New things we can now do

Important changes in different places

HTTP handler signature

It changed from func(request, ctx) (interface{}, error) to func(writer, request, ctx) error.

The main reason for this were two-fold

  1. writer needed to be included, so websocket could be upgraded (websocket is just a spicy GET request)
  2. return needs to be an error only (to be picked up by an error handling middleware, more on this later), which means the response, upon a successful handling of whatever, should be sent from the core handler. We need this because previously we returned an interface, and then a wrapping function did the actual response. There was an http and a websocket version of this, but because I'm making websocket just another http handler, and the http response function was mangling / overriding response codes and whatnot, the websocket 101 upgrade never made it back to the client.

The respond functions

There are a bunch: RespondJSON, RespondString, RespondBytes. They should be called from within the core handler once you're ready to send back data to the client. They has the flexibility that we can choose not to send back anything in case we want to write directly to the responsewriter, such as in the case of a websocket handler.

It also has two extra types: RawString and RawBytes. These exist because the response will json encode things, so if you pass in a naked string, like hello, it will be returned as a json string, which means wrapped in double quotes.

If you send the data into the respond function as a rawstring / rawbytes, it will NOT be json encoded, so the text won't be wrapped in quotes.

The reasoning for that is because I did not adjust any of the tests assertions, and some of them were expecting an unquoted string.

The error middleware

Every outer group should have the error middleware attached to it. What it does is calls the inner handler (core handler + whatever other middleware we wrapped it in (panic should be immediately wrapping it), and if that returns an error, it checks whether it's a trusted error (I repurposed vk.E for this), I return the vk.E json encoded with the status code in it.

Otherwise I don't trust them, so they get a generic 500 internal server error.

And then return nil, because we handled the error.

Websocket

So websockets are just GET requests that get upgraded, and a connection made, and then the connection receives messages and sends messages. Which also means we can use the http handlers here, so an entire branch of 🕸️ 🧦 code is just gone.

There's a convenience wrapper around websocket handlers that takes care of the upgrading and whatnot, but you can write a plain old http handler and upgrade the connection manually if you so wish. It's flexible! And passes the test.

Out of scope for this PR

ctx

I left ctx as is. Technically there are two problems with it:

  1. It should be the very first argument because that's a pattern in Go-the-language
  2. We should be using the standard context.Context rather than vk.Ctx

logs

No change to logging has been done. Currently vk.Ctx has log in it. Once vk.Ctx is gone, logger will be passed into the different middlewares.

tracing middleware

Also out of scope, because this PR is the base work that enables new bits to be attached to everything.

removing custom groups

http tree mux has groups inside of it by default: https://github.com/dimfeld/httptreemux#routing-groups

We don't need to reinvent the wheel.