Unmarshal messages correctly for all subtypes (#1408)

When we get a 'message' event, the message details (blocks, attachments,
etc.) are all included on the top level struct. When it's a
'message_changed' event, these details are instead nested under a
`message` key. This is really really confusing!

We can't just nest a `Msg` struct inside our MessageEvent, because the
keys of other message events conflict with the `Msg` keys (e.g. both a
`message` and a `message_changed` event will have a `user` key).

To avoid this, we implement a custom unmarshaller which will:
1. If there's a `message` key in the payload, unmarshal it into the
Message field
2. If there's no `message` key, populate the Message field with the
event itself, assuming that it's an event with no subtype and so the
message info is at the top level.

This means some information is represented twice in the same struct, but
that is less bad than it being unclear to a caller where they should go
to access e.g. the message text.

Also reworks the tests so they use examples pulled directly from Slack's
documentation.

Author: nlopes

slackevents/inner_events.go

@@ -3,6 +3,8 @@
 package slackevents
 
 import (
+	"encoding/json"
+
 	"github.com/slack-go/slack"
 )
 
@@ -283,9 +285,16 @@ type MessageEvent struct {
 	UserTeam   string `json:"user_team,omitempty"`
 	SourceTeam string `json:"source_team,omitempty"`
 
+	// When we get a 'message' event with no subtype, i.e. telling us about a new
+	// message, the message information is stored at the top level. But when we get
+	// a 'message_changed' event, the message information is stored in
+	// the Message property. This is really hard to represent nicely in Go, so we use
+	// a custom JSON unmarshaller to populate the Message field in both cases.
+	Message *slack.Msg `json:"message,omitempty"`
+	// Root is set if the SubType is `thread_broadcast`.
+	Root *slack.Msg `json:"root,omitempty"`
 	// Edited Message
-	Message         *slack.Message `json:"message,omitempty"`
-	PreviousMessage *slack.Message `json:"previous_message,omitempty"`
+	PreviousMessage *slack.Msg `json:"previous_message,omitempty"`
 
 	// Deleted Message
 	DeletedTimeStamp string `json:"deleted_ts,omitempty"`
@@ -299,6 +308,40 @@ type MessageEvent struct {
 	Icons    *Icon  `json:"icons,omitempty"`
 }
 
+// UnmarshalJSON implements the json.Unmarshaler interface for MessageEvent.
+// This custom unmarshaler handles both regular messages and message_changed events
+// by normalizing the message data into the Message field.
+func (e *MessageEvent) UnmarshalJSON(data []byte) error {
+	// First, unmarshal into an anonymous struct to avoid infinite recursion
+	// when calling json.Unmarshal on the MessageEvent type itself
+	type MessageEventAlias MessageEvent
+	alias := struct {
+		MessageEventAlias
+	}{}
+
+	if err := json.Unmarshal(data, &alias.MessageEventAlias); err != nil {
+		return err
+	}
+
+	// Copy all fields from alias to the original struct
+	*e = MessageEvent(alias.MessageEventAlias)
+
+	// Now check if there's no Message field (which would happen for regular messages)
+	if e.Message == nil {
+		// For regular messages, the message content is at the top level,
+		// so we need to unmarshal the data again into a slack.Msg
+		msg := &slack.Msg{}
+		if err := json.Unmarshal(data, msg); err != nil {
+			return err
+		}
+
+		// Set the Message field to the unmarshaled msg
+		e.Message = msg
+	}
+
+	return nil
+}
+
 // MemberJoinedChannelEvent A member joined a public or private channel
 type MemberJoinedChannelEvent struct {
 	Type           string `json:"type"`

slackevents/inner_events_test.go

@@ -346,28 +346,71 @@ func TestMessageEvent(t *testing.T) {
 				"channel_type": "channel",
 				"source_team": "T3MQV36V7",
 				"user_team": "T3MQV36V7",
-				"message": {
-					"text": "To infinity and beyond.",
-					"edited": {
-						"user": "U2147483697",
-						"ts": "1355517524.000000"
-					}
-				},
 				"metadata": {
 					"event_type": "example",
 					"event_payload": {
 						"key": "value"
 					}
-				},
-				"previous_message": {
-					"text": "Live long and prospect."
 				}
 		}
 	`)
-	err := json.Unmarshal(rawE, &MessageEvent{})
+	var e MessageEvent
+	err := json.Unmarshal(rawE, &e)
 	if err != nil {
 		t.Error(err)
 	}
+
+	if e.Channel != "G024BE91L" {
+		t.Error(fmt.Errorf("expected channel G024BE91L, got %s", e.Channel))
+	}
+	if e.User != "U2147483697" {
+		t.Error(fmt.Errorf("expected user U2147483697, got %s", e.User))
+	}
+	if e.Text != "Live long and prospect." {
+		t.Error(fmt.Errorf("expected e.Text Live long and prospect., got %s", e.Text))
+	}
+	if e.Message.Text != "Live long and prospect." {
+		t.Error(fmt.Errorf("expected e.Message.Text Live long and prospect., got %s", e.Message.Text))
+	}
+
+}
+
+func TestMessageChangedEvent(t *testing.T) {
+	rawE := []byte(`
+		{
+			"type": "message",
+			"subtype": "message_changed",
+			"hidden": true,
+			"channel": "G024BE91L",
+			"ts": "1358878755.000001",
+			"message": {
+				"type": "message",
+				"user": "U123ABC456",
+				"text": "Live long and prospect.",
+				"ts": "1355517523.000005",
+				"edited": {
+					"user": "U123ABC456",
+					"ts": "1358878755.000001"
+				}
+			}
+		}
+	`)
+
+	var e MessageEvent
+	err := json.Unmarshal(rawE, &e)
+	if err != nil {
+		t.Error(err)
+	}
+
+	if e.Channel != "G024BE91L" {
+		t.Error(fmt.Errorf("expected channel G024BE91L, got %s", e.Channel))
+	}
+	if e.Message.Text != "Live long and prospect." {
+		t.Error(fmt.Errorf("expected e.Message.Text Live long and prospect., got %s", e.Message.Text))
+	}
+	if e.Message.Edited.User != "U123ABC456" {
+		t.Error(fmt.Errorf("expected e.Message.Edited.User U123ABC456, got %s", e.Message.Edited.User))
+	}
 }
 
 func TestBotMessageEvent(t *testing.T) {
@@ -393,42 +436,74 @@ func TestThreadBroadcastEvent(t *testing.T) {
 			{
 				"type": "message",
 				"subtype": "thread_broadcast",
-				"channel": "G024BE91L",
-				"user": "U2147483697",
-				"text": "Live long and prospect.",
-				"ts": "1355517523.000005",
-				"event_ts": "1355517523.000005",
-				"channel_type": "channel",
-				"source_team": "T3MQV36V7",
-				"user_team": "T3MQV36V7",
-				"message": {
-					"text": "To infinity and beyond.",
-					"root": {
-						"text": "To infinity and beyond.",
-						"ts": "1355517523.000005"
-					},
-					"edited": {
-						"user": "U2147483697",
-						"ts": "1355517524.000000"
-					}
+				"text": "broadcasting this reply",
+				"user": "U123ABC456",
+				"ts": "1673464745.620769",
+				"thread_ts": "1673464730.703009",
+				"root": {
+					"client_msg_id": "123abc456-...",
+					"type": "message",
+					"text": "This is the original message",
+					"user": "U123ABC456",
+					"ts": "1673464730.703009",
+					"blocks": [
+						{
+							"type": "rich_text",
+							"block_id": "qTg",
+							"elements": [
+								{
+									"type": "rich_text_section",
+									"elements": [
+										{
+											"type": "text",
+											"text": "This is the original message"
+										}
+									]
+								}
+							]
+						}
+					],
+					"team": "T123ABC456",
+					"thread_ts": "1673464730.703009",
+					"reply_count": 1,
+					"reply_users_count": 1,
+					"latest_reply": "1673464745.620769",
+					"reply_users": [
+						"U123ABC456"
+					],
+					"is_locked": false
 				},
-				"previous_message": {
-					"text": "Live long and prospect."
-				}
-		}
+				"blocks": [
+					{
+						"type": "rich_text",
+						"block_id": "BVp",
+						"elements": [
+							{
+								"type": "rich_text_section",
+								"elements": [
+									{
+										"type": "text",
+										"text": "broadcasting this reply"
+									}
+								]
+							}
+						]
+					}
+				],
+				"client_msg_id": "123abc456-...",
+				"channel": "C123ABC456",
+				"event_ts": "1673464745.620769",
+				"channel_type": "channel"
+			}
 	`)
 
 	var me MessageEvent
 	if err := json.Unmarshal(rawE, &me); err != nil {
 		t.Error(err)
 	}
 
-	if me.Message.Root == nil {
-		t.Fatal("me.Message.Root is nil")
-	}
-
-	if me.Message.Root.Timestamp != "1355517523.000005" {
-		t.Errorf("me.Message.Root.Timestamp = %q, want %q", me.Message.Root.Timestamp, "1355517523.000005")
+	if me.Root.Timestamp != "1673464730.703009" {
+		t.Errorf("me.Root.Timestamp = %q, want %q", me.Root.Timestamp, "1673464730.703009")
 	}
 }