feat: add option skip malformed parts

[dmytrokasianenko-outreach]

This PR is proposal for the feature which allows to skip parts that can't be parsed. It won't return error, just add them to "problems"

This can be useful if you need to parse everything what is possible to parse and turned off by default.

For example, we have emails which have body of the email starting at the beginning of the part (without headers). Currently parser returns error, but it can be beneficial to skip this part.

...
Content-Type: multipart/report; report-type=delivery-status;
	boundary="12345/example.com"
Auto-Submitted: auto-generated (failure)
MIME-Version: 1.0

--12345/example.com
[EXTERNAL EMAIL]

The original message was received
...

[jhillyerd]

I'm in favor of this, will take a bit of time to review, and may hold off merging until after the next release.

Edit: or if you think you will have follow-ups, we can merge into a dedicated branch for now

[iredmail]

How about a new ReadEnvelopeWithOptions() function instead of modifying existing ReadEnvelope()?

I'd like to add options like:

• skip reading / parsing mail body (just parse headers in some cases)
• Skip (base64) decoding attachments (no need to decode it in some cases, so why not save some system resources)

[jhillyerd]

My initial plan for options was like @iredmail describes above, however I think if I designed it today, I'd have a options builder struct with ReadEnvelope/etc as methods on it. This would make it easier for people to mock in tests. The original package funcs could just forward to those methods on a default options object.

That being said, I haven't thought about the strategy @dmytrokasianenko-outreach used before, so I will spend some time on it. Probably not this weekend though. Most code would continue to work with that change, but I think I've seen breakage from that type of change in the past. I expect we'd want to go from 0.9 series to 0.10 or 1.0 in that case.

[dmytrokasianenko-outreach]

Probably better solutions could be to use option interfaces, see Uber Go Style Guide for functional options. Mainly:

Note that there's a method of implementing this pattern with closures but we believe that the pattern above provides more flexibility for authors and is easier to debug and test for users. In particular, it allows options to be compared against each other in tests and mocks, versus closures where this is impossible. Further, it lets options implement other interfaces, including fmt.Stringer which allows for user-readable string representations of the options.

WDYT?

[kadukf-outreach]

Q1: @jhillyerd so that it would be possible to replace some part of the message parsing flow? So that we will have open/close principle in place - open for extension but closed for modifications - closed part would be message parsing orchestration (method ReadParts) which drives boundary reader but the particular steps would be replaceable (use defaults it nothing provided), right? IMO the question is how much of the openings is needed/desired.

Q2: would it be possible to introduce custom error for malformed parts so that when processing "problems" it would be possible to get a pointer to the malformed part, type of issue (header, content, etc).

Q3: Shouldn't explicitly "skip malformed parts" be part of the default behavior so that after parsing we get some results with references to the failed parts so that we can handle it accordingly, wdyt?

[jhillyerd]

Probably better solutions could be to use option interfaces, see Uber Go Style Guide for functional options. Mainly:

Note that there's a method of implementing this pattern with closures but we believe that the pattern above provides more flexibility for authors and is easier to debug and test for users. In particular, it allows options to be compared against each other in tests and mocks, versus closures where this is impossible. Further, it lets options implement other interfaces, including fmt.Stringer which allows for user-readable string representations of the options.

WDYT?

After giving it some thought, I'm OK with the functional options pattern you are using, and it seems more acceptable in Go than a builder pattern. What I'm not OK with is forcing our users to build their options from scratch for every call to enmime, most users will be using identical options for every parse. Thoughts on making config public (while keeping the fields private)?

Edit: I didn't quite grok the interfaces part on the first read, you're right that would be better than public config struct.

[jhillyerd]

Q1: @jhillyerd so that it would be possible to replace some part of the message parsing flow? So that we will have open/close principle in place - open for extension but closed for modifications - closed part would be message parsing orchestration (method ReadParts) which drives boundary reader but the particular steps would be replaceable (use defaults it nothing provided), right? IMO the question is how much of the openings is needed/desired.

This seems way out of scope for this PR, or even the 1.0 release of enmime.

Q2: would it be possible to introduce custom error for malformed parts so that when processing "problems" it would be possible to get a pointer to the malformed part, type of issue (header, content, etc).

Probably, but that can come later.

Q3: Shouldn't explicitly "skip malformed parts" be part of the default behavior so that after parsing we get some results with references to the failed parts so that we can handle it accordingly, wdyt?

No, this may have security implications for existing users. I expect very few users ever inspect the soft errors enmime returns.

[dmytrokasianenko-outreach]

What I'm not OK with is forcing our users to build their options from scratch for every call to enmime, most users will be using identical options for every parse. Thoughts on making config public (while keeping the fields private)?

I have a idea, but it would require API changes:

// build parser once, with all configs
p := enmime.NewParser(ops...)
// use multiple times
p.Parse(...)
p.Parse(...)

[iredmail]

What I'm not OK with is forcing our users to build their options from scratch for every call to enmime, most users will be using identical options for every parse. Thoughts on making config public (while keeping the fields private)?

I have a idea, but it would require API changes:

// build parser once, with all configs
p := enmime.NewParser(ops...)
// use multiple times
p.Parse(...)
p.Parse(...)

A new ReadEnvelopeWithOptions() is better IMO, no backward-comptability concern.

[jhillyerd]

I have a idea, but it would require API changes

I like that. We could modify the existing ReadEnvelope + ReadParts to use a default set of options (private), so the old API continues to work.

[dmytrokasianenko-outreach]

A new ReadEnvelopeWithOptions() is better IMO, no backward-comptability concern.

function will still require to pass options and build config every time you want to parse an email.

[dmytrokasianenko-outreach]

@jhillyerd updated code to use functional options as interfaces and added parser constructor which consumes options.
How does it look to you now?

[jhillyerd]

Overall this looks great, I added one question and a couple changes.

My plan is to merge this into a separate branch, release the final 0.9.x, and then merge this to master in the next week or so. This will kick off 0.10, which I hope will be a rather short series of releases before going 1.0. 🚀

[jhillyerd]

Thanks! This is probably one of the most exciting changes for enmime in years. I'll try to release 0.9.x & merge it to master this week.

[jhillyerd]

Merged to master