Support custom responses from event handlers (#21)

This introduces two new mechanisms for customizing responses:

  * Clients can configure a response callback to send custom responses
    for all events.
  * Individual handlers may call SetResponder() to send custom responses
    in specific situations.

While there should be little need to customize responses, I believe some
combination of these two options will satisfy most use cases. Note that
introducing the new callback changes the signature of NewEventDispatcher
and some other types; most applications are not using these directly.

On the other hand, I chose to introduce SetResponder() to avoid breaking
the EventHandler interface, even though it is more idiomatic to either
provide the handler with access to the request and response writer or
allow it to return more information.

Author: bluekeyes

README.md

@@ -251,6 +251,25 @@ secure `StateStore` implementation. `oauth2.SessionStateStore` is a good choice
 that uses [alexedwards/scs](https://github.com/alexedwards/scs) to store the
 state in a session.
 
+## Customizing Webhook Responses
+
+For most applications, the default responses should be sufficient: they use
+correct status codes and include enough information to match up GitHub delivery
+records with request logs. If your application has additional requirements for
+responses, two methods are provided for customization:
+
+- Error responses can be modified with a custom error callback. Use the
+  `WithErrorCallback` option when creating an event dispatcher.
+
+- Non-error responses can be modified with a custom response callback. Use the
+  `WithResponseCallback` option when creating an event dispatcher.
+
+- Individual hook responses can be modified by calling the `SetResponder`
+  function before the handler returns. Note that if you register a custom
+  response handler as described above, you must make it aware of handler-level
+  responders if you want to keep using `SetResponder`. See the default response
+  callback for an example of how to implement this.
+
 ## Stability and Versioning Guarantees
 
 While we've used this library to build multiple applications internally,

githubapp/dispatcher.go

@@ -41,16 +41,47 @@ type EventHandler interface {
 	Handle(ctx context.Context, eventType, deliveryID string, payload []byte) error
 }
 
-type ErrorHandler func(http.ResponseWriter, *http.Request, error)
+// ErrorCallback is called when an event handler returns an error. The error
+// from the handler is passed directly as the final argument.
+type ErrorCallback func(w http.ResponseWriter, r *http.Request, err error)
+
+// ResponseCallback is called to send a response to GitHub after an event is
+// handled. It is passed the event type and a flag indicating if an event
+// handler was called for the event.
+type ResponseCallback func(w http.ResponseWriter, r *http.Request, event string, handled bool)
+
+// DispatcherOption configures properties of an event dispatcher.
+type DispatcherOption func(*eventDispatcher)
+
+// WithErrorCallback sets the error callback for an event dispatcher.
+func WithErrorCallback(onError ErrorCallback) DispatcherOption {
+	return func(d *eventDispatcher) {
+		if onError != nil {
+			d.onError = onError
+		}
+	}
+}
+
+// WithResponseCallback sets the response callback for an event dispatcher.
+func WithResponseCallback(onResponse ResponseCallback) DispatcherOption {
+	return func(d *eventDispatcher) {
+		if onResponse != nil {
+			d.onResponse = onResponse
+		}
+	}
+}
 
 type eventDispatcher struct {
 	handlerMap map[string]EventHandler
 	secret     string
-	onError    ErrorHandler
+
+	onError    ErrorCallback
+	onResponse ResponseCallback
 }
 
-// NewDefaultEventDispatcher is a convenience method to create an
-// EventDispatcher from configuration using the default error handler.
+// NewDefaultEventDispatcher is a convenience method to create an event
+// dispatcher from configuration using the default error and response
+// callbacks.
 func NewDefaultEventDispatcher(c Config, handlers ...EventHandler) http.Handler {
 	return NewEventDispatcher(handlers, c.App.WebhookSecret, nil)
 }
@@ -59,10 +90,9 @@ func NewDefaultEventDispatcher(c Config, handlers ...EventHandler) http.Handler
 // requests to the appropriate event handlers. It validates payload integrity
 // using the given secret value.
 //
-// If an error occurs during handling, the error handler is called with the
-// error and should write an appropriate response. If the error handler is nil,
-// a default handler is used.
-func NewEventDispatcher(handlers []EventHandler, secret string, onError ErrorHandler) http.Handler {
+// Responses are controlled by optional error and response callbacks. If these
+// options are not provided, default callbacks are used.
+func NewEventDispatcher(handlers []EventHandler, secret string, opts ...DispatcherOption) http.Handler {
 	handlerMap := make(map[string]EventHandler)
 
 	// Iterate in reverse so the first entries in the slice have priority
@@ -72,35 +102,46 @@ func NewEventDispatcher(handlers []EventHandler, secret string, onError ErrorHan
 		}
 	}
 
-	if onError == nil {
-		onError = DefaultErrorHandler
-	}
-
-	return &eventDispatcher{
+	d := &eventDispatcher{
 		handlerMap: handlerMap,
 		secret:     secret,
-		onError:    onError,
+		onError:    DefaultErrorCallback,
+		onResponse: DefaultResponseCallback,
 	}
+
+	for _, opt := range opts {
+		opt(d)
+	}
+
+	return d
 }
 
-// ServeHTTP to implement http.Handler
+// ServeHTTP processes a webhook request from GitHub.
 func (d *eventDispatcher) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
 
+	// initialize context for SetResponder/GetResponder
+	// we store a pointer in the context so that functions deeper in the call
+	// tree can modify the value without creating a new context
+	var responder func(http.ResponseWriter, *http.Request)
+	ctx = context.WithValue(ctx, responderKey{}, &responder)
+	r = r.WithContext(ctx)
+
 	eventType := r.Header.Get("X-GitHub-Event")
+	deliveryID := r.Header.Get("X-GitHub-Delivery")
+
 	if eventType == "" {
 		// ACK payload that was received but won't be processed
 		w.WriteHeader(http.StatusAccepted)
 		return
 	}
-	deliveryID := r.Header.Get("X-GitHub-Delivery")
 
 	logger := zerolog.Ctx(ctx).With().
 		Str(LogKeyEventType, eventType).
 		Str(LogKeyDeliveryID, deliveryID).
 		Logger()
 
-	// update context and request to contain new log fields
+	// initialize context with event logger
 	ctx = logger.WithContext(ctx)
 	r = r.WithContext(ctx)
 
@@ -111,28 +152,65 @@ func (d *eventDispatcher) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	logger.Info().Msgf("Received webhook event")
-	handler, ok := d.handlerMap[eventType]
 
-	switch {
-	case ok:
+	handler, ok := d.handlerMap[eventType]
+	if ok {
 		if err := handler.Handle(ctx, eventType, deliveryID, payloadBytes); err != nil {
-			// pass error directly so handler can inspect types if needed
 			d.onError(w, r, err)
 			return
 		}
-		w.WriteHeader(http.StatusOK)
-	case eventType == "ping":
-		w.WriteHeader(http.StatusOK)
-	default:
-		w.WriteHeader(http.StatusAccepted)
 	}
+	d.onResponse(w, r, eventType, ok)
 }
 
-// DefaultErrorHandler logs errors and responds with a 500 status code.
-func DefaultErrorHandler(w http.ResponseWriter, r *http.Request, err error) {
+// DefaultErrorCallback logs errors and responds with a 500 status code.
+func DefaultErrorCallback(w http.ResponseWriter, r *http.Request, err error) {
 	logger := zerolog.Ctx(r.Context())
 	logger.Error().Err(err).Msg("Unexpected error handling webhook request")
+	http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
+}
 
-	msg := http.StatusText(http.StatusInternalServerError)
-	http.Error(w, msg, http.StatusInternalServerError)
+// DefaultResponseCallback responds with a 200 OK for handled events and a 202
+// Accepted status for all other events. By default, responses are empty.
+// Event handlers may send custom responses by calling the SetResponder
+// function before returning.
+func DefaultResponseCallback(w http.ResponseWriter, r *http.Request, event string, handled bool) {
+	if !handled && event != "ping" {
+		w.WriteHeader(http.StatusAccepted)
+		return
+	}
+
+	if res := GetResponder(r.Context()); res != nil {
+		res(w, r)
+	} else {
+		w.WriteHeader(http.StatusOK)
+	}
+}
+
+type responderKey struct{}
+
+// SetResponder sets a function that sends a response to GitHub after event
+// processing completes. This function may only be called from event handler
+// functions invoked by the event dispatcher.
+//
+// Customizing individual handler responses should be rare. Applications that
+// want to modify the standard responses should consider registering a response
+// callback before using this function.
+func SetResponder(ctx context.Context, responder func(http.ResponseWriter, *http.Request)) {
+	r, ok := ctx.Value(responderKey{}).(*func(http.ResponseWriter, *http.Request))
+	if !ok || r == nil {
+		panic("SetResponder() must be called from an event handler invoked by the go-githubapp event dispatcher")
+	}
+	*r = responder
+}
+
+// GetResponder returns the response function that was set by an event handler.
+// If no response function exists, it returns nil. There is usually no reason
+// to call this outside of a response callback implementation.
+func GetResponder(ctx context.Context) func(http.ResponseWriter, *http.Request) {
+	r, ok := ctx.Value(responderKey{}).(*func(http.ResponseWriter, *http.Request))
+	if !ok || r == nil {
+		return nil
+	}
+	return *r
 }