From 17bb2987b2fdddaa10d392dbb20cf1ae18750b3f Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 23:43:51 +0000 Subject: [PATCH] refactor: route the snapshot cmdlets through SnapshotService (#126, pattern) (#196) New-PveSnapshot, Remove-PveSnapshot and Restore-PveSnapshot built their own PveHttpClient and parsed the response inline while SnapshotService carried the same three requests with no callers. The cmdlets now call the service, so the path and form each sends is asserted offline (ADR 0021). Reconciled toward the shipped cmdlet behaviour: CreateSnapshot omits vmstate unless it is true, and ParseTask stamps Status = "running" on a UPID-string response. Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- .../Services/SnapshotService.cs | 15 +- .../Cmdlets/Snapshots/NewPveSnapshotCmdlet.cs | 23 +- .../Snapshots/RemovePveSnapshotCmdlet.cs | 16 +- .../Snapshots/RestorePveSnapshotCmdlet.cs | 16 +- .../Services/SnapshotServiceTests.cs | 219 +++++++++++++++--- 5 files changed, 202 insertions(+), 87 deletions(-) diff --git a/src/PSProxmoxVE.Core/Services/SnapshotService.cs b/src/PSProxmoxVE.Core/Services/SnapshotService.cs index 932c743..0d180b0 100644 --- a/src/PSProxmoxVE.Core/Services/SnapshotService.cs +++ b/src/PSProxmoxVE.Core/Services/SnapshotService.cs @@ -45,14 +45,14 @@ namespace PSProxmoxVE.Core.Services } /// - /// Creates a snapshot of a VM. Returns the task UPID. + /// Creates a snapshot of a VM. Returns the task PVE started. /// /// The authenticated PVE session. /// The cluster node name. /// The VM ID. /// Snapshot name (alphanumeric, no spaces). /// Optional description. - /// Whether to save VM RAM state (live snapshot). Default false. + /// Whether to save VM RAM state (live snapshot). Sent only when true. public PveTask CreateSnapshot( PveSession session, string node, @@ -67,11 +67,12 @@ namespace PSProxmoxVE.Core.Services var formData = new Dictionary { - ["snapname"] = snapname, - ["vmstate"] = vmstate ? "1" : "0" + ["snapname"] = snapname }; if (!string.IsNullOrEmpty(description)) formData["description"] = description!; + if (vmstate) + formData["vmstate"] = "1"; return Invoke(session, client => { @@ -82,7 +83,7 @@ namespace PSProxmoxVE.Core.Services } /// - /// Removes a snapshot from a VM. Returns the task UPID. + /// Removes a snapshot from a VM. Returns the task PVE started. /// /// The authenticated PVE session. /// The cluster node name. @@ -107,7 +108,7 @@ namespace PSProxmoxVE.Core.Services } /// - /// Rolls a VM back to a snapshot. Returns the task UPID. + /// Rolls a VM back to a snapshot. Returns the task PVE started. /// /// The authenticated PVE session. /// The cluster node name. @@ -139,7 +140,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/Snapshots/NewPveSnapshotCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Snapshots/NewPveSnapshotCmdlet.cs index 8231d3f..6dc0345 100644 --- a/src/PSProxmoxVE/Cmdlets/Snapshots/NewPveSnapshotCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Snapshots/NewPveSnapshotCmdlet.cs @@ -1,8 +1,4 @@ -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; @@ -50,26 +46,15 @@ namespace PSProxmoxVE.Cmdlets.Snapshots return; var session = GetSession(); - using var client = new PveHttpClient(session); WriteVerbose($"Creating snapshot '{Name}' for VM {VmId}..."); - var data = new Dictionary - { - ["snapname"] = Name - }; - if (!string.IsNullOrEmpty(Description)) data["description"] = Description!; - if (IncludeVmState.IsPresent) data["vmstate"] = "1"; + var service = new SnapshotService(); + var task = service.CreateSnapshot(session, Node, VmId, Name, Description, IncludeVmState.IsPresent); - var json = client.PostAsync($"nodes/{Uri.EscapeDataString(Node)}/qemu/{VmId}/snapshot", 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)) + if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid)) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, upid); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Snapshots/RemovePveSnapshotCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Snapshots/RemovePveSnapshotCmdlet.cs index ca66bd2..1ed5146 100644 --- a/src/PSProxmoxVE/Cmdlets/Snapshots/RemovePveSnapshotCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Snapshots/RemovePveSnapshotCmdlet.cs @@ -1,7 +1,4 @@ -using System; using System.Management.Automation; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; @@ -45,18 +42,13 @@ namespace PSProxmoxVE.Cmdlets.Snapshots return; WriteVerbose($"Removing snapshot '{Name}' from VM {VmId}..."); - using var client = new PveHttpClient(session); + var service = new SnapshotService(); + var task = service.RemoveSnapshot(session, Node, VmId, Name); - var json = client.DeleteAsync($"nodes/{Uri.EscapeDataString(Node)}/qemu/{VmId}/snapshot/{Uri.EscapeDataString(Name)}").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)) + if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid)) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, upid); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/src/PSProxmoxVE/Cmdlets/Snapshots/RestorePveSnapshotCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Snapshots/RestorePveSnapshotCmdlet.cs index d987337..480c168 100644 --- a/src/PSProxmoxVE/Cmdlets/Snapshots/RestorePveSnapshotCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Snapshots/RestorePveSnapshotCmdlet.cs @@ -1,7 +1,4 @@ -using System; using System.Management.Automation; -using Newtonsoft.Json.Linq; -using PSProxmoxVE.Core.Client; using PSProxmoxVE.Core.Models.Vms; using PSProxmoxVE.Core.Services; @@ -48,18 +45,13 @@ namespace PSProxmoxVE.Cmdlets.Snapshots return; WriteVerbose($"Restoring snapshot '{Name}' on VM {VmId}..."); - using var client = new PveHttpClient(session); + var service = new SnapshotService(); + var task = service.RollbackSnapshot(session, Node, VmId, Name); - var json = client.PostAsync($"nodes/{Uri.EscapeDataString(Node)}/qemu/{VmId}/snapshot/{Uri.EscapeDataString(Name)}/rollback").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)) + if (Wait.IsPresent && !string.IsNullOrEmpty(task.Upid)) { var taskService = new TaskService(); - task = taskService.WaitForTask(session, Node, upid); + task = taskService.WaitForTask(session, Node, task.Upid); } WriteObject(task); diff --git a/tests/PSProxmoxVE.Core.Tests/Services/SnapshotServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/SnapshotServiceTests.cs index 41f16b9..f682aaa 100644 --- a/tests/PSProxmoxVE.Core.Tests/Services/SnapshotServiceTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Services/SnapshotServiceTests.cs @@ -13,6 +13,14 @@ namespace PSProxmoxVE.Core.Tests.Services { private const string Node = "pve1"; private const int VmId = 100; + private const string CreateUpid = "UPID:pve1:000ABC:00000001:5F1234AB:qmsnapshot:100:root@pam:"; + + private sealed class CapturedPost + { + public int Calls { get; set; } + public string? Path { get; set; } + public Dictionary? Form { get; set; } + } private static PveSession CreateSession() { @@ -20,6 +28,22 @@ namespace PSProxmoxVE.Core.Tests.Services "root@pam!testtoken=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"); } + private static string UpidJson(string upid) => $@"{{""data"": ""{upid}""}}"; + + 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; + } + [Fact] public void GetSnapshots_ReturnsSnapshotArray() { @@ -86,65 +110,154 @@ namespace PSProxmoxVE.Core.Tests.Services } [Fact] - public void CreateSnapshot_CallsPostAsync_ReturnsUpid() + public void CreateSnapshot_NameOnly_SendsOnlySnapname() { // Arrange - const string upid = "UPID:pve1:000ABC:00000001:5F1234AB:qmsnapshot:100:root@pam:"; - var json = $@"{{""data"": ""{upid}""}}"; var mockClient = new Mock(); - mockClient.Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) - .ReturnsAsync(json); + var captured = CapturePost(mockClient, UpidJson(CreateUpid)); + var service = new SnapshotService(mockClient.Object); + // Act + var task = service.CreateSnapshot(CreateSession(), Node, VmId, "my-snap"); + + // Assert + Assert.Equal(1, captured.Calls); + Assert.Equal($"nodes/{Node}/qemu/{VmId}/snapshot", captured.Path); + Assert.NotNull(captured.Form); + Assert.Equal("my-snap", captured.Form!["snapname"]); + Assert.False(captured.Form.ContainsKey("description")); + Assert.False(captured.Form.ContainsKey("vmstate")); + Assert.Single(captured.Form); + + Assert.Equal(CreateUpid, task.Upid); + Assert.Equal(Node, task.Node); + Assert.Equal("running", task.Status); + } + + [Fact] + public void CreateSnapshot_WithDescription_SendsDescription() + { + // Arrange + var mockClient = new Mock(); + var captured = CapturePost(mockClient, UpidJson(CreateUpid)); + var service = new SnapshotService(mockClient.Object); + + // Act + service.CreateSnapshot(CreateSession(), Node, VmId, "my-snap", "Test snapshot"); + + // Assert + Assert.Equal(1, captured.Calls); + Assert.NotNull(captured.Form); + Assert.Equal("my-snap", captured.Form!["snapname"]); + Assert.Equal("Test snapshot", captured.Form["description"]); + Assert.False(captured.Form.ContainsKey("vmstate")); + Assert.Equal(2, captured.Form.Count); + } + + [Fact] + public void CreateSnapshot_WithVmState_SendsVmstateOne() + { + // Arrange + var mockClient = new Mock(); + var captured = CapturePost(mockClient, UpidJson(CreateUpid)); + var service = new SnapshotService(mockClient.Object); + + // Act + service.CreateSnapshot(CreateSession(), Node, VmId, "my-snap", vmstate: true); + + // Assert + Assert.Equal(1, captured.Calls); + Assert.NotNull(captured.Form); + Assert.Equal("my-snap", captured.Form!["snapname"]); + Assert.Equal("1", captured.Form["vmstate"]); + Assert.False(captured.Form.ContainsKey("description")); + Assert.Equal(2, captured.Form.Count); + } + + [Fact] + public void CreateSnapshot_AllFields_SendsExactForm() + { + // Arrange + var mockClient = new Mock(); + var captured = CapturePost(mockClient, UpidJson(CreateUpid)); var service = new SnapshotService(mockClient.Object); // Act var task = service.CreateSnapshot(CreateSession(), Node, VmId, "my-snap", "Test snapshot", vmstate: true); // Assert - Assert.Equal(upid, task.Upid); + Assert.Equal(1, captured.Calls); + Assert.Equal($"nodes/{Node}/qemu/{VmId}/snapshot", captured.Path); + Assert.NotNull(captured.Form); + Assert.Equal("my-snap", captured.Form!["snapname"]); + Assert.Equal("Test snapshot", captured.Form["description"]); + Assert.Equal("1", captured.Form["vmstate"]); + Assert.Equal(3, captured.Form.Count); + + Assert.Equal(CreateUpid, task.Upid); Assert.Equal(Node, task.Node); - mockClient.Verify(c => c.PostAsync( - $"nodes/{Node}/qemu/{VmId}/snapshot", - It.Is>(d => - d["snapname"] == "my-snap" && - d["vmstate"] == "1" && - d["description"] == "Test snapshot")), - Times.Once); + Assert.Equal("running", task.Status); } [Fact] - public void CreateSnapshot_WithoutDescription_OmitsDescriptionField() + public void CreateSnapshot_EscapesNodeInPath() { // Arrange - const string upid = "UPID:pve1:000ABC:00000001:5F1234AB:qmsnapshot:100:root@pam:"; - var json = $@"{{""data"": ""{upid}""}}"; var mockClient = new Mock(); - mockClient.Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) - .ReturnsAsync(json); - + var captured = CapturePost(mockClient, UpidJson(CreateUpid)); var service = new SnapshotService(mockClient.Object); // Act - service.CreateSnapshot(CreateSession(), Node, VmId, "my-snap"); + service.CreateSnapshot(CreateSession(), "pve node", VmId, "my-snap"); // Assert - mockClient.Verify(c => c.PostAsync( - It.IsAny(), - It.Is>(d => - !d.ContainsKey("description") && - d["vmstate"] == "0")), - Times.Once); + Assert.Equal($"nodes/pve%20node/qemu/{VmId}/snapshot", captured.Path); } [Fact] - public void RemoveSnapshot_CallsDeleteAsync_ReturnsUpid() + public void CreateSnapshot_NullData_ReturnsEmptyUpidWithoutStatus() + { + // Arrange + var mockClient = new Mock(); + CapturePost(mockClient, @"{""data"": null}"); + var service = new SnapshotService(mockClient.Object); + + // Act + var task = service.CreateSnapshot(CreateSession(), Node, VmId, "my-snap"); + + // Assert + Assert.Equal(string.Empty, task.Upid); + Assert.Equal(Node, task.Node); + Assert.Null(task.Status); + } + + [Fact] + public void CreateSnapshot_ObjectShapedData_ReturnsTaskFields() + { + // Arrange + var json = $@"{{""data"": {{""upid"": ""{CreateUpid}"", ""status"": ""stopped"", ""exitstatus"": ""OK""}}}}"; + var mockClient = new Mock(); + CapturePost(mockClient, json); + var service = new SnapshotService(mockClient.Object); + + // Act + var task = service.CreateSnapshot(CreateSession(), Node, VmId, "my-snap"); + + // Assert + Assert.Equal(CreateUpid, task.Upid); + Assert.Equal(Node, task.Node); + Assert.Equal("stopped", task.Status); + Assert.Equal("OK", task.ExitStatus); + } + + [Fact] + public void RemoveSnapshot_CallsDeleteAsync_ReturnsRunningTask() { // Arrange const string upid = "UPID:pve1:000DEF:00000002:5F1234AC:qmdelsnap:100:root@pam:"; - var json = $@"{{""data"": ""{upid}""}}"; var mockClient = new Mock(); mockClient.Setup(c => c.DeleteAsync(It.IsAny())) - .ReturnsAsync(json); + .ReturnsAsync(UpidJson(upid)); var service = new SnapshotService(mockClient.Object); @@ -154,18 +267,35 @@ namespace PSProxmoxVE.Core.Tests.Services // Assert Assert.Equal(upid, task.Upid); Assert.Equal(Node, task.Node); + Assert.Equal("running", task.Status); mockClient.Verify(c => c.DeleteAsync($"nodes/{Node}/qemu/{VmId}/snapshot/clean-install"), Times.Once); + mockClient.VerifyNoOtherCalls(); } [Fact] - public void RollbackSnapshot_CallsPostAsync_ReturnsUpid() + public void RemoveSnapshot_EscapesNodeAndSnapnameInPath() + { + // Arrange + var mockClient = new Mock(); + mockClient.Setup(c => c.DeleteAsync(It.IsAny())) + .ReturnsAsync(UpidJson("UPID:pve1:000DEF:00000002:5F1234AC:qmdelsnap:100:root@pam:")); + + var service = new SnapshotService(mockClient.Object); + + // Act + service.RemoveSnapshot(CreateSession(), "pve node", VmId, "snap name"); + + // Assert + mockClient.Verify(c => c.DeleteAsync($"nodes/pve%20node/qemu/{VmId}/snapshot/snap%20name"), Times.Once); + } + + [Fact] + public void RollbackSnapshot_CallsPostAsync_ReturnsRunningTask() { // Arrange const string upid = "UPID:pve1:000GHI:00000003:5F1234AD:qmrollback:100:root@pam:"; - var json = $@"{{""data"": ""{upid}""}}"; var mockClient = new Mock(); - mockClient.Setup(c => c.PostAsync(It.IsAny(), It.IsAny>())) - .ReturnsAsync(json); + var captured = CapturePost(mockClient, UpidJson(upid)); var service = new SnapshotService(mockClient.Object); @@ -175,10 +305,25 @@ namespace PSProxmoxVE.Core.Tests.Services // Assert Assert.Equal(upid, task.Upid); Assert.Equal(Node, task.Node); - mockClient.Verify(c => c.PostAsync( - $"nodes/{Node}/qemu/{VmId}/snapshot/clean-install/rollback", - It.IsAny>()), - Times.Once); + Assert.Equal("running", task.Status); + Assert.Equal($"nodes/{Node}/qemu/{VmId}/snapshot/clean-install/rollback", captured.Path); + Assert.Null(captured.Form); + Assert.Equal(1, captured.Calls); + } + + [Fact] + public void RollbackSnapshot_EscapesNodeAndSnapnameInPath() + { + // Arrange + var mockClient = new Mock(); + var captured = CapturePost(mockClient, UpidJson("UPID:pve1:000GHI:00000003:5F1234AD:qmrollback:100:root@pam:")); + var service = new SnapshotService(mockClient.Object); + + // Act + service.RollbackSnapshot(CreateSession(), "pve node", VmId, "snap name"); + + // Assert + Assert.Equal($"nodes/pve%20node/qemu/{VmId}/snapshot/snap%20name/rollback", captured.Path); } [Fact]