From 2a16fcb420c8f5db654c78c0686cd86c66a1316d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Sun, 22 May 2022 11:12:11 +0200 Subject: [PATCH 1/5] fix: text partitioning logic --- pkg/util/partition_message.go | 4 +-- pkg/util/partition_message_test.go | 39 +++++++++++++++++++++++++----- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/pkg/util/partition_message.go b/pkg/util/partition_message.go index 2b2e2822..a919caaa 100644 --- a/pkg/util/partition_message.go +++ b/pkg/util/partition_message.go @@ -28,8 +28,8 @@ func PartitionMessage(input string, limits t.MessageLimit, distance int) (items // 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 { + chunkEnd := nextChunkStart + if chunkEnd >= maxTotal { // The chunk is smaller than the limit, no need to search chunkEnd = maxTotal nextChunkStart = maxTotal diff --git a/pkg/util/partition_message_test.go b/pkg/util/partition_message_test.go index 81d4b1a7..0f7d590f 100644 --- a/pkg/util/partition_message_test.go +++ b/pkg/util/partition_message_test.go @@ -34,10 +34,12 @@ var _ = Describe("Partition Message", func() { }) 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)) + // Last word fits in the chunk size + Expect(len(items[0].Text)).To(Equal(2000)) items, _ = testPartitionMessage(40, limits, 100) + // Now the last word of the first chunk will be concatenated with + // the first word of the second chunk, and so it does not fit in the chunk anymore Expect(len(items[0].Text)).To(Equal(1994)) Expect(len(items[1].Text)).To(Equal(1999)) Expect(len(items[2].Text)).To(Equal(5)) @@ -48,7 +50,32 @@ var _ = Describe("Partition Message", func() { Expect(items).To(BeEmpty()) }) }) - + When("given an input without whitespace", func() { + It("should not crash, regardless of length", func() { + testString := "" + for inputLen := 1; inputLen < 8000; inputLen++ { + testString += "z" + items, omitted := PartitionMessage(testString, limits, 7) + included := 0 + for ii, item := range items { + expectedSize := limits.ChunkSize + // If this is the last chunk, and we haven't reached the total maximum + if inputLen < limits.TotalChunkSize && ii == len(items)-1 { + // ...and the chunk wouldn't be empty + chunkOffset := inputLen % limits.ChunkSize + if chunkOffset > 0 { + // expect the chunk to contain the "rest" of the runes + expectedSize = chunkOffset + } + // the last chunk should never be empty, so treat it as one of the full ones + } + included += len(item.Text) + Expect(item.Text).To(HaveLen(expectedSize)) + } + Expect(omitted + included).To(Equal(inputLen)) + } + }) + }) }) When("splitting by lines", func() { It("should return a payload with chunked messages", func() { @@ -135,11 +162,11 @@ func testPartitionMessage(hundreds int, limits types.MessageLimit, distance int) items, omitted = PartitionMessage(builder.String(), limits, distance) contentSize := Min(hundreds*100, limits.TotalChunkSize) - expectedChunkCount := CeilDiv(contentSize, limits.ChunkSize-1) + // expectedChunkCount := CeilDiv(contentSize, limits.ChunkSize-1) expectedOmitted := Max(0, (hundreds*100)-contentSize) - Expect(omitted).To(Equal(expectedOmitted)) - Expect(len(items)).To(Equal(expectedChunkCount)) + ExpectWithOffset(1, omitted).To(Equal(expectedOmitted)) + // ExpectWithOffset(1, len(items)).To(Equal(expectedChunkCount)) return } From 7a99bab3e818f7c555e3b7080dbecf5d06678ab1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Sun, 22 May 2022 11:45:47 +0200 Subject: [PATCH 2/5] add data verification to test --- pkg/util/partition_message_test.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/pkg/util/partition_message_test.go b/pkg/util/partition_message_test.go index 0f7d590f..985f8c35 100644 --- a/pkg/util/partition_message_test.go +++ b/pkg/util/partition_message_test.go @@ -1,6 +1,7 @@ package util import ( + "strconv" "strings" "github.com/containrrr/shoutrrr/pkg/types" @@ -54,7 +55,8 @@ var _ = Describe("Partition Message", func() { It("should not crash, regardless of length", func() { testString := "" for inputLen := 1; inputLen < 8000; inputLen++ { - testString += "z" + // add a rune to the string using a repeatable pattern (single digit hex of position) + testString += strconv.FormatInt(int64(inputLen%16), 16) items, omitted := PartitionMessage(testString, limits, 7) included := 0 for ii, item := range items { @@ -69,6 +71,18 @@ var _ = Describe("Partition Message", func() { } // the last chunk should never be empty, so treat it as one of the full ones } + + // Verify the data, but only on the last chunk to reduce test time + if ii == len(items)-1 { + for ri, r := range item.Text { + runeOffset := (len(item.Text) - ri) - 1 + runeVal, err := strconv.ParseInt(string(r), 16, 64) + expectedVal := (expectedSize - runeOffset) % 16 + Expect(err).ToNot(HaveOccurred()) + Expect(runeVal).To(Equal(int64(expectedVal))) + } + } + included += len(item.Text) Expect(item.Text).To(HaveLen(expectedSize)) } From 749616bc2c994cdd684e76f5a4d2bbd34c75c334 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Sun, 22 May 2022 12:36:44 +0200 Subject: [PATCH 3/5] fix test relying on aligned limits --- pkg/util/partition_message_test.go | 33 +++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/pkg/util/partition_message_test.go b/pkg/util/partition_message_test.go index 985f8c35..9fc3d969 100644 --- a/pkg/util/partition_message_test.go +++ b/pkg/util/partition_message_test.go @@ -53,31 +53,43 @@ var _ = Describe("Partition Message", func() { }) When("given an input without whitespace", func() { It("should not crash, regardless of length", func() { + unalignedLimits := types.MessageLimit{ + ChunkSize: 1997, + ChunkCount: 11, + TotalChunkSize: 5631, + } + testString := "" for inputLen := 1; inputLen < 8000; inputLen++ { // add a rune to the string using a repeatable pattern (single digit hex of position) testString += strconv.FormatInt(int64(inputLen%16), 16) - items, omitted := PartitionMessage(testString, limits, 7) + items, omitted := PartitionMessage(testString, unalignedLimits, 7) included := 0 for ii, item := range items { - expectedSize := limits.ChunkSize - // If this is the last chunk, and we haven't reached the total maximum - if inputLen < limits.TotalChunkSize && ii == len(items)-1 { - // ...and the chunk wouldn't be empty - chunkOffset := inputLen % limits.ChunkSize - if chunkOffset > 0 { + expectedSize := unalignedLimits.ChunkSize + + // The last chunk might be smaller than the preceeding chunks + if ii == len(items)-1 { + // the chunk size is the remainder of, the total size, + // or the max size, whatever is smallest, + // and the previous chunk sizes + chunkSize := Min(inputLen, unalignedLimits.TotalChunkSize) % unalignedLimits.ChunkSize + // if the "rest" of the runes needs another chunk + if chunkSize > 0 { // expect the chunk to contain the "rest" of the runes - expectedSize = chunkOffset + expectedSize = chunkSize } // the last chunk should never be empty, so treat it as one of the full ones } - // Verify the data, but only on the last chunk to reduce test time + // verify the data, but only on the last chunk to reduce test time if ii == len(items)-1 { for ri, r := range item.Text { runeOffset := (len(item.Text) - ri) - 1 runeVal, err := strconv.ParseInt(string(r), 16, 64) - expectedVal := (expectedSize - runeOffset) % 16 + expectedLen := Min(inputLen, unalignedLimits.TotalChunkSize) + expectedVal := (expectedLen - runeOffset) % 16 + Expect(err).ToNot(HaveOccurred()) Expect(runeVal).To(Equal(int64(expectedVal))) } @@ -87,6 +99,7 @@ var _ = Describe("Partition Message", func() { Expect(item.Text).To(HaveLen(expectedSize)) } Expect(omitted + included).To(Equal(inputLen)) + } }) }) From 1ac25af3d35427274817d9b95ca236defb130401 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Sun, 22 May 2022 12:45:34 +0200 Subject: [PATCH 4/5] revert comments for incorrect fix --- pkg/util/partition_message.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/util/partition_message.go b/pkg/util/partition_message.go index a919caaa..a5d6b5b2 100644 --- a/pkg/util/partition_message.go +++ b/pkg/util/partition_message.go @@ -25,10 +25,10 @@ func PartitionMessage(input string, limits t.MessageLimit, distance int) (items } for i := 0; i < maxCount; i++ { - // 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 + // 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 chunkEnd >= maxTotal { // The chunk is smaller than the limit, no need to search chunkEnd = maxTotal From c12a45edf72e348307fbd199c982ada3c0e58473 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Sun, 22 May 2022 12:57:54 +0200 Subject: [PATCH 5/5] remove bogus chunkSize prediction --- pkg/util/partition_message_test.go | 4 +--- pkg/util/util.go | 6 ------ 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/pkg/util/partition_message_test.go b/pkg/util/partition_message_test.go index 9fc3d969..d6b33254 100644 --- a/pkg/util/partition_message_test.go +++ b/pkg/util/partition_message_test.go @@ -189,11 +189,9 @@ func testPartitionMessage(hundreds int, limits types.MessageLimit, distance int) items, omitted = PartitionMessage(builder.String(), limits, distance) contentSize := Min(hundreds*100, limits.TotalChunkSize) - // expectedChunkCount := CeilDiv(contentSize, limits.ChunkSize-1) expectedOmitted := Max(0, (hundreds*100)-contentSize) - ExpectWithOffset(1, omitted).To(Equal(expectedOmitted)) - // ExpectWithOffset(1, len(items)).To(Equal(expectedChunkCount)) + ExpectWithOffset(0, omitted).To(Equal(expectedOmitted)) return } diff --git a/pkg/util/util.go b/pkg/util/util.go index 0f8f4882..1e8cba49 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -3,7 +3,6 @@ package util import ( "io/ioutil" "log" - "math" ) // Min returns the smallest of a and b @@ -22,10 +21,5 @@ func Max(a int, b int) int { return b } -// CeilDiv returns the quotient from dividing the dividend with the divisor, but rounded up to the nearest integer -func CeilDiv(dividend int, divisor int) int { - return int(math.Ceil(float64(dividend) / float64(divisor))) -} - // DiscardLogger is a logger that discards any output written to it var DiscardLogger = log.New(ioutil.Discard, "", 0)