From fc5bc947aa4d190ce1c00e23e0edca65d981586e Mon Sep 17 00:00:00 2001 From: Chad Tolkien Date: Sat, 22 Jul 2017 13:09:33 +1000 Subject: [PATCH] Fixes #1621 NewRepositoryWebHook no longer creates new instance... (#1623) Fixes #1621 NewRepositoryWebHook should not discard existing fields --- .../Models/NewRepositoryWebHookTests.cs | 97 ++++++++++++++++--- .../RepositoryWebHookConfigException.cs | 62 ------------ Octokit/Models/Request/NewRepositoryHook.cs | 2 +- .../Models/Request/NewRepositoryWebHook.cs | 27 ++---- 4 files changed, 94 insertions(+), 94 deletions(-) delete mode 100644 Octokit/Exceptions/RepositoryWebHookConfigException.cs diff --git a/Octokit.Tests/Models/NewRepositoryWebHookTests.cs b/Octokit.Tests/Models/NewRepositoryWebHookTests.cs index ca95ec70..287e8aa4 100644 --- a/Octokit.Tests/Models/NewRepositoryWebHookTests.cs +++ b/Octokit.Tests/Models/NewRepositoryWebHookTests.cs @@ -7,9 +7,6 @@ namespace Octokit.Tests.Models { public class TheCtor { - string ExpectedRepositoryWebHookConfigExceptionMessage = - "Duplicate webhook config values found - these values: Url should not be passed in as part of the config values. Use the properties on the NewRepositoryWebHook class instead."; - [Fact] public void UsesDefaultValuesForDefaultConfig() { @@ -78,7 +75,67 @@ namespace Octokit.Tests.Models } [Fact] - public void ShouldThrowRepositoryWebHookConfigExceptionWhenDuplicateKeysExists() + public void CanSetHookAsActive() + { + var config = new Dictionary + { + {"hostname", "http://hostname.url"}, + {"username", "username"}, + {"password", "password"} + }; + + var create = new NewRepositoryWebHook("web", config, "http://test.com/example") + { + Active = true + }; + + var request = create.ToRequest(); + + Assert.True(request.Active); + } + + [Fact] + public void CanSetHookEvents() + { + var create = new NewRepositoryWebHook("web", new Dictionary(), "http://test.com/example") + { + Events = new List { "*" } + }; + + var request = create.ToRequest(); + + Assert.Contains("*", request.Events); + } + + [Fact] + public void EnsureCanCallToRequestMultipleTimes() + { + var create = new NewRepositoryWebHook("web", new Dictionary(), "http://test.com/example") + { + Events = new List { "*" } + }; + + var request = create.ToRequest(); + var requestRepeated = create.ToRequest(); + + Assert.Contains("*", request.Events); + Assert.Contains("*", requestRepeated.Events); + } + + [Fact] + public void ShouldNotContainDuplicateConfigEntriesOnSubsequentRequests() + { + var create = new NewRepositoryWebHook("web", new Dictionary(), "http://test.com/example"); + + var request = create.ToRequest(); + var requestRepeated = create.ToRequest(); + + Assert.Equal(request.Config.Count, 4); + Assert.Equal(requestRepeated.Config.Count, 4); + } + + [Fact] + public void ShouldNotContainDuplicateConfigEntriesOnSubsequentRequestsWithCustomisedConfig() { var config = new Dictionary { @@ -88,20 +145,32 @@ namespace Octokit.Tests.Models {"password", "password"} }; - var create = new NewRepositoryWebHook("windowsazure", config, "http://test.com/example") + var create = new NewRepositoryWebHook("web", config, "http://test.com/example"); + + var request = create.ToRequest(); + var requestRepeated = create.ToRequest(); + + //This is not 8, because `url` used in config, is already part of the base config + Assert.Equal(request.Config.Count, 7); + Assert.Equal(requestRepeated.Config.Count, 7); + } + + [Fact] + public void PropertiesShouldTakePrecedenceOverConfigPassedIn() + { + var config = new Dictionary { - ContentType = WebHookContentType.Json, - Secret = string.Empty, - InsecureSsl = true + {"url", "http://originalurl.com/test"}, }; - Assert.Equal(create.Url, "http://test.com/example"); - Assert.Equal(create.ContentType, WebHookContentType.Json); - Assert.Empty(create.Secret); - Assert.True(create.InsecureSsl); + var create = new NewRepositoryWebHook("web", config, "http://test.com/example"); - var ex = Assert.Throws(() => create.ToRequest()); - Assert.Equal(ExpectedRepositoryWebHookConfigExceptionMessage, ex.Message); + var request = create.ToRequest(); + + Assert.Equal(request.Config["url"], "http://test.com/example"); + + var subsequentRequest = create.ToRequest(); + Assert.Equal(subsequentRequest.Config["url"], "http://test.com/example"); } } } diff --git a/Octokit/Exceptions/RepositoryWebHookConfigException.cs b/Octokit/Exceptions/RepositoryWebHookConfigException.cs deleted file mode 100644 index c1be7e46..00000000 --- a/Octokit/Exceptions/RepositoryWebHookConfigException.cs +++ /dev/null @@ -1,62 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; -using System.Globalization; -using System.Linq; -#if !NO_SERIALIZABLE -using System.Runtime.Serialization; -#endif -using System.Security; - -namespace Octokit -{ -#if !NO_SERIALIZABLE - [Serializable] -#endif - [SuppressMessage("Microsoft.Design", "CA1032:ImplementStandardExceptionConstructors", - Justification = "These exceptions are specific to the GitHub API and not general purpose exceptions")] - public class RepositoryWebHookConfigException : Exception - { - readonly string message; - - public RepositoryWebHookConfigException(IEnumerable invalidConfig) - { - var parameterList = string.Join(", ", invalidConfig.Select(ic => ic.FromRubyCase())); - message = string.Format(CultureInfo.InvariantCulture, - "Duplicate webhook config values found - these values: {0} should not be passed in as part of the config values. Use the properties on the NewRepositoryWebHook class instead.", - parameterList); - } - - public override string Message - { - get { return message; } - } - -#if !NO_SERIALIZABLE - /// - /// Constructs an instance of RepositoryWebHookConfigException - /// - /// - /// The that holds the - /// serialized object data about the exception being thrown. - /// - /// - /// The that contains - /// contextual information about the source or destination. - /// - protected RepositoryWebHookConfigException(SerializationInfo info, StreamingContext context) - : base(info, context) - { - if (info == null) return; - message = info.GetString("Message"); - } - - [SecurityCritical] - public override void GetObjectData(SerializationInfo info, StreamingContext context) - { - base.GetObjectData(info, context); - info.AddValue("Message", Message); - } -#endif - } -} diff --git a/Octokit/Models/Request/NewRepositoryHook.cs b/Octokit/Models/Request/NewRepositoryHook.cs index 43c032bf..35f5e861 100644 --- a/Octokit/Models/Request/NewRepositoryHook.cs +++ b/Octokit/Models/Request/NewRepositoryHook.cs @@ -81,7 +81,7 @@ namespace Octokit /// /// The configuration. /// - public IReadOnlyDictionary Config { get; private set; } + public IReadOnlyDictionary Config { get; protected set; } /// /// Determines what events the hook is triggered for. Default: ["push"] diff --git a/Octokit/Models/Request/NewRepositoryWebHook.cs b/Octokit/Models/Request/NewRepositoryWebHook.cs index ceae9c6d..14d009cc 100644 --- a/Octokit/Models/Request/NewRepositoryWebHook.cs +++ b/Octokit/Models/Request/NewRepositoryWebHook.cs @@ -32,8 +32,8 @@ namespace Octokit /// /// insecure_ssl: /// - /// An optional string that determines whether the SSL certificate of the host for url will be verified when - /// delivering payloads. Supported values include "0" (verification is performed) and "1" (verification is not + /// An optional string that determines whether the SSL certificate of the host for url will be verified when + /// delivering payloads. Supported values include "0" (verification is performed) and "1" (verification is not /// performed). The default is "0". /// /// @@ -41,16 +41,16 @@ namespace Octokit /// /// API: https://developer.github.com/v3/repos/hooks/#create-a-hook /// - /// + /// [DebuggerDisplay("{DebuggerDisplay,nq}")] public class NewRepositoryWebHook : NewRepositoryHook { /// - /// Initializes a new instance of the class. - /// Using default values for ContentType, Secret and InsecureSsl. + /// Initializes a new instance of the class. + /// Using default values for ContentType, Secret and InsecureSsl. /// /// - /// Use "web" for a webhook or use the name of a valid service. (See + /// Use "web" for a webhook or use the name of a valid service. (See /// https://api.github.com/hooks for the list of valid service /// names.) /// @@ -99,29 +99,22 @@ namespace Octokit public string Secret { get; set; } /// - /// Gets whether the SSL certificate of the host will be verified when + /// Gets whether the SSL certificate of the host will be verified when /// delivering payloads. The default is `false`. /// /// - /// true if SSL certificate verification is not performed; + /// true if SSL certificate verification is not performed; /// otherwise, false. /// public bool InsecureSsl { get; set; } public override NewRepositoryHook ToRequest() { - var webHookConfig = GetWebHookConfig(); - if (Config.Any(c => webHookConfig.ContainsKey(c.Key))) - { - var invalidConfigs = Config.Where(c => webHookConfig.ContainsKey(c.Key)).Select(c => c.Key); - throw new RepositoryWebHookConfigException(invalidConfigs); - } - - var config = webHookConfig + Config = GetWebHookConfig() .Union(Config, new WebHookConfigComparer()) .ToDictionary(k => k.Key, v => v.Value); - return new NewRepositoryHook(Name, config); + return this; } Dictionary GetWebHookConfig()