Merge pull request #133 from PassivePicasso/path-resolution-safety

Path Components, EnvironmentVariable, EnvironmentSpecialFolder, Tests, Safety

Author: PassivePicasso

CHANGELOG.md

@@ -14,6 +14,16 @@
     which carries no version resource outside Windows
   * The **Installed Unity Games** window reports these games' Unity version instead of
     falling back to scanning file headers
+* Two `PathComponent`s resolve per-machine locations so a shared PathReference no longer
+  has to be hand-edited per collaborator —
+  [SpecialFolder](Editor/Core/Paths/Components/SpecialFolder.cs) and
+  [EnvironmentVariable](Editor/Core/Paths/Components/EnvironmentVariable.cs)
+  * `SpecialFolder` selects an `Environment.SpecialFolder`, so `ApplicationData` resolves
+    to `%APPDATA%`, `~/.config` or `~/Library/Application Support` from one asset
+  * `EnvironmentVariable` looks a variable up by name with an optional fallback, so an
+    unset variable is reported rather than becoming a literal path segment
+  * Neither parses `%VAR%` or `$VAR`, so the same asset resolves on Windows, Linux and
+    macOS
 
 ### Fixes
 
@@ -35,6 +45,20 @@
     executor sub-assets, preserving each executor's enabled state
   * Equal-priority executors are ordered by type name, so the saved order no longer
     depends on Unity's sub-asset ordering
+* Path resolution reports authoring mistakes instead of failing opaquely — new
+  [PathAssembler](Editor/Core/Paths/PathAssembler.cs) and
+  [PathResolutionScope](Editor/Core/Paths/PathResolutionScope.cs) sit behind
+  `PathReference.GetPath`
+  * A cycle through `OutputReference` or a `Resolver` token names the chain that caused
+    it, rather than recursing until `StackOverflowException` terminates the Editor
+  * Null, invalid, drive-relative and misplaced rooted segments are refused and name the
+    `PathComponent` that produced them, so a segment can no longer silently discard the
+    components before it
+  * Two `PathReference` assets sharing a name report both assets instead of surfacing as
+    a dictionary key collision
+* [PathReference](Editor/Core/Paths/PathReference.cs) `ElementTemplate` scaffolds
+  `GetPathInternal`, so generated `PathComponent`s compile — it previously declared an
+  override of the non-virtual `GetPath`
 
 ### Tests
 
@@ -47,6 +71,20 @@
 * Added [PlayerDataResolverTests](Tests/Editor/PlayerDataResolverTests.cs) covering player
   layout selection and the bundle branch against a synthesized UnityFS bundle, so the
   compressed layout is exercised without a multi-megabyte fixture
+* Added coverage for the path component system across
+  [PathComponentTests](Tests/Editor/PathComponentTests.cs),
+  [PathComponentFileSystemTests](Tests/Editor/PathComponentFileSystemTests.cs),
+  [PathReferenceCombineTests](Tests/Editor/PathReferenceCombineTests.cs),
+  [PathReferenceCycleTests](Tests/Editor/PathReferenceCycleTests.cs) and
+  [PathReferenceAssetTests](Tests/Editor/PathReferenceAssetTests.cs)
+  * Pins the authoring patterns that ship in `Templates/`, including `Constant("..")` and
+    a rooted first component, so future validation cannot outlaw them
+  * Records that `ManifestName` and `ManifestVersion` return null rather than entering
+    their reported-error paths, and that `FindFile` and `FindDirectory` lose the
+    underlying cause outside pipeline execution
+* [PathComponentEnvironmentTests](Tests/Editor/PathComponentEnvironmentTests.cs) cover both
+  new components, including the unset-variable diagnostic and that shell syntax in a
+  variable name is looked up verbatim rather than unwrapped
 
 ## 9.4.3
 

Editor/Core/Paths/Components/EnvironmentVariable.cs

@@ -0,0 +1,34 @@
+using System;
+using ThunderKit.Core.Pipelines;
+using UnityEngine;
+
+namespace ThunderKit.Core.Paths.Components
+{
+    public class EnvironmentVariable : PathComponent
+    {
+        [Tooltip("Name only, without % or $. Names differ per platform; prefer SpecialFolder for a portable location")]
+        public string VariableName;
+
+        [Tooltip("Used when the variable is not set. Leave empty to require it")]
+        public string Fallback;
+
+        protected override string GetPathInternal(PathReference output, Pipeline pipeline)
+        {
+            if (string.IsNullOrEmpty(VariableName))
+                throw new InvalidOperationException(
+                    $"{PathDiagnostics.Link(output, this)} has no variable name assigned.");
+
+            var value = Environment.GetEnvironmentVariable(VariableName);
+            if (!string.IsNullOrEmpty(value))
+                return value;
+
+            if (!string.IsNullOrEmpty(Fallback))
+                return Fallback;
+
+            throw new InvalidOperationException(
+                $"{PathDiagnostics.Link(output, this)} requires environment variable \"{VariableName}\", " +
+                "which is not set for the Editor process. The Editor reads the environment it was " +
+                "launched with, so a newly added variable needs an Editor restart.");
+        }
+    }
+}

Editor/Core/Paths/Components/EnvironmentVariable.cs.meta

@@ -1,7 +1,5 @@
 using System;
 using ThunderKit.Core.Pipelines;
-using UnityEditor;
-using UnityEngine.Networking;
 
 namespace ThunderKit.Core.Paths.Components
 {
@@ -16,14 +14,12 @@ protected override string GetPathInternal(PathReference output, Pipeline pipelin
             }
             catch (NullReferenceException nre)
             {
-                var pathReferencePath = UnityWebRequest.EscapeURL(AssetDatabase.GetAssetPath(output));
-                var pathReferenceLink = $"[{output.name}.{name}.reference](assetlink://{pathReferencePath})";
+                var pathReferenceLink = PathDiagnostics.Link(output, this, ".reference");
                 throw new InvalidOperationException($"Error {pathReferenceLink} is unassigned or null", nre);
             }
             catch (Exception e)
             {
-                var pathReferencePath = UnityWebRequest.EscapeURL(AssetDatabase.GetAssetPath(output));
-                var pathReferenceLink = $"[{output.name}.{name}.reference({reference.name})](assetlink://{pathReferencePath})";
+                var pathReferenceLink = PathDiagnostics.Link(output, this, $".reference({reference.name})");
                 throw new InvalidOperationException($"Error Invoking PathReference: {pathReferenceLink}", e);
             }
         }

Editor/Core/Paths/Components/OutputReference.cs

@@ -0,0 +1,33 @@
+using System;
+using ThunderKit.Core.Pipelines;
+using UnityEngine;
+
+namespace ThunderKit.Core.Paths.Components
+{
+    public class SpecialFolder : PathComponent
+    {
+        [Tooltip("Resolved per platform. ApplicationData: %APPDATA%, ~/.config, ~/Library/Application Support")]
+        public Environment.SpecialFolder Folder = Environment.SpecialFolder.ApplicationData;
+
+        protected override string GetPathInternal(PathReference output, Pipeline pipeline)
+        {
+            string folderPath;
+            try
+            {
+                folderPath = Environment.GetFolderPath(Folder);
+            }
+            catch (ArgumentException argumentException)
+            {
+                throw new InvalidOperationException(
+                    $"{PathDiagnostics.Link(output, this)} is set to {(int)Folder}, which is not a known special folder.",
+                    argumentException);
+            }
+
+            if (string.IsNullOrEmpty(folderPath))
+                throw new InvalidOperationException(
+                    $"{PathDiagnostics.Link(output, this)} requested {Folder}, which has no location on this platform.");
+
+            return folderPath;
+        }
+    }
+}

Editor/Core/Paths/Components/SpecialFolder.cs

@@ -0,0 +1,77 @@
+using System;
+using System.IO;
+using System.Linq;
+using ThunderKit.Core.Pipelines;
+
+namespace ThunderKit.Core.Paths
+{
+    // Every PathComponent result converges here, so this is the only place that can
+    // enforce that a component cannot silently corrupt the assembled path.
+    internal static class PathAssembler
+    {
+        static readonly char[] InvalidPathChars = Path.GetInvalidPathChars();
+
+        // Drive-relative segments ("C:") only carry that meaning where paths are DOS
+        // shaped; ':' is an ordinary filename character elsewhere.
+        static readonly bool DosPaths = Path.DirectorySeparatorChar == '\\';
+
+        public static string Assemble(PathReference output, Pipeline pipeline, ComposableElement[] data)
+        {
+            if (data == null)
+                return string.Empty;
+
+            var components = data.OfType<PathComponent>().ToArray();
+            var segments = new string[components.Length];
+            for (int index = 0; index < components.Length; index++)
+            {
+                var component = components[index];
+                segments[index] = Validated(output, component, component.GetPath(output, pipeline), index);
+            }
+
+            if (segments.Length == 0)
+                return string.Empty;
+
+            return Path.Combine(segments);
+        }
+
+        // Ordered so that Path.IsPathRooted and Path.Combine, which reject invalid
+        // characters on the .NET Framework profile, are never reached with them.
+        static string Validated(PathReference output, PathComponent component, string segment, int index)
+        {
+            if (segment == null)
+                throw new InvalidOperationException(
+                    $"{PathDiagnostics.Link(output, component)} returned null. " +
+                    "A PathComponent must return a path segment or an empty string.");
+
+            if (segment.IndexOfAny(InvalidPathChars) >= 0)
+                throw new InvalidOperationException(
+                    $"{PathDiagnostics.Link(output, component)} returned \"{segment}\", " +
+                    "which contains characters that are not valid in a path.");
+
+            if (DosPaths && IsDriveRelative(segment))
+                throw new InvalidOperationException(
+                    $"{PathDiagnostics.Link(output, component)} returned the drive-relative path \"{segment}\", " +
+                    "which resolves against the current directory of that drive rather than its root. " +
+                    "Add a trailing separator to mean the drive root.");
+
+            if (index > 0 && Path.IsPathRooted(segment))
+                throw new InvalidOperationException(
+                    $"{PathDiagnostics.Link(output, component)} returned the rooted path \"{segment}\" at position {index}. " +
+                    "Combining a rooted segment discards every component before it; " +
+                    "move it to the first position or make it relative.");
+
+            return segment;
+        }
+
+        static bool IsDriveRelative(string segment)
+        {
+            if (segment.Length < 2 || segment[1] != ':' || !char.IsLetter(segment[0]))
+                return false;
+
+            if (segment.Length == 2)
+                return true;
+
+            return segment[2] != '\\' && segment[2] != '/';
+        }
+    }
+}

Editor/Core/Paths/Components/SpecialFolder.cs.meta

@@ -0,0 +1,27 @@
+using UnityEditor;
+using UnityEngine.Networking;
+using Object = UnityEngine.Object;
+
+namespace ThunderKit.Core.Paths
+{
+    // Pipeline logs render assetlink:// URIs as clickable links to the offending
+    // asset, so every path resolution failure is reported through this shape.
+    internal static class PathDiagnostics
+    {
+        public static string Link(PathReference output, PathComponent component, string suffix = null)
+        {
+            return Link(output, $"{Describe(output)}.{Describe(component)}{suffix}");
+        }
+
+        public static string Link(PathReference output, string label)
+        {
+            var assetPath = UnityWebRequest.EscapeURL(AssetDatabase.GetAssetPath(output));
+            return $"[{label}](assetlink://{assetPath})";
+        }
+
+        static string Describe(Object target)
+        {
+            return target ? target.name : "<unassigned>";
+        }
+    }
+}