refactor: route the storage cmdlets through StorageService (#126) (#203)

* refactor: route the storage cmdlets through StorageService (#126)

New-PveStorage, Invoke-PveStorageDownload and Send-PveFile each built
their own PveHttpClient. They now call StorageService, which already
had CreateStorage/DownloadUrl/UploadIso with zero callers.

StorageService.UploadIso previously hardcoded content=iso regardless
of the cmdlet's ContentType parameter (iso/vztmpl/import), which would
have silently broken vztmpl/import uploads on conversion; it now takes
an optional contentType parameter. DownloadUrl gained an optional
timeout parameter so Invoke-PveStorageDownload -TimeoutSeconds keeps
working, matching UploadIso's existing default. Per issue #194, both
cmdlets construct a fresh StorageService() with no injected client so
the timeout override reaches PveServiceBase.CreateClient instead of
being silently dropped.

ParseTask now stamps Status = "running" on the UPID-string branch,
matching what the cmdlets stamped locally before conversion (same
rule PR #196 established for SnapshotService).

* test: add StorageService coverage for the #126 storage seam

---------

Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>

src/PSProxmoxVE.Core/Services/StorageService.cs

@@ -102,6 +102,9 @@ namespace PSProxmoxVE.Core.Services
         /// HTTP timeout override for this upload. Defaults to 30 minutes, overriding the
         /// session's default 100-second timeout so that large files have time to transfer.
         /// </param>
+        /// <param name="contentType">
+        /// The storage content type to upload as (e.g. "iso", "vztmpl", "import"). Defaults to "iso".
+        /// </param>
         public PveTask UploadIso(
             PveSession session,
             string node,
@@ -110,16 +113,18 @@ namespace PSProxmoxVE.Core.Services
             string? checksum = null,
             string? checksumAlgorithm = null,
             Action<long, long>? progressCallback = null,
-            TimeSpan? timeout = null)
+            TimeSpan? timeout = null,
+            string contentType = "iso")
         {
             if (session == null) throw new ArgumentNullException(nameof(session));
             if (string.IsNullOrWhiteSpace(node)) throw new ArgumentNullException(nameof(node));
             if (string.IsNullOrWhiteSpace(storage)) throw new ArgumentNullException(nameof(storage));
             if (string.IsNullOrWhiteSpace(filePath)) throw new ArgumentNullException(nameof(filePath));
+            if (string.IsNullOrWhiteSpace(contentType)) throw new ArgumentNullException(nameof(contentType));
 
             var formFields = new Dictionary<string, string>
             {
-                ["content"] = "iso"
+                ["content"] = contentType
             };
 
             return Invoke(session, timeout ?? TimeSpan.FromMinutes(30), client =>
@@ -145,13 +150,19 @@ namespace PSProxmoxVE.Core.Services
         /// <param name="url">The URL to download from.</param>
         /// <param name="filename">The target filename on the storage.</param>
         /// <param name="contentType">The content type (e.g. "iso", "vztmpl").</param>
+        /// <param name="timeout">
+        /// HTTP timeout override for this request. Defaults to 30 minutes, matching
+        /// <see cref="UploadIso"/>, since scheduling a download can outlast the session's
+        /// default 100-second timeout on a slow or busy node.
+        /// </param>
         public PveTask DownloadUrl(
             PveSession session,
             string node,
             string storage,
             string url,
             string filename,
-            string contentType)
+            string contentType,
+            TimeSpan? timeout = null)
         {
             if (session == null) throw new ArgumentNullException(nameof(session));
             if (string.IsNullOrWhiteSpace(node)) throw new ArgumentNullException(nameof(node));
@@ -167,7 +178,7 @@ namespace PSProxmoxVE.Core.Services
                 ["content"] = contentType
             };
 
-            return Invoke(session, client =>
+            return Invoke(session, timeout ?? TimeSpan.FromMinutes(30), client =>
             {
                 var response = client.PostAsync($"nodes/{Uri.EscapeDataString(node)}/storage/{Uri.EscapeDataString(storage)}/download-url", formData)
                     .GetAwaiter().GetResult();
@@ -196,8 +207,12 @@ namespace PSProxmoxVE.Core.Services
                     kvp => kvp.Key,
                     kvp => kvp.Value?.ToString() ?? string.Empty);
                 var response = client.PostAsync("storage", formData).GetAwaiter().GetResult();
+                if (string.IsNullOrWhiteSpace(response))
+                    return new PveStorage();
                 var data = JObject.Parse(response)["data"];
-                return data?.ToObject<PveStorage>() ?? new PveStorage();
+                return data?.Type == JTokenType.Object
+                    ? data.ToObject<PveStorage>() ?? new PveStorage()
+                    : new PveStorage();
             });
         }
 
@@ -334,7 +349,7 @@ namespace PSProxmoxVE.Core.Services
         {
             var data = JObject.Parse(response)["data"];
             if (data?.Type == JTokenType.String)
-                return new PveTask { Upid = data.ToString(), Node = node };
+                return new PveTask { Upid = data.ToString(), Node = node, Status = "running" };
 
             var task = data?.ToObject<PveTask>() ?? new PveTask();
             task.Node = node;

src/PSProxmoxVE/Cmdlets/Storage/InvokePveStorageDownloadCmdlet.cs

@@ -1,8 +1,5 @@
 using System;
-using System.Collections.Generic;
 using System.Management.Automation;
-using Newtonsoft.Json.Linq;
-using PSProxmoxVE.Core.Client;
 using PSProxmoxVE.Core.Models.Vms;
 using PSProxmoxVE.Core.Services;
 
@@ -73,28 +70,12 @@ namespace PSProxmoxVE.Cmdlets.Storage
             {
                 timeout = TimeSpan.FromMinutes(30);
             }
-            using var client = new PveHttpClient(session, timeout);
 
             WriteVerbose($"Downloading '{Url}' to {Node}/{Storage}...");
-            var resource = $"nodes/{Uri.EscapeDataString(Node)}/storage/{Uri.EscapeDataString(Storage)}/download-url";
-            var data = new Dictionary<string, string>
-            {
-                ["url"]      = Url,
-                ["filename"] = Filename,
-                ["content"]  = ContentType
-            };
+            var task = new StorageService().DownloadUrl(session, Node, Storage, Url, Filename, ContentType, timeout);
 
-            var json = client.PostAsync(resource, data).GetAwaiter().GetResult();
-            var root = JObject.Parse(json);
-            var upid = root["data"]?.ToString() ?? string.Empty;
-
-            var task = new PveTask { Upid = upid, Node = Node, Status = "running" };
-
-            if (Wait.IsPresent && !string.IsNullOrEmpty(upid))
-            {
-                var taskService = new TaskService();
-                task = taskService.WaitForTask(session, Node, upid);
-            }
+            if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid))
+                task = new TaskService().WaitForTask(session, Node, task.Upid);
 
             WriteObject(task);
         }

src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs

@@ -1,9 +1,8 @@
 using System;
 using System.Collections.Generic;
 using System.Management.Automation;
-using Newtonsoft.Json.Linq;
-using PSProxmoxVE.Core.Client;
 using PSProxmoxVE.Core.Models.Storage;
+using PSProxmoxVE.Core.Services;
 
 namespace PSProxmoxVE.Cmdlets.Storage
 {
@@ -84,7 +83,7 @@ namespace PSProxmoxVE.Cmdlets.Storage
         [Parameter(Mandatory = false, HelpMessage = "Limit access to these nodes (comma-separated).")]
         public string? Nodes { get; set; }
 
-        private static void AddIfNotEmpty(Dictionary<string, string> data, string key, string? value)
+        private static void AddIfNotEmpty(Dictionary<string, object> data, string key, string? value)
         {
             if (!string.IsNullOrEmpty(value))
                 data[key] = value!;
@@ -104,10 +103,9 @@ namespace PSProxmoxVE.Cmdlets.Storage
             }
 
             var session = GetSession();
-            using var client = new PveHttpClient(session);
 
             WriteVerbose($"Creating storage '{Storage}'...");
-            var data = new Dictionary<string, string>
+            var data = new Dictionary<string, object>
             {
                 ["storage"] = Storage,
                 ["type"]    = Type
@@ -145,9 +143,8 @@ namespace PSProxmoxVE.Cmdlets.Storage
             if (Shared.IsPresent)  data["shared"]  = "1";
             if (Disable.IsPresent) data["disable"] = "1";
 
-            client.PostAsync("storage", data).GetAwaiter().GetResult();
+            new StorageService().CreateStorage(session, data);
 
-            // Return the storage object representing what was created
             var storage = new PveStorage
             {
                 Storage = Storage,

src/PSProxmoxVE/Cmdlets/Storage/SendPveFileCmdlet.cs

@@ -1,8 +1,6 @@
 using System;
 using System.IO;
 using System.Management.Automation;
-using Newtonsoft.Json.Linq;
-using PSProxmoxVE.Core.Client;
 using PSProxmoxVE.Core.Models.Vms;
 using PSProxmoxVE.Core.Services;
 
@@ -95,10 +93,8 @@ namespace PSProxmoxVE.Cmdlets.Storage
             {
                 timeout = TimeSpan.FromMinutes(30);
             }
-            using var client = new PveHttpClient(session, timeout);
 
             WriteVerbose($"Uploading {fileName} to {Node}/{Storage} (content={ContentType})...");
-            var resource = $"nodes/{Uri.EscapeDataString(Node)}/storage/{Uri.EscapeDataString(Storage)}/upload";
             var totalBytes = new System.IO.FileInfo(Path).Length;
 
             var activityId = 1;
@@ -111,18 +107,19 @@ namespace PSProxmoxVE.Cmdlets.Storage
             // callback directly would invoke it from the HTTP serialization thread
             // and cause PowerShell to throw an InvalidOperationException mid-upload.
             long progressBytes = 0;
+            var storageService = new StorageService();
             var uploadTask = System.Threading.Tasks.Task.Run(() =>
-                client.UploadFileAsync(
-                    resource,
+                storageService.UploadIso(
+                    session,
+                    Node,
+                    Storage,
                     Path,
-                    formFields: new System.Collections.Generic.Dictionary<string, string>
-                    {
-                        ["content"] = ContentType
-                    },
                     checksum: Checksum,
                     checksumAlgorithm: ChecksumAlgorithm,
                     progressCallback: (bytesSent, _) =>
-                        System.Threading.Interlocked.Exchange(ref progressBytes, bytesSent)));
+                        System.Threading.Interlocked.Exchange(ref progressBytes, bytesSent),
+                    timeout: timeout,
+                    contentType: ContentType));
 
             // Poll progress on the pipeline thread while the upload runs.
             while (!uploadTask.IsCompleted)
@@ -138,21 +135,13 @@ namespace PSProxmoxVE.Cmdlets.Storage
                 }
             }
 
-            var json = uploadTask.GetAwaiter().GetResult();
+            var task = uploadTask.GetAwaiter().GetResult();
 
             progressRecord.RecordType = ProgressRecordType.Completed;
             WriteProgress(progressRecord);
 
-            var root = JObject.Parse(json);
-            var upid = root["data"]?.ToString() ?? string.Empty;
-
-            var task = new PveTask { Upid = upid, Node = Node, Status = "running" };
-
-            if (Wait.IsPresent && !string.IsNullOrEmpty(upid))
-            {
-                var taskService = new TaskService();
-                task = taskService.WaitForTask(session, Node, upid);
-            }
+            if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid))
+                task = new TaskService().WaitForTask(session, Node, task.Upid);
 
             WriteObject(task);
         }

tests/PSProxmoxVE.Core.Tests/Services/StorageServiceTests.cs

@@ -11,6 +11,69 @@ namespace PSProxmoxVE.Core.Tests.Services
 {
     public class StorageServiceTests
     {
+        private sealed class CapturedPost
+        {
+            public int Calls { get; set; }
+            public string? Path { get; set; }
+            public Dictionary<string, string>? Form { get; set; }
+        }
+
+        private static CapturedPost CapturePost(Mock<IPveHttpClient> mockClient, string json)
+        {
+            var captured = new CapturedPost();
+            mockClient.Setup(c => c.PostAsync(It.IsAny<string>(), It.IsAny<Dictionary<string, string>>()))
+                .Callback<string, Dictionary<string, string>?>((path, form) =>
+                {
+                    captured.Calls++;
+                    captured.Path = path;
+                    captured.Form = form;
+                })
+                .ReturnsAsync(json);
+            return captured;
+        }
+
+        private sealed class CapturedUpload
+        {
+            public int Calls { get; set; }
+            public string? Path { get; set; }
+            public Dictionary<string, string>? Fields { get; set; }
+        }
+
+        private static CapturedUpload CaptureUpload(Mock<IPveHttpClient> mockClient, string json)
+        {
+            var captured = new CapturedUpload();
+            mockClient.Setup(c => c.UploadFileAsync(
+                    It.IsAny<string>(), It.IsAny<string>(), It.IsAny<Dictionary<string, string>>(),
+                    It.IsAny<string>(), It.IsAny<string>(), It.IsAny<Action<long, long>>()))
+                .Callback<string, string, Dictionary<string, string>?, string?, string?, Action<long, long>?>(
+                    (path, _, fields, _, _, _) =>
+                    {
+                        captured.Calls++;
+                        captured.Path = path;
+                        captured.Fields = fields;
+                    })
+                .ReturnsAsync(json);
+            return captured;
+        }
+
+        /// <summary>Test seam that records the timeout override the base class receives for a call.</summary>
+        private sealed class TimeoutCapturingStorageService : StorageService
+        {
+            private readonly IPveHttpClient _client;
+            public TimeSpan? SeenTimeout;
+
+            public TimeoutCapturingStorageService(IPveHttpClient client)
+            {
+                _client = client;
+            }
+
+            internal override IPveHttpClient CreateClient(PveSession session, TimeSpan? timeoutOverride)
+            {
+                SeenTimeout = timeoutOverride;
+                return _client;
+            }
+        }
+
         private readonly Mock<IPveHttpClient> _mockClient;
         private readonly StorageService _service;
         private readonly PveSession _session;
@@ -179,6 +242,67 @@ namespace PSProxmoxVE.Core.Tests.Services
             _mockClient.Verify(c => c.PostAsync("storage", It.IsAny<Dictionary<string, string>>()), Times.Once);
         }
 
+        [Fact]
+        public void CreateStorage_SendsExactForm()
+        {
+            // Arrange
+            var captured = CapturePost(_mockClient, @"{""data"":{""storage"":""local"",""type"":""dir""}}");
+            var config = new Dictionary<string, object>
+            {
+                ["storage"] = "local",
+                ["type"] = "dir",
+                ["shared"] = "1"
+            };
+
+            // Act
+            _service.CreateStorage(_session, config);
+
+            // Assert
+            Assert.Equal(1, captured.Calls);
+            Assert.Equal("storage", captured.Path);
+            Assert.NotNull(captured.Form);
+            Assert.Equal("local", captured.Form!["storage"]);
+            Assert.Equal("dir", captured.Form["type"]);
+            Assert.Equal("1", captured.Form["shared"]);
+            Assert.Equal(3, captured.Form.Count);
+        }
+
+        [Fact]
+        public void CreateStorage_NonStringValues_AreStringified()
+        {
+            // Arrange
+            var captured = CapturePost(_mockClient, @"{""data"":{""storage"":""local"",""type"":""dir""}}");
+            var config = new Dictionary<string, object>
+            {
+                ["storage"] = "local",
+                ["shared"] = 1,
+                ["disable"] = true,
+                ["notes"] = null!
+            };
+
+            // Act
+            _service.CreateStorage(_session, config);
+
+            // Assert
+            Assert.NotNull(captured.Form);
+            Assert.Equal("1", captured.Form!["shared"]);
+            Assert.Equal("True", captured.Form["disable"]);
+            Assert.Equal(string.Empty, captured.Form["notes"]);
+        }
+
+        [Fact]
+        public void CreateStorage_EmptyResponseBody_ReturnsEmptyStorage()
+        {
+            // Arrange
+            CapturePost(_mockClient, string.Empty);
+
+            // Act
+            var result = _service.CreateStorage(_session, new Dictionary<string, object> { ["storage"] = "local" });
+
+            // Assert
+            Assert.Equal(string.Empty, result.Storage);
+        }
+
         [Fact]
         public void CreateStorage_NullSession_ThrowsArgumentNullException()
         {
@@ -398,6 +522,88 @@ namespace PSProxmoxVE.Core.Tests.Services
             // Assert
             Assert.Contains("UPID:pve1", result.Upid);
             Assert.Equal("pve1", result.Node);
+            Assert.Equal("running", result.Status);
+        }
+
+        [Fact]
+        public void UploadIso_DefaultContentType_SendsIso()
+        {
+            // Arrange
+            var captured = CaptureUpload(_mockClient, @"{""data"":""UPID:pve1:000AAA:00000001:65F00000:upload:local:root@pam:""}");
+
+            // Act
+            _service.UploadIso(_session, "pve1", "local", "/tmp/debian-12.iso");
+
+            // Assert
+            Assert.Equal(1, captured.Calls);
+            Assert.Equal("nodes/pve1/storage/local/upload", captured.Path);
+            Assert.NotNull(captured.Fields);
+            Assert.Equal("iso", captured.Fields!["content"]);
+            Assert.Single(captured.Fields);
+        }
+
+        [Fact]
+        public void UploadIso_ExplicitContentType_SendsIt()
+        {
+            // Arrange
+            var captured = CaptureUpload(_mockClient, @"{""data"":""UPID:pve1:000AAA:00000001:65F00000:upload:local:root@pam:""}");
+
+            // Act
+            _service.UploadIso(_session, "pve1", "local", "/tmp/rootfs.tar.zst", contentType: "vztmpl");
+
+            // Assert
+            Assert.Equal(1, captured.Calls);
+            Assert.NotNull(captured.Fields);
+            Assert.Equal("vztmpl", captured.Fields!["content"]);
+            Assert.Single(captured.Fields);
+        }
+
+        [Fact]
+        public void UploadIso_EscapesNodeAndStorageInPath()
+        {
+            // Arrange
+            var captured = CaptureUpload(_mockClient, @"{""data"":""UPID:pve1:000AAA:00000001:65F00000:upload:local:root@pam:""}");
+
+            // Act
+            _service.UploadIso(_session, "pve node", "local storage", "/tmp/debian-12.iso");
+
+            // Assert
+            Assert.Equal("nodes/pve%20node/storage/local%20storage/upload", captured.Path);
+        }
+
+        [Fact]
+        public void UploadIso_WhitespaceContentType_ThrowsArgumentNullException()
+        {
+            Assert.Throws<ArgumentNullException>(() =>
+                _service.UploadIso(_session, "pve1", "local", "/tmp/test.iso", contentType: " "));
+        }
+
+        [Fact]
+        public void UploadIso_TimeoutOverride_PassesItToClientConstruction()
+        {
+            // Arrange
+            var captured = CaptureUpload(_mockClient, @"{""data"":""UPID:pve1:000AAA:00000001:65F00000:upload:local:root@pam:""}");
+            var service = new TimeoutCapturingStorageService(_mockClient.Object);
+
+            // Act
+            service.UploadIso(_session, "pve1", "local", "/tmp/debian-12.iso", timeout: TimeSpan.FromMinutes(45));
+
+            // Assert
+            Assert.Equal(TimeSpan.FromMinutes(45), service.SeenTimeout);
+        }
+
+        [Fact]
+        public void UploadIso_NoTimeout_DefaultsToThirtyMinutes()
+        {
+            // Arrange
+            CaptureUpload(_mockClient, @"{""data"":""UPID:pve1:000AAA:00000001:65F00000:upload:local:root@pam:""}");
+            var service = new TimeoutCapturingStorageService(_mockClient.Object);
+
+            // Act
+            service.UploadIso(_session, "pve1", "local", "/tmp/debian-12.iso");
+
+            // Assert
+            Assert.Equal(TimeSpan.FromMinutes(30), service.SeenTimeout);
         }
 
         [Fact]
@@ -434,6 +640,73 @@ namespace PSProxmoxVE.Core.Tests.Services
             // Assert
             Assert.Contains("UPID:pve1", result.Upid);
             Assert.Equal("pve1", result.Node);
+            Assert.Equal("running", result.Status);
+        }
+
+        [Fact]
+        public void DownloadUrl_SendsExactForm()
+        {
+            // Arrange
+            var captured = CapturePost(_mockClient, @"{""data"":""UPID:pve1:000BBB:00000002:65F00001:download:local:root@pam:""}");
+
+            // Act
+            _service.DownloadUrl(
+                _session, "pve1", "local",
+                "https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img",
+                "noble-server-cloudimg-amd64.img", "iso");
+
+            // Assert
+            Assert.Equal(1, captured.Calls);
+            Assert.Equal("nodes/pve1/storage/local/download-url", captured.Path);
+            Assert.NotNull(captured.Form);
+            Assert.Equal("https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img", captured.Form!["url"]);
+            Assert.Equal("noble-server-cloudimg-amd64.img", captured.Form["filename"]);
+            Assert.Equal("iso", captured.Form["content"]);
+            Assert.Equal(3, captured.Form.Count);
+        }
+
+        [Fact]
+        public void DownloadUrl_EscapesNodeAndStorageInPath()
+        {
+            // Arrange
+            var captured = CapturePost(_mockClient, @"{""data"":""UPID:pve1:000BBB:00000002:65F00001:download:local:root@pam:""}");
+
+            // Act
+            _service.DownloadUrl(_session, "pve node", "local storage", "https://example.com/f.iso", "f.iso", "iso");
+
+            // Assert
+            Assert.Equal(1, captured.Calls);
+            Assert.Equal("nodes/pve%20node/storage/local%20storage/download-url", captured.Path);
+        }
+
+        [Fact]
+        public void DownloadUrl_TimeoutOverride_PassesItToClientConstruction()
+        {
+            // Arrange
+            var captured = CapturePost(_mockClient, @"{""data"":""UPID:pve1:000BBB:00000002:65F00001:download:local:root@pam:""}");
+            var service = new TimeoutCapturingStorageService(_mockClient.Object);
+
+            // Act
+            service.DownloadUrl(_session, "pve1", "local", "https://example.com/f.iso", "f.iso", "iso",
+                timeout: TimeSpan.FromMinutes(45));
+
+            // Assert
+            Assert.Equal(TimeSpan.FromMinutes(45), service.SeenTimeout);
+            Assert.Equal(1, captured.Calls);
+        }
+
+        [Fact]
+        public void DownloadUrl_NoTimeout_DefaultsToThirtyMinutes()
+        {
+            // Arrange
+            CapturePost(_mockClient, @"{""data"":""UPID:pve1:000BBB:00000002:65F00001:download:local:root@pam:""}");
+            var service = new TimeoutCapturingStorageService(_mockClient.Object);
+
+            // Act
+            service.DownloadUrl(_session, "pve1", "local", "https://example.com/f.iso", "f.iso", "iso");
+
+            // Assert
+            Assert.Equal(TimeSpan.FromMinutes(30), service.SeenTimeout);
         }
 
         [Fact]