fix(discord): message size fixes (#242)

* fix(discord): handle empty messages without panic
* fix: off by one error when partitioning messages
* test(discord): add tests for Send failures
* fix(discord): typo in meta message

Co-authored-by: Justin Steven <justin@justinsteven.com>

pkg/services/discord/discord.go

@@ -4,12 +4,13 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"net/http"
+	"net/url"
+
 	"github.com/containrrr/shoutrrr/pkg/format"
 	"github.com/containrrr/shoutrrr/pkg/services/standard"
 	"github.com/containrrr/shoutrrr/pkg/types"
 	"github.com/containrrr/shoutrrr/pkg/util"
-	"net/http"
-	"net/url"
 )
 
 // Service providing Discord as a notification service
@@ -32,15 +33,20 @@ const (
 )
 
 // Send a notification message to discord
-func (service *Service) Send(message string, params *types.Params) error {
-
+func (service *Service) Send(message string, params *types.Params) (err error) {
 	if service.config.JSON {
 		postURL := CreateAPIURLFromConfig(service.config)
-		return doSend([]byte(message), postURL)
+		err = doSend([]byte(message), postURL)
+	} else {
+		items, omitted := CreateItemsFromPlain(message, service.config.SplitLines)
+		err = service.sendItems(items, params, omitted)
 	}
 
-	items, omitted := CreateItemsFromPlain(message, service.config.SplitLines)
-	return service.sendItems(items, params, omitted)
+	if err != nil {
+		err = fmt.Errorf("failed to send discord notification: %v", err)
+	}
+
+	return
 }
 
 // SendItems sends items with additional meta data and richer appearance
@@ -121,9 +127,5 @@ func doSend(payload []byte, postURL string) error {
 		err = fmt.Errorf("response status code %s", res.Status)
 	}
 
-	if err != nil {
-		return fmt.Errorf("failed to send discord notification: %v", err)
-	}
-
-	return nil
+	return err
 }

pkg/services/discord/discord_json.go

@@ -2,16 +2,17 @@ package discord
 
 import (
 	"fmt"
+	"time"
+
 	"github.com/containrrr/shoutrrr/pkg/types"
 	"github.com/containrrr/shoutrrr/pkg/util"
-	"time"
 )
 
 // WebhookPayload is the webhook endpoint payload
 type WebhookPayload struct {
-	Embeds   []embedItem `json:"embeds"`
-	Username string      `json:"username,omitempty"`
-	AvatarURL string     `json:"avatar_url,omitempty"`
+	Embeds    []embedItem `json:"embeds"`
+	Username  string      `json:"username,omitempty"`
+	AvatarURL string      `json:"avatar_url,omitempty"`
 }
 
 // JSON is the actual notification payload
@@ -32,6 +33,10 @@ type embedFooter struct {
 // CreatePayloadFromItems creates a JSON payload to be sent to the discord webhook API
 func CreatePayloadFromItems(items []types.MessageItem, title string, colors [types.MessageLevelCount]uint, omitted int) (WebhookPayload, error) {
 
+	if len(items) < 1 {
+		return WebhookPayload{}, fmt.Errorf("message is empty")
+	}
+
 	metaCount := 1
 	if omitted < 1 && len(title) < 1 {
 		metaCount = 0
@@ -65,10 +70,14 @@ func CreatePayloadFromItems(items []types.MessageItem, title string, colors [typ
 		embeds = append(embeds, ei)
 	}
 
-	embeds[0].Title = title
-	if omitted > 0 {
-		embeds[0].Footer = &embedFooter{
-			Text: fmt.Sprintf("... (%v character(s) where omitted)", omitted),
+	// This should not happen, but it's better to leave the index check before dereferencing the array
+	if len(embeds) > 0 {
+		embeds[0].Title = title
+
+		if omitted > 0 {
+			embeds[0].Footer = &embedFooter{
+				Text: fmt.Sprintf("... (%v character(s) were omitted)", omitted),
+			}
 		}
 	}
 

pkg/services/discord/discord_test.go

@@ -1,6 +1,7 @@
 package discord_test
 
 import (
+	"fmt"
 	"log"
 	"time"
 
@@ -115,12 +116,24 @@ var _ = Describe("the discord service", func() {
 		})
 	})
 	Describe("creating a json payload", func() {
-		//When("given a blank message", func() {
-		//	It("should return an error", func() {
-		//		_, err := CreatePayloadFromItems("", false)
-		//		Expect(err).To(HaveOccurred())
-		//	})
-		//})
+		When("given a blank message", func() {
+			When("split lines is enabled", func() {
+				It("should return an error", func() {
+					items, omitted := CreateItemsFromPlain("", true)
+					Expect(items).To(BeEmpty())
+					_, err := CreatePayloadFromItems(items, "title", dummyColors, omitted)
+					Expect(err).To(HaveOccurred())
+				})
+			})
+			When("split lines is disabled", func() {
+				It("should return an error", func() {
+					items, omitted := CreateItemsFromPlain("", false)
+					Expect(items).To(BeEmpty())
+					_, err := CreatePayloadFromItems(items, "title", dummyColors, omitted)
+					Expect(err).To(HaveOccurred())
+				})
+			})
+		})
 		When("given a message that exceeds the max length", func() {
 			It("should return a payload with chunked messages", func() {
 
@@ -204,29 +217,45 @@ var _ = Describe("the discord service", func() {
 	})
 
 	Describe("sending the payload", func() {
-		var err error
+		var dummyConfig = Config{
+			WebhookID: "1",
+			Token:     "dummyToken",
+		}
+		var service Service
 		BeforeEach(func() {
 			httpmock.Activate()
+			service = Service{}
+			if err := service.Initialize(dummyConfig.GetURL(), logger); err != nil {
+				panic(fmt.Errorf("service initialization failed: %v", err))
+			}
 		})
 		AfterEach(func() {
 			httpmock.DeactivateAndReset()
 		})
 		It("should not report an error if the server accepts the payload", func() {
-			config := Config{
-				WebhookID: "1",
-				Token:     "dummyToken",
-			}
-			serviceURL := config.GetURL()
-			service := Service{}
-			err = service.Initialize(serviceURL, logger)
-			Expect(err).NotTo(HaveOccurred())
+			setupResponder(&dummyConfig, 204, "")
 
-			setupResponder(&config, 204, "")
-
-			err = service.Send("Message", nil)
-			Expect(err).NotTo(HaveOccurred())
+			Expect(service.Send("Message", nil)).To(Succeed())
+		})
+		It("should report an error if the server response is not OK", func() {
+			setupResponder(&dummyConfig, 400, "")
+			Expect(service.Initialize(dummyConfig.GetURL(), logger)).To(Succeed())
+			Expect(service.Send("Message", nil)).NotTo(Succeed())
+		})
+		It("should report an error if the message is empty", func() {
+			setupResponder(&dummyConfig, 204, "")
+			Expect(service.Initialize(dummyConfig.GetURL(), logger)).To(Succeed())
+			Expect(service.Send("", nil)).NotTo(Succeed())
+		})
+		When("using a custom json payload", func() {
+			It("should report an error if the server response is not OK", func() {
+				config := dummyConfig
+				config.JSON = true
+				setupResponder(&config, 400, "")
+				Expect(service.Initialize(config.GetURL(), logger)).To(Succeed())
+				Expect(service.Send("Message", nil)).NotTo(Succeed())
+			})
 		})
-
 	})
 })
 

pkg/util/partition_message.go

@@ -18,11 +18,17 @@ func PartitionMessage(input string, limits t.MessageLimit, distance int) (items
 	maxTotal := Min(len(runes), limits.TotalChunkSize)
 	maxCount := limits.ChunkCount - 1
 
+	if len(input) == 0 {
+		// If the message is empty, return an empty array
+		omitted = 0
+		return
+	}
+
 	for i := 0; i < maxCount; i++ {
-		// If no suitable split point is found, use the chunkSize
-		chunkEnd := chunkOffset + limits.ChunkSize
-		// ... and start next chunk directly after this one
-		nextChunkStart := chunkEnd
+		// If no suitable split point is found, start next chunk at chunkSize from chunk start
+		nextChunkStart := chunkOffset + limits.ChunkSize
+		// ... and set the chunk end to the rune before the next chunk
+		chunkEnd := nextChunkStart - 1
 		if chunkEnd > maxTotal {
 			// The chunk is smaller than the limit, no need to search
 			chunkEnd = maxTotal

pkg/util/partition_message_test.go

@@ -1,85 +1,101 @@
 package util
 
 import (
+	"strings"
+
 	"github.com/containrrr/shoutrrr/pkg/types"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-	"strings"
 )
 
 var _ = Describe("Partition Message", func() {
-	Describe("creating a json payload", func() {
-		limits := types.MessageLimit{
-			ChunkSize:      2000,
-			TotalChunkSize: 6000,
-			ChunkCount:     10,
-		}
-		When("given a message that exceeds the max length", func() {
-			When("not splitting by lines", func() {
-				It("should return a payload with chunked messages", func() {
+	limits := types.MessageLimit{
+		ChunkSize:      2000,
+		TotalChunkSize: 6000,
+		ChunkCount:     10,
+	}
+	When("given a message that exceeds the max length", func() {
+		When("not splitting by lines", func() {
+			It("should return a payload with chunked messages", func() {
 
-					items, _ := testPartitionMessage(42, limits, 100)
+				items, _ := testPartitionMessage(42, limits, 100)
 
-					Expect(len(items[0].Text)).To(Equal(1994))
-					Expect(len(items[1].Text)).To(Equal(1999))
-					Expect(len(items[2].Text)).To(Equal(205))
-				})
-				It("omit characters above total max", func() {
-					items, _ := testPartitionMessage(62, limits, 100)
+				Expect(len(items[0].Text)).To(Equal(1994))
+				Expect(len(items[1].Text)).To(Equal(1999))
+				Expect(len(items[2].Text)).To(Equal(205))
+			})
+			It("omit characters above total max", func() {
+				items, _ := testPartitionMessage(62, limits, 100)
 
-					Expect(len(items[0].Text)).To(Equal(1994))
-					Expect(len(items[1].Text)).To(Equal(1999))
-					Expect(len(items[2].Text)).To(Equal(1999))
-					Expect(len(items[3].Text)).To(Equal(5))
+				Expect(len(items[0].Text)).To(Equal(1994))
+				Expect(len(items[1].Text)).To(Equal(1999))
+				Expect(len(items[2].Text)).To(Equal(1999))
+				Expect(len(items[3].Text)).To(Equal(5))
+			})
+			It("should handle messages with a size modulus of chunksize", func() {
+				items, _ := testPartitionMessage(20, limits, 100)
+				Expect(len(items[0].Text)).To(Equal(1994))
+				Expect(len(items[1].Text)).To(Equal(5))
+
+				items, _ = testPartitionMessage(40, limits, 100)
+				Expect(len(items[0].Text)).To(Equal(1994))
+				Expect(len(items[1].Text)).To(Equal(1999))
+				Expect(len(items[2].Text)).To(Equal(5))
+			})
+			When("the message is empty", func() {
+				It("should return no items", func() {
+					items, _ := testPartitionMessage(0, limits, 100)
+					Expect(items).To(BeEmpty())
 				})
 			})
-			When("splitting by lines", func() {
-				It("should return a payload with chunked messages", func() {
-					items, omitted := testMessageItemsFromLines(18, limits, 2)
 
-					Expect(len(items[0].Text)).To(Equal(200))
-					Expect(len(items[8].Text)).To(Equal(200))
+		})
+		When("splitting by lines", func() {
+			It("should return a payload with chunked messages", func() {
+				items, omitted := testMessageItemsFromLines(18, limits, 2)
 
-					Expect(omitted).To(Equal(0))
-				})
-				It("omit characters above total max", func() {
-					items, omitted := testMessageItemsFromLines(19, limits, 2)
+				Expect(len(items[0].Text)).To(Equal(200))
+				Expect(len(items[8].Text)).To(Equal(200))
 
-					Expect(len(items[0].Text)).To(Equal(200))
-					Expect(len(items[8].Text)).To(Equal(200))
+				Expect(omitted).To(Equal(0))
+			})
+			It("omit characters above total max", func() {
+				items, omitted := testMessageItemsFromLines(19, limits, 2)
 
-					Expect(omitted).To(Equal(100))
-				})
-				It("should trim characters above chunk size", func() {
-					hundreds := 42
-					repeat := 21
-					items, omitted := testMessageItemsFromLines(hundreds, limits, repeat)
+				Expect(len(items[0].Text)).To(Equal(200))
+				Expect(len(items[8].Text)).To(Equal(200))
 
-					Expect(len(items[0].Text)).To(Equal(limits.ChunkSize))
-					Expect(len(items[1].Text)).To(Equal(limits.ChunkSize))
+				Expect(omitted).To(Equal(100))
+			})
+			It("should trim characters above chunk size", func() {
+				hundreds := 42
+				repeat := 21
+				items, omitted := testMessageItemsFromLines(hundreds, limits, repeat)
 
-					// Trimmed characters do not count towards the total omitted count
-					Expect(omitted).To(Equal(0))
-				})
+				Expect(len(items[0].Text)).To(Equal(limits.ChunkSize))
+				Expect(len(items[1].Text)).To(Equal(limits.ChunkSize))
 
-				It("omit characters above total chunk size", func() {
-					hundreds := 100
-					repeat := 20
-					items, omitted := testMessageItemsFromLines(hundreds, limits, repeat)
+				// Trimmed characters do not count towards the total omitted count
+				Expect(omitted).To(Equal(0))
+			})
 
-					Expect(len(items[0].Text)).To(Equal(limits.ChunkSize))
-					Expect(len(items[1].Text)).To(Equal(limits.ChunkSize))
-					Expect(len(items[2].Text)).To(Equal(limits.ChunkSize))
+			It("omit characters above total chunk size", func() {
+				hundreds := 100
+				repeat := 20
+				items, omitted := testMessageItemsFromLines(hundreds, limits, repeat)
 
-					maxRunes := hundreds * 100
-					expectedOmitted := maxRunes - limits.TotalChunkSize
+				Expect(len(items[0].Text)).To(Equal(limits.ChunkSize))
+				Expect(len(items[1].Text)).To(Equal(limits.ChunkSize))
+				Expect(len(items[2].Text)).To(Equal(limits.ChunkSize))
 
-					Expect(omitted).To(Equal(expectedOmitted))
-				})
+				maxRunes := hundreds * 100
+				expectedOmitted := maxRunes - limits.TotalChunkSize
 
+				Expect(omitted).To(Equal(expectedOmitted))
 			})
 
 		})
+
 	})
 })