Imrpove message -> eml conversion (#4651)

<!-- PR description-->

---

#### Does this PR need a docs update or release note?

- [ ] ✅ Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [x] ⛔ No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] 🌻 Feature
- [x] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [ ] 🧹 Tech Debt/Cleanup

#### Issue(s)

<!-- Can reference multiple issues. Use one of the following "magic words" - "closes, fixes" to auto-close the Github issue. -->
* #<issue>

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E

src/go.mod

@@ -14,6 +14,7 @@ require (
 	github.com/google/uuid v1.4.0
 	github.com/h2non/gock v1.2.0
 	github.com/jaytaylor/html2text v0.0.0-20230321000545-74c2419ad056
+	github.com/jhillyerd/enmime v1.0.1
 	github.com/kopia/kopia v0.13.0
 	github.com/microsoft/kiota-abstractions-go v1.4.0
 	github.com/microsoft/kiota-authentication-azure-go v1.0.1
@@ -48,9 +49,11 @@ require (
 	github.com/acarl005/stripansi v0.0.0-20180116102854-5a71ef0e047d // indirect
 	github.com/andybalholm/brotli v1.0.6 // indirect
 	github.com/aws/aws-sdk-go v1.47.9 // indirect
+	github.com/cention-sany/utf7 v0.0.0-20170124080048-26cad61bd60a // indirect
 	github.com/fsnotify/fsnotify v1.6.0 // indirect
 	github.com/go-test/deep v1.1.0 // indirect
 	github.com/gofrs/flock v0.8.1 // indirect
+	github.com/gogs/chardet v0.0.0-20211120154057-b7413eaefb8f // indirect
 	github.com/google/go-cmp v0.5.9 // indirect
 	github.com/h2non/parth v0.0.0-20190131123155-b4df798d6542 // indirect
 	github.com/hashicorp/cronexpr v1.1.2 // indirect

src/go.sum

@@ -82,6 +82,8 @@ github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869/go.mod h1:Ekp36dR
 github.com/cenkalti/backoff/v4 v4.2.1 h1:y4OZtCnogmCPw98Zjyt5a6+QwPLGkiQsYW5oUqylYbM=
 github.com/cenkalti/backoff/v4 v4.2.1/go.mod h1:Y3VNntkOUPxTVeUxJ/G5vcM//AlwfmyYozVcomhLiZE=
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
+github.com/cention-sany/utf7 v0.0.0-20170124080048-26cad61bd60a h1:MISbI8sU/PSK/ztvmWKFcI7UGb5/HQT7B+i3a2myKgI=
+github.com/cention-sany/utf7 v0.0.0-20170124080048-26cad61bd60a/go.mod h1:2GxOXOlEPAMFPfp014mK1SWq8G8BN8o7/dfYqJrVGn8=
 github.com/cespare/xxhash/v2 v2.1.1/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
 github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44=
 github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
@@ -141,6 +143,8 @@ github.com/godbus/dbus/v5 v5.1.0/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5x
 github.com/gofrs/flock v0.8.1 h1:+gYjHKf32LDeiEEFhQaotPbLuUXjY5ZqxKgXy7n59aw=
 github.com/gofrs/flock v0.8.1/go.mod h1:F1TvTiK9OcQqauNUHlbJvyl9Qa1QvF/gOUDKA14jxHU=
 github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
+github.com/gogs/chardet v0.0.0-20211120154057-b7413eaefb8f h1:3BSP1Tbs2djlpprl7wCLuiqMaUh5SJkkzI2gDs+FgLs=
+github.com/gogs/chardet v0.0.0-20211120154057-b7413eaefb8f/go.mod h1:Pcatq5tYkCW2Q6yrR2VRHlbHpZ/R4/7qyL1TCF7vl14=
 github.com/golang-jwt/jwt/v4 v4.5.0 h1:7cYmW1XlMY7h7ii7UhUyChSgS5wUJEnm9uZVTGqOWzg=
 github.com/golang-jwt/jwt/v4 v4.5.0/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0=
 github.com/golang-jwt/jwt/v5 v5.1.0 h1:UGKbA/IPjtS6zLcdB7i5TyACMgSbOTiR8qzXgw8HWQU=
@@ -239,6 +243,8 @@ github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2
 github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw=
 github.com/jaytaylor/html2text v0.0.0-20230321000545-74c2419ad056 h1:iCHtR9CQyktQ5+f3dMVZfwD2KWJUgm7M0gdL9NGr8KA=
 github.com/jaytaylor/html2text v0.0.0-20230321000545-74c2419ad056/go.mod h1:CVKlgaMiht+LXvHG173ujK6JUhZXKb2u/BQtjPDIvyk=
+github.com/jhillyerd/enmime v1.0.1 h1:y6RyqIgBOI2hIinOXIzmeB+ITRVls0zTJIm5GwgXnjE=
+github.com/jhillyerd/enmime v1.0.1/go.mod h1:LMMbm6oTlzWHghPavqHtOrP/NosVv3l42CUrZjn03/Q=
 github.com/jmespath/go-jmespath v0.4.0 h1:BEgLn5cpjn8UN1mAw4NjwDrS35OdebyEtFe+9YPoQUg=
 github.com/jmespath/go-jmespath v0.4.0/go.mod h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHWvzYPziyZiYoo=
 github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8=

src/internal/converters/eml/eml.go

@@ -13,6 +13,7 @@ import (
 	"fmt"
 
 	"github.com/alcionai/clues"
+	"github.com/microsoftgraph/msgraph-sdk-go/models"
 	mail "github.com/xhit/go-simple-mail/v2"
 
 	"github.com/alcionai/corso/src/internal/common/ptr"
@@ -21,10 +22,21 @@ import (
 )
 
 const (
-	addressFormat = "%s <%s>"
+	addressFormat = `"%s" <%s>`
 	dateFormat    = "2006-01-02 15:04:05 MST" // from xhit/go-simple-mail
 )
 
+func formatAddress(entry models.EmailAddressable) string {
+	name := ptr.Val(entry.GetName())
+	email := ptr.Val(entry.GetAddress())
+
+	if name == email || len(name) == 0 {
+		return email
+	}
+
+	return fmt.Sprintf(addressFormat, name, email)
+}
+
 // FromJSON converts a Messageable (as json) to .eml format
 func FromJSON(ctx context.Context, body []byte) (string, error) {
 	data, err := api.BytesToMessageable(body)
@@ -32,43 +44,31 @@ func FromJSON(ctx context.Context, body []byte) (string, error) {
 		return "", clues.Wrap(err, "converting to messageble")
 	}
 
+	ctx = clues.Add(ctx, "id", ptr.Val(data.GetId()))
+
 	email := mail.NewMSG()
+	email.AllowDuplicateAddress = true // More "correct" conversion
+	email.AddBccToHeader = true        // Don't ignore Bcc
 
 	if data.GetFrom() != nil {
-		email.SetFrom(
+		email.SetFrom(formatAddress(data.GetFrom().GetEmailAddress()))
-			fmt.Sprintf(
-				addressFormat,
-				ptr.Val(data.GetFrom().GetEmailAddress().GetName()),
-				ptr.Val(data.GetFrom().GetEmailAddress().GetAddress())))
 	}
 
 	if data.GetToRecipients() != nil {
 		for _, recipient := range data.GetToRecipients() {
-			email.AddTo(
+			email.AddTo(formatAddress(recipient.GetEmailAddress()))
-				fmt.Sprintf(
-					addressFormat,
-					ptr.Val(recipient.GetEmailAddress().GetName()),
-					ptr.Val(recipient.GetEmailAddress().GetAddress())))
 		}
 	}
 
 	if data.GetCcRecipients() != nil {
 		for _, recipient := range data.GetCcRecipients() {
-			email.AddCc(
+			email.AddCc(formatAddress(recipient.GetEmailAddress()))
-				fmt.Sprintf(
-					addressFormat,
-					ptr.Val(recipient.GetEmailAddress().GetName()),
-					ptr.Val(recipient.GetEmailAddress().GetAddress())))
 		}
 	}
 
 	if data.GetBccRecipients() != nil {
 		for _, recipient := range data.GetBccRecipients() {
-			email.AddBcc(
+			email.AddBcc(formatAddress(recipient.GetEmailAddress()))
-				fmt.Sprintf(
-					addressFormat,
-					ptr.Val(recipient.GetEmailAddress().GetName()),
-					ptr.Val(recipient.GetEmailAddress().GetAddress())))
 		}
 	}
 
@@ -76,15 +76,12 @@ func FromJSON(ctx context.Context, body []byte) (string, error) {
 		rts := data.GetReplyTo()
 		if len(rts) > 1 {
 			logger.Ctx(ctx).
-				With("id", ptr.Val(data.GetId()),
+				With("reply_to_count", len(rts)).
-					"reply_to_count", len(rts)).
+				Warn("more than 1 Reply-To, adding only the first one")
-				Warn("more than 1 reply to")
+		}
-		} else if len(rts) != 0 {
+
-			email.SetReplyTo(
+		if len(rts) != 0 {
-				fmt.Sprintf(
+			email.SetReplyTo(formatAddress(rts[0].GetEmailAddress()))
-					addressFormat,
-					ptr.Val(rts[0].GetEmailAddress().GetName()),
-					ptr.Val(rts[0].GetEmailAddress().GetAddress())))
 		}
 	}
 
@@ -109,8 +106,7 @@ func FromJSON(ctx context.Context, body []byte) (string, error) {
 				// https://learn.microsoft.com/en-us/graph/api/resources/itembody?view=graph-rest-1.0#properties
 				// This should not be possible according to the documentation
 				logger.Ctx(ctx).
-					With("body_type", data.GetBody().GetContentType().String(),
+					With("body_type", data.GetBody().GetContentType().String()).
-						"id", ptr.Val(data.GetId())).
 					Info("unknown body content type")
 
 				contentType = mail.TextPlain
@@ -126,12 +122,12 @@ func FromJSON(ctx context.Context, body []byte) (string, error) {
 
 			bytes, err := attachment.GetBackingStore().Get("contentBytes")
 			if err != nil {
-				return "", clues.Wrap(err, "failed to get attachment bytes")
+				return "", clues.WrapWC(ctx, err, "failed to get attachment bytes")
 			}
 
 			bts, ok := bytes.([]byte)
 			if !ok {
-				return "", clues.Wrap(err, "invalid content bytes")
+				return "", clues.WrapWC(ctx, err, "invalid content bytes")
 			}
 
 			email.Attach(&mail.File{
@@ -143,5 +139,9 @@ func FromJSON(ctx context.Context, body []byte) (string, error) {
 		}
 	}
 
+	if email.GetError() != nil {
+		return "", clues.WrapWC(ctx, email.Error, "converting to eml")
+	}
+
 	return email.GetMessage(), nil
 }

src/internal/converters/eml/eml_test.go

@@ -1,10 +1,13 @@
 package eml
 
 import (
-	"fmt"
+	"regexp"
+	"strings"
 	"testing"
 	"time"
 
+	"github.com/jhillyerd/enmime"
+	"github.com/microsoftgraph/msgraph-sdk-go/models"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
@@ -23,6 +26,50 @@ func TestEMLUnitSuite(t *testing.T) {
 	suite.Run(t, &EMLUnitSuite{Suite: tester.NewUnitSuite(t)})
 }
 
+func (suite *EMLUnitSuite) TestFormatAddress() {
+	t := suite.T()
+
+	tests := []struct {
+		tname    string
+		name     string
+		email    string
+		expected string
+	}{
+		{
+			tname:    "different name and email",
+			name:     "John Doe",
+			email:    "johndoe@provider.com",
+			expected: `"John Doe" <johndoe@provider.com>`,
+		},
+		{
+			tname:    "same name and email",
+			name:     "johndoe@provider.com",
+			email:    "johndoe@provider.com",
+			expected: "johndoe@provider.com",
+		},
+		{
+			tname:    "only email",
+			name:     "",
+			email:    "johndoe@provider.com",
+			expected: "johndoe@provider.com",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.tname, func(t *testing.T) {
+			entity := models.NewEmailAddress()
+			if len(tt.name) != 0 {
+				entity.SetName(ptr.To(tt.name))
+			}
+			if len(tt.email) != 0 {
+				entity.SetAddress(ptr.To(tt.email))
+			}
+
+			assert.Equal(t, tt.expected, formatAddress(entity))
+		})
+	}
+}
+
 func (suite *EMLUnitSuite) TestConvert_messageble_to_eml() {
 	t := suite.T()
 
@@ -37,49 +84,36 @@ func (suite *EMLUnitSuite) TestConvert_messageble_to_eml() {
 	msg, err := api.BytesToMessageable(body)
 	require.NoError(t, err, "creating message")
 
-	assert.Contains(t, out, fmt.Sprintf("Subject: %s", ptr.Val(msg.GetSubject())))
+	eml, err := enmime.ReadEnvelope(strings.NewReader(out))
-	assert.Contains(t, out, fmt.Sprintf("Date: %s", msg.GetSentDateTime().Format(time.RFC1123Z)))
+	require.NoError(t, err, "reading created eml")
-	assert.Contains(
-		t,
-		out,
-		fmt.Sprintf(
-			`From: "%s" <%s>`,
-			ptr.Val(msg.GetFrom().GetEmailAddress().GetName()),
-			ptr.Val(msg.GetFrom().GetEmailAddress().GetAddress())))
 
-	for _, addr := range msg.GetToRecipients() {
+	assert.Equal(t, ptr.Val(msg.GetSubject()), eml.GetHeader("Subject"))
-		assert.Contains(
+	assert.Equal(t, msg.GetSentDateTime().Format(time.RFC1123Z), eml.GetHeader("Date"))
-			t,
+
-			out,
+	assert.Equal(t, formatAddress(msg.GetFrom().GetEmailAddress()), eml.GetHeader("From"))
-			fmt.Sprintf(
+
-				`To: "%s" <%s>`,
+	ccs := strings.Split(eml.GetHeader("Cc"), ", ")
-				ptr.Val(addr.GetEmailAddress().GetName()),
+	for _, cc := range msg.GetCcRecipients() {
-				ptr.Val(addr.GetEmailAddress().GetAddress())))
+		assert.Contains(t, ccs, formatAddress(cc.GetEmailAddress()))
 	}
 
-	for _, addr := range msg.GetCcRecipients() {
+	bccs := strings.Split(eml.GetHeader("Bcc"), ", ")
-		assert.Contains(
+	for _, bcc := range msg.GetBccRecipients() {
-			t,
+		assert.Contains(t, bccs, formatAddress(bcc.GetEmailAddress()))
-			out,
-			fmt.Sprintf(
-				`Cc: "%s" <%s>`,
-				ptr.Val(addr.GetEmailAddress().GetName()),
-				ptr.Val(addr.GetEmailAddress().GetAddress())))
 	}
 
-	for _, addr := range msg.GetBccRecipients() {
+	tos := strings.Split(eml.GetHeader("To"), ", ")
-		assert.Contains(
+	for _, to := range msg.GetToRecipients() {
-			t,
+		assert.Contains(t, tos, formatAddress(to.GetEmailAddress()))
-			out,
-			fmt.Sprintf(
-				`Bcc: "%s" <%s>`,
-				ptr.Val(addr.GetEmailAddress().GetName()),
-				ptr.Val(addr.GetEmailAddress().GetAddress())))
 	}
 
-	// Only fist 30 chars as the .eml generator can introduce a
+	source := strings.ReplaceAll(eml.HTML, "\n", "")
-	// newline in between the text to limit the column width of the
+	target := strings.ReplaceAll(ptr.Val(msg.GetBody().GetContent()), "\n", "")
-	// output. It does not affect the data, but can break our tests and
+
-	// so using 30 as a safe limit to test.
+	// replace the cid with a constant value to make the comparison
-	assert.Contains(t, out, ptr.Val(msg.GetBody().GetContent())[:30], "body")
+	re := regexp.MustCompile(`src="cid:[^"]*"`)
+	source = re.ReplaceAllString(source, `src="cid:replaced"`)
+	target = re.ReplaceAllString(target, `src="cid:replaced"`)
+
+	assert.Equal(t, source, target)
 }