From a0ce44c9fc0fbfb578945f2f0f037270b4614e75 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Sat, 31 Oct 2020 22:49:12 +0100 Subject: [PATCH 1/4] Changed configuration of first run time for health check notifier from a time string to a cron expression. --- .../Validation/ConfigurationValidatorBase.cs | 6 +-- .../HealthChecksSettingsValidator.cs | 2 +- src/Umbraco.Core/DateTimeExtensions.cs | 37 --------------- src/Umbraco.Core/StringExtensions.cs | 15 ++++-- .../HealthCheckSettingsExtensions.cs | 12 +++-- .../HostedServices/HealthCheckNotifier.cs | 2 +- .../Umbraco.Infrastructure.csproj | 1 + .../HealthCheckSettingsExtensionsTests.cs | 22 +++++---- .../Umbraco.Core/DateTimeExtensionsTests.cs | 46 ------------------- .../StringExtensionsTests.cs | 11 ++--- 10 files changed, 42 insertions(+), 112 deletions(-) rename src/{Umbraco.Core/Configuration/Models => Umbraco.Infrastructure/Configuration}/Extensions/HealthCheckSettingsExtensions.cs (54%) delete mode 100644 src/Umbraco.Tests.UnitTests/Umbraco.Core/DateTimeExtensionsTests.cs diff --git a/src/Umbraco.Core/Configuration/Models/Validation/ConfigurationValidatorBase.cs b/src/Umbraco.Core/Configuration/Models/Validation/ConfigurationValidatorBase.cs index 9b7f02a60c..343ba5656b 100644 --- a/src/Umbraco.Core/Configuration/Models/Validation/ConfigurationValidatorBase.cs +++ b/src/Umbraco.Core/Configuration/Models/Validation/ConfigurationValidatorBase.cs @@ -42,11 +42,11 @@ namespace Umbraco.Core.Configuration.Models.Validation return true; } - public bool ValidateOptionalTime(string configPath, string value, out string message) + public bool ValidateOptionalCronTab(string configPath, string value, out string message) { - if (!string.IsNullOrEmpty(value) && !value.IsValidTimeSpan()) + if (!string.IsNullOrEmpty(value) && !value.IsValidCronTab()) { - message = $"Configuration entry {configPath} contains an invalid time value."; + message = $"Configuration entry {configPath} contains an invalid cron expression."; return false; } diff --git a/src/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidator.cs b/src/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidator.cs index fe6a4d056b..9ecb6fb118 100644 --- a/src/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidator.cs +++ b/src/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidator.cs @@ -16,7 +16,7 @@ namespace Umbraco.Core.Configuration.Models.Validation private bool ValidateNotificationFirstRunTime(string value, out string message) { - return ValidateOptionalTime($"{Constants.Configuration.ConfigHealthChecks}:{nameof(HealthChecksSettings.Notification)}:{nameof(HealthChecksSettings.Notification.FirstRunTime)}", value, out message); + return ValidateOptionalCronTab($"{Constants.Configuration.ConfigHealthChecks}:{nameof(HealthChecksSettings.Notification)}:{nameof(HealthChecksSettings.Notification.FirstRunTime)}", value, out message); } } } diff --git a/src/Umbraco.Core/DateTimeExtensions.cs b/src/Umbraco.Core/DateTimeExtensions.cs index 1920205b69..378d06a637 100644 --- a/src/Umbraco.Core/DateTimeExtensions.cs +++ b/src/Umbraco.Core/DateTimeExtensions.cs @@ -43,42 +43,5 @@ namespace Umbraco.Core Minute, Second } - - /// - /// Calculates the number of minutes from a date time, on a rolling daily basis (so if - /// date time is before the time, calculate onto next day). - /// - /// Date to start from - /// Time to compare against (in Hmm form, e.g. 330, 2200) - /// - public static int PeriodicMinutesFrom(this DateTime fromDateTime, string scheduledTime) - { - // Ensure time provided is 4 digits long - if (scheduledTime.Length == 3) - { - scheduledTime = "0" + scheduledTime; - } - - var scheduledHour = int.Parse(scheduledTime.Substring(0, 2)); - var scheduledMinute = int.Parse(scheduledTime.Substring(2)); - - DateTime scheduledDateTime; - if (IsScheduledInRemainingDay(fromDateTime, scheduledHour, scheduledMinute)) - { - scheduledDateTime = new DateTime(fromDateTime.Year, fromDateTime.Month, fromDateTime.Day, scheduledHour, scheduledMinute, 0); - } - else - { - var nextDay = fromDateTime.AddDays(1); - scheduledDateTime = new DateTime(nextDay.Year, nextDay.Month, nextDay.Day, scheduledHour, scheduledMinute, 0); - } - - return (int)(scheduledDateTime - fromDateTime).TotalMinutes; - } - - private static bool IsScheduledInRemainingDay(DateTime fromDateTime, int scheduledHour, int scheduledMinute) - { - return scheduledHour > fromDateTime.Hour || (scheduledHour == fromDateTime.Hour && scheduledMinute >= fromDateTime.Minute); - } } } diff --git a/src/Umbraco.Core/StringExtensions.cs b/src/Umbraco.Core/StringExtensions.cs index 93d664a2bd..0cf8158518 100644 --- a/src/Umbraco.Core/StringExtensions.cs +++ b/src/Umbraco.Core/StringExtensions.cs @@ -1480,18 +1480,23 @@ namespace Umbraco.Core } /// - /// Validates a string matches a time stamp. + /// Validates a string matches a cron tab (for length only). /// - /// String with timespan representation (in standard timespan format: https://docs.microsoft.com/en-us/dotnet/standard/base-types/standard-timespan-format-strings) - /// - public static bool IsValidTimeSpan(this string input) + /// String with timespan representation (in cron tab format: https://github.com/atifaziz/NCrontab/wiki/Crontab-Expression) + /// True if string matches a valid cron expression, false if not. + /// + /// Considering an expression as valid if it's supported by https://github.com/atifaziz/NCrontab/wiki/Crontab-Expression, + /// so only 5 or 6 values are expected. + /// + public static bool IsValidCronTab(this string input) { if (string.IsNullOrEmpty(input)) { return false; } - return TimeSpan.TryParse(input, out var _); + var parts = input.Split(new string[] { " " }, StringSplitOptions.RemoveEmptyEntries); + return parts.Length == 5 || parts.Length == 6; } } } diff --git a/src/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensions.cs b/src/Umbraco.Infrastructure/Configuration/Extensions/HealthCheckSettingsExtensions.cs similarity index 54% rename from src/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensions.cs rename to src/Umbraco.Infrastructure/Configuration/Extensions/HealthCheckSettingsExtensions.cs index 3fa37a0b19..57ae58928d 100644 --- a/src/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensions.cs +++ b/src/Umbraco.Infrastructure/Configuration/Extensions/HealthCheckSettingsExtensions.cs @@ -1,6 +1,8 @@ using System; +using NCrontab; +using Umbraco.Core.Configuration.Models; -namespace Umbraco.Core.Configuration.Models.Extensions +namespace Umbraco.Infrastructure.Configuration.Extensions { public static class HealthCheckSettingsExtensions { @@ -14,9 +16,11 @@ namespace Umbraco.Core.Configuration.Models.Extensions } else { - // Otherwise start at scheduled time. - var delay = TimeSpan.FromMinutes(now.PeriodicMinutesFrom(firstRunTime)); - return (delay < defaultDelay) + // Otherwise start at scheduled time according to cron expression, unless within the default delay period. + var firstRunTimeCronExpression = CrontabSchedule.Parse(firstRunTime); + var firstRunOccurance = firstRunTimeCronExpression.GetNextOccurrence(now); + var delay = firstRunOccurance - now; + return delay < defaultDelay ? defaultDelay : delay; } diff --git a/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs b/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs index af0a0f335c..f4cf33fa61 100644 --- a/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs +++ b/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs @@ -4,11 +4,11 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Umbraco.Core; using Umbraco.Core.Configuration.Models; -using Umbraco.Core.Configuration.Models.Extensions; using Umbraco.Core.HealthCheck; using Umbraco.Core.Logging; using Umbraco.Core.Scoping; using Umbraco.Core.Sync; +using Umbraco.Infrastructure.Configuration.Extensions; using Umbraco.Infrastructure.HealthCheck; using Umbraco.Web.HealthCheck; diff --git a/src/Umbraco.Infrastructure/Umbraco.Infrastructure.csproj b/src/Umbraco.Infrastructure/Umbraco.Infrastructure.csproj index 2343ea806a..2f681efde3 100644 --- a/src/Umbraco.Infrastructure/Umbraco.Infrastructure.csproj +++ b/src/Umbraco.Infrastructure/Umbraco.Infrastructure.csproj @@ -20,6 +20,7 @@ + diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensionsTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensionsTests.cs index 3ec9af0b5a..d1819fd83e 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensionsTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensionsTests.cs @@ -1,26 +1,30 @@ using System; using NUnit.Framework; using Umbraco.Core.Configuration.Models; -using Umbraco.Core.Configuration.Models.Extensions; +using Umbraco.Infrastructure.Configuration.Extensions; namespace Umbraco.Tests.UnitTests.Umbraco.Core.Configuration.Models.Extensions { [TestFixture] public class HealthCheckSettingsExtensionsTests { - [Test] - public void Returns_Notification_Delay_From_Provided_Time() + [TestCase("30 12 * * *", 30)] + [TestCase("15 18 * * *", 60 * 6 + 15)] + [TestCase("0 3 * * *", 60 * 15)] + [TestCase("0 3 2 * *", 24 * 60 * 1 + 60 * 15)] + [TestCase("0 6 * * 3", 24 * 60 * 3 + 60 * 18)] + public void Returns_Notification_Delay_From_Provided_Time(string firstRunTimeCronExpression, int expectedDelayInMinutes) { var settings = new HealthChecksSettings { Notification = new HealthChecksNotificationSettings { - FirstRunTime = "1230", + FirstRunTime = firstRunTimeCronExpression, } }; - var now = DateTime.Now.Date.AddHours(12); + var now = new DateTime(2020, 10, 31, 12, 0, 0); var result = settings.GetNotificationDelay(now, TimeSpan.Zero); - Assert.AreEqual(30, result.Minutes); + Assert.AreEqual(expectedDelayInMinutes, result.TotalMinutes); } [Test] @@ -30,12 +34,12 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Configuration.Models.Extensions { Notification = new HealthChecksNotificationSettings { - FirstRunTime = "1230", + FirstRunTime = "30 12 * * *", } }; - var now = DateTime.Now.Date.AddHours(12).AddMinutes(25); + var now = new DateTime(2020, 10, 31, 12, 25, 0); var result = settings.GetNotificationDelay(now, TimeSpan.FromMinutes(10)); - Assert.AreEqual(10, result.Minutes); + Assert.AreEqual(10, result.TotalMinutes); } } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/DateTimeExtensionsTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/DateTimeExtensionsTests.cs deleted file mode 100644 index b1585aa17f..0000000000 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/DateTimeExtensionsTests.cs +++ /dev/null @@ -1,46 +0,0 @@ -using System; -using NUnit.Framework; -using Umbraco.Core; - -namespace Umbraco.Tests.UnitTests.Umbraco.Core -{ - [TestFixture] - public class DateTimeExtensionsTests - { - [Test] - public void PeriodicMinutesFrom_PostTime_CalculatesMinutesBetween() - { - var nowDateTime = new DateTime(2017, 1, 1, 10, 30, 0); - var scheduledTime = "1145"; - var minutesBetween = nowDateTime.PeriodicMinutesFrom(scheduledTime); - Assert.AreEqual(75, minutesBetween); - } - - [Test] - public void PeriodicMinutesFrom_PriorTime_CalculatesMinutesBetween() - { - var nowDateTime = new DateTime(2017, 1, 1, 10, 30, 0); - var scheduledTime = "900"; - var minutesBetween = nowDateTime.PeriodicMinutesFrom(scheduledTime); - Assert.AreEqual(1350, minutesBetween); - } - - [Test] - public void PeriodicMinutesFrom_PriorTime_WithLeadingZero_CalculatesMinutesBetween() - { - var nowDateTime = new DateTime(2017, 1, 1, 10, 30, 0); - var scheduledTime = "0900"; - var minutesBetween = nowDateTime.PeriodicMinutesFrom(scheduledTime); - Assert.AreEqual(1350, minutesBetween); - } - - [Test] - public void PeriodicMinutesFrom_SameTime_CalculatesMinutesBetween() - { - var nowDateTime = new DateTime(2017, 1, 1, 10, 30, 0); - var scheduledTime = "1030"; - var minutesBetween = nowDateTime.PeriodicMinutesFrom(scheduledTime); - Assert.AreEqual(0, minutesBetween); - } - } -} diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/ShortStringHelper/StringExtensionsTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/ShortStringHelper/StringExtensionsTests.cs index 1644b9049e..427ae532f5 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/ShortStringHelper/StringExtensionsTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/ShortStringHelper/StringExtensionsTests.cs @@ -302,13 +302,12 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.ShortStringHelper } [TestCase("", false)] - [TestCase("12:34", true)] - [TestCase("1:14:23", true)] - [TestCase("25:03", false)] - [TestCase("18:61", false)] - public void IsValidTimeSpan(string input, bool expected) + [TestCase("* * * * 1", true)] + [TestCase("* * * * * 1", true)] + [TestCase("* * * 1", false)] + public void IsValidCronTab(string input, bool expected) { - var result = input.IsValidTimeSpan(); + var result = input.IsValidCronTab(); Assert.AreEqual(expected, result); } } From ed8c29f9028ab480071e77ec0171ffbaa6838aee Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 30 Oct 2020 19:56:26 +0100 Subject: [PATCH 2/4] Fixes issue with introduced MSDI abstractions where a single implemention is used for two interfaces, ensuring only one singleton instance is created. --- src/Umbraco.Core/ServiceCollectionExtensions.cs | 15 +++++++++++++++ .../Runtime/AspNetCoreComposer.cs | 7 ++----- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Core/ServiceCollectionExtensions.cs b/src/Umbraco.Core/ServiceCollectionExtensions.cs index 3202cc3a38..e6fcd43af1 100644 --- a/src/Umbraco.Core/ServiceCollectionExtensions.cs +++ b/src/Umbraco.Core/ServiceCollectionExtensions.cs @@ -12,6 +12,21 @@ namespace Umbraco.Core where TImplementing : class, TService => services.Replace(ServiceDescriptor.Singleton()); + /// + /// Registers a unique service as a single instance implementing two interfaces. + /// + /// + /// Hat-tip: https://stackoverflow.com/a/55402016/489433 + /// + public static void AddUnique(this IServiceCollection services) + where TService1 : class + where TService2 : class + where TImplementing : class, TService1, TService2 + { + services.Replace(ServiceDescriptor.Singleton()); + services.Replace(ServiceDescriptor.Singleton(x => (TImplementing)x.GetService())); + } + public static void AddUnique(this IServiceCollection services) where TImplementing : class => services.Replace(ServiceDescriptor.Singleton()); diff --git a/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs b/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs index 39742dcb0d..832cfdb488 100644 --- a/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs +++ b/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs @@ -55,13 +55,11 @@ namespace Umbraco.Web.Common.Runtime composition.Services.AddUnique(); // The umbraco request lifetime - composition.Services.AddUnique(); - composition.Services.AddUnique(); + composition.Services.AddUnique(); - //Password hasher + // Password hasher composition.Services.AddUnique(); - composition.Services.AddUnique(); composition.Services.AddTransient(); composition.Services.AddUnique(); @@ -76,7 +74,6 @@ namespace Umbraco.Web.Common.Runtime composition.Services.AddUnique(); composition.Services.AddUnique(); - // register the umbraco context factory composition.Services.AddUnique(); composition.Services.AddUnique(); From 0bffe9aa554d6c3455cf8b875e7c42cff9ac81ed Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Sat, 31 Oct 2020 22:53:16 +0100 Subject: [PATCH 3/4] Updated health check configuration validation tests to match change to cron expression. --- .../HostedServices/HealthCheckNotifier.cs | 1 - .../Models/Validation/HealthChecksSettingsValidatorTests.cs | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs b/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs index f4cf33fa61..b9e41c8410 100644 --- a/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs +++ b/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs @@ -52,7 +52,6 @@ namespace Umbraco.Infrastructure.HostedServices _logger = logger; _profilingLogger = profilingLogger; } - public override async void ExecuteAsync(object state) { diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidatorTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidatorTests.cs index e6ca06a34b..d38b41720e 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidatorTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidatorTests.cs @@ -21,12 +21,12 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Configuration.Models.Validation public void Returns_Fail_For_Configuration_With_Invalid_Notification_FirstRunTime() { var validator = new HealthChecksSettingsValidator(); - var options = BuildHealthChecksSettings(firstRunTime: "25:00"); + var options = BuildHealthChecksSettings(firstRunTime: "0 3 *"); var result = validator.Validate("settings", options); Assert.False(result.Succeeded); } - private static HealthChecksSettings BuildHealthChecksSettings(string firstRunTime = "12:00") + private static HealthChecksSettings BuildHealthChecksSettings(string firstRunTime = "0 3 * * *") { return new HealthChecksSettings { From 8e44d89856f31a459c618c9c97f53077d3ebbccd Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Tue, 3 Nov 2020 09:02:15 +0100 Subject: [PATCH 4/4] Introducing interface for our needs of a CronTabParser. And inject it where needed Signed-off-by: Bjarke Berg --- .../Configuration/ICronTabParser.cs | 10 ++++++ .../Validation/ConfigurationValidatorBase.cs | 10 ------ .../HealthChecksSettingsValidator.cs | 19 +++++++++++ src/Umbraco.Core/StringExtensions.cs | 20 ------------ .../HealthCheckSettingsExtensions.cs | 6 ++-- .../Configuration/NCronTabParser.cs | 24 ++++++++++++++ .../HostedServices/HealthCheckNotifier.cs | 8 +++-- .../Runtime/CoreInitialComposer.cs | 4 ++- .../HealthCheckSettingsExtensionsTests.cs | 7 ++-- .../HealthChecksSettingsValidatorTests.cs | 5 +-- .../Configurations/NCronTabParserTests.cs | 32 +++++++++++++++++++ .../StringExtensionsTests.cs | 9 ------ .../HealthCheckNotifierTests.cs | 3 +- 13 files changed, 106 insertions(+), 51 deletions(-) create mode 100644 src/Umbraco.Core/Configuration/ICronTabParser.cs create mode 100644 src/Umbraco.Infrastructure/Configuration/NCronTabParser.cs create mode 100644 src/Umbraco.Tests.UnitTests/Umbraco.Core/Configurations/NCronTabParserTests.cs diff --git a/src/Umbraco.Core/Configuration/ICronTabParser.cs b/src/Umbraco.Core/Configuration/ICronTabParser.cs new file mode 100644 index 0000000000..2238be4a4c --- /dev/null +++ b/src/Umbraco.Core/Configuration/ICronTabParser.cs @@ -0,0 +1,10 @@ +using System; + +namespace Umbraco.Core.Configuration +{ + public interface ICronTabParser + { + bool IsValidCronTab(string cronTab); + DateTime GetNextOccurrence(string cronTab, DateTime time); + } +} diff --git a/src/Umbraco.Core/Configuration/Models/Validation/ConfigurationValidatorBase.cs b/src/Umbraco.Core/Configuration/Models/Validation/ConfigurationValidatorBase.cs index 343ba5656b..6c0af9802b 100644 --- a/src/Umbraco.Core/Configuration/Models/Validation/ConfigurationValidatorBase.cs +++ b/src/Umbraco.Core/Configuration/Models/Validation/ConfigurationValidatorBase.cs @@ -42,17 +42,7 @@ namespace Umbraco.Core.Configuration.Models.Validation return true; } - public bool ValidateOptionalCronTab(string configPath, string value, out string message) - { - if (!string.IsNullOrEmpty(value) && !value.IsValidCronTab()) - { - message = $"Configuration entry {configPath} contains an invalid cron expression."; - return false; - } - message = string.Empty; - return true; - } } } diff --git a/src/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidator.cs b/src/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidator.cs index 9ecb6fb118..f26fce9bd9 100644 --- a/src/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidator.cs +++ b/src/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidator.cs @@ -4,6 +4,13 @@ namespace Umbraco.Core.Configuration.Models.Validation { public class HealthChecksSettingsValidator : ConfigurationValidatorBase, IValidateOptions { + private readonly ICronTabParser _cronTabParser; + + public HealthChecksSettingsValidator(ICronTabParser cronTabParser) + { + _cronTabParser = cronTabParser; + } + public ValidateOptionsResult Validate(string name, HealthChecksSettings options) { if (!ValidateNotificationFirstRunTime(options.Notification.FirstRunTime, out var message)) @@ -18,5 +25,17 @@ namespace Umbraco.Core.Configuration.Models.Validation { return ValidateOptionalCronTab($"{Constants.Configuration.ConfigHealthChecks}:{nameof(HealthChecksSettings.Notification)}:{nameof(HealthChecksSettings.Notification.FirstRunTime)}", value, out message); } + + public bool ValidateOptionalCronTab(string configPath, string value, out string message) + { + if (!string.IsNullOrEmpty(value) && !_cronTabParser.IsValidCronTab(value)) + { + message = $"Configuration entry {configPath} contains an invalid cron expression."; + return false; + } + + message = string.Empty; + return true; + } } } diff --git a/src/Umbraco.Core/StringExtensions.cs b/src/Umbraco.Core/StringExtensions.cs index 0cf8158518..67b0b49a45 100644 --- a/src/Umbraco.Core/StringExtensions.cs +++ b/src/Umbraco.Core/StringExtensions.cs @@ -1478,25 +1478,5 @@ namespace Umbraco.Core { return shortStringHelper.CleanStringForSafeFileName(text, culture); } - - /// - /// Validates a string matches a cron tab (for length only). - /// - /// String with timespan representation (in cron tab format: https://github.com/atifaziz/NCrontab/wiki/Crontab-Expression) - /// True if string matches a valid cron expression, false if not. - /// - /// Considering an expression as valid if it's supported by https://github.com/atifaziz/NCrontab/wiki/Crontab-Expression, - /// so only 5 or 6 values are expected. - /// - public static bool IsValidCronTab(this string input) - { - if (string.IsNullOrEmpty(input)) - { - return false; - } - - var parts = input.Split(new string[] { " " }, StringSplitOptions.RemoveEmptyEntries); - return parts.Length == 5 || parts.Length == 6; - } } } diff --git a/src/Umbraco.Infrastructure/Configuration/Extensions/HealthCheckSettingsExtensions.cs b/src/Umbraco.Infrastructure/Configuration/Extensions/HealthCheckSettingsExtensions.cs index 57ae58928d..92c3608ac1 100644 --- a/src/Umbraco.Infrastructure/Configuration/Extensions/HealthCheckSettingsExtensions.cs +++ b/src/Umbraco.Infrastructure/Configuration/Extensions/HealthCheckSettingsExtensions.cs @@ -1,12 +1,13 @@ using System; using NCrontab; +using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.Models; namespace Umbraco.Infrastructure.Configuration.Extensions { public static class HealthCheckSettingsExtensions { - public static TimeSpan GetNotificationDelay(this HealthChecksSettings settings, DateTime now, TimeSpan defaultDelay) + public static TimeSpan GetNotificationDelay(this HealthChecksSettings settings, ICronTabParser cronTabParser, DateTime now, TimeSpan defaultDelay) { // If first run time not set, start with just small delay after application start. var firstRunTime = settings.Notification.FirstRunTime; @@ -17,8 +18,7 @@ namespace Umbraco.Infrastructure.Configuration.Extensions else { // Otherwise start at scheduled time according to cron expression, unless within the default delay period. - var firstRunTimeCronExpression = CrontabSchedule.Parse(firstRunTime); - var firstRunOccurance = firstRunTimeCronExpression.GetNextOccurrence(now); + var firstRunOccurance = cronTabParser.GetNextOccurrence(firstRunTime, now); var delay = firstRunOccurance - now; return delay < defaultDelay ? defaultDelay diff --git a/src/Umbraco.Infrastructure/Configuration/NCronTabParser.cs b/src/Umbraco.Infrastructure/Configuration/NCronTabParser.cs new file mode 100644 index 0000000000..5642c882e6 --- /dev/null +++ b/src/Umbraco.Infrastructure/Configuration/NCronTabParser.cs @@ -0,0 +1,24 @@ +using System; +using NCrontab; + +namespace Umbraco.Core.Configuration +{ + + public class NCronTabParser : ICronTabParser + { + public bool IsValidCronTab(string cronTab) + { + var result = CrontabSchedule.TryParse(cronTab); + + return !(result is null); + } + + public DateTime GetNextOccurrence(string cronTab, DateTime time) + { + var result = CrontabSchedule.Parse(cronTab); + + return result.GetNextOccurrence(time); + } + } + +} diff --git a/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs b/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs index c3cd2864dc..58decb80c4 100644 --- a/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs +++ b/src/Umbraco.Infrastructure/HostedServices/HealthCheckNotifier.cs @@ -4,6 +4,7 @@ using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Umbraco.Core; +using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.Models; using Umbraco.Core.HealthCheck; using Umbraco.Core.Logging; @@ -39,9 +40,10 @@ namespace Umbraco.Infrastructure.HostedServices IMainDom mainDom, IScopeProvider scopeProvider, ILogger logger, - IProfilingLogger profilingLogger) + IProfilingLogger profilingLogger, + ICronTabParser cronTabParser) : base(healthChecksSettings.Value.Notification.Period, - healthChecksSettings.Value.GetNotificationDelay(DateTime.Now, DefaultDelay)) + healthChecksSettings.Value.GetNotificationDelay(cronTabParser, DateTime.Now, DefaultDelay)) { _healthChecksSettings = healthChecksSettings.Value; _healthChecks = healthChecks; @@ -53,7 +55,7 @@ namespace Umbraco.Infrastructure.HostedServices _logger = logger; _profilingLogger = profilingLogger; } - + internal override async Task PerformExecuteAsync(object state) { if (_healthChecksSettings.Notification.Enabled == false) diff --git a/src/Umbraco.Infrastructure/Runtime/CoreInitialComposer.cs b/src/Umbraco.Infrastructure/Runtime/CoreInitialComposer.cs index 01bfb79e26..ca282fc6ab 100644 --- a/src/Umbraco.Infrastructure/Runtime/CoreInitialComposer.cs +++ b/src/Umbraco.Infrastructure/Runtime/CoreInitialComposer.cs @@ -208,7 +208,7 @@ namespace Umbraco.Core.Runtime // Config manipulator composition.Services.AddUnique(); - + // register the umbraco context factory // composition.Services.AddUnique(); composition.Services.AddUnique(); @@ -370,6 +370,8 @@ namespace Umbraco.Core.Runtime composition.Services.AddUnique(); composition.Services.AddUnique(); + + composition.Services.AddUnique(); } } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensionsTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensionsTests.cs index d1819fd83e..833e5c865b 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensionsTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Extensions/HealthCheckSettingsExtensionsTests.cs @@ -1,5 +1,6 @@ using System; using NUnit.Framework; +using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.Models; using Umbraco.Infrastructure.Configuration.Extensions; @@ -8,6 +9,8 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Configuration.Models.Extensions [TestFixture] public class HealthCheckSettingsExtensionsTests { + private ICronTabParser CronTabParser => new NCronTabParser(); + [TestCase("30 12 * * *", 30)] [TestCase("15 18 * * *", 60 * 6 + 15)] [TestCase("0 3 * * *", 60 * 15)] @@ -23,7 +26,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Configuration.Models.Extensions } }; var now = new DateTime(2020, 10, 31, 12, 0, 0); - var result = settings.GetNotificationDelay(now, TimeSpan.Zero); + var result = settings.GetNotificationDelay(CronTabParser, now, TimeSpan.Zero); Assert.AreEqual(expectedDelayInMinutes, result.TotalMinutes); } @@ -38,7 +41,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Configuration.Models.Extensions } }; var now = new DateTime(2020, 10, 31, 12, 25, 0); - var result = settings.GetNotificationDelay(now, TimeSpan.FromMinutes(10)); + var result = settings.GetNotificationDelay(CronTabParser, now, TimeSpan.FromMinutes(10)); Assert.AreEqual(10, result.TotalMinutes); } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidatorTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidatorTests.cs index d38b41720e..64a8be79a8 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidatorTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Validation/HealthChecksSettingsValidatorTests.cs @@ -1,5 +1,6 @@ using System; using NUnit.Framework; +using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.Models; using Umbraco.Core.Configuration.Models.Validation; @@ -11,7 +12,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Configuration.Models.Validation [Test] public void Returns_Success_ForValid_Configuration() { - var validator = new HealthChecksSettingsValidator(); + var validator = new HealthChecksSettingsValidator(new NCronTabParser()); var options = BuildHealthChecksSettings(); var result = validator.Validate("settings", options); Assert.True(result.Succeeded); @@ -20,7 +21,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Configuration.Models.Validation [Test] public void Returns_Fail_For_Configuration_With_Invalid_Notification_FirstRunTime() { - var validator = new HealthChecksSettingsValidator(); + var validator = new HealthChecksSettingsValidator(new NCronTabParser()); var options = BuildHealthChecksSettings(firstRunTime: "0 3 *"); var result = validator.Validate("settings", options); Assert.False(result.Succeeded); diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configurations/NCronTabParserTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configurations/NCronTabParserTests.cs new file mode 100644 index 0000000000..4a38de831d --- /dev/null +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Configurations/NCronTabParserTests.cs @@ -0,0 +1,32 @@ +using NUnit.Framework; +using Umbraco.Core.Configuration; + +namespace Umbraco.Tests.UnitTests.Umbraco.Core.Configurations +{ + [TestFixture] + public class NCronTabParserTests + { + private ICronTabParser Sut => new NCronTabParser(); + + [TestCase("", ExpectedResult = false)] + [TestCase("* * * * 1", ExpectedResult = true)] + [TestCase("* * * * * 1", ExpectedResult = false)] + [TestCase("* * * 1", ExpectedResult = false)] + [TestCase("Invalid", ExpectedResult = false)] + [TestCase("I n v a l", ExpectedResult = false)] + [TestCase("23 0-20/2 * * *", ExpectedResult = true)] + [TestCase("5 4 * * sun", ExpectedResult = true)] + [TestCase("0 0,12 1 */2 *", ExpectedResult = true)] + [TestCase("0 0 1,15 * 3", ExpectedResult = true)] + [TestCase("5 0 * 8 *", ExpectedResult = true)] + [TestCase("22 * * 1-5 *", ExpectedResult = true)] + [TestCase("23 0-20/2 * * *", ExpectedResult = true)] + [TestCase("23 0-20/2 * * sun-sat", ExpectedResult = true)] + [TestCase("23 0-20/2 * jan-dec sun-sat", ExpectedResult = true)] + [TestCase("* * 32 * *", ExpectedResult = false)] + public bool IsValidCronTab(string input) + { + return Sut.IsValidCronTab(input); + } + } +} diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/ShortStringHelper/StringExtensionsTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/ShortStringHelper/StringExtensionsTests.cs index 427ae532f5..03c84a03b8 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/ShortStringHelper/StringExtensionsTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/ShortStringHelper/StringExtensionsTests.cs @@ -301,14 +301,5 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.ShortStringHelper Assert.AreEqual(expected, output); } - [TestCase("", false)] - [TestCase("* * * * 1", true)] - [TestCase("* * * * * 1", true)] - [TestCase("* * * 1", false)] - public void IsValidCronTab(string input, bool expected) - { - var result = input.IsValidCronTab(); - Assert.AreEqual(expected, result); - } } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/HostedServices/HealthCheckNotifierTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/HostedServices/HealthCheckNotifierTests.cs index bbbb4035c5..6c1284854e 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/HostedServices/HealthCheckNotifierTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/HostedServices/HealthCheckNotifierTests.cs @@ -7,6 +7,7 @@ using Microsoft.Extensions.Options; using Moq; using NUnit.Framework; using Umbraco.Core; +using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.Models; using Umbraco.Core.HealthCheck; using Umbraco.Core.Logging; @@ -141,7 +142,7 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Infrastructure.HostedServices return new HealthCheckNotifier(Options.Create(settings), checks, notifications, mockRunTimeState.Object, mockServerRegistrar.Object, mockMainDom.Object, mockScopeProvider.Object, - mockLogger.Object, mockProfilingLogger.Object); + mockLogger.Object, mockProfilingLogger.Object, Mock.Of()); } private void VerifyNotificationsNotSent()