go-smtp icon indicating copy to clipboard operation
go-smtp copied to clipboard

Read server response on DATA command

Open kayrus opened this issue 3 years ago • 9 comments

This is a braking change. Now Data() method will accept a callaback function and LMTPData() method callbacks will receive success statuses instead nil. Therefore dependent LMTPData code must check for status response code.

Resolves #189

P.S. I rewrote the (d *dataCloser) Close() code to make it more readable.

kayrus avatar Jun 03 '22 19:06 kayrus

I came here with the exact same dilemma -- I need to capture & log the 250 response to all DATA commands. I was about to write a PR myself then saw this. 😄

The semantics of returning an SMTPError on a 250 response seems a little off to me since this isn't really an error condition. My thought on this was to add some kind of additional property to the client struct that was a string called DataResponse or something like that. It just holds the response to the last DATA command issued (when not an error, errors would still be returned as usual). This would also avoid the breaking change to Close() as discussed.

But I'll gladly take this as well. Any indication of whether this is going to be accepted?

Slugger avatar Jun 12 '22 21:06 Slugger

@Slugger then I can simply add the same functionality as a callback function in LMTP session.

kayrus avatar Jun 13 '22 09:06 kayrus

@Slugger @emersion I updated the PR, let me know your feedback.

kayrus avatar Jun 13 '22 11:06 kayrus

I like this approach better for sure. Now I can handle the DATA response as needed via a callback.

Slugger avatar Jun 13 '22 16:06 Slugger

@emersion kindly ping

kayrus avatar Jul 05 '22 10:07 kayrus

Sorry for the delay. I think in general I'm not a huge fan of callbacks... Could we instead turn the returned io.WriteCloser into something like a *DataCommand, and expose the info message as a method?

emersion avatar Feb 03 '24 21:02 emersion

Agreed with emersion, I think adding an explicit method looks like better design to me.

I am also not sure if SMTPError is the right type to use here, I would at least create an alias named SMTPResponse or something - some careful refactoring may be needed here.

foxcpp avatar Feb 05 '24 15:02 foxcpp

Do we really need to expose the full response? Is there anything interesting in there apart from the human-readable message?

emersion avatar Feb 05 '24 15:02 emersion

Usually a message ID from the upstream receiver. My use case at the time was that I was trying to use this as a piece in an email forwarding service and when I forwarded messages upstream I wanted to capture the message id from the receiver for logging, tracking and auditing purposes.

Slugger avatar Feb 05 '24 17:02 Slugger

Closing in favor of https://github.com/emersion/go-smtp/pull/263

emersion avatar Apr 24 '24 14:04 emersion