Maintain an internal copy of net/textproto with few fixes for email header

[iredmail]

fixes #282

Introduce internal package textproto, it's a clone of net/textproto with few fixes for email header.

Changes in new file textproto/reader_email.go:

• New: ReadEmailMIMEHeader() and readEmailMIMEHeader().
  • Not validate email header value byte.
• New: CanonicalEmailMIMEHeaderKey() and canonicalEmailMIMEHeaderKey()
  • Not convert header field to upper/lower cases.
• New: ValidEmailHeaderFieldByte().
  • It validates all characters allowed to be used in email header field. Note: validHeaderFieldByte() validates characters for http header field instead of email header field.

One change in textproto/header.go:

• MIMEHeader methods call CanonicalEmailMIMEHeaderKey instead of CanonicalMIMEHeaderKey.

[dcormier]

Out of curiosity, is there an issue that was opened in the golang repo related to this?

[iredmail]

Out of curiosity, is there an issue that was opened in the golang repo related to this?

I reported this issue in upstream too: golang/go#58862

If you search "net/mail" in golang repo issue tracker, there're many issues similar to this one, all caused due to net/textproto is designed for http, not for email.

[jhillyerd]

Few questions before I start reviewing:

1. What version of Go did the textproto get copied from?
2. I'd much prefer it live in internal/textproto, otherwise I'm massively expanding my public API. Was that already tried?
3. Was it necessary to copy all of textproto? Or is this just the fastest way?

[iredmail]

1. What version of Go did the textproto get copied from?

Latest development edition on github.com/golang/go. I can use v1.20.1 if you prefer.

2. I'd much prefer it live in internal/textproto, otherwise I'm massively expanding my public API. Was that already tried?

Will move soon.

3. Was it necessary to copy all of textproto? Or is this just the fastest way?

The fastest way.

[jhillyerd]

Overall looks good, just one possible change request.

[jhillyerd]

should we eliminate this func and just call the new one you defined in internal/textproto?

[iredmail]

Will do. It was left there on purpose because we may need to remove internal/textproto someday if upstream fixes it.

[jhillyerd]

Yes, I had similar thought, but we probably will not switch back to upstream quickly.

[iredmail]

Would you like to save a patch file in enmime git repo to clearly indicate the changes compared to net/textproto? It might be easier for us to sync internal/textproto with upstream but still maintain our own version for email.

[jhillyerd]

Sounds good to me.

[iredmail]

@jhillyerd Removed duplicate validHeaderFieldByte.

[iredmail]

@jhillyerd Could you help review again? also #284

[iredmail]

hi @jhillyerd,

GitHub Actions failed due to we copied net/textproto from Go v1.20.2 and it uses bytes.Clone() which introduced in v1.20.0.

Question: would you like to update go.mod to require Go 1.20, or i modify internal/textproto to replace bytes.Clone() by the code used in Go 1.16.x to keep backward-compatibility?

[jhillyerd]

As a library, I prefer to maintain backwards compatibility. Probably go 1.18 & 1.19 is enough, 1.16.x is pretty old so you could drop that. Thank you.

[iredmail]

GitHub Actions are pending for your action.

[iredmail]

Replaced bytes.Clone() to support GO v1.18 (code snippet copied from Go v1.18).

		 buf := make([]byte, len(line))
		copy(buf, line)
		line = buf

@jhillyerd Please help approve the GitHub Actions.

[jhillyerd]

Obviously we want the Build and Test runs to pass, but don't worry about the Lint, that can be addressed later.

[iredmail]

Obviously we want the Build and Test runs to pass, but don't worry about the Lint, that can be addressed later.

It's better to allow GitHub Actions when repo receives a new PR or PR gets updated, so that no need to wait for your approval to start the tests. It slows down the contribution process.

[jhillyerd]

They should auto run now, given this repo doesn't have any deploy keys it should be safe.

[iredmail]

hi @jhillyerd

There's a change i'd like to discuss with you first before i modify the code.

net/textproto modifies header cases, for example:

• MIME-version: -> Mime-Version: (IME are lower cases, v is upper case)
• Message-ID: -> Message-Id: (d is lower case)

I removed the code for such convert in our internal textproto package, but as you can see, many test cases failed due to this change.

Question for you:

1. Should we do case convert in this PR to keep enmime backward compatible?
2. Or, we keep original cases in this PR, and use original cases in test cases? This may break backward compatibility. But there's no need to convert cases at all for email headers.

Of course we can tag a new version first with option 1 implemented first, then release a new version with option 2 implemented as a breaking change.

Let me know your opinions and final decision.

[jhillyerd]

For this release, I'd like to prioritize backwards compatibility, to give a smooth upgrade path to Go 1.20 for users.

I am wondering if we would be better off just copying in Go 1.19's textproto package, instead of 1.20's? You probably have a better idea of which is worth the effort than me.

[iredmail]

For this release, I'd like to prioritize backwards compatibility, to give a smooth upgrade path to Go 1.20 for users.

OK.

I am wondering if we would be better off just copying in Go 1.19's textproto package, instead of 1.20's? You probably have a better idea of which is worth the effort than me.

• I personally prefer current internal package cloned from 1.20 with backport (bytes.Clone()).
• For backward compatibility, we will revert some minor changes, then not many changes made in this internal package.

I will send new commits tonight or tomorrow. Stay tuned and please help review.

[jhillyerd]

Thanks, hopefully this gets us running on 1.20.

I've confirmed that the textproto files being brought in are from go @ 54d05e4e25 with minor changes for backwards compat.

[jhillyerd]

Big thankyou for your help on this one!