Initial commit to System.Net.Mail. #12416

Priya91 · 2016-10-06T05:41:45Z

Changes from Desktop implementation:

SmtpConnection uses TcpClient instead of ServicePointManager.
SslStream instead of internal TlsStream.
Negotiate, NTLM, Digest authentication protocols not ported due to
dependency on internal NTAuthentication type. Authentication currently happens uses plain text login, SSL connections need to be used.
TcpClient connections are not pooled.
Getting mail settings from web.config files is not supported, as netstandard2.0 doesn't expose the System.Net.Configuration apis.
Sending mail to IIS pickup directory currently throws NotSupportedException. Desktop uses COM types for this API, investigation on partially supporting this feature on Windows is pending.

Notes:

Tests will be added separately.
Please review SmtpConnection.cs and TlsStream.cs files for correctness, they have changed functionally from Desktop code.
The TcpClient connect does not have Connect(), BeginConnect() and EndConnect() in .NET Core contract yet, once that's available, the SendAsync should use the async connect methods. - Update: Added now

Did a test locally using the SmtpClient.Send() and SmtpClient.SendAsync() , was able to successfully send emails to google smpt server with tls authentication.

cc @stephentoub @ericeil @CIPop @davidsh @geoffkizer @tijoytom @karelz

davidsh · 2016-10-06T14:18:15Z

I'm a little confused. I thought we were putting System.Net.Mail.* into the System.Net.Mail contract and not the Mime contract? And I also thought we were merging System.Net.Mime contract back into System.Net.Mail?

So, should we just assume, for now, that this entire contract implementation of System.Net.Mime (which now will include System.Net.Mail.* types) will be renamed to System.Net.Mail contract?

stephentoub · 2016-10-06T14:35:50Z

So, should we just assume, for now, that this entire contract implementation of System.Net.Mime (which now will include System.Net.Mail.* types) will be renamed to System.Net.Mail contract?

Yes, the contract/assembly should be renamed to System.Net.Mail. @Priya91, can you do that as part of this change?

justinvp · 2016-10-06T16:16:08Z

src/Common/src/System/Net/Mail/MailAddressParser.cs

+        // Parse a comma separated list of MailAddress's
+        //
+        // Throws a FormatException if any MailAddress is invalid.
+        internal static IList<MailAddress> ParseMultipleAddresses(string data)


Nit: I realize the desktop code returns IList<MailAddress>, but it would be more efficient for callers to enumerate the result if this was changed to List<MailAddress>.

justinvp · 2016-10-06T16:17:35Z

src/Common/src/System/Net/Mail/MailAddressParser.cs

@@ -283,7 +302,7 @@ private static string ParseDisplayName(string data, ref int index, bool expectMu

                // Do not include the Comma (if any), and because there were no bounding quotes, 
                // trim extra whitespace.
-                displayName = data.SubstringTrim(index + 1, startingIndex - index);
+                displayName = data.Substring(index + 1, startingIndex - index).Trim();


Was there a reason this changed? SubstringTrim will only allocate one string instead of two strings for the separate Substring and Trim operations.

justinvp · 2016-10-06T16:19:27Z

src/Common/src/System/Net/Mail/MailAddressParser.cs

@@ -4,6 +4,7 @@

 using System.Diagnostics;
 using System.Net.Mime;
+using System.Collections.Generic;


Nit: "Remove and Sort" usings

justinvp · 2016-10-06T16:24:28Z

src/System.Net.Mime/src/System/Net/DelegatedStream.cs

+        protected DelegatedStream(Stream stream)
+        {
+            if (stream == null)
+                throw new ArgumentNullException("stream");


Nit: nameof(stream)

justinvp · 2016-10-06T16:26:12Z

src/System.Net.Mime/src/System/Net/Mail/AlternateView.cs

+            {
+                if (disposed)
+                {
+                    throw new ObjectDisposedException(this.GetType().FullName);


Nit: the this. isn't necessary and can be removed.

justinvp · 2016-10-06T16:27:35Z

src/System.Net.Mime/src/System/Net/Mail/AlternateViewCollection.cs

+        {
+            if (_disposed)
+            {
+                throw new ObjectDisposedException(this.GetType().FullName);


Nit: the this. is unnecessary and can be removed. Applies throughout (I'll stop commenting on it).

justinvp · 2016-10-06T16:28:08Z

src/System.Net.Mime/src/System/Net/Mail/AlternateViewCollection.cs

+
+            if (item == null)
+            {
+                throw new ArgumentNullException("item");


Nit: nameof(item). Applies throughout (I'll stop commenting on it).

justinvp · 2016-10-06T16:32:59Z

src/System.Net.Mime/src/System/Net/Mail/AlternateViewCollection.cs

+                return;
+            }
+
+            foreach (AlternateView view in this)


Not necessarily for this PR, but a follow-up improvement would be to explicitly pass a new List<AlternateView> to the base Collection<T> constructor from this type's constructor , and then cast Items to List<AlternateView> here to enumerate the collection more efficiently (avoiding the enumerator allocation and overhead).

justinvp · 2016-10-06T16:35:24Z

src/System.Net.Mime/src/System/Net/Mail/Attachment.cs

+        internal static string ShortNameFromFile(string fileName)
+        {
+            string name;
+            int start = fileName.LastIndexOfAny(new char[] { '\\', ':' }, fileName.Length - 1, fileName.Length);


Nit: Cache the char[] in a readonly static field.

justinvp · 2016-10-06T16:36:25Z

src/System.Net.Mime/src/System/Net/Mail/Attachment.cs

+                }
+                else
+                {
+                    if (value.IndexOfAny(new char[] { '<', '>' }) != -1)


Nit: Cache the char[] in a static readonly field.

justinvp · 2016-10-06T16:40:10Z