From 9eb14f3c51c3d25a86856949e4f1753284adc213 Mon Sep 17 00:00:00 2001 From: Haacked Date: Wed, 26 Feb 2014 18:38:57 -0800 Subject: [PATCH] Make readonly collections truly readonly These methods actually returned mutable collections that happen to implement the readonly interfaces. User could cast them to the actual type and add things. I'd rather avoid even that possibility by making these truly return readonly stuff. --- Octokit.Tests/Models/PunchCardTests.cs | 32 +++++++++++++------ .../Models/SearchCodeRequestTests.cs | 20 ++++++++++++ .../Models/SearchIssuesRequestTests.cs | 20 ++++++++++++ .../Models/SearchUsersRequestTests.cs | 20 ++++++++++++ Octokit.Tests/Octokit.Tests.csproj | 3 ++ Octokit/Models/Request/SearchCodeRequest.cs | 3 +- Octokit/Models/Request/SearchIssuesRequest.cs | 5 +-- Octokit/Models/Response/PunchCard.cs | 4 ++- 8 files changed, 94 insertions(+), 13 deletions(-) create mode 100644 Octokit.Tests/Models/SearchCodeRequestTests.cs create mode 100644 Octokit.Tests/Models/SearchIssuesRequestTests.cs create mode 100644 Octokit.Tests/Models/SearchUsersRequestTests.cs diff --git a/Octokit.Tests/Models/PunchCardTests.cs b/Octokit.Tests/Models/PunchCardTests.cs index c46baf0e..05f6f19e 100644 --- a/Octokit.Tests/Models/PunchCardTests.cs +++ b/Octokit.Tests/Models/PunchCardTests.cs @@ -1,30 +1,31 @@ using System; using System.Collections.Generic; +using System.Collections.ObjectModel; using Octokit.Response; using Xunit; -namespace Octokit.Tests.Models +public class PunchCardTests { - public class PunchCardTests + public class TheConstructor { [Fact] public void ThrowsExceptionWithNullPunchCardPoints() { - Assert.Throws(()=>new PunchCard(null)); + Assert.Throws(() => new PunchCard(null)); } [Fact] public void ThrowsExceptionWhenPunchCardPointsHaveIncorrectFormat() { - IList point1 = new []{1,2,3,4,5,6}; - IEnumerable> points = new List>{point1}; + IList point1 = new[] { 1, 2, 3, 4, 5, 6 }; + IEnumerable> points = new List> { point1 }; Assert.Throws(() => new PunchCard(points)); } [Fact] public void DoesNotThrowExceptionWhenPunchPointsHaveCorrectFormat() { - IList point1 = new[] { 1, 2, 3}; + IList point1 = new[] { 1, 2, 3 }; IEnumerable> points = new List> { point1 }; Assert.DoesNotThrow(() => new PunchCard(points)); } @@ -35,7 +36,7 @@ namespace Octokit.Tests.Models IList point1 = new[] { 1, 0, 3 }; IList point2 = new[] { 1, 1, 4 }; IList point3 = new[] { 1, 2, 0 }; - IEnumerable> points = new List> { point1,point2,point3 }; + IEnumerable> points = new List> { point1, point2, point3 }; var punchCard = new PunchCard(points); @@ -44,10 +45,23 @@ namespace Octokit.Tests.Models 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 point1 = new[] { 1, 0, 3 }; + IList point2 = new[] { 1, 1, 4 }; + IList point3 = new[] { 1, 2, 0 }; + IEnumerable> points = new List> { point1, point2, point3 }; + + var punchCard = new PunchCard(points); + + Assert.Null(punchCard.PunchPoints as ICollection); + } } -} +} \ No newline at end of file diff --git a/Octokit.Tests/Models/SearchCodeRequestTests.cs b/Octokit.Tests/Models/SearchCodeRequestTests.cs new file mode 100644 index 00000000..7035c6e4 --- /dev/null +++ b/Octokit.Tests/Models/SearchCodeRequestTests.cs @@ -0,0 +1,20 @@ +using System.Collections.Generic; +using Octokit; +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. + Assert.Null(result as ICollection); + } + } +} diff --git a/Octokit.Tests/Models/SearchIssuesRequestTests.cs b/Octokit.Tests/Models/SearchIssuesRequestTests.cs new file mode 100644 index 00000000..b26d343c --- /dev/null +++ b/Octokit.Tests/Models/SearchIssuesRequestTests.cs @@ -0,0 +1,20 @@ +using System.Collections.Generic; +using Octokit; +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. + Assert.Null(result as ICollection); + } + } +} diff --git a/Octokit.Tests/Models/SearchUsersRequestTests.cs b/Octokit.Tests/Models/SearchUsersRequestTests.cs new file mode 100644 index 00000000..7a6a3a39 --- /dev/null +++ b/Octokit.Tests/Models/SearchUsersRequestTests.cs @@ -0,0 +1,20 @@ +using System.Collections.Generic; +using Octokit; +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. + Assert.Null(result as ICollection); + } + } +} diff --git a/Octokit.Tests/Octokit.Tests.csproj b/Octokit.Tests/Octokit.Tests.csproj index d60624e7..a2dd679f 100644 --- a/Octokit.Tests/Octokit.Tests.csproj +++ b/Octokit.Tests/Octokit.Tests.csproj @@ -134,6 +134,9 @@ + + + diff --git a/Octokit/Models/Request/SearchCodeRequest.cs b/Octokit/Models/Request/SearchCodeRequest.cs index 4a55d60e..baacdc58 100644 --- a/Octokit/Models/Request/SearchCodeRequest.cs +++ b/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 @@ namespace Octokit parameters.Add(String.Format(CultureInfo.InvariantCulture, "repo:{0}", Repo)); } - return parameters; + return new ReadOnlyCollection(parameters); } internal string DebuggerDisplay diff --git a/Octokit/Models/Request/SearchIssuesRequest.cs b/Octokit/Models/Request/SearchIssuesRequest.cs index 2bcdaace..37e1ec9e 100644 --- a/Octokit/Models/Request/SearchIssuesRequest.cs +++ b/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 @@ namespace Octokit parameters.Add(String.Format(CultureInfo.InvariantCulture, "repo:{0}", Repo)); } - return parameters; + return new ReadOnlyCollection(parameters); } internal string DebuggerDisplay diff --git a/Octokit/Models/Response/PunchCard.cs b/Octokit/Models/Response/PunchCard.cs index ef2a7e9d..7180478d 100644 --- a/Octokit/Models/Response/PunchCard.cs +++ b/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 @@ namespace Octokit.Response public PunchCard(IEnumerable> punchCardData) { Ensure.ArgumentNotNull(punchCardData, "punchCardData"); - PunchPoints = punchCardData.Select(point => new PunchCardPoint(point)).ToList(); + PunchPoints = new ReadOnlyCollection( + punchCardData.Select(point => new PunchCardPoint(point)).ToList()); } ///