Repository: hyperledger/firefly
Pull request #1156 (2023-01-24 14:59:30)

Fix panic on webhooks without messages
Resolves https://github.com/hyperledger/firefly/issues/1155

This PR changes the behavior of webhook callbacks in several ways:

- If `reply` is specified on the subscription it no longer panics if there is no Message on the event (such as is the case with custom contract events)
- If `withData` is specified subscription, but no Message is on the event, the event payload itself will still be sent

This means that subscriptions can be setup with either the `reply` and `withData` fields set (or both) and still use the same subscription for custom contract events as well.


Patch:
diff --git a/internal/events/webhooks/webhooks.go b/internal/events/webhooks/webhooks.go
index 1d9facbb2..4bc7a2fcb 100644
--- a/internal/events/webhooks/webhooks.go
+++ b/internal/events/webhooks/webhooks.go
@@ -211,18 +211,19 @@ func (wh *WebHooks) attemptRequest(sub *core.Subscription, event *core.EventDeli
 
 	if req.method == http.MethodPost || req.method == http.MethodPatch || req.method == http.MethodPut {
 		switch {
-		case !withData:
-			// We are just sending the event itself
-			req.r.SetBody(event)
 		case req.body != nil:
 			// We might have been told to extract a body from the first data record
 			req.r.SetBody(req.body)
 		case len(allData) > 1:
 			// We've got an array of data to POST
 			req.r.SetBody(allData)
-		default:
-			// Otherwise just send the first object directly
+		case len(allData) == 1:
+			// Just send the first object directly
 			req.r.SetBody(firstData)
+		default:
+			// Just send the event itself
+			req.r.SetBody(event)
+
 		}
 	}
 
@@ -292,7 +293,7 @@ func (wh *WebHooks) doDelivery(connID string, reply bool, sub *core.Subscription
 	log.L(wh.ctx).Tracef("Webhook response: %s", string(b))
 
 	// Emit the response
-	if reply {
+	if reply && event.Message != nil {
 		txType := fftypes.FFEnum(strings.ToLower(sub.Options.TransportOptions().GetString("replytx")))
 		if req != nil && req.replyTx != "" {
 			txType = fftypes.FFEnum(strings.ToLower(req.replyTx))
@@ -332,13 +333,8 @@ func (wh *WebHooks) doDelivery(connID string, reply bool, sub *core.Subscription
 }
 
 func (wh *WebHooks) DeliveryRequest(connID string, sub *core.Subscription, event *core.EventDelivery, data core.DataArray) error {
-	if event.Message == nil && sub.Options.WithData != nil && *sub.Options.WithData {
-		log.L(wh.ctx).Debugf("Webhook withData=true subscription called with non-message event '%s'", event.ID)
-		return nil
-	}
-
 	reply := sub.Options.TransportOptions().GetBool("reply")
-	if reply && event.Message.Header.CID != nil {
+	if reply && event.Message != nil && event.Message.Header.CID != nil {
 		// We cowardly refuse to dispatch a message that is itself a reply, as it's hard for users to
 		// avoid loops - and there's no way for us to detect here if a user has configured correctly
 		// to avoid a loop.


Test patch:
diff --git a/internal/events/webhooks/webhooks_test.go b/internal/events/webhooks/webhooks_test.go
index aab2cb572..33a56aacc 100644
--- a/internal/events/webhooks/webhooks_test.go
+++ b/internal/events/webhooks/webhooks_test.go
@@ -715,6 +715,8 @@ func TestWebhookFailFastAsk(t *testing.T) {
 func TestDeliveryRequestNilMessage(t *testing.T) {
 	wh, cancel := newTestWebHooks(t)
 	defer cancel()
+	mcb := wh.callbacks["ns1"].(*eventsmocks.Callbacks)
+	mcb.On("DeliveryResponse", mock.Anything, mock.Anything).Return("", &core.EventDelivery{})
 
 	yes := true
 	sub := &core.Subscription{
@@ -741,6 +743,7 @@ func TestDeliveryRequestNilMessage(t *testing.T) {
 
 	err := wh.DeliveryRequest(mock.Anything, sub, event, nil)
 	assert.NoError(t, err)
+	mcb.AssertExpectations(t)
 }
 
 func TestDeliveryRequestReplyToReply(t *testing.T) {