Merge pull request #716 from Cyberboss/715-FixCustomCommands

Fix custom chat commands being displayed twice in bot help text

README.md

@@ -57,15 +57,15 @@ Note although `/app/lib` is specified as a volume mount point in the `Dockerfile
 
 ### Configuring
 
-Create an `appsettings.Production.json` file next to `appsettings.json`. This will override the default settings in appsettings.json with your production settings. There are a few keys meant to be changed by hosts: 
-
-- `General:LogFileDirectory`: Override the default directory where server logs are stored. Default is C:/ProgramData/tgstation-server/logs on Windows, /usr/share/tgstation-server/logs otherwise
+Create an `appsettings.Production.json` file next to `appsettings.json`. This will override the default settings in appsettings.json with your production settings. There are a few keys meant to be changed by hosts. Note these are all case-sensitive: 
 
 - `General:MinimumPasswordLength`: Minimum password length requirement for database users
 
 - `General:GitHubAccessToken`: Specify a GitHub personal access token with no scopes here to highly mitigate the possiblity of 429 response codes from GitHub requests
 
-- `General:LogFileLevel`: Can be one of `Trace`, `Debug`, `Information`, `Warning`, `Error`, or `Critical`. Restricts what is put into the log files. Currently `Debug` is reccommended for help with error reporting.
+- `FileLogging:Directory`: Override the default directory where server logs are stored. Default is C:/ProgramData/tgstation-server/logs on Windows, /usr/share/tgstation-server/logs otherwise
+
+- `FileLogging:LogLevel`: Can be one of `Trace`, `Debug`, `Information`, `Warning`, `Error`, or `Critical`. Restricts what is put into the log files. Currently `Debug` is reccommended for help with error reporting.
 
 - `Kestrel:Endpoints:Http:Url`: The URL (i.e. interface and ports) your application should listen on. General use case should be `http://localhost:<port>` for restricted local connections. See the Remote Access section for configuring public access to the World Wide Web. This doesn't need to be changed using the docker setup and should be mapped with the `-p` option instead
 

src/Tgstation.Server.Host/Components/Chat/Chat.cs

@@ -39,6 +39,11 @@ namespace Tgstation.Server.Host.Components.Chat
 		/// </summary>
 		readonly IRestartRegistration restartRegistration;
 
+		/// <summary>
+		/// The <see cref="ILoggerFactory"/> for the <see cref="Chat"/>
+		/// </summary>
+		readonly ILoggerFactory loggerFactory;
+
 		/// <summary>
 		/// The <see cref="ILogger"/> for the <see cref="Chat"/>
 		/// </summary>
@@ -104,17 +109,19 @@ namespace Tgstation.Server.Host.Components.Chat
 		/// </summary>
 		/// <param name="providerFactory">The value of <see cref="providerFactory"/></param>
 		/// <param name="ioManager">The value of <see cref="ioManager"/></param>
+		/// <param name="loggerFactory">The value of <see cref="loggerFactory"/></param>
 		/// <param name="logger">The value of <see cref="logger"/></param>
 		/// <param name="commandFactory">The value of <see cref="commandFactory"/></param>
 		/// <param name="serverControl">The <see cref="IServerControl"/> to populate <see cref="restartRegistration"/> with</param>
 		/// <param name="initialChatBots">The <see cref="IEnumerable{T}"/> used to populate <see cref="initialChatBots"/></param>
-		public Chat(IProviderFactory providerFactory, IIOManager ioManager, ICommandFactory commandFactory, IServerControl serverControl, ILogger<Chat> logger, IEnumerable<Models.ChatBot> initialChatBots)
+		public Chat(IProviderFactory providerFactory, IIOManager ioManager, ICommandFactory commandFactory, IServerControl serverControl, ILoggerFactory loggerFactory, ILogger<Chat> logger, IEnumerable<Models.ChatBot> initialChatBots)
 		{
 			this.providerFactory = providerFactory ?? throw new ArgumentNullException(nameof(providerFactory));
 			this.ioManager = ioManager ?? throw new ArgumentNullException(nameof(ioManager));
 			this.commandFactory = commandFactory ?? throw new ArgumentNullException(nameof(commandFactory));
 			if (serverControl == null)
 				throw new ArgumentNullException(nameof(serverControl));
+			this.loggerFactory = loggerFactory ?? throw new ArgumentNullException(nameof(loggerFactory));
 			this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
 			this.initialChatBots = initialChatBots?.ToList() ?? throw new ArgumentNullException(nameof(initialChatBots));
 
@@ -263,7 +270,7 @@ namespace Tgstation.Server.Host.Components.Chat
 					if (splits.Count == 0)
 					{
 						var allCommands = builtinCommands.Select(x => x.Value).ToList();
-						var tasks = trackingContexts.Select(x => x.GetCustomCommands(cancellationToken));
+						var tasks = trackingContexts.Select(x => x.GetCustomCommands(cancellationToken)).ToList();
 						await Task.WhenAll(tasks).ConfigureAwait(false);
 						allCommands.AddRange(tasks.SelectMany(x => x.Result));
 						helpText = String.Format(CultureInfo.InvariantCulture, "Available commands (Type '?' or 'help' and then a command name for more details): {0}", String.Join(", ", allCommands.Select(x => x.Name)));
@@ -529,7 +536,7 @@ namespace Tgstation.Server.Host.Components.Chat
 			if (customCommandHandler == null)
 				throw new InvalidOperationException("RegisterCommandHandler() hasn't been called!");
 			JsonTrackingContext context = null;
-			context = new JsonTrackingContext(ioManager, customCommandHandler, () =>
+			context = new JsonTrackingContext(ioManager, customCommandHandler, loggerFactory.CreateLogger<JsonTrackingContext>(), () =>
 			{
 				lock (trackingContexts)
 					trackingContexts.Remove(context);

src/Tgstation.Server.Host/Components/Chat/ChatFactory.cs

@@ -39,6 +39,6 @@ namespace Tgstation.Server.Host.Components.Chat
 		}
 
 		/// <inheritdoc />
-		public IChat CreateChat(IIOManager ioManager, ICommandFactory commandFactory, IEnumerable<Models.ChatBot> initialChatBots) => new Chat(providerFactory, ioManager, commandFactory, serverControl, loggerFactory.CreateLogger<Chat>(), initialChatBots);
+		public IChat CreateChat(IIOManager ioManager, ICommandFactory commandFactory, IEnumerable<Models.ChatBot> initialChatBots) => new Chat(providerFactory, ioManager, commandFactory, serverControl, loggerFactory, loggerFactory.CreateLogger<Chat>(), initialChatBots);
 	}
 }

src/Tgstation.Server.Host/Components/Chat/IJsonTrackingContext.cs

@@ -11,6 +11,11 @@ namespace Tgstation.Server.Host.Components.Chat
 	/// </summary>
 	public interface IJsonTrackingContext : IDisposable
 	{
+		/// <summary>
+		/// If the <see cref="IJsonTrackingContext"/> should be used for <see cref="GetCustomCommands(CancellationToken)"/>
+		/// </summary>
+		bool Active { get; set; }
+
 		/// <summary>
 		/// Read <see cref="CustomCommand"/>s from the <see cref="IJsonTrackingContext"/>
 		/// </summary>

src/Tgstation.Server.Host/Components/Chat/JsonTrackingContext.cs

@@ -1,4 +1,5 @@
-using Newtonsoft.Json;
+using Microsoft.Extensions.Logging;
+using Newtonsoft.Json;
 using Newtonsoft.Json.Serialization;
 using System;
 using System.Collections.Generic;
@@ -14,8 +15,20 @@ namespace Tgstation.Server.Host.Components.Chat
 	/// <inheritdoc />
 	sealed class JsonTrackingContext : IJsonTrackingContext
 	{
+		/// <inheritdoc />
+		public bool Active
+		{
+			get => active;
+			set
+			{
+				active = true;
+				logger.LogDebug("Tracking {0}activated", !active ? "de" : String.Empty);
+			}
+		}
+
 		readonly IIOManager ioManager;
 		readonly ICustomCommandHandler customCommandHandler;
+		readonly ILogger<JsonTrackingContext> logger;
 		readonly Action onDispose;
 
 		readonly string commandsPath;
@@ -23,52 +36,71 @@ namespace Tgstation.Server.Host.Components.Chat
 
 		readonly SemaphoreSlim channelsSemaphore;
 
-		public JsonTrackingContext(IIOManager ioManager, ICustomCommandHandler customCommandHandler, Action onDispose, string commandsPath, string channelsPath)
+		bool active;
+
+		public JsonTrackingContext(IIOManager ioManager, ICustomCommandHandler customCommandHandler, ILogger<JsonTrackingContext> logger, Action onDispose, string commandsPath, string channelsPath)
 		{
 			this.ioManager = ioManager ?? throw new ArgumentNullException(nameof(ioManager));
 			this.customCommandHandler = customCommandHandler ?? throw new ArgumentNullException(nameof(customCommandHandler));
+			this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
 			this.onDispose = onDispose ?? throw new ArgumentNullException(nameof(onDispose));
 			this.commandsPath = commandsPath ?? throw new ArgumentNullException(nameof(commandsPath));
 			this.channelsPath = channelsPath ?? throw new ArgumentNullException(nameof(channelsPath));
 
 			channelsSemaphore = new SemaphoreSlim(1);
+			active = false;
+
+			logger.LogTrace("Created tracking context for {0} and {1}", commandsPath, channelsPath);
 		}
 
 		/// <inheritdoc />
-		public void Dispose() => onDispose();
+		public void Dispose()
+		{
+			logger.LogTrace("Disposing...");
+			onDispose();
+		}
 
 		/// <inheritdoc />
 		public async Task<IReadOnlyList<CustomCommand>> GetCustomCommands(CancellationToken cancellationToken)
 		{
 			try
 			{
-				var resultBytes = await ioManager.ReadAllBytes(commandsPath, cancellationToken).ConfigureAwait(false);
-				var resultJson = Encoding.UTF8.GetString(resultBytes);
-				var result = JsonConvert.DeserializeObject<List<CustomCommand>>(resultJson, new JsonSerializerSettings
+				if (Active && await ioManager.FileExists(commandsPath, cancellationToken).ConfigureAwait(false))
 				{
-					ContractResolver = new DefaultContractResolver
+					var resultBytes = await ioManager.ReadAllBytes(commandsPath, cancellationToken).ConfigureAwait(false);
+					var resultJson = Encoding.UTF8.GetString(resultBytes);
+					logger.LogTrace("Read commands JSON: {0}", resultJson);
+					var result = JsonConvert.DeserializeObject<List<CustomCommand>>(resultJson, new JsonSerializerSettings
 					{
-						NamingStrategy = new SnakeCaseNamingStrategy()
-					}
-				});
-				foreach (var I in result)
-					I.SetHandler(customCommandHandler);
-				return result;
+						ContractResolver = new DefaultContractResolver
+						{
+							NamingStrategy = new SnakeCaseNamingStrategy()
+						}
+					});
+					foreach (var I in result)
+						I.SetHandler(customCommandHandler);
+					return result;
+				}
 			}
-			catch
+			catch (Exception e)
 			{
-				return new List<CustomCommand>();
+				logger.LogWarning("Error retrieving custom commands! Exception: {0}", e);
 			}
+			return new List<CustomCommand>();
 		}
 
 		/// <inheritdoc />
 		public async Task SetChannels(IEnumerable<Channel> channels, CancellationToken cancellationToken)
 		{
 			using (await SemaphoreSlimContext.Lock(channelsSemaphore, cancellationToken).ConfigureAwait(false))
-				await ioManager.WriteAllBytes(channelsPath, Encoding.UTF8.GetBytes(JsonConvert.SerializeObject(channels, Formatting.Indented, new JsonSerializerSettings
+			{
+				var json = JsonConvert.SerializeObject(channels, new JsonSerializerSettings
 				{
 					ContractResolver = new CamelCasePropertyNamesContractResolver()
-				})), cancellationToken).ConfigureAwait(false);
+				});
+				logger.LogTrace("Writing channels JSON: {0}", json);
+				await ioManager.WriteAllBytes(channelsPath, Encoding.UTF8.GetBytes(json), cancellationToken).ConfigureAwait(false);
+			}
 		}
 	}
 }

src/Tgstation.Server.Host/Components/Watchdog/ISessionController.cs

@@ -88,5 +88,10 @@ namespace Tgstation.Server.Host.Components.Watchdog
 		/// Changes <see cref="RebootState"/> to <see cref="Components.Watchdog.RebootState.Normal"/> without telling the DMAPI
 		/// </summary>
 		void ResetRebootState();
+
+		/// <summary>
+		/// Enables the reading of custom chat commands from the <see cref="ISessionController"/>
+		/// </summary>
+		void EnableCustomChatCommands();
 	}
 }

src/Tgstation.Server.Host/Components/Watchdog/SessionController.cs

@@ -200,6 +200,8 @@ namespace Tgstation.Server.Host.Components.Watchdog
 
 			rebootTcs = new TaskCompletionSource<object>();
 
+			process.Lifetime.ContinueWith(x => chatJsonTrackingContext.Active = false, TaskScheduler.Current);
+
 			async Task<LaunchResult> GetLaunchResult()
 			{
 				var startTime = DateTimeOffset.Now;
@@ -353,6 +355,7 @@ namespace Tgstation.Server.Host.Components.Watchdog
 					case Constants.DMCommandWorldReboot:
 						if (ClosePortOnReboot)
 						{
+							chatJsonTrackingContext.Active = false;
 							content = new Dictionary<string, int> { { Constants.DMParameterData, 0 } };
 							portClosedForReboot = true;
 						}
@@ -372,7 +375,7 @@ namespace Tgstation.Server.Host.Components.Watchdog
 			var response = await SendCommand(String.Format(CultureInfo.InvariantCulture, "{0}&{1}={2}", byondTopicSender.SanitizeString(Constants.DMTopicInteropResponse), byondTopicSender.SanitizeString(Constants.DMParameterData), byondTopicSender.SanitizeString(json)), overrideResponsePort, cancellationToken).ConfigureAwait(false);
 
 			if (response != Constants.DMResponseSuccess)
-				logger.LogWarning("Recieved error response while responding to interop: {0}", response);
+				logger.LogWarning("Received error response while responding to interop: {0}", response);
 
 			postRespond?.Invoke();
 		}
@@ -386,6 +389,9 @@ namespace Tgstation.Server.Host.Components.Watchdog
 				throw new ObjectDisposedException(nameof(SessionController));
 		}
 
+		/// <inheritdoc />
+		public void EnableCustomChatCommands() => chatJsonTrackingContext.Active = true;
+
 		/// <inheritdoc />
 		public ReattachInformation Release()
 		{

src/Tgstation.Server.Host/Components/Watchdog/SessionControllerFactory.cs

@@ -175,7 +175,7 @@ namespace Tgstation.Server.Host.Components.Watchdog
 
 			var interopJsonFile = JsonFile("interop");
 
-			var interopJson = JsonConvert.SerializeObject(interopInfo, Formatting.Indented, new JsonSerializerSettings
+			var interopJson = JsonConvert.SerializeObject(interopInfo, new JsonSerializerSettings
 			{
 				ContractResolver = new CamelCasePropertyNamesContractResolver(),
 				ReferenceLoopHandling = ReferenceLoopHandling.Ignore

src/Tgstation.Server.Host/Components/Watchdog/Watchdog.cs

@@ -275,6 +275,7 @@ namespace Tgstation.Server.Host.Components.Watchdog
 				monitorState.ActiveServer = monitorState.InactiveServer;
 				monitorState.InactiveServer = tmp;
 				AlphaIsActive = !AlphaIsActive;
+				monitorState.ActiveServer.EnableCustomChatCommands();
 				return true;
 			}
 
@@ -598,10 +599,11 @@ namespace Tgstation.Server.Host.Components.Watchdog
 				}
 				catch (Exception e)
 				{
-					logger.LogError("Monitor crashed! Iteration: {0}, State: {1}", iteration, JsonConvert.SerializeObject(monitorState));
+					logger.LogError("Monitor crashed! Iteration: {0}, State: {1}, Exception: {2}", iteration, JsonConvert.SerializeObject(monitorState), e);
 					await chat.SendWatchdogMessage(String.Format(CultureInfo.InvariantCulture, "Monitor crashed, this should NEVER happen! Please report this, full details in logs! Restarting monitor... Error: {0}", e.Message), cancellationToken).ConfigureAwait(false);
 				}
 			}
+			logger.LogTrace("Monitor exiting...");
 		}
 
 		async Task<bool> StopMonitor()
@@ -716,7 +718,10 @@ namespace Tgstation.Server.Host.Components.Watchdog
 						//both servers are now running, alpha is the active server(unless reattach), huzzah
 						AlphaIsActive = doReattach ? reattachInfo?.AlphaIsActive ?? true : true;
 						LastLaunchResult = alphaLrt.Result;
-						(AlphaIsActive ? alphaServer : bravoServer).ClosePortOnReboot = true;
+
+						var activeServer = AlphaIsActive ? alphaServer : bravoServer;
+						activeServer.EnableCustomChatCommands();
+						activeServer.ClosePortOnReboot = true;
 
 						logger.LogInformation("Launched servers successfully");
 						Running = true;

src/Tgstation.Server.Host/Core/Application.cs

@@ -201,15 +201,13 @@ namespace Tgstation.Server.Host.Core
 
 			var databaseConfiguration = databaseConfigurationSection.Get<DatabaseConfiguration>();
 
-			void ConfigureDatabase(DbContextOptionsBuilder builder)
-			{
-				if (hostingEnvironment.IsDevelopment())
-					builder.EnableSensitiveDataLogging();
-			};
-
 			void AddTypedContext<TContext>() where TContext : DatabaseContext<TContext>
 			{
-				services.AddDbContext<TContext>(ConfigureDatabase);
+				services.AddDbContext<TContext>(builder =>
+				{
+					if (hostingEnvironment.IsDevelopment())
+						builder.EnableSensitiveDataLogging();
+				});
 				services.AddScoped<IDatabaseContext>(x => x.GetRequiredService<TContext>());
 			}
 
@@ -297,6 +295,8 @@ namespace Tgstation.Server.Host.Core
 				throw new ArgumentNullException(nameof(applicationBuilder));
 			if (logger == null)
 				throw new ArgumentNullException(nameof(logger));
+			if (serverControl == null)
+				throw new ArgumentNullException(nameof(serverControl));
 
 			logger.LogInformation(VersionString);