Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attachment issues #110

Closed
Karitham opened this issue Jan 30, 2023 · 4 comments · Fixed by #111
Closed

Attachment issues #110

Karitham opened this issue Jan 30, 2023 · 4 comments · Fixed by #111
Assignees
Labels
bug Something isn't working

Comments

@Karitham
Copy link

Description

Multiple calls to WriteTo lose files (and embeds).

To Reproduce

Call WriteTo multiple times.

for example, msg.WriteFile and then client.Send(msg)

Expected behaviour

Attachments are written with every call to WriteTo

Screenshots

No response

Attempted Fixes

I haven't attempted any fix yet, but expect that breaking the io.Reader interface to an io.ReadSeeker would be the simplest fix to that issue.

Another way to avoid breaking compatibility would be to call the fileOptions on every call to write, and set the writer in a closure, like this.

		msg.AttachFile(a.Name, func(f *mail.File) {
			buf := bytes.NewBuffer(a.Content)
			f.Writer = func(w io.Writer) (int64, error) {
				return buf.WriteTo(w)
			}
			f.Name = a.Name
			f.Header.Set(string(mail.HeaderContentType), a.ContentType)
		})

If the options are called on WriteTo, subsequent calls to WriteTo get a new writer, or the user can try to seek if that's better.

Additional context

No response

@Karitham Karitham added the bug Something isn't working label Jan 30, 2023
@wneessen
Copy link
Owner

Hi @Karitham,

thanks for the report but I can't reproduce the behaviour you are experiencing.

I've quickly used this test code:

package main

import (
	"fmt"
	"os"

	"github.com/wneessen/go-mail"
)

func main() {
	m := mail.NewMsg()
	m.Subject("Subject")
	_ = m.To("[email protected]")
	_ = m.From("[email protected]")
	m.SetBodyString(mail.TypeTextPlain, "Content")
	m.AttachFile("main.go")
	m.EmbedFile("main.go")

	fmt.Println("First try:")
	_, _ = m.WriteTo(os.Stdout)
	fmt.Println("Second try:")
	_, _ = m.WriteTo(os.Stdout)
	fmt.Println("Third try:")
	_ = m.WriteToFile("tempfile.out")

	fmt.Println("Send mail:")
	tu := os.Getenv("TEST_USER")
	tp := os.Getenv("TEST_PASS")
	c, err := mail.NewClient(os.Getenv("TEST_HOST"), mail.WithTLSPolicy(mail.TLSMandatory),
		mail.WithSMTPAuth(mail.SMTPAuthPlain), mail.WithUsername(tu),
		mail.WithPassword(tp), mail.WithPort(587))
	if err != nil {
		fmt.Printf("failed to create new client: %s\n", err)
		os.Exit(1)
	}
	if err := c.DialAndSend(m); err != nil {
		fmt.Printf("failed to dial: %s\n", err)
		os.Exit(1)
	}
}

And both the embed and the attached files are present in all 4 outputs (meaning, the two msg.WriteTo() to os.Stdout, the msg.WriteToFile() and the final sending via a mail server to an actual mail account.

Is it possible that you are using an old version of go-mail. I remembers that we had an issue with consecutive writes in a previous version. If you are not using the latest version, can you please retry with v0.3.8, please? In case you are using the latest version, would you be able to provide a proof of concept code snippet that shows the issue in action? Maybe also provide some environment details like OS/Go version, so I can try to reproduce?

@wneessen wneessen added the more details needed This issue is currently waiting for more details from the reporter label Jan 30, 2023
@wneessen wneessen self-assigned this Jan 30, 2023
@Karitham
Copy link
Author

Karitham commented Jan 31, 2023

version info

github.com/wneessen/go-mail v0.3.8 h1:ja5D/o/RVwrtRIYFlrO7GmtcjDNeMakGQuwQRZYv0JM=
github.com/wneessen/go-mail v0.3.8/go.mod h1:m25lkU2GYQnlVr6tdwK533/UXxo57V0kLOjaFYmub0E=

reproduction

func main() {
	m := mail.NewMsg()
	m.AttachReader("hi", bytes.NewBuffer([]byte("oh hi mark!")))

	o1 := bytes.Buffer{}
	m.WriteTo(&o1)

	o2 := bytes.Buffer{}
	m.WriteTo(&o2)

	fmt.Println(len(o1.String()), len(o2.String()))
	fmt.Println("----")
	fmt.Println(o1.String())
	fmt.Println("----")
	fmt.Println(o2.String())
}
Output
406 388
----
Date: Tue, 31 Jan 2023 08:51:28 +0100
MIME-Version: 1.0
Message-ID: <114365750.248008344750.QOChqrQRdM1XBKAFa@nixos>
User-Agent: go-mail v0.3.8 // https://github.com/wneessen/go-mail
X-Mailer: go-mail v0.3.8 // https://github.com/wneessen/go-mail
Content-Type: application/octet-stream; name="hi"
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="hi"

b2ggaGkgbWFyayE=

----
Date: Tue, 31 Jan 2023 08:51:28 +0100
MIME-Version: 1.0
Message-ID: <114365750.248008344750.QOChqrQRdM1XBKAFa@nixos>
User-Agent: go-mail v0.3.8 // https://github.com/wneessen/go-mail
X-Mailer: go-mail v0.3.8 // https://github.com/wneessen/go-mail
Content-Type: application/octet-stream; name="hi"
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename="hi"


more description of the issue

Using raw files works because it's implemented as

return &File{
		Name:   filepath.Base(n),
		Header: make(map[string][]string),
		Writer: func(w io.Writer) (int64, error) {
			h, err := os.Open(n)
			if err != nil {
				return 0, err
			}
			nb, err := io.Copy(w, h)
			if err != nil {
				_ = h.Close()
				return nb, fmt.Errorf("failed to copy file to io.Writer: %w", err)
			}
			return nb, h.Close()
		},
	}

which opens the file every time and copies.

AttachReader doesn't have that option.

Also sorry about trying to mess around with AttachFile, it was simply wrong use.
This also brings up another issue: AttachFile should probably fail properly and return an error if Stating the file fails.

@wneessen
Copy link
Owner

Thanks, again. This helps. I'll have a deeper look soon.

@wneessen wneessen added TODO WIP Work is in progress and removed more details needed This issue is currently waiting for more details from the reporter TODO labels Jan 31, 2023
wneessen added a commit that referenced this issue Jan 31, 2023
This PR addresses #110.
`Msg.AttachReader()` would not output the attached file after consecutive writes (e.g. a write to a file and then send via Client).

This PR fixes this behaviour by first reading the io.Reader into memory and then creating a new `bytes.Reader`, which does support seeking. In the writeFunc we then seek to position 0 after a successful `io.Copy`. This is probably not the most memory efficient way of handling this, but otherwise we'll have to break the `io.Reader` interface.

Additionally, a new way of attaching/embedding files has been added: `Msg.AttachReadSeeker()` and `Msg.EmbedReadSeeker()` which take a ´io.ReadSeeker` as argument instead. These two methods will skip the reading into memory and make use of the `Seek` method of the corresponding interface instead.
@wneessen wneessen linked a pull request Jan 31, 2023 that will close this issue
@wneessen
Copy link
Owner

This has been fixed with PR #111. To not break the current implementation that requires an io.Reader the data is read into memory and then buffered in a io.ReadSeeker. Additionally, there we now have Msg.AttachReadSeeker() and Msg.EmbedReadSeeker() which require an io.ReadSeeker interface to be provided as argument, which should be more efficient since the reading into memory is skipped and left to the io.Seeker.

We are just in the mids of finishing some features, so you can expect a new release containing this fix soon. In the meantime you can work with main.

@wneessen wneessen removed the WIP Work is in progress label Jan 31, 2023
wneessen added a commit that referenced this issue Oct 28, 2024
Added tests for `AttachReader`, `AttachReadSeeker`, `EmbedReader`, and `EmbedReadSeeker` methods with consecutive `WriteTo` calls to ensure attachments are not lost. This addresses issue #110 on GitHub. Also, added tests for `SetBodyWriter` with a nil option and improved existing tests for `hasAlt` and `hasMixed` methods. This commit concludes the tests for msg.go. We have achieved the highest possible coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants