Merge pull request #394 from octokit/haacked/proper-readonly-collections

Make readonly collections truly readonly

Octokit.Tests/Helpers/AssertEx.cs

@@ -58,5 +58,12 @@ public static async Task ThrowsWhenGivenWhitespaceArgument(Func<string, Task> ac
                 await Throws<ArgumentException>(async () => await action(argument));
             }
         }
+
+        public static void IsReadOnlyCollection<T>(object instance) where T : class
+        {
+            var collection = instance as ICollection<T>;
+            // The collection == null case is for .NET 4.0
+            Assert.True(instance is IReadOnlyCollection<T> && (collection == null || collection.IsReadOnly));
+        }
     }
 }

Octokit.Tests/Models/PunchCardTests.cs

@@ -1,30 +1,32 @@
 using System;
 using System.Collections.Generic;
+using System.Collections.ObjectModel;
 using Octokit.Response;
+using Octokit.Tests.Helpers;
 using Xunit;
 
-namespace Octokit.Tests.Models
+public class PunchCardTests
 {
-    public class PunchCardTests
+    public class TheConstructor
     {
         [Fact]
         public void ThrowsExceptionWithNullPunchCardPoints()
         {
-            Assert.Throws<ArgumentNullException>(()=>new PunchCard(null));
+            Assert.Throws<ArgumentNullException>(() => new PunchCard(null));
         }
 
         [Fact]
         public void ThrowsExceptionWhenPunchCardPointsHaveIncorrectFormat()
         {
-            IList<int> point1 = new []{1,2,3,4,5,6};
-            IEnumerable<IList<int>> points = new List<IList<int>>{point1};
+            IList<int> point1 = new[] { 1, 2, 3, 4, 5, 6 };
+            IEnumerable<IList<int>> points = new List<IList<int>> { point1 };
             Assert.Throws<ArgumentException>(() => new PunchCard(points));
         }
 
         [Fact]
         public void DoesNotThrowExceptionWhenPunchPointsHaveCorrectFormat()
         {
-            IList<int> point1 = new[] { 1, 2, 3};
+            IList<int> point1 = new[] { 1, 2, 3 };
             IEnumerable<IList<int>> points = new List<IList<int>> { point1 };
             Assert.DoesNotThrow(() => new PunchCard(points));
         }
@@ -35,7 +37,7 @@ public void CanQueryCommitsForDayAndHour()
             IList<int> point1 = new[] { 1, 0, 3 };
             IList<int> point2 = new[] { 1, 1, 4 };
             IList<int> point3 = new[] { 1, 2, 0 };
-            IEnumerable<IList<int>> points = new List<IList<int>> { point1,point2,point3 };
+            IEnumerable<IList<int>> points = new List<IList<int>> { point1, point2, point3 };
 
             var punchCard = new PunchCard(points);
 
@@ -44,10 +46,23 @@ public void CanQueryCommitsForDayAndHour()
             var commitsAtMondayAt2Am = punchCard.GetCommitCountFor(DayOfWeek.Monday, 2);
             var commitsAtTuesdayAt2Am = punchCard.GetCommitCountFor(DayOfWeek.Tuesday, 2);
 
-            Assert.Equal(3,commitsAtMondayAt12Am);
+            Assert.Equal(3, commitsAtMondayAt12Am);
             Assert.Equal(4, commitsAtMondayAt1Am);
             Assert.Equal(0, commitsAtMondayAt2Am);
             Assert.Equal(0, commitsAtTuesdayAt2Am);
         }
+
+        [Fact]
+        public void SetsPunchPointsAsReadOnlyDictionary()
+        {
+            IList<int> point1 = new[] { 1, 0, 3 };
+            IList<int> point2 = new[] { 1, 1, 4 };
+            IList<int> point3 = new[] { 1, 2, 0 };
+            IEnumerable<IList<int>> points = new List<IList<int>> { point1, point2, point3 };
+
+            var punchCard = new PunchCard(points);
+
+            AssertEx.IsReadOnlyCollection<PunchCardPoint>(punchCard.PunchPoints);
+        }
     }
-}
+}

Octokit.Tests/Models/SearchCodeRequestTests.cs

@@ -0,0 +1,20 @@
+using Octokit;
+using Octokit.Tests.Helpers;
+using Xunit;
+
+public class SearchCodeRequestTests
+{
+    public class TheMergedQualifiersMethod
+    {
+        [Fact]
+        public void ReturnsAReadOnlyDictionary()
+        {
+            var request = new SearchCodeRequest("test");
+
+            var result = request.MergedQualifiers();
+
+            // If I can cast this to a writeable collection, then that defeats the purpose of a read only.
+            AssertEx.IsReadOnlyCollection<string>(result);
+        }
+    }
+}

Octokit.Tests/Models/SearchIssuesRequestTests.cs

@@ -0,0 +1,20 @@
+using Octokit;
+using Octokit.Tests.Helpers;
+using Xunit;
+
+internal class SearchIssuesRequestTests
+{
+    public class TheMergedQualifiersMethod
+    {
+        [Fact]
+        public void ReturnsAReadOnlyDictionary()
+        {
+            var request = new SearchIssuesRequest("test");
+
+            var result = request.MergedQualifiers();
+
+            // If I can cast this to a writeable collection, then that defeats the purpose of a read only.
+            AssertEx.IsReadOnlyCollection<string>(result);
+        }
+    }
+}

Octokit.Tests/Models/SearchUsersRequestTests.cs

@@ -0,0 +1,20 @@
+using Octokit;
+using Octokit.Tests.Helpers;
+using Xunit;
+
+internal class SearchUsersRequestTests
+{
+    public class TheMergedQualifiersMethod
+    {
+        [Fact]
+        public void ReturnsAReadOnlyDictionary()
+        {
+            var request = new SearchUsersRequest("test");
+
+            var result = request.MergedQualifiers();
+
+            // If I can cast this to a writeable collection, then that defeats the purpose of a read only.
+            AssertEx.IsReadOnlyCollection<string>(result);
+        }
+    }
+}

Octokit.Tests/Octokit.Tests.csproj

@@ -134,6 +134,9 @@
     <Compile Include="Models\ReadOnlyPagedCollectionTests.cs" />
     <Compile Include="Models\RepositoryUpdateTests.cs" />
     <Compile Include="Models\RequestParametersTests.cs" />
+    <Compile Include="Models\SearchCodeRequestTests.cs" />
+    <Compile Include="Models\SearchUsersRequestTests.cs" />
+    <Compile Include="Models\SearchIssuesRequestTests.cs" />
     <Compile Include="Properties\AssemblyInfo.cs" />
     <Compile Include="Helpers\StringExtensionsTests.cs" />
     <Compile Include="Clients\RepositoriesClientTests.cs" />

Octokit/Models/Request/SearchCodeRequest.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Collections.ObjectModel;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
@@ -157,7 +158,7 @@ public override IReadOnlyCollection<string> MergedQualifiers()
                 parameters.Add(String.Format(CultureInfo.InvariantCulture, "repo:{0}", Repo));
             }
 
-            return parameters;
+            return new ReadOnlyCollection<string>(parameters);
         }
 
         internal string DebuggerDisplay

Octokit/Models/Request/SearchIssuesRequest.cs

@@ -1,4 +1,5 @@
-using System.Diagnostics;
+using System.Collections.ObjectModel;
+using System.Diagnostics;
 using Octokit.Internal;
 using System;
 using System.Collections.Generic;
@@ -259,7 +260,7 @@ public override IReadOnlyCollection<string> MergedQualifiers()
                 parameters.Add(String.Format(CultureInfo.InvariantCulture, "repo:{0}", Repo));
             }
 
-            return parameters;
+            return new ReadOnlyCollection<string>(parameters);
         }
 
         internal string DebuggerDisplay

Octokit/Models/Response/PunchCard.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Collections.ObjectModel;
 using System.Diagnostics;
 using System.Globalization;
 using System.Linq;
@@ -12,7 +13,8 @@ public class PunchCard
         public PunchCard(IEnumerable<IList<int>> punchCardData)
         {
             Ensure.ArgumentNotNull(punchCardData, "punchCardData");
-            PunchPoints = punchCardData.Select(point => new PunchCardPoint(point)).ToList();
+            PunchPoints = new ReadOnlyCollection<PunchCardPoint>(
+                punchCardData.Select(point => new PunchCardPoint(point)).ToList());
         }
 
         /// <summary>