Fixing ScriptHost SettingsManager bug

mathewc · mathewc · commit 2cefd8093e47 · 2017-01-27T10:26:24.000-08:00
diff --git a/src/WebJobs.Script/Host/ScriptHost.cs b/src/WebJobs.Script/Host/ScriptHost.cs
@@ -49,17 +49,24 @@ public class ScriptHost : JobHost
         private static readonly TimeSpan MinTimeout = TimeSpan.FromSeconds(1);
         private static readonly TimeSpan MaxTimeout = TimeSpan.FromMinutes(5);
         private static readonly Regex FunctionNameValidationRegex = new Regex(@"^[a-z][a-z0-9_\-]{0,127}$(?<!^host$)", RegexOptions.Compiled | RegexOptions.IgnoreCase);
-        private static ScriptSettingsManager _settingsManager;
+        private ScriptSettingsManager _settingsManager;
 
-        protected ScriptHost(ScriptHostConfiguration scriptConfig)
+        protected ScriptHost(ScriptHostConfiguration scriptConfig = null, ScriptSettingsManager settingsManager = null)
             : base(scriptConfig.HostConfig)
         {
+            scriptConfig = scriptConfig ?? new ScriptHostConfiguration();
+            if (!Path.IsPathRooted(scriptConfig.RootScriptPath))
+            {
+                scriptConfig.RootScriptPath = Path.Combine(Environment.CurrentDirectory, scriptConfig.RootScriptPath);
+            }
             ScriptConfig = scriptConfig;
             FunctionErrors = new Dictionary<string, Collection<string>>(StringComparer.OrdinalIgnoreCase);
 #if FEATURE_NODE
             NodeFunctionInvoker.UnhandledException += OnUnhandledException;
 #endif
             TraceWriter = ScriptConfig.TraceWriter;
+
+            _settingsManager = settingsManager ?? ScriptSettingsManager.Instance;
         }
 
         public static readonly string Version = GetAssemblyFileVersion(typeof(ScriptHost).Assembly);
@@ -474,23 +481,9 @@ private void PurgeOldLogDirectories()
 
         public static ScriptHost Create(ScriptHostConfiguration scriptConfig = null, ScriptSettingsManager settingsManager = null)
         {
-            if (scriptConfig == null)
-            {
-                scriptConfig = new ScriptHostConfiguration();
-            }
-
-            _settingsManager = settingsManager;
-            if (settingsManager == null)
-            {
-                _settingsManager = ScriptSettingsManager.Instance;
-            }
-
-            if (!Path.IsPathRooted(scriptConfig.RootScriptPath))
-            {
-                scriptConfig.RootScriptPath = Path.Combine(Environment.CurrentDirectory, scriptConfig.RootScriptPath);
-            }
+            scriptConfig = scriptConfig ?? new ScriptHostConfiguration();
 
-            ScriptHost scriptHost = new ScriptHost(scriptConfig);
+            ScriptHost scriptHost = new ScriptHost(scriptConfig, settingsManager);
             try
             {
                 scriptHost.Initialize();
@@ -549,7 +542,7 @@ private static Collection<ScriptBindingProvider> LoadBindingProviders(ScriptHost
             return bindingProviders;
         }
 
-        private static FunctionMetadata ParseFunctionMetadata(string functionName, JObject configMetadata)
+        private static FunctionMetadata ParseFunctionMetadata(string functionName, JObject configMetadata, ScriptSettingsManager settingsManager)
         {
             FunctionMetadata functionMetadata = new FunctionMetadata
             {
@@ -577,8 +570,8 @@ private static FunctionMetadata ParseFunctionMetadata(string functionName, JObje
             }
 
             // A function can be disabled at the trigger or function level
-            if (IsDisabled(triggerDisabledValue) ||
-                IsDisabled((JValue)configMetadata["disabled"]))
+            if (IsDisabled(triggerDisabledValue, settingsManager) || 
+                IsDisabled((JValue)configMetadata["disabled"], settingsManager))
             {
                 functionMetadata.IsDisabled = true;
             }
@@ -593,9 +586,10 @@ private static FunctionMetadata ParseFunctionMetadata(string functionName, JObje
             return functionMetadata;
         }
 
-        public static Collection<FunctionMetadata> ReadFunctionMetadata(ScriptHostConfiguration config, TraceWriter traceWriter, Dictionary<string, Collection<string>> functionErrors)
+        public static Collection<FunctionMetadata> ReadFunctionMetadata(ScriptHostConfiguration config, TraceWriter traceWriter, Dictionary<string, Collection<string>> functionErrors, ScriptSettingsManager settingsManager = null)
         {
             var functions = new Collection<FunctionMetadata>();
+            settingsManager = settingsManager ?? ScriptSettingsManager.Instance;
 
             foreach (var scriptDir in Directory.EnumerateDirectories(config.RootScriptPath))
             {
@@ -629,7 +623,7 @@ public static Collection<FunctionMetadata> ReadFunctionMetadata(ScriptHostConfig
                     string functionError = null;
                     FunctionMetadata functionMetadata = null;
                     var mappedHttpFunctions = new Dictionary<string, HttpTriggerBindingMetadata>();
-                    if (!TryParseFunctionMetadata(functionName, functionConfig, mappedHttpFunctions, traceWriter, scriptDir, out functionMetadata, out functionError))
+                    if (!TryParseFunctionMetadata(functionName, functionConfig, mappedHttpFunctions, traceWriter, scriptDir, settingsManager, out functionMetadata, out functionError))
                     {
                         // for functions in error, log the error and don't
                         // add to the functions collection
@@ -652,12 +646,12 @@ public static Collection<FunctionMetadata> ReadFunctionMetadata(ScriptHostConfig
         }
 
         internal static bool TryParseFunctionMetadata(string functionName, JObject functionConfig, Dictionary<string, HttpTriggerBindingMetadata> mappedHttpFunctions,
-            TraceWriter traceWriter, string scriptDirectory, out FunctionMetadata functionMetadata, out string error, IFileSystem fileSystem = null)
+            TraceWriter traceWriter, string scriptDirectory, ScriptSettingsManager settingsManager, out FunctionMetadata functionMetadata, out string error, IFileSystem fileSystem = null)
         {
             fileSystem = fileSystem ?? new FileSystem();
 
             error = null;
-            functionMetadata = ParseFunctionMetadata(functionName, functionConfig);
+            functionMetadata = ParseFunctionMetadata(functionName, functionConfig, settingsManager);
 
             if (functionMetadata.IsExcluded)
             {
@@ -839,7 +833,7 @@ private static ScriptType ParseScriptType(string scriptFilePath)
 
         private Collection<FunctionDescriptor> GetFunctionDescriptors()
         {
-            var functions = ReadFunctionMetadata(ScriptConfig, TraceWriter, FunctionErrors);
+            var functions = ReadFunctionMetadata(ScriptConfig, TraceWriter, FunctionErrors, _settingsManager);
 
             var descriptorProviders = new List<FunctionDescriptorProvider>()
                 {
@@ -1155,7 +1149,7 @@ internal static string GetRelativeDirectory(string path, string scriptRoot)
             return string.Empty;
         }
 
-        private static bool IsDisabled(JToken isDisabledValue)
+        private static bool IsDisabled(JToken isDisabledValue, ScriptSettingsManager settingsManager)
         {
             if (isDisabledValue != null)
             {
@@ -1166,7 +1160,7 @@ private static bool IsDisabled(JToken isDisabledValue)
                 else
                 {
                     string settingName = (string)isDisabledValue;
-                    string value = _settingsManager.GetSetting(settingName);
+                    string value = settingsManager.GetSetting(settingName);
                     if (!string.IsNullOrEmpty(value) &&
                         (string.Compare(value, "1", StringComparison.OrdinalIgnoreCase) == 0 ||
                          string.Compare(value, "true", StringComparison.OrdinalIgnoreCase) == 0))
diff --git a/test/WebJobs.Script.Tests.Integration/Host/ScriptHostManagerTests.cs b/test/WebJobs.Script.Tests.Integration/Host/ScriptHostManagerTests.cs
@@ -190,7 +190,7 @@ public void RunAndBlock_DisposesOfHost_WhenExceptionIsThrown()
                 RootScriptPath = Environment.CurrentDirectory
             };
 
-            var hostMock = new Mock<ScriptHost>(config);
+            var hostMock = new Mock<ScriptHost>(config, null);
             var factoryMock = new Mock<IScriptHostFactory>();
             factoryMock.Setup(f => f.Create(_settingsManager, It.IsAny<ScriptHostConfiguration>()))
                 .Returns(hostMock.Object);
diff --git a/test/WebJobs.Script.Tests/Controllers/Admin/AdminControllerTests.cs b/test/WebJobs.Script.Tests/Controllers/Admin/AdminControllerTests.cs
@@ -37,7 +37,7 @@ public AdminControllerTests()
             testFunctions = new Collection<FunctionDescriptor>();
 
             var config = new ScriptHostConfiguration();
-            hostMock = new Mock<ScriptHost>(MockBehavior.Strict, new object[] { config });
+            hostMock = new Mock<ScriptHost>(MockBehavior.Strict, new object[] { config, null });
             hostMock.Setup(p => p.Functions).Returns(testFunctions);
 
             WebHostSettings settings = new WebHostSettings();
diff --git a/test/WebJobs.Script.Tests/Description/DotNet/DotNetFunctionInvokerTests.cs b/test/WebJobs.Script.Tests/Description/DotNet/DotNetFunctionInvokerTests.cs
@@ -201,7 +201,7 @@ private RunDependencies CreateDependencies(TraceLevel traceLevel = TraceLevel.In
 
             scriptHostConfiguration.HostConfig.Tracing.ConsoleLevel = System.Diagnostics.TraceLevel.Verbose;
 
-            var host = new Mock<ScriptHost>(scriptHostConfiguration);
+            var host = new Mock<ScriptHost>(scriptHostConfiguration, null);
             host.SetupGet(h => h.IsPrimary).Returns(true);
 
             var entrypointResolver = new Mock<IFunctionEntryPointResolver>();
diff --git a/test/WebJobs.Script.Tests/ScriptHostTests.cs b/test/WebJobs.Script.Tests/ScriptHostTests.cs
@@ -853,7 +853,7 @@ public void TryParseFunctionMetadata_ValidatesHttpRoutes()
             fileSystem.AddFile(@"c:\functions\test3\run.csx", new MockFileData(string.Empty));
             fileSystem.AddFile(@"c:\functions\test4\run.csx", new MockFileData(string.Empty));
             fileSystem.AddFile(@"c:\functions\test5\run.csx", new MockFileData(string.Empty));
-            bool result = ScriptHost.TryParseFunctionMetadata("test", functionConfig, mappedHttpFunctions, traceWriter, @"c:\functions\test", out functionMetadata, out functionError, fileSystem);
+            bool result = ScriptHost.TryParseFunctionMetadata("test", functionConfig, mappedHttpFunctions, traceWriter, @"c:\functions\test", ScriptSettingsManager.Instance, out functionMetadata, out functionError, fileSystem);
             Assert.True(result);
             Assert.NotNull(functionMetadata);
             Assert.Null(functionError);
@@ -872,7 +872,7 @@ public void TryParseFunctionMetadata_ValidatesHttpRoutes()
             });
             functionMetadata = null;
             functionError = null;
-            result = ScriptHost.TryParseFunctionMetadata("test2", functionConfig, mappedHttpFunctions, traceWriter, @"c:\functions\test2", out functionMetadata, out functionError, fileSystem);
+            result = ScriptHost.TryParseFunctionMetadata("test2", functionConfig, mappedHttpFunctions, traceWriter, @"c:\functions\test2", ScriptSettingsManager.Instance, out functionMetadata, out functionError, fileSystem);
             Assert.True(result);
             Assert.NotNull(functionMetadata);
             Assert.Null(functionError);
@@ -890,7 +890,7 @@ public void TryParseFunctionMetadata_ValidatesHttpRoutes()
             });
             functionMetadata = null;
             functionError = null;
-            result = ScriptHost.TryParseFunctionMetadata("test3", functionConfig, mappedHttpFunctions, traceWriter, @"c:\functions\test3", out functionMetadata, out functionError, fileSystem);
+            result = ScriptHost.TryParseFunctionMetadata("test3", functionConfig, mappedHttpFunctions, traceWriter, @"c:\functions\test3", ScriptSettingsManager.Instance, out functionMetadata, out functionError, fileSystem);
             Assert.True(result);
             Assert.NotNull(functionMetadata);
             Assert.Null(functionError);
@@ -908,7 +908,7 @@ public void TryParseFunctionMetadata_ValidatesHttpRoutes()
             });
             functionMetadata = null;
             functionError = null;
-            result = ScriptHost.TryParseFunctionMetadata("test4", functionConfig, mappedHttpFunctions, traceWriter, @"c:\functions\test4", out functionMetadata, out functionError, fileSystem);
+            result = ScriptHost.TryParseFunctionMetadata("test4", functionConfig, mappedHttpFunctions, traceWriter, @"c:\functions\test4", ScriptSettingsManager.Instance, out functionMetadata, out functionError, fileSystem);
             Assert.False(result);
             Assert.NotNull(functionMetadata);
             Assert.True(functionError.StartsWith("The route specified conflicts with the route defined by function"));
@@ -924,7 +924,7 @@ public void TryParseFunctionMetadata_ValidatesHttpRoutes()
             });
             functionMetadata = null;
             functionError = null;
-            result = ScriptHost.TryParseFunctionMetadata("test5", functionConfig, mappedHttpFunctions, traceWriter, @"c:\functions\test5", out functionMetadata, out functionError, fileSystem);
+            result = ScriptHost.TryParseFunctionMetadata("test5", functionConfig, mappedHttpFunctions, traceWriter, @"c:\functions\test5", ScriptSettingsManager.Instance, out functionMetadata, out functionError, fileSystem);
             Assert.False(result);
             Assert.NotNull(functionMetadata);
             Assert.Equal(3, mappedHttpFunctions.Count);
@@ -941,14 +941,11 @@ public override object[] GetCustomAttributes(Type attributeType, bool inherit)
 
         public class TestFixture
         {
-            private readonly ScriptSettingsManager _settingsManager;
-
             public TestFixture()
             {
                 ScriptHostConfiguration config = new ScriptHostConfiguration();
                 config.HostConfig.HostId = ID;
-                _settingsManager = ScriptSettingsManager.Instance;
-                Host = ScriptHost.Create(config, _settingsManager);
+                Host = ScriptHost.Create(config);
             }
 
             public ScriptHost Host { get; private set; }

Original file line number	Diff line number	Diff line change
`@@ -49,17 +49,24 @@ public class ScriptHost : JobHost`
`49`	`49`	`private static readonly TimeSpan MinTimeout = TimeSpan.FromSeconds(1);`
`50`	`50`	`private static readonly TimeSpan MaxTimeout = TimeSpan.FromMinutes(5);`
`51`	`51`	`private static readonly Regex FunctionNameValidationRegex = new Regex(@"^[a-z][a-z0-9_\-]{0,127}$(?<!^host$)", RegexOptions.Compiled \| RegexOptions.IgnoreCase);`
`52`		`- private static ScriptSettingsManager _settingsManager;`
	`52`	`+ private ScriptSettingsManager _settingsManager;`
`53`	`53`
`54`		`- protected ScriptHost(ScriptHostConfiguration scriptConfig)`
	`54`	`+ protected ScriptHost(ScriptHostConfiguration scriptConfig = null, ScriptSettingsManager settingsManager = null)`
`55`	`55`	`: base(scriptConfig.HostConfig)`
`56`	`56`	`{`
	`57`	`+ scriptConfig = scriptConfig ?? new ScriptHostConfiguration();`
	`58`	`+ if (!Path.IsPathRooted(scriptConfig.RootScriptPath))`
	`59`	`+ {`
	`60`	`+ scriptConfig.RootScriptPath = Path.Combine(Environment.CurrentDirectory, scriptConfig.RootScriptPath);`
	`61`	`+ }`
`57`	`62`	`ScriptConfig = scriptConfig;`
`58`	`63`	`FunctionErrors = new Dictionary<string, Collection<string>>(StringComparer.OrdinalIgnoreCase);`
`59`	`64`	`#if FEATURE_NODE`
`60`	`65`	`NodeFunctionInvoker.UnhandledException += OnUnhandledException;`
`61`	`66`	`#endif`
`62`	`67`	`TraceWriter = ScriptConfig.TraceWriter;`
	`68`	`+`
	`69`	`+ _settingsManager = settingsManager ?? ScriptSettingsManager.Instance;`
`63`	`70`	`}`
`64`	`71`
`65`	`72`	`public static readonly string Version = GetAssemblyFileVersion(typeof(ScriptHost).Assembly);`
`@@ -474,23 +481,9 @@ private void PurgeOldLogDirectories()`
`474`	`481`
`475`	`482`	`public static ScriptHost Create(ScriptHostConfiguration scriptConfig = null, ScriptSettingsManager settingsManager = null)`
`476`	`483`	`{`
`477`		`- if (scriptConfig == null)`
`478`		`- {`
`479`		`- scriptConfig = new ScriptHostConfiguration();`
`480`		`- }`
`481`		`-`
`482`		`- _settingsManager = settingsManager;`
`483`		`- if (settingsManager == null)`
`484`		`- {`
`485`		`- _settingsManager = ScriptSettingsManager.Instance;`
`486`		`- }`
`487`		`-`
`488`		`- if (!Path.IsPathRooted(scriptConfig.RootScriptPath))`
`489`		`- {`
`490`		`- scriptConfig.RootScriptPath = Path.Combine(Environment.CurrentDirectory, scriptConfig.RootScriptPath);`
`491`		`- }`
	`484`	`+ scriptConfig = scriptConfig ?? new ScriptHostConfiguration();`
`492`	`485`
`493`		`- ScriptHost scriptHost = new ScriptHost(scriptConfig);`
	`486`	`+ ScriptHost scriptHost = new ScriptHost(scriptConfig, settingsManager);`
`494`	`487`	`try`
`495`	`488`	`{`
`496`	`489`	`scriptHost.Initialize();`
`@@ -549,7 +542,7 @@ private static Collection<ScriptBindingProvider> LoadBindingProviders(ScriptHost`
`549`	`542`	`return bindingProviders;`
`550`	`543`	`}`
`551`	`544`
`552`		`- private static FunctionMetadata ParseFunctionMetadata(string functionName, JObject configMetadata)`
	`545`	`+ private static FunctionMetadata ParseFunctionMetadata(string functionName, JObject configMetadata, ScriptSettingsManager settingsManager)`
`553`	`546`	`{`
`554`	`547`	`FunctionMetadata functionMetadata = new FunctionMetadata`
`555`	`548`	`{`
`@@ -577,8 +570,8 @@ private static FunctionMetadata ParseFunctionMetadata(string functionName, JObje`
`577`	`570`	`}`
`578`	`571`
`579`	`572`	`// A function can be disabled at the trigger or function level`
`580`		`- if (IsDisabled(triggerDisabledValue) \|\|`
`581`		`- IsDisabled((JValue)configMetadata["disabled"]))`
	`573`	`+ if (IsDisabled(triggerDisabledValue, settingsManager) \|\|`
	`574`	`+ IsDisabled((JValue)configMetadata["disabled"], settingsManager))`
`582`	`575`	`{`
`583`	`576`	`functionMetadata.IsDisabled = true;`
`584`	`577`	`}`
`@@ -593,9 +586,10 @@ private static FunctionMetadata ParseFunctionMetadata(string functionName, JObje`
`593`	`586`	`return functionMetadata;`
`594`	`587`	`}`
`595`	`588`
`596`		`- public static Collection<FunctionMetadata> ReadFunctionMetadata(ScriptHostConfiguration config, TraceWriter traceWriter, Dictionary<string, Collection<string>> functionErrors)`
	`589`	`+ public static Collection<FunctionMetadata> ReadFunctionMetadata(ScriptHostConfiguration config, TraceWriter traceWriter, Dictionary<string, Collection<string>> functionErrors, ScriptSettingsManager settingsManager = null)`
`597`	`590`	`{`
`598`	`591`	`var functions = new Collection<FunctionMetadata>();`
	`592`	`+ settingsManager = settingsManager ?? ScriptSettingsManager.Instance;`
`599`	`593`
`600`	`594`	`foreach (var scriptDir in Directory.EnumerateDirectories(config.RootScriptPath))`
`601`	`595`	`{`
`@@ -629,7 +623,7 @@ public static Collection<FunctionMetadata> ReadFunctionMetadata(ScriptHostConfig`
`629`	`623`	`string functionError = null;`
`630`	`624`	`FunctionMetadata functionMetadata = null;`
`631`	`625`	`var mappedHttpFunctions = new Dictionary<string, HttpTriggerBindingMetadata>();`
`632`		`- if (!TryParseFunctionMetadata(functionName, functionConfig, mappedHttpFunctions, traceWriter, scriptDir, out functionMetadata, out functionError))`
	`626`	`+ if (!TryParseFunctionMetadata(functionName, functionConfig, mappedHttpFunctions, traceWriter, scriptDir, settingsManager, out functionMetadata, out functionError))`
`633`	`627`	`{`
`634`	`628`	`// for functions in error, log the error and don't`
`635`	`629`	`// add to the functions collection`
`@@ -652,12 +646,12 @@ public static Collection<FunctionMetadata> ReadFunctionMetadata(ScriptHostConfig`
`652`	`646`	`}`
`653`	`647`
`654`	`648`	`internal static bool TryParseFunctionMetadata(string functionName, JObject functionConfig, Dictionary<string, HttpTriggerBindingMetadata> mappedHttpFunctions,`
`655`		`- TraceWriter traceWriter, string scriptDirectory, out FunctionMetadata functionMetadata, out string error, IFileSystem fileSystem = null)`
	`649`	`+ TraceWriter traceWriter, string scriptDirectory, ScriptSettingsManager settingsManager, out FunctionMetadata functionMetadata, out string error, IFileSystem fileSystem = null)`
`656`	`650`	`{`
`657`	`651`	`fileSystem = fileSystem ?? new FileSystem();`
`658`	`652`
`659`	`653`	`error = null;`
`660`		`- functionMetadata = ParseFunctionMetadata(functionName, functionConfig);`
	`654`	`+ functionMetadata = ParseFunctionMetadata(functionName, functionConfig, settingsManager);`
`661`	`655`
`662`	`656`	`if (functionMetadata.IsExcluded)`
`663`	`657`	`{`
`@@ -839,7 +833,7 @@ private static ScriptType ParseScriptType(string scriptFilePath)`
`839`	`833`
`840`	`834`	`private Collection<FunctionDescriptor> GetFunctionDescriptors()`
`841`	`835`	`{`
`842`		`- var functions = ReadFunctionMetadata(ScriptConfig, TraceWriter, FunctionErrors);`
	`836`	`+ var functions = ReadFunctionMetadata(ScriptConfig, TraceWriter, FunctionErrors, _settingsManager);`
`843`	`837`
`844`	`838`	`var descriptorProviders = new List<FunctionDescriptorProvider>()`
`845`	`839`	`{`
`@@ -1155,7 +1149,7 @@ internal static string GetRelativeDirectory(string path, string scriptRoot)`
`1155`	`1149`	`return string.Empty;`
`1156`	`1150`	`}`
`1157`	`1151`
`1158`		`- private static bool IsDisabled(JToken isDisabledValue)`
	`1152`	`+ private static bool IsDisabled(JToken isDisabledValue, ScriptSettingsManager settingsManager)`
`1159`	`1153`	`{`
`1160`	`1154`	`if (isDisabledValue != null)`
`1161`	`1155`	`{`
`@@ -1166,7 +1160,7 @@ private static bool IsDisabled(JToken isDisabledValue)`
`1166`	`1160`	`else`
`1167`	`1161`	`{`
`1168`	`1162`	`string settingName = (string)isDisabledValue;`
`1169`		`- string value = _settingsManager.GetSetting(settingName);`
	`1163`	`+ string value = settingsManager.GetSetting(settingName);`
`1170`	`1164`	`if (!string.IsNullOrEmpty(value) &&`
`1171`	`1165`	`(string.Compare(value, "1", StringComparison.OrdinalIgnoreCase) == 0 \|\|`
`1172`	`1166`	`string.Compare(value, "true", StringComparison.OrdinalIgnoreCase) == 0))`