Use best-practices for Exception classes

Fixes #12285
Related #15214

It turns out that the code causing #15214 to happen is not due to normal serialization, and hence it is likely not possible to fix this in EF code. (See comments on the issue.) However, the issue (or similar) would still exist if the exception was serialized, and the fix is to use best practices for exceptions, so doing that.

For reference, see https://docs.microsoft.com/en-us/dotnet/standard/exceptions/best-practices-for-exceptions

Author: ajcvickers

src/EFCore.Design/Design/OperationException.cs

@@ -2,15 +2,24 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Runtime.Serialization;
 using JetBrains.Annotations;
 
 namespace Microsoft.EntityFrameworkCore.Design
 {
     /// <summary>
     ///     Represents an exception whose stack trace should, by default, not be reported by the commands.
     /// </summary>
+    [Serializable]
     public class OperationException : Exception
     {
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="OperationException" /> class.
+        /// </summary>
+        public OperationException()
+        {
+        }
+
         /// <summary>
         ///     Initializes a new instance of the <see cref="OperationException" /> class.
         /// </summary>
@@ -29,5 +38,15 @@ public OperationException([NotNull] string message, [CanBeNull] Exception innerE
             : base(message, innerException)
         {
         }
+
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="DbUpdateException" /> class from a serialized form.
+        /// </summary>
+        /// <param name="info"> The serialization info. </param>
+        /// <param name="context"> The streaming context being used. </param>
+        public OperationException(SerializationInfo info, StreamingContext context)
+            : base(info, context)
+        {
+        }
     }
 }

src/EFCore/DbUpdateConcurrencyException.cs

@@ -1,7 +1,9 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Collections.Generic;
+using System.Runtime.Serialization;
 using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.Update;
 
@@ -12,8 +14,49 @@ namespace Microsoft.EntityFrameworkCore
     ///     occurs when an unexpected number of rows are affected during save. This is usually because the data in the database has
     ///     been modified since it was loaded into memory.
     /// </summary>
+    [Serializable]
     public class DbUpdateConcurrencyException : DbUpdateException
     {
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="DbUpdateConcurrencyException" /> class.
+        /// </summary>
+        public DbUpdateConcurrencyException()
+        {
+        }
+
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="DbUpdateConcurrencyException" /> class.
+        /// </summary>
+        /// <param name="message"> The error message that explains the reason for the exception. </param>
+        public DbUpdateConcurrencyException([NotNull] string message)
+            : base(message)
+        {
+        }
+
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="DbUpdateConcurrencyException" /> class.
+        /// </summary>
+        /// <param name="message"> The error message that explains the reason for the exception. </param>
+        /// <param name="innerException"> The exception that is the cause of the current exception. </param>
+        public DbUpdateConcurrencyException([NotNull] string message, [CanBeNull] Exception innerException)
+            : base(message, innerException)
+        {
+        }
+
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="DbUpdateConcurrencyException" /> class.
+        /// </summary>
+        /// <param name="message"> The error message that explains the reason for the exception. </param>
+        /// <param name="innerException"> The exception that is the cause of the current exception. </param>
+        /// <param name="entries"> The entries that were involved in the error. </param>
+        public DbUpdateConcurrencyException(
+            [NotNull] string message,
+            [CanBeNull] Exception innerException,
+            [NotNull] IReadOnlyList<IUpdateEntry> entries)
+            : base(message, innerException, entries)
+        {
+        }
+
         /// <summary>
         ///     Initializes a new instance of the <see cref="DbUpdateConcurrencyException" /> class.
         /// </summary>
@@ -25,5 +68,15 @@ public DbUpdateConcurrencyException(
             : base(message, entries)
         {
         }
+
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="DbUpdateException" /> class from a serialized form.
+        /// </summary>
+        /// <param name="info"> The serialization info. </param>
+        /// <param name="context"> The streaming context being used. </param>
+        public DbUpdateConcurrencyException(SerializationInfo info, StreamingContext context)
+            : base(info, context)
+        {
+        }
     }
 }

src/EFCore/DbUpdateException.cs

@@ -4,9 +4,9 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Runtime.Serialization;
 using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.ChangeTracking;
-using Microsoft.EntityFrameworkCore.Internal;
 using Microsoft.EntityFrameworkCore.Update;
 using Microsoft.EntityFrameworkCore.Utilities;
 
@@ -15,8 +15,28 @@ namespace Microsoft.EntityFrameworkCore
     /// <summary>
     ///     An exception that is thrown when an error is encountered while saving to the database.
     /// </summary>
+    [Serializable]
     public class DbUpdateException : Exception
     {
+        [NonSerialized]
+        private IReadOnlyList<EntityEntry> _entries;
+
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="DbUpdateException" /> class.
+        /// </summary>
+        public DbUpdateException()
+        {
+        }
+
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="DbUpdateException" /> class.
+        /// </summary>
+        /// <param name="message"> The error message that explains the reason for the exception. </param>
+        public DbUpdateException([NotNull] string message)
+            : base(message)
+        {
+        }
+
         /// <summary>
         ///     Initializes a new instance of the <see cref="DbUpdateException" /> class.
         /// </summary>
@@ -25,7 +45,6 @@ public class DbUpdateException : Exception
         public DbUpdateException([NotNull] string message, [CanBeNull] Exception innerException)
             : base(message, innerException)
         {
-            Entries = new List<EntityEntry>();
         }
 
         /// <summary>
@@ -44,8 +63,8 @@ public DbUpdateException(
         ///     Initializes a new instance of the <see cref="DbUpdateException" /> class.
         /// </summary>
         /// <param name="message"> The error message that explains the reason for the exception. </param>
-        /// <param name="entries"> The entries that were involved in the error. </param>
         /// <param name="innerException"> The exception that is the cause of the current exception. </param>
+        /// <param name="entries"> The entries that were involved in the error. </param>
         public DbUpdateException(
             [NotNull] string message,
             [CanBeNull] Exception innerException,
@@ -54,15 +73,25 @@ public DbUpdateException(
         {
             Check.NotEmpty(entries, nameof(entries));
 
-            Entries = entries
+            _entries = entries
                 .Where(e => e.EntityState != EntityState.Unchanged)
                 .Select(e => e.ToEntityEntry()).ToList();
         }
 
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="DbUpdateException" /> class from a serialized form.
+        /// </summary>
+        /// <param name="info"> The serialization info. </param>
+        /// <param name="context"> The streaming context being used. </param>
+        public DbUpdateException(SerializationInfo info, StreamingContext context)
+            : base(info, context)
+        {
+        }
+
         /// <summary>
         ///     Gets the entries that were involved in the error. Typically this is a single entry, but in some cases it
         ///     may be zero or multiple entries.
         /// </summary>
-        public virtual IReadOnlyList<EntityEntry> Entries { get; }
+        public virtual IReadOnlyList<EntityEntry> Entries => _entries ??= new List<EntityEntry>();
     }
 }

src/EFCore/Storage/RetryLimitExceededException.cs

@@ -2,15 +2,24 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Runtime.Serialization;
 using JetBrains.Annotations;
 
 namespace Microsoft.EntityFrameworkCore.Storage
 {
     /// <summary>
     ///     The exception that is thrown when the action failed more times than the configured limit.
     /// </summary>
+    [Serializable]
     public class RetryLimitExceededException : Exception
     {
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="RetryLimitExceededException" /> class.
+        /// </summary>
+        public RetryLimitExceededException()
+        {
+        }
+
         /// <summary>
         ///     Initializes a new instance of the <see cref="RetryLimitExceededException" /> class with a specified error message.
         /// </summary>
@@ -29,5 +38,15 @@ public RetryLimitExceededException([NotNull] string message, [NotNull] Exception
             : base(message, innerException)
         {
         }
+
+        /// <summary>
+        ///     Initializes a new instance of the <see cref="DbUpdateException" /> class from a serialized form.
+        /// </summary>
+        /// <param name="info"> The serialization info. </param>
+        /// <param name="context"> The streaming context being used. </param>
+        public RetryLimitExceededException(SerializationInfo info, StreamingContext context)
+            : base(info, context)
+        {
+        }
     }
 }

test/EFCore.Design.Tests/ApiConsistencyTest.cs

@@ -4,7 +4,12 @@
 using System;
 using System.Collections.Generic;
 using System.Reflection;
+using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
 using Microsoft.EntityFrameworkCore.Design;
+using Microsoft.EntityFrameworkCore.Metadata;
+using Microsoft.EntityFrameworkCore.Metadata.Conventions;
+using Microsoft.EntityFrameworkCore.TestUtilities;
+using Microsoft.EntityFrameworkCore.Update;
 using Microsoft.Extensions.DependencyInjection;
 
 namespace Microsoft.EntityFrameworkCore

test/EFCore.Design.Tests/DesignExceptionTest.cs

@@ -0,0 +1,61 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+using System.IO;
+using System.Runtime.Serialization.Formatters.Binary;
+using Microsoft.EntityFrameworkCore.Design;
+using Xunit;
+
+namespace Microsoft.EntityFrameworkCore
+{
+    public class DesignExceptionTest
+    {
+        [Fact]
+        public void OperationException_exposes_public_empty_constructor()
+        {
+            new OperationException();
+        }
+
+        [Fact]
+        public void OperationException_exposes_public_string_constructor()
+        {
+            Assert.Equal("Foo", new OperationException("Foo").Message);
+        }
+
+        [Fact]
+        public void OperationException_exposes_public_string_and_inner_exception_constructor()
+        {
+            var inner = new Exception();
+
+            var ex = new OperationException("Foo", inner);
+
+            Assert.Equal("Foo", ex.Message);
+            Assert.Same(inner, ex.InnerException);
+        }
+
+        [Fact]
+        public void Deserialized_OperationException_can_be_serialized_and_deserialized_again()
+        {
+            var transportedException = SerializeAndDeserialize(
+                SerializeAndDeserialize(
+                    new OperationException(
+                        "But somehow the vital connection is made",
+                        new Exception("Bang!"))));
+
+            Assert.Equal("But somehow the vital connection is made", transportedException.Message);
+            Assert.Equal("Bang!", transportedException.InnerException.Message);
+        }
+
+        private TException SerializeAndDeserialize<TException>(TException exception) where TException : Exception
+        {
+            var stream = new MemoryStream();
+            var formatter = new BinaryFormatter();
+
+            formatter.Serialize(stream, exception);
+            stream.Seek(0, SeekOrigin.Begin);
+
+            return (TException)formatter.Deserialize(stream);
+        }
+    }
+}