Merge pull request #881 from tylergibbs2/detailed-manifest-errors

Add detailed manifest validation errors at build time

4 files changed, 155 insertions, 102 deletions

diff --git a/src/SMAPI.ModBuildConfig/DeployModTask.cs b/src/SMAPI.ModBuildConfig/DeployModTask.cs
index 88412d92..3508a6db 100644
--- a/src/SMAPI.ModBuildConfig/DeployModTask.cs
+++ b/src/SMAPI.ModBuildConfig/DeployModTask.cs
@@ -7,7 +7,11 @@ using System.Reflection;
 using System.Text.RegularExpressions;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Utilities;
+using Newtonsoft.Json;
 using StardewModdingAPI.ModBuildConfig.Framework;
+using StardewModdingAPI.Toolkit.Framework;
+using StardewModdingAPI.Toolkit.Serialization;
+using StardewModdingAPI.Toolkit.Serialization.Models;
 using StardewModdingAPI.Toolkit.Utilities;
 
 namespace StardewModdingAPI.ModBuildConfig
@@ -75,9 +79,41 @@ namespace StardewModdingAPI.ModBuildConfig
                 this.Log.LogMessage(MessageImportance.High, $"[mod build package] Handling build with options {string.Join(", ", properties)}");
             }
 
+            // skip if nothing to do
+            // (This must be checked before the manifest validation, to allow cases like unit test projects.)
             if (!this.EnableModDeploy && !this.EnableModZip)
-                return true; // nothing to do
+                return true;
+
+            // validate the manifest file
+            IManifest manifest;
+            {
+                try
+                {
+                    string manifestPath = Path.Combine(this.ProjectDir, "manifest.json");
+                    if (!new JsonHelper().ReadJsonFileIfExists(manifestPath, out Manifest rawManifest))
+                    {
+                        this.Log.LogError("[mod build package] The mod's manifest.json file doesn't exist.");
+                        return false;
+                    }
+                    manifest = rawManifest;
+                }
+                catch (JsonReaderException ex)
+                {
+                    // log the inner exception, otherwise the message will be generic
+                    Exception exToShow = ex.InnerException ?? ex;
+                    this.Log.LogError($"[mod build package] The mod's manifest.json file isn't valid JSON: {exToShow.Message}");
+                    return false;
+                }
+
+                // validate manifest fields
+                if (!ManifestValidator.TryValidateFields(manifest, out string error))
+                {
+                    this.Log.LogError($"[mod build package] The mod's manifest.json file is invalid: {error}");
+                    return false;
+                }
+            }
 
+            // deploy files
             try
             {
                 // parse extra DLLs to bundle
@@ -101,7 +137,7 @@ namespace StardewModdingAPI.ModBuildConfig
                 // create release zip
                 if (this.EnableModZip)
                 {
-                    string zipName = this.EscapeInvalidFilenameCharacters($"{this.ModFolderName} {package.GetManifestVersion()}.zip");
+                    string zipName = this.EscapeInvalidFilenameCharacters($"{this.ModFolderName} {manifest.Version}.zip");
                     string zipPath = Path.Combine(this.ModZipPath, zipName);
 
                     this.Log.LogMessage(MessageImportance.High, $"[mod build package] Generating the release zip at {zipPath}...");
diff --git a/src/SMAPI.ModBuildConfig/Framework/ModFileManager.cs b/src/SMAPI.ModBuildConfig/Framework/ModFileManager.cs
index 80955f67..00f3f439 100644
--- a/src/SMAPI.ModBuildConfig/Framework/ModFileManager.cs
+++ b/src/SMAPI.ModBuildConfig/Framework/ModFileManager.cs
@@ -3,8 +3,6 @@ using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 using System.Text.RegularExpressions;
-using StardewModdingAPI.Toolkit.Serialization;
-using StardewModdingAPI.Toolkit.Serialization.Models;
 using StardewModdingAPI.Toolkit.Utilities;
 
 namespace StardewModdingAPI.ModBuildConfig.Framework
@@ -113,16 +111,6 @@ namespace StardewModdingAPI.ModBuildConfig.Framework
             return new Dictionary<string, FileInfo>(this.Files, StringComparer.OrdinalIgnoreCase);
         }
 
-        /// <summary>Get a semantic version from the mod manifest.</summary>
-        /// <exception cref="UserErrorException">The manifest is missing or invalid.</exception>
-        public string GetManifestVersion()
-        {
-            if (!this.Files.TryGetValue(this.ManifestFileName, out FileInfo manifestFile) || !new JsonHelper().ReadJsonFileIfExists(manifestFile.FullName, out Manifest manifest))
-                throw new InvalidOperationException($"The mod does not have a {this.ManifestFileName} file."); // shouldn't happen since we validate in constructor
-
-            return manifest.Version.ToString();
-        }
-
 
         /*********
         ** Private methods
diff --git a/src/SMAPI.Toolkit/Framework/ManifestValidator.cs b/src/SMAPI.Toolkit/Framework/ManifestValidator.cs
new file mode 100644
index 00000000..461dc325
--- /dev/null
+++ b/src/SMAPI.Toolkit/Framework/ManifestValidator.cs
@@ -0,0 +1,106 @@
+using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
+using System.IO;
+using System.Linq;
+using StardewModdingAPI.Toolkit.Utilities;
+
+namespace StardewModdingAPI.Toolkit.Framework
+{
+    /// <summary>Validates manifest fields.</summary>
+    public static class ManifestValidator
+    {
+        /// <summary>Validate a manifest's fields.</summary>
+        /// <param name="manifest">The manifest to validate.</param>
+        /// <param name="error">The error message indicating why validation failed, if applicable.</param>
+        /// <returns>Returns whether all manifest fields validated successfully.</returns>
+        [SuppressMessage("ReSharper", "ConditionIsAlwaysTrueOrFalseAccordingToNullableAPIContract", Justification = "This is the method that ensures those annotations are respected.")]
+        public static bool TryValidateFields(IManifest manifest, out string error)
+        {
+            //
+            // Note: SMAPI assumes that it can grammatically append the returned sentence in the
+            // form "failed loading <mod> because its <error>". Any errors returned should be valid
+            // in that format, unless the SMAPI call is adjusted accordingly.
+            //
+
+            bool hasDll = !string.IsNullOrWhiteSpace(manifest.EntryDll);
+            bool isContentPack = manifest.ContentPackFor != null;
+
+            // validate use of EntryDll vs ContentPackFor fields
+            if (hasDll == isContentPack)
+            {
+                error = hasDll
+                    ? $"manifest sets both {nameof(IManifest.EntryDll)} and {nameof(IManifest.ContentPackFor)}, which are mutually exclusive."
+                    : $"manifest has no {nameof(IManifest.EntryDll)} or {nameof(IManifest.ContentPackFor)} field; must specify one.";
+                return false;
+            }
+
+            // validate EntryDll/ContentPackFor format
+            if (hasDll)
+            {
+                if (manifest.EntryDll!.Intersect(Path.GetInvalidFileNameChars()).Any())
+                {
+                    error = $"manifest has invalid filename '{manifest.EntryDll}' for the {nameof(IManifest.EntryDll)} field.";
+                    return false;
+                }
+            }
+            else
+            {
+                if (string.IsNullOrWhiteSpace(manifest.ContentPackFor!.UniqueID))
+                {
+                    error = $"manifest declares {nameof(IManifest.ContentPackFor)} without its required {nameof(IManifestContentPackFor.UniqueID)} field.";
+                    return false;
+                }
+            }
+
+            // validate required fields
+            {
+                List<string> missingFields = new List<string>(3);
+
+                if (string.IsNullOrWhiteSpace(manifest.Name))
+                    missingFields.Add(nameof(IManifest.Name));
+                if (manifest.Version == null || manifest.Version.ToString() == "0.0.0")
+                    missingFields.Add(nameof(IManifest.Version));
+                if (string.IsNullOrWhiteSpace(manifest.UniqueID))
+                    missingFields.Add(nameof(IManifest.UniqueID));
+
+                if (missingFields.Any())
+                {
+                    error = $"manifest is missing required fields ({string.Join(", ", missingFields)}).";
+                    return false;
+                }
+            }
+
+            // validate ID format
+            if (!PathUtilities.IsSlug(manifest.UniqueID))
+            {
+                error = "manifest specifies an invalid ID (IDs must only contain letters, numbers, underscores, periods, or hyphens).";
+                return false;
+            }
+
+            // validate dependency format
+            foreach (IManifestDependency? dependency in manifest.Dependencies)
+            {
+                if (dependency == null)
+                {
+                    error = $"manifest has a null entry under {nameof(IManifest.Dependencies)}.";
+                    return false;
+                }
+
+                if (string.IsNullOrWhiteSpace(dependency.UniqueID))
+                {
+                    error = $"manifest has a {nameof(IManifest.Dependencies)} entry with no {nameof(IManifestDependency.UniqueID)} field.";
+                    return false;
+                }
+
+                if (!PathUtilities.IsSlug(dependency.UniqueID))
+                {
+                    error = $"manifest has a {nameof(IManifest.Dependencies)} entry with an invalid {nameof(IManifestDependency.UniqueID)} field (IDs must only contain letters, numbers, underscores, periods, or hyphens).";
+                    return false;
+                }
+            }
+
+            error = "";
+            return true;
+        }
+    }
+}
diff --git a/src/SMAPI/Framework/ModLoading/ModResolver.cs b/src/SMAPI/Framework/ModLoading/ModResolver.cs
index fe56f4d2..9db9db99 100644
--- a/src/SMAPI/Framework/ModLoading/ModResolver.cs
+++ b/src/SMAPI/Framework/ModLoading/ModResolver.cs
@@ -4,11 +4,11 @@ using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Linq;
 using StardewModdingAPI.Toolkit;
+using StardewModdingAPI.Toolkit.Framework;
 using StardewModdingAPI.Toolkit.Framework.ModData;
 using StardewModdingAPI.Toolkit.Framework.ModScanning;
 using StardewModdingAPI.Toolkit.Framework.UpdateData;
 using StardewModdingAPI.Toolkit.Serialization.Models;
-using StardewModdingAPI.Toolkit.Utilities;
 using StardewModdingAPI.Toolkit.Utilities.PathLookups;
 
 namespace StardewModdingAPI.Framework.ModLoading
@@ -126,100 +126,23 @@ namespace StardewModdingAPI.Framework.ModLoading
                     continue;
                 }
 
-                // validate DLL / content pack fields
+                // validate manifest format
+                if (!ManifestValidator.TryValidateFields(mod.Manifest, out string manifestError))
                 {
-                    bool hasDll = !string.IsNullOrWhiteSpace(mod.Manifest.EntryDll);
-                    bool isContentPack = mod.Manifest.ContentPackFor != null;
-
-                    // validate field presence
-                    if (!hasDll && !isContentPack)
-                    {
-                        mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its manifest has no {nameof(IManifest.EntryDll)} or {nameof(IManifest.ContentPackFor)} field; must specify one.");
-                        continue;
-                    }
-                    if (hasDll && isContentPack)
-                    {
-                        mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its manifest sets both {nameof(IManifest.EntryDll)} and {nameof(IManifest.ContentPackFor)}, which are mutually exclusive.");
-                        continue;
-                    }
-
-                    // validate DLL
-                    if (hasDll)
-                    {
-                        // invalid filename format
-                        if (mod.Manifest.EntryDll!.Intersect(Path.GetInvalidFileNameChars()).Any())
-                        {
-                            mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its manifest has invalid filename '{mod.Manifest.EntryDll}' for the EntryDLL field.");
-                            continue;
-                        }
-
-                        // file doesn't exist
-                        if (validateFilesExist)
-                        {
-                            IFileLookup pathLookup = getFileLookup(mod.DirectoryPath);
-                            FileInfo file = pathLookup.GetFile(mod.Manifest.EntryDll!);
-                            if (!file.Exists)
-                            {
-                                mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its DLL '{mod.Manifest.EntryDll}' doesn't exist.");
-                                continue;
-                            }
-                        }
-                    }
-
-                    // validate content pack
-                    else
-                    {
-                        // invalid content pack ID
-                        if (string.IsNullOrWhiteSpace(mod.Manifest.ContentPackFor!.UniqueID))
-                        {
-                            mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its manifest declares {nameof(IManifest.ContentPackFor)} without its required {nameof(IManifestContentPackFor.UniqueID)} field.");
-                            continue;
-                        }
-                    }
-                }
-
-                // validate required fields
-                {
-                    List<string> missingFields = new List<string>(3);
-
-                    if (string.IsNullOrWhiteSpace(mod.Manifest.Name))
-                        missingFields.Add(nameof(IManifest.Name));
-                    if (mod.Manifest.Version == null || mod.Manifest.Version.ToString() == "0.0.0")
-                        missingFields.Add(nameof(IManifest.Version));
-                    if (string.IsNullOrWhiteSpace(mod.Manifest.UniqueID))
-                        missingFields.Add(nameof(IManifest.UniqueID));
-
-                    if (missingFields.Any())
-                    {
-                        mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its manifest is missing required fields ({string.Join(", ", missingFields)}).");
-                        continue;
-                    }
+                    mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its {manifestError}");
+                    continue;
                 }
 
-                // validate ID format
-                if (!PathUtilities.IsSlug(mod.Manifest.UniqueID))
-                    mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, "its manifest specifies an invalid ID (IDs must only contain letters, numbers, underscores, periods, or hyphens).");
-
-                // validate dependencies
-                foreach (IManifestDependency? dependency in mod.Manifest.Dependencies)
+                // check that DLL exists if applicable
+                if (!string.IsNullOrEmpty(mod.Manifest.EntryDll) && validateFilesExist)
                 {
-                    // null dependency
-                    if (dependency == null)
+                    IFileLookup pathLookup = getFileLookup(mod.DirectoryPath);
+                    FileInfo file = pathLookup.GetFile(mod.Manifest.EntryDll!);
+                    if (!file.Exists)
                     {
-                        mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its manifest has a null entry under {nameof(IManifest.Dependencies)}.");
+                        mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its DLL '{mod.Manifest.EntryDll}' doesn't exist.");
                         continue;
                     }
-
-                    // missing ID
-                    if (string.IsNullOrWhiteSpace(dependency.UniqueID))
-                    {
-                        mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its manifest has a {nameof(IManifest.Dependencies)} entry with no {nameof(IManifestDependency.UniqueID)} field.");
-                        continue;
-                    }
-
-                    // invalid ID
-                    if (!PathUtilities.IsSlug(dependency.UniqueID))
-                        mod.SetStatus(ModMetadataStatus.Failed, ModFailReason.InvalidManifest, $"its manifest has a {nameof(IManifest.Dependencies)} entry with an invalid {nameof(IManifestDependency.UniqueID)} field (IDs must only contain letters, numbers, underscores, periods, or hyphens).");
                 }
             }