From b43948b696b2ca4c5034f043b17c6b109cf78cac Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 00:15:41 +0000 Subject: [PATCH] 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> --- .../Services/StorageService.cs | 27 +- .../Storage/InvokePveStorageDownloadCmdlet.cs | 25 +- .../Cmdlets/Storage/NewPveStorageCmdlet.cs | 11 +- .../Cmdlets/Storage/SendPveFileCmdlet.cs | 33 +-- .../Services/StorageServiceTests.cs | 273 ++++++++++++++++++ 5 files changed, 312 insertions(+), 57 deletions(-) diff --git a/src/PSProxmoxVE.Core/Services/StorageService.cs b/src/PSProxmoxVE.Core/Services/StorageService.cs index e537c53..1560f92 100644 --- a/src/PSProxmoxVE.Core/Services/StorageService.cs +++ b/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. /// + /// + /// The storage content type to upload as (e.g. "iso", "vztmpl", "import"). Defaults to "iso". + /// public PveTask UploadIso( PveSession session, string node, @@ -110,16 +113,18 @@ namespace PSProxmoxVE.Core.Services string? checksum = null, string? checksumAlgorithm = null, Action? 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 { - ["content"] = "iso" + ["content"] = contentType }; return Invoke(session, timeout ?? TimeSpan.FromMinutes(30), client => @@ -145,13 +150,19 @@ namespace PSProxmoxVE.Core.Services /// The URL to download from. /// The target filename on the storage. /// The content type (e.g. "iso", "vztmpl"). + /// + /// HTTP timeout override for this request. Defaults to 30 minutes, matching + /// , since scheduling a download can outlast the session's + /// default 100-second timeout on a slow or busy node. + /// 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() ?? new PveStorage(); + return data?.Type == JTokenType.Object + ? data.ToObject() ?? 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() ?? new PveTask(); task.Node = node; diff --git a/src/PSProxmoxVE/Cmdlets/Storage/InvokePveStorageDownloadCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Storage/InvokePveStorageDownloadCmdlet.cs index 1ee3259..0cc524a 100644 --- a/src/PSProxmoxVE/Cmdlets/Storage/InvokePveStorageDownloadCmdlet.cs +++ b/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 - { - ["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); } diff --git a/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs index 40f1fe4..1ef8335 100644 --- a/src/PSProxmoxVE/Cmdlets/Storage/NewPveStorageCmdlet.cs +++ b/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 data, string key, string? value) + private static void AddIfNotEmpty(Dictionary 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 + var data = new Dictionary { ["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, diff --git a/src/PSProxmoxVE/Cmdlets/Storage/SendPveFileCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Storage/SendPveFileCmdlet.cs index a2d4431..4b7ac7b 100644 --- a/src/PSProxmoxVE/Cmdlets/Storage/SendPveFileCmdlet.cs +++ b/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 - { - ["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); } diff --git a/tests/PSProxmoxVE.Core.Tests/Services/StorageServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/StorageServiceTests.cs index f2c78c1..cc1af84 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/StorageServiceTests.cs +++ b/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? Form { get; set; } + } + + private static CapturedPost CapturePost(Mock mockClient, string json) + { + var captured = new CapturedPost(); + mockClient.Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) + .Callback?>((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? Fields { get; set; } + } + + private static CapturedUpload CaptureUpload(Mock mockClient, string json) + { + var captured = new CapturedUpload(); + mockClient.Setup(c => c.UploadFileAsync( + It.IsAny(), It.IsAny(), It.IsAny>(), + It.IsAny(), It.IsAny(), It.IsAny>())) + .Callback?, string?, string?, Action?>( + (path, _, fields, _, _, _) => + { + captured.Calls++; + captured.Path = path; + captured.Fields = fields; + }) + .ReturnsAsync(json); + return captured; + } + + /// Test seam that records the timeout override the base class receives for a call. + 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 _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>()), Times.Once); } + [Fact] + public void CreateStorage_SendsExactForm() + { + // Arrange + var captured = CapturePost(_mockClient, @"{""data"":{""storage"":""local"",""type"":""dir""}}"); + var config = new Dictionary + { + ["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 + { + ["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 { ["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(() => + _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]