Make FileSink constructor changes non-breaking; fix a bug whereby a s…

…pecified encoding is not used; clean and tidy

src/Serilog.Sinks.File/FileLifecycleHooks.cs

@@ -1,21 +1,37 @@
+// Copyright 2019 Serilog Contributors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System.IO;
 
 namespace Serilog
 {
-    using System.IO;
-
     /// <summary>
-    /// Enables hooking into log file lifecycle events
+    /// Enables hooking into log file lifecycle events.
     /// </summary>
     public abstract class FileLifecycleHooks
     {
         /// <summary>
-        /// Wraps <paramref name="underlyingStream"/> in another stream, such as a GZipStream, then returns the wrapped stream
+        /// Initialize or wrap the <paramref name="underlyingStream"/> opened on the log file. This can be used to write
+        /// file headers, or wrap the stream in another that adds buffering, compression, encryption, etc. The underlying
+        /// file may or may not be empty when this method is called.
         /// </summary>
         /// <remarks>
-        /// Serilog is responsible for disposing of the wrapped stream
+        /// A value must be returned from overrides of this method. Serilog will flush and/or dispose the returned value, but will not
+        /// dispose the stream initially passed in unless it is itself returned.
         /// </remarks>
-        /// <param name="underlyingStream">The underlying log file stream</param>
-        /// <returns>The wrapped stream</returns>
-        public abstract Stream Wrap(Stream underlyingStream);
+        /// <param name="underlyingStream">The underlying <see cref="Stream"/> opened on the log file.</param>
+        /// <returns>The <see cref="Stream"/> Serilog should use when writing events to the log file.</returns>
+        public virtual Stream OnOpened(Stream underlyingStream) => underlyingStream;
     }
 }

src/Serilog.Sinks.File/FileLoggerConfigurationExtensions.cs

@@ -301,7 +301,7 @@ public static class FileLoggerConfigurationExtensions
             if (fileSizeLimitBytes.HasValue && fileSizeLimitBytes < 0) throw new ArgumentException("Negative value provided; file size limit must be non-negative.", nameof(fileSizeLimitBytes));
             if (retainedFileCountLimit.HasValue && retainedFileCountLimit < 1) throw new ArgumentException("At least one file must be retained.", nameof(retainedFileCountLimit));
             if (shared && buffered) throw new ArgumentException("Buffered writes are not available when file sharing is enabled.", nameof(buffered));
-            if (shared && hooks != null) throw new ArgumentException("Unable to use {hooks.GetType().Name} FileLifecycleHooks output stream wrapper - these are not supported for shared log files", nameof(hooks));
+            if (shared && hooks != null) throw new ArgumentException("File lifecycle hooks are not currently supported for shared log files.", nameof(hooks));
 
             ILogEventSink sink;
 
@@ -313,16 +313,16 @@ public static class FileLoggerConfigurationExtensions
             {
                 try
                 {
-#pragma warning disable 618
                     if (shared)
                     {
-                        sink = new SharedFileSink(path, formatter, fileSizeLimitBytes);
+#pragma warning disable 618
+                        sink = new SharedFileSink(path, formatter, fileSizeLimitBytes, encoding);
+#pragma warning restore 618
                     }
                     else
                     {
-                        sink = new FileSink(path, formatter, fileSizeLimitBytes, buffered: buffered, hooks: hooks);
+                        sink = new FileSink(path, formatter, fileSizeLimitBytes, encoding, buffered, hooks);
                     }
-#pragma warning restore 618
                 }
                 catch (Exception ex)
                 {

src/Serilog.Sinks.File/Sinks/File/FileSink.cs

@@ -23,7 +23,6 @@ namespace Serilog.Sinks.File
     /// <summary>
     /// Write log events to a disk file.
     /// </summary>
-    [Obsolete("This type will be removed from the public API in a future version; use `WriteTo.File()` instead.")]
     public sealed class FileSink : IFileSink, IDisposable
     {
         readonly TextWriter _output;
@@ -43,18 +42,27 @@ public sealed class FileSink : IFileSink, IDisposable
         /// <param name="encoding">Character encoding used to write the text file. The default is UTF-8 without BOM.</param>
         /// <param name="buffered">Indicates if flushing to the output file can be buffered or not. The default
         /// is false.</param>
-        /// <param name="hooks">Optionally enables hooking into log file lifecycle events.</param>
         /// <returns>Configuration object allowing method chaining.</returns>
-        /// <remarks>The file will be written using the UTF-8 character set.</remarks>
+        /// <remarks>This constructor preserves compatibility with early versions of the public API. New code should not depend on this type.</remarks>
         /// <exception cref="IOException"></exception>
-        public FileSink(string path, ITextFormatter textFormatter, long? fileSizeLimitBytes, Encoding encoding = null, bool buffered = false,
-            FileLifecycleHooks hooks = null)
+        [Obsolete("This type and constructor will be removed from the public API in a future version; use `WriteTo.File()` instead.")]
+        public FileSink(string path, ITextFormatter textFormatter, long? fileSizeLimitBytes, Encoding encoding = null, bool buffered = false)
+            : this(path, textFormatter, fileSizeLimitBytes, encoding, buffered, null)
+        {
+        }
+
+        // This overload should be used internally; the overload above maintains compatibility with the earlier public API.
+        internal FileSink(
+            string path,
+            ITextFormatter textFormatter,
+            long? fileSizeLimitBytes,
+            Encoding encoding,
+            bool buffered,
+            FileLifecycleHooks hooks)
         {
             if (path == null) throw new ArgumentNullException(nameof(path));
-            if (textFormatter == null) throw new ArgumentNullException(nameof(textFormatter));
             if (fileSizeLimitBytes.HasValue && fileSizeLimitBytes < 0) throw new ArgumentException("Negative value provided; file size limit must be non-negative.");
-
-            _textFormatter = textFormatter;
+            _textFormatter = textFormatter ?? throw new ArgumentNullException(nameof(textFormatter));
             _fileSizeLimitBytes = fileSizeLimitBytes;
             _buffered = buffered;
 
@@ -72,12 +80,8 @@ public sealed class FileSink : IFileSink, IDisposable
 
             if (hooks != null)
             {
-                outputStream = hooks.Wrap(outputStream);
-
-                if (outputStream == null)
-                {
-                    throw new InvalidOperationException($"{hooks.GetType().Name}.Wrap returned null when wrapping the output stream");
-                }
+                outputStream = hooks.OnOpened(outputStream) ??
+                               throw new InvalidOperationException($"The file lifecycle hooks `{nameof(FileLifecycleHooks.OnOpened)}()` returned `null` when called with the output stream.");
             }
 
             _output = new StreamWriter(outputStream, encoding ?? new UTF8Encoding(encoderShouldEmitUTF8Identifier: false));

src/Serilog.Sinks.File/Sinks/File/RollingFileSink.cs

@@ -12,8 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#pragma warning disable 618
-
 using System;
 using System.IO;
 using System.Linq;
@@ -52,11 +50,11 @@ sealed class RollingFileSink : ILogEventSink, IFlushableFileSink, IDisposable
                               bool shared,
                               RollingInterval rollingInterval,
                               bool rollOnFileSizeLimit,
-                              FileLifecycleHooks hooks = null)
+                              FileLifecycleHooks hooks)
         {
             if (path == null) throw new ArgumentNullException(nameof(path));
-            if (fileSizeLimitBytes.HasValue && fileSizeLimitBytes < 0) throw new ArgumentException("Negative value provided; file size limit must be non-negative");
-            if (retainedFileCountLimit.HasValue && retainedFileCountLimit < 1) throw new ArgumentException("Zero or negative value provided; retained file count limit must be at least 1");
+            if (fileSizeLimitBytes.HasValue && fileSizeLimitBytes < 0) throw new ArgumentException("Negative value provided; file size limit must be non-negative.");
+            if (retainedFileCountLimit.HasValue && retainedFileCountLimit < 1) throw new ArgumentException("Zero or negative value provided; retained file count limit must be at least 1.");
 
             _roller = new PathRoller(path, rollingInterval);
             _textFormatter = textFormatter;
@@ -149,8 +147,11 @@ void OpenFile(DateTime now, int? minSequence = null)
                 try
                 {
                     _currentFile = _shared ?
+#pragma warning disable 618
                         (IFileSink)new SharedFileSink(path, _textFormatter, _fileSizeLimitBytes, _encoding) :
+#pragma warning restore 618
                         new FileSink(path, _textFormatter, _fileSizeLimitBytes, _encoding, _buffered, _hooks);
+
                     _currentFileSequence = sequence;
                 }
                 catch (IOException ex)

src/Serilog.Sinks.File/Sinks/File/SharedFileSink.AtomicAppend.cs

@@ -18,7 +18,6 @@
 using System.IO;
 using System.Security.AccessControl;
 using System.Text;
-using Serilog.Core;
 using Serilog.Events;
 using Serilog.Formatting;
 
@@ -51,17 +50,14 @@ public sealed class SharedFileSink : IFileSink, IDisposable
         /// will be written in full even if it exceeds the limit.</param>
         /// <param name="encoding">Character encoding used to write the text file. The default is UTF-8 without BOM.</param>
         /// <returns>Configuration object allowing method chaining.</returns>
-        /// <remarks>The file will be written using the UTF-8 character set.</remarks>
         /// <exception cref="IOException"></exception>
         public SharedFileSink(string path, ITextFormatter textFormatter, long? fileSizeLimitBytes, Encoding encoding = null)
         {
-            if (path == null) throw new ArgumentNullException(nameof(path));
-            if (textFormatter == null) throw new ArgumentNullException(nameof(textFormatter));
             if (fileSizeLimitBytes.HasValue && fileSizeLimitBytes < 0)
                 throw new ArgumentException("Negative value provided; file size limit must be non-negative");
 
-            _path = path;
-            _textFormatter = textFormatter;
+            _path = path ?? throw new ArgumentNullException(nameof(path));
+            _textFormatter = textFormatter ?? throw new ArgumentNullException(nameof(textFormatter));
             _fileSizeLimitBytes = fileSizeLimitBytes;
 
             var directory = Path.GetDirectoryName(path);

test/Serilog.Sinks.File.Tests/FileSinkTests.cs

@@ -153,15 +153,15 @@ public void WhenStreamWrapperIsSpecifiedOutputStreamIsWrapped()
                 var nonexistent = tmp.AllocateFilename("txt");
                 var evt = Some.LogEvent("Hello, world!");
 
-                using (var sink = new FileSink(nonexistent, new JsonFormatter(), null, hooks: gzipWrapper))
+                using (var sink = new FileSink(nonexistent, new JsonFormatter(), null, null, false, gzipWrapper))
                 {
                     sink.Emit(evt);
                     sink.Emit(evt);
                 }
 
                 // Ensure the data was written through the wrapping GZipStream, by decompressing and comparing against
                 // what we wrote
-                var lines = new List<string>();
+                List<string> lines;
                 using (var textStream = new MemoryStream())
                 {
                     using (var fs = System.IO.File.OpenRead(nonexistent))
@@ -185,7 +185,7 @@ static void WriteTwoEventsAndCheckOutputFileLength(long? maxBytes, Encoding enco
             {
                 var path = tmp.AllocateFilename("txt");
                 var evt = Some.LogEvent("Irrelevant as it will be replaced by the formatter");
-                var actualEventOutput = "x";
+                const string actualEventOutput = "x";
                 var formatter = new FixedOutputFormatter(actualEventOutput);
                 var eventOuputLength = encoding.GetByteCount(actualEventOutput);
 

test/Serilog.Sinks.File.Tests/Support/GZipHooks.cs

@@ -16,9 +16,9 @@ public GZipHooks(int bufferSize = 1024 * 32)
             _bufferSize = bufferSize;
         }
 
-        public override Stream Wrap(Stream sourceStream)
+        public override Stream OnOpened(Stream underlyingStream)
         {
-            var compressStream = new GZipStream(sourceStream, CompressionMode.Compress);
+            var compressStream = new GZipStream(underlyingStream, CompressionMode.Compress);
             return new BufferedStream(compressStream, _bufferSize);
         }
     }