From 1780ae5934a9d53980de89855c0f0ae63e8ced45 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 8 Jun 2021 14:27:23 +0200 Subject: [PATCH 1/4] move the validaiton logic to FileStreamOptions --- .../tests/FileStream/FileStreamOptions.cs | 138 ++++++++++++++++++ .../tests/FileStream/ctor_options_as.cs | 2 + .../tests/FileStream/ctor_str_fm.cs | 6 +- .../tests/FileStream/ctor_str_fm_fa.cs | 4 +- .../tests/FileStream/ctor_str_fm_fa_fs.cs | 4 +- .../FileStream/ctor_str_fm_fa_fs_buffer.cs | 4 +- .../FileStream/ctor_str_fm_fa_fs_buffer_fo.cs | 4 +- .../tests/System.IO.FileSystem.Tests.csproj | 1 + .../src/System/IO/FileStream.cs | 27 +++- .../src/System/IO/FileStreamOptions.cs | 121 ++++++++++++++- .../src/System/Runtime/GCSettings.cs | 4 +- .../src/System/ThrowHelper.cs | 6 + 12 files changed, 300 insertions(+), 21 deletions(-) create mode 100644 src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs new file mode 100644 index 0000000000000..b35d9bb1ae05d --- /dev/null +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs @@ -0,0 +1,138 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Linq; +using Xunit; + +namespace System.IO.Tests +{ + public class FileStream_FileStreamOptions : FileSystemTest + { + private static IEnumerable WritingModes() + { + yield return FileMode.Create; + yield return FileMode.CreateNew; + yield return FileMode.Append; + yield return FileMode.Truncate; + } + + [Fact] + public void NullOptionsThrows() + { + ArgumentNullException ex = Assert.Throws(() => new FileStream(GetTestFilePath(), options: null)); + Assert.Equal("options", ex.ParamName); + } + + [Fact] + public void Mode() + { + FileMode[] validValues = Enum.GetValues(); + + foreach (var vaidValue in validValues) + { + Assert.Equal(vaidValue, (new FileStreamOptions { Mode = vaidValue }).Mode); + } + + Assert.Throws(() => new FileStreamOptions { Mode = validValues.Min() - 1 }); + Assert.Throws(() => new FileStreamOptions { Mode = validValues.Max() + 1 }); + + var readOnlyOptions = new FileStreamOptions { Access = FileAccess.Read }; + foreach (FileMode writingMode in WritingModes()) + { + Assert.Throws(() => readOnlyOptions.Mode = writingMode); + } + var readWriteOptions = new FileStreamOptions { Access = FileAccess.ReadWrite }; + Assert.Throws(() => readWriteOptions.Mode = FileMode.Append); + } + + [Fact] + public void Access() + { + Assert.Equal(FileAccess.ReadWrite, new FileStreamOptions().Access); + Assert.Equal(FileAccess.Write, new FileStreamOptions() { Mode = FileMode.Append }.Access); + + FileAccess[] validValues = Enum.GetValues(); + + foreach (var vaidValue in validValues) + { + Assert.Equal(vaidValue, (new FileStreamOptions { Access = vaidValue }).Access); + } + + Assert.Throws(() => new FileStreamOptions { Access = validValues.Min() - 1 }); + Assert.Throws(() => new FileStreamOptions { Access = validValues.Max() + 1 }); + + var appendOptions = new FileStreamOptions { Mode = FileMode.Append }; + Assert.Throws(() => appendOptions.Access = FileAccess.Read); + Assert.Throws(() => appendOptions.Access = FileAccess.ReadWrite); + + foreach (FileMode writingMode in WritingModes()) + { + FileStreamOptions writingOptions = new FileStreamOptions { Access = FileAccess.Write, Mode = writingMode }; + Assert.Throws(() => writingOptions.Access = FileAccess.Read); + } + } + + [Fact] + public void Share() + { + Assert.Equal(FileShare.Read, new FileStreamOptions().Share); + + FileShare[] validValues = Enum.GetValues(); + + foreach (var vaidValue in validValues) + { + Assert.Equal(vaidValue, (new FileStreamOptions { Share = vaidValue }).Share); + } + + FileShare all = validValues.Aggregate((x, y) => x | y); + Assert.Equal(all, (new FileStreamOptions { Share = all }).Share); + + Assert.Throws(() => new FileStreamOptions { Share = validValues.Min() - 1 }); + Assert.Throws(() => new FileStreamOptions { Share = all + 1 }); + } + + [Fact] + public void Options() + { + Assert.Equal(FileOptions.None, new FileStreamOptions().Options); + + FileOptions[] validValues = Enum.GetValues(); + + foreach (var option in validValues) + { + Assert.Equal(option, (new FileStreamOptions { Options = option }).Options); + } + + FileOptions all = validValues.Aggregate((x, y) => x | y); + Assert.Equal(all, (new FileStreamOptions { Options = all }).Options); + + Assert.Throws(() => new FileStreamOptions { Options = validValues.Min() - 1 }); + Assert.Throws(() => new FileStreamOptions { Options = all + 1 }); + } + + [Fact] + public void PreallocationSize() + { + Assert.Equal(0, new FileStreamOptions().PreallocationSize); + + Assert.Equal(0, new FileStreamOptions { PreallocationSize = 0 }.PreallocationSize); + Assert.Equal(1, new FileStreamOptions { PreallocationSize = 1 }.PreallocationSize); + Assert.Equal(123, new FileStreamOptions { PreallocationSize = 123 }.PreallocationSize); + + Assert.Throws(() => new FileStreamOptions { PreallocationSize = -1 }); + } + + [Fact] + public void BufferSize() + { + Assert.Equal(4096, new FileStreamOptions().BufferSize); + + Assert.Equal(0, new FileStreamOptions { BufferSize = 0 }.BufferSize); + Assert.Equal(1, new FileStreamOptions { BufferSize = 1 }.BufferSize); + Assert.Equal(123, new FileStreamOptions { BufferSize = 123 }.BufferSize); + + Assert.Throws(() => new FileStreamOptions { BufferSize = -1 }); + } + } +} diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.cs index a202814d7f256..3190e66bdc836 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options_as.cs @@ -9,6 +9,8 @@ public abstract class FileStream_ctor_options_as_base : FileStream_ctor_str_fm_f { protected abstract long PreallocationSize { get; } + protected override string GetExpectedParamName(string paramName) => "value"; + protected override FileStream CreateFileStream(string path, FileMode mode) => new FileStream(path, new FileStreamOptions diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm.cs index f442fbd432219..60e3cd49ea71c 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm.cs @@ -14,6 +14,8 @@ protected virtual FileStream CreateFileStream(string path, FileMode mode) protected virtual long InitialLength => 0; + protected virtual string GetExpectedParamName(string paramName) => paramName; + [Fact] public void NullPathThrows() { @@ -35,7 +37,9 @@ public void DirectoryThrows() [Fact] public void InvalidModeThrows() { - AssertExtensions.Throws("mode", () => CreateFileStream(GetTestFilePath(), ~FileMode.Open)); + AssertExtensions.Throws( + GetExpectedParamName("mode"), + () => CreateFileStream(GetTestFilePath(), ~FileMode.Open)); } [Theory, MemberData(nameof(TrailingCharacters))] diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa.cs index 6ab972644c300..810444ba051a0 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa.cs @@ -21,7 +21,9 @@ protected virtual FileStream CreateFileStream(string path, FileMode mode, FileAc [Fact] public void InvalidAccessThrows() { - AssertExtensions.Throws("access", () => CreateFileStream(GetTestFilePath(), FileMode.Create, ~FileAccess.Read)); + AssertExtensions.Throws( + GetExpectedParamName("access"), + () => CreateFileStream(GetTestFilePath(), FileMode.Create, ~FileAccess.Read)); } [Fact] diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs.cs index 805354e31cd44..386b2992ee3e8 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs.cs @@ -18,7 +18,9 @@ protected virtual FileStream CreateFileStream(string path, FileMode mode, FileAc [Fact] public void InvalidShareThrows() { - AssertExtensions.Throws("share", () => CreateFileStream(GetTestFilePath(), FileMode.Create, FileAccess.ReadWrite, ~FileShare.None)); + AssertExtensions.Throws( + GetExpectedParamName("share"), + () => CreateFileStream(GetTestFilePath(), FileMode.Create, FileAccess.ReadWrite, ~FileShare.None)); } private static readonly FileShare[] s_shares = diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs_buffer.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs_buffer.cs index 621586be7fb7d..20c4832a68a3e 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs_buffer.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs_buffer.cs @@ -22,7 +22,9 @@ protected virtual FileStream CreateFileStream(string path, FileMode mode, FileAc [Fact] public void NegativeBufferSizeThrows() { - AssertExtensions.Throws("bufferSize", () => CreateFileStream(GetTestFilePath(), FileMode.Create, FileAccess.ReadWrite, FileShare.Read, -1)); + AssertExtensions.Throws( + GetExpectedParamName("bufferSize"), + () => CreateFileStream(GetTestFilePath(), FileMode.Create, FileAccess.ReadWrite, FileShare.Read, -1)); } [Fact] diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs_buffer_fo.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs_buffer_fo.cs index 26f3a2fc55d2b..f325fc2ed099d 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs_buffer_fo.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/ctor_str_fm_fa_fs_buffer_fo.cs @@ -21,7 +21,9 @@ protected virtual FileStream CreateFileStream(string path, FileMode mode, FileAc [Fact] public void InvalidFileOptionsThrow() { - AssertExtensions.Throws("options", () => CreateFileStream(GetTestFilePath(), FileMode.Create, FileAccess.ReadWrite, FileShare.Read, c_DefaultBufferSize, ~FileOptions.None)); + AssertExtensions.Throws( + GetExpectedParamName("options"), + () => CreateFileStream(GetTestFilePath(), FileMode.Create, FileAccess.ReadWrite, FileShare.Read, c_DefaultBufferSize, ~FileOptions.None)); } [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] diff --git a/src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj b/src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj index dba6172218ca0..0bc213c3a2ee3 100644 --- a/src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj +++ b/src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj @@ -90,6 +90,7 @@ + diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs index 7cbe9e32bcbd4..c76e7b7e435c9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -144,16 +144,11 @@ public FileStream(string path, FileMode mode, FileAccess access, FileShare share /// /// A relative or absolute path for the file that the current instance will encapsulate. /// An object that describes optional parameters to use. - /// is . + /// or is . /// is an empty string (""), contains only white space, or contains one or more invalid characters. /// -or- /// refers to a non-file device, such as CON:, COM1:, LPT1:, etc. in an NTFS environment. /// refers to a non-file device, such as CON:, COM1:, LPT1:, etc. in a non-NTFS environment. - /// is negative. - /// -or- - /// is negative. - /// -or- - /// , , or contain an invalid value. /// The file cannot be found, such as when is or , and the file specified by does not exist. The file must already exist in these modes. /// An I/O error, such as specifying when the file specified by already exists, occurred. /// -or- @@ -169,8 +164,26 @@ public FileStream(string path, FileMode mode, FileAccess access, FileShare share /// is specified for , but file encryption is not supported on the current platform. /// The specified path, file name, or both exceed the system-defined maximum length. public FileStream(string path, FileStreamOptions options) - : this(path, options.Mode, options.Access, options.Share, options.BufferSize, options.Options, options.PreallocationSize) { + if (path is null) + { + throw new ArgumentNullException(nameof(path), SR.ArgumentNull_Path); + } + else if (path.Length == 0) + { + throw new ArgumentException(SR.Argument_EmptyPath, nameof(path)); + } + else if (options is null) + { + throw new ArgumentNullException(nameof(options)); + } + else if ((options.Access & FileAccess.Write) == FileAccess.Write) + { + SerializationInfo.ThrowIfDeserializationInProgress("AllowFileWrites", ref s_cachedSerializationSwitch); + } + + _strategy = FileStreamHelpers.ChooseStrategy( + this, path, options.Mode, options.Access, options.Share, options.BufferSize, options.Options, options.PreallocationSize); } private FileStream(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options, long preallocationSize) diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamOptions.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamOptions.cs index b2279d4c9256e..2900265448e68 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamOptions.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamOptions.cs @@ -5,32 +5,139 @@ namespace System.IO { public sealed class FileStreamOptions { + private FileMode? _mode; + private FileAccess? _access; + private FileShare _share = FileStream.DefaultShare; + private FileOptions _options; + private long _preallocationSize; + private int _bufferSize = FileStream.DefaultBufferSize; + /// /// One of the enumeration values that determines how to open or create the file. /// - public FileMode Mode { get; set; } + /// When contains an invalid value. + /// When is but is not . + /// -or- + /// is while given mode requires . + /// + public FileMode Mode + { + get => _mode.HasValue ? _mode.GetValueOrDefault() : FileMode.Open; + set + { + if (value < FileMode.CreateNew || value > FileMode.Append) + { + ThrowHelper.ArgumentOutOfRangeException_Enum_Value(); + } + else if (_access.HasValue && (_access & FileAccess.Read) != 0 && value == FileMode.Append) + { + throw new ArgumentException(SR.Argument_InvalidAppendMode, nameof(value)); + } + else if (_access.HasValue && (_access & FileAccess.Write) == 0) + { + if (value == FileMode.Truncate || value == FileMode.CreateNew || value == FileMode.Create || value == FileMode.Append) + { + throw new ArgumentException(SR.Format(SR.Argument_InvalidFileModeAndAccessCombo, value, _access), nameof(value)); + } + } + + _mode = value; + } + } + /// /// A bitwise combination of the enumeration values that determines how the file can be accessed by the object. This also determines the values returned by the and properties of the object. /// - public FileAccess Access { get; set; } = FileAccess.Read; + /// When contains an invalid value. + /// When is but is not . + /// -or- + /// requires while is . + /// + public FileAccess Access + { + get => _access.HasValue ? _access.GetValueOrDefault() : (_mode.HasValue && _mode == FileMode.Append) ? FileAccess.Write : FileAccess.ReadWrite; + set + { + if (value < FileAccess.Read || value > FileAccess.ReadWrite) + { + ThrowHelper.ArgumentOutOfRangeException_Enum_Value(); + } + else if ((value & FileAccess.Read) != 0 && _mode.HasValue && _mode == FileMode.Append) + { + throw new ArgumentException(SR.Argument_InvalidAppendMode, nameof(value)); + } + else if ((value & FileAccess.Write) == 0 && _mode.HasValue) + { + if (_mode == FileMode.Truncate || _mode == FileMode.CreateNew || _mode == FileMode.Create || _mode == FileMode.Append) + { + throw new ArgumentException(SR.Format(SR.Argument_InvalidFileModeAndAccessCombo, _mode, value), nameof(value)); + } + } + + _access = value; + } + } + /// /// A bitwise combination of the enumeration values that determines how the file will be shared by processes. The default value is . /// - public FileShare Share { get; set; } = FileStream.DefaultShare; + /// When contains an invalid value. + public FileShare Share + { + get => _share; + set + { + // don't include inheritable in our bounds check for share + FileShare tempshare = value & ~FileShare.Inheritable; + if (tempshare < FileShare.None || tempshare > (FileShare.ReadWrite | FileShare.Delete)) + { + ThrowHelper.ArgumentOutOfRangeException_Enum_Value(); + } + + _share = value; + } + } + /// /// A bitwise combination of the enumeration values that specifies additional file options. The default value is , which indicates synchronous IO. /// - public FileOptions Options { get; set; } + /// When contains an invalid value. + public FileOptions Options + { + get => _options; + set + { + // NOTE: any change to FileOptions enum needs to be matched here in the error validation + if (value != FileOptions.None && (value & ~(FileOptions.WriteThrough | FileOptions.Asynchronous | FileOptions.RandomAccess | FileOptions.DeleteOnClose | FileOptions.SequentialScan | FileOptions.Encrypted | (FileOptions)0x20000000 /* NoBuffering */)) != 0) + { + ThrowHelper.ArgumentOutOfRangeException_Enum_Value(); + } + + _options = value; + } + } + /// /// The initial allocation size in bytes for the file. A positive value is effective only when a regular file is being created, overwritten, or replaced. - /// When the value is negative, the constructor throws an . + /// Negative values are not allowed. /// In other cases (including the default 0 value), it's ignored. /// - public long PreallocationSize { get; set; } + /// When is negative. + public long PreallocationSize + { + get => _preallocationSize; + set => _preallocationSize = value >= 0 ? value : throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_NeedNonNegNum); + } + /// /// The size of the buffer used by for buffering. The default buffer size is 4096. /// 0 or 1 means that buffering should be disabled. Negative values are not allowed. /// - public int BufferSize { get; set; } = FileStream.DefaultBufferSize; + /// When is negative. + public int BufferSize + { + get => _bufferSize; + set => _bufferSize = value >= 0 ? value : throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_NeedNonNegNum); + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/GCSettings.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/GCSettings.cs index 92ae76559932d..6670c738f8bdd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/GCSettings.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/GCSettings.cs @@ -37,7 +37,7 @@ public static GCLatencyMode LatencyMode if ((value < GCLatencyMode.Batch) || (value > GCLatencyMode.SustainedLowLatency)) { - throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_Enum); + ThrowHelper.ArgumentOutOfRangeException_Enum_Value(); } SetLatencyModeStatus status = SetGCLatencyMode(value); @@ -58,7 +58,7 @@ public static GCLargeObjectHeapCompactionMode LargeObjectHeapCompactionMode if ((value < GCLargeObjectHeapCompactionMode.Default) || (value > GCLargeObjectHeapCompactionMode.CompactOnce)) { - throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_Enum); + ThrowHelper.ArgumentOutOfRangeException_Enum_Value(); } SetLOHCompactionMode(value); diff --git a/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs b/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs index d061bddce9cab..a79210cb523bb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs +++ b/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs @@ -474,6 +474,12 @@ internal static void ThrowArgumentOutOfRangeException_SymbolDoesNotFit() throw new ArgumentOutOfRangeException("symbol", SR.Argument_BadFormatSpecifier); } + [DoesNotReturn] + internal static void ArgumentOutOfRangeException_Enum_Value() + { + throw new ArgumentOutOfRangeException("value", SR.ArgumentOutOfRange_Enum); + } + private static Exception GetArraySegmentCtorValidationFailedException(Array? array, int offset, int count) { if (array == null) From 8f83d178d00ccea29ccc4a526a1eb500631d3b55 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 8 Jun 2021 18:21:06 +0200 Subject: [PATCH 2/4] move the validation of multiple properties back to FileStream ctor --- .../tests/FileStream/FileStreamOptions.cs | 49 +++++++++++-------- .../src/System/IO/FileStream.cs | 11 +++++ .../src/System/IO/FileStreamOptions.cs | 38 ++------------ 3 files changed, 44 insertions(+), 54 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs index b35d9bb1ae05d..7d0f38e739854 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs @@ -24,9 +24,37 @@ public void NullOptionsThrows() Assert.Equal("options", ex.ParamName); } + [Theory] + [InlineData(FileMode.Create)] + [InlineData(FileMode.CreateNew)] + [InlineData(FileMode.Append)] + [InlineData(FileMode.Truncate)] + public void ModesThatRequireWriteAccessThrowWhenReadAccessIsProvided(FileMode fileMode) + { + Assert.Throws(() => new FileStream(GetTestFilePath(), new FileStreamOptions + { + Mode = fileMode, + Access = FileAccess.Read + })); + } + + [Theory] + [InlineData(FileAccess.Read)] + [InlineData(FileAccess.ReadWrite)] + public void AppendWorksOnlyForWriteAccess(FileAccess fileAccess) + { + Assert.Throws(() => new FileStream(GetTestFilePath(), new FileStreamOptions + { + Mode = FileMode.Append, + Access = fileAccess + })); + } + [Fact] public void Mode() { + Assert.Equal(FileMode.Open, new FileStreamOptions().Mode); + FileMode[] validValues = Enum.GetValues(); foreach (var vaidValue in validValues) @@ -36,21 +64,12 @@ public void Mode() Assert.Throws(() => new FileStreamOptions { Mode = validValues.Min() - 1 }); Assert.Throws(() => new FileStreamOptions { Mode = validValues.Max() + 1 }); - - var readOnlyOptions = new FileStreamOptions { Access = FileAccess.Read }; - foreach (FileMode writingMode in WritingModes()) - { - Assert.Throws(() => readOnlyOptions.Mode = writingMode); - } - var readWriteOptions = new FileStreamOptions { Access = FileAccess.ReadWrite }; - Assert.Throws(() => readWriteOptions.Mode = FileMode.Append); } [Fact] public void Access() { - Assert.Equal(FileAccess.ReadWrite, new FileStreamOptions().Access); - Assert.Equal(FileAccess.Write, new FileStreamOptions() { Mode = FileMode.Append }.Access); + Assert.Equal(FileAccess.Read, new FileStreamOptions().Access); FileAccess[] validValues = Enum.GetValues(); @@ -61,16 +80,6 @@ public void Access() Assert.Throws(() => new FileStreamOptions { Access = validValues.Min() - 1 }); Assert.Throws(() => new FileStreamOptions { Access = validValues.Max() + 1 }); - - var appendOptions = new FileStreamOptions { Mode = FileMode.Append }; - Assert.Throws(() => appendOptions.Access = FileAccess.Read); - Assert.Throws(() => appendOptions.Access = FileAccess.ReadWrite); - - foreach (FileMode writingMode in WritingModes()) - { - FileStreamOptions writingOptions = new FileStreamOptions { Access = FileAccess.Write, Mode = writingMode }; - Assert.Throws(() => writingOptions.Access = FileAccess.Read); - } } [Fact] diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs index c76e7b7e435c9..f892c572a0ec5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -177,6 +177,17 @@ public FileStream(string path, FileStreamOptions options) { throw new ArgumentNullException(nameof(options)); } + else if ((options.Access & FileAccess.Read) != 0 && options.Mode == FileMode.Append) + { + throw new ArgumentException(SR.Argument_InvalidAppendMode, nameof(options)); + } + else if ((options.Access & FileAccess.Write) == 0) + { + if (options.Mode == FileMode.Truncate || options.Mode == FileMode.CreateNew || options.Mode == FileMode.Create || options.Mode == FileMode.Append) + { + throw new ArgumentException(SR.Format(SR.Argument_InvalidFileModeAndAccessCombo, options.Mode, options.Access), nameof(options)); + } + } else if ((options.Access & FileAccess.Write) == FileAccess.Write) { SerializationInfo.ThrowIfDeserializationInProgress("AllowFileWrites", ref s_cachedSerializationSwitch); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamOptions.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamOptions.cs index 2900265448e68..491d739de40f5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamOptions.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStreamOptions.cs @@ -5,8 +5,8 @@ namespace System.IO { public sealed class FileStreamOptions { - private FileMode? _mode; - private FileAccess? _access; + private FileMode _mode = FileMode.Open; + private FileAccess _access = FileAccess.Read; private FileShare _share = FileStream.DefaultShare; private FileOptions _options; private long _preallocationSize; @@ -16,30 +16,15 @@ public sealed class FileStreamOptions /// One of the enumeration values that determines how to open or create the file. /// /// When contains an invalid value. - /// When is but is not . - /// -or- - /// is while given mode requires . - /// public FileMode Mode { - get => _mode.HasValue ? _mode.GetValueOrDefault() : FileMode.Open; + get => _mode; set { if (value < FileMode.CreateNew || value > FileMode.Append) { ThrowHelper.ArgumentOutOfRangeException_Enum_Value(); } - else if (_access.HasValue && (_access & FileAccess.Read) != 0 && value == FileMode.Append) - { - throw new ArgumentException(SR.Argument_InvalidAppendMode, nameof(value)); - } - else if (_access.HasValue && (_access & FileAccess.Write) == 0) - { - if (value == FileMode.Truncate || value == FileMode.CreateNew || value == FileMode.Create || value == FileMode.Append) - { - throw new ArgumentException(SR.Format(SR.Argument_InvalidFileModeAndAccessCombo, value, _access), nameof(value)); - } - } _mode = value; } @@ -49,30 +34,15 @@ public FileMode Mode /// A bitwise combination of the enumeration values that determines how the file can be accessed by the object. This also determines the values returned by the and properties of the object. /// /// When contains an invalid value. - /// When is but is not . - /// -or- - /// requires while is . - /// public FileAccess Access { - get => _access.HasValue ? _access.GetValueOrDefault() : (_mode.HasValue && _mode == FileMode.Append) ? FileAccess.Write : FileAccess.ReadWrite; + get => _access; set { if (value < FileAccess.Read || value > FileAccess.ReadWrite) { ThrowHelper.ArgumentOutOfRangeException_Enum_Value(); } - else if ((value & FileAccess.Read) != 0 && _mode.HasValue && _mode == FileMode.Append) - { - throw new ArgumentException(SR.Argument_InvalidAppendMode, nameof(value)); - } - else if ((value & FileAccess.Write) == 0 && _mode.HasValue) - { - if (_mode == FileMode.Truncate || _mode == FileMode.CreateNew || _mode == FileMode.Create || _mode == FileMode.Append) - { - throw new ArgumentException(SR.Format(SR.Argument_InvalidFileModeAndAccessCombo, _mode, value), nameof(value)); - } - } _access = value; } From e3a3ac7b04cc2c37f96082f914842947c583cc7b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 8 Jun 2021 18:55:14 +0200 Subject: [PATCH 3/4] remove unused method --- .../tests/FileStream/FileStreamOptions.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs index 7d0f38e739854..d30a9dd622e9c 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs @@ -9,14 +9,6 @@ namespace System.IO.Tests { public class FileStream_FileStreamOptions : FileSystemTest { - private static IEnumerable WritingModes() - { - yield return FileMode.Create; - yield return FileMode.CreateNew; - yield return FileMode.Append; - yield return FileMode.Truncate; - } - [Fact] public void NullOptionsThrows() { From 356e3900667a51e0742dffdb66a4e5d537c56eea Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 9 Jun 2021 08:42:07 +0200 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Stephen Toub --- .../System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs index d30a9dd622e9c..d078deb07e55e 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs @@ -12,8 +12,7 @@ public class FileStream_FileStreamOptions : FileSystemTest [Fact] public void NullOptionsThrows() { - ArgumentNullException ex = Assert.Throws(() => new FileStream(GetTestFilePath(), options: null)); - Assert.Equal("options", ex.ParamName); + AssertExtensions.Throws("options", () => new FileStream(GetTestFilePath(), options: null)); } [Theory]