Merge pull request #274 from Tinyblargon/Add-Type-SnapshotName

Add type `SnapshotName`

cli/command/create/create-snapshot.go

@@ -18,7 +18,7 @@ var (
 			id := cli.ValidateIntIDset(args, "GuestID")
 			snapName := cli.RequiredIDset(args, 1, "SnapshotName")
 			config := proxmox.ConfigSnapshot{
-				Name:        snapName,
+				Name:        proxmox.SnapshotName(snapName),
 				Description: cli.OptionalIDset(args, 2),
 				VmState:     memory,
 			}

cli/command/delete/delete-snapshot.go

@@ -14,7 +14,7 @@ var (
 		RunE: func(cmd *cobra.Command, args []string) (err error) {
 			id := cli.ValidateIntIDset(args, "GuestID")
 			snapName := cli.RequiredIDset(args, 1, "SnapshotName")
-			_, err = proxmox.DeleteSnapshot(cli.NewClient(), proxmox.NewVmRef(id), snapName)
+			_, err = proxmox.DeleteSnapshot(cli.NewClient(), proxmox.NewVmRef(id), proxmox.SnapshotName(snapName))
 			if err != nil {
 				return
 			}

cli/command/update/update-snapshotdescription.go

@@ -14,7 +14,7 @@ var update_snapshotCmd = &cobra.Command{
 		id := cli.ValidateIntIDset(args, "GuestID")
 		snapName := cli.RequiredIDset(args, 1, "SnapshotName")
 		des := cli.OptionalIDset(args, 2)
-		err = proxmox.UpdateSnapshotDescription(cli.NewClient(), proxmox.NewVmRef(id), snapName, des)
+		err = proxmox.UpdateSnapshotDescription(cli.NewClient(), proxmox.NewVmRef(id), proxmox.SnapshotName(snapName), des)
 		if err != nil {
 			return
 		}

proxmox/client.go

@@ -691,7 +691,7 @@ func (c *Client) CreateQemuSnapshot(vmr *VmRef, snapshotName string) (exitStatus
 
 // DEPRECATED superseded by DeleteSnapshot()
 func (c *Client) DeleteQemuSnapshot(vmr *VmRef, snapshotName string) (exitStatus string, err error) {
-	return DeleteSnapshot(c, vmr, snapshotName)
+	return DeleteSnapshot(c, vmr, SnapshotName(snapshotName))
 }
 
 // DEPRECATED superseded by ListSnapshots()

proxmox/snapshot.go

@@ -3,15 +3,18 @@ package proxmox
 import (
 	"encoding/json"
 	"fmt"
+	"regexp"
 	"strconv"
+	"unicode"
 )
 
 type ConfigSnapshot struct {
-	Name        string `json:"name,omitempty"`
-	Description string `json:"description,omitempty"`
-	VmState     bool   `json:"ram,omitempty"`
+	Name        SnapshotName `json:"name,omitempty"`
+	Description string       `json:"description,omitempty"`
+	VmState     bool         `json:"ram,omitempty"`
 }
 
+// TODO write tests for this
 func (config ConfigSnapshot) mapToApiValues() map[string]interface{} {
 	return map[string]interface{}{
 		"snapname":    config.Name,
@@ -21,11 +24,15 @@ func (config ConfigSnapshot) mapToApiValues() map[string]interface{} {
 }
 
 func (config ConfigSnapshot) CreateSnapshot(c *Client, vmr *VmRef) (err error) {
-	params := config.mapToApiValues()
 	err = c.CheckVmRef(vmr)
 	if err != nil {
 		return
 	}
+	err = config.Validate()
+	if err != nil {
+		return
+	}
+	params := config.mapToApiValues()
 	_, err = c.PostWithTask(params, "/nodes/"+vmr.node+"/"+vmr.vmType+"/"+strconv.Itoa(vmr.vmId)+"/snapshot/")
 	if err != nil {
 		params, _ := json.Marshal(&params)
@@ -34,6 +41,10 @@ func (config ConfigSnapshot) CreateSnapshot(c *Client, vmr *VmRef) (err error) {
 	return
 }
 
+func (config ConfigSnapshot) Validate() error {
+	return config.Name.Validate()
+}
+
 type rawSnapshots []interface{}
 
 func ListSnapshots(c *Client, vmr *VmRef) (rawSnapshots, error) {
@@ -45,20 +56,28 @@ func ListSnapshots(c *Client, vmr *VmRef) (rawSnapshots, error) {
 }
 
 // Can only be used to update the description of an already existing snapshot
-func UpdateSnapshotDescription(c *Client, vmr *VmRef, snapshot, description string) (err error) {
+func UpdateSnapshotDescription(c *Client, vmr *VmRef, snapshot SnapshotName, description string) (err error) {
 	err = c.CheckVmRef(vmr)
 	if err != nil {
 		return
 	}
-	return c.Put(map[string]interface{}{"description": description}, "/nodes/"+vmr.node+"/"+vmr.vmType+"/"+strconv.Itoa(vmr.vmId)+"/snapshot/"+snapshot+"/config")
+	err = snapshot.Validate()
+	if err != nil {
+		return
+	}
+	return c.Put(map[string]interface{}{"description": description}, "/nodes/"+vmr.node+"/"+vmr.vmType+"/"+strconv.Itoa(vmr.vmId)+"/snapshot/"+string(snapshot)+"/config")
 }
 
-func DeleteSnapshot(c *Client, vmr *VmRef, snapshot string) (exitStatus string, err error) {
+func DeleteSnapshot(c *Client, vmr *VmRef, snapshot SnapshotName) (exitStatus string, err error) {
 	err = c.CheckVmRef(vmr)
 	if err != nil {
 		return
 	}
-	return c.DeleteWithTask("/nodes/" + vmr.node + "/" + vmr.vmType + "/" + strconv.Itoa(vmr.vmId) + "/snapshot/" + snapshot)
+	err = snapshot.Validate()
+	if err != nil {
+		return
+	}
+	return c.DeleteWithTask("/nodes/" + vmr.node + "/" + vmr.vmType + "/" + strconv.Itoa(vmr.vmId) + "/snapshot/" + string(snapshot))
 }
 
 func RollbackSnapshot(c *Client, vmr *VmRef, snapshot string) (exitStatus string, err error) {
@@ -71,12 +90,12 @@ func RollbackSnapshot(c *Client, vmr *VmRef, snapshot string) (exitStatus string
 
 // Used for formatting the output when retrieving snapshots
 type Snapshot struct {
-	Name        string      `json:"name"`
-	SnapTime    uint        `json:"time,omitempty"`
-	Description string      `json:"description,omitempty"`
-	VmState     bool        `json:"ram,omitempty"`
-	Children    []*Snapshot `json:"children,omitempty"`
-	Parent      string      `json:"parent,omitempty"`
+	Name        SnapshotName `json:"name"`
+	SnapTime    uint         `json:"time,omitempty"`
+	Description string       `json:"description,omitempty"`
+	VmState     bool         `json:"ram,omitempty"`
+	Children    []*Snapshot  `json:"children,omitempty"`
+	Parent      SnapshotName `json:"parent,omitempty"`
 }
 
 // Formats the taskResponse as a list of snapshots
@@ -88,10 +107,10 @@ func (raw rawSnapshots) FormatSnapshotsList() (list []*Snapshot) {
 			list[i].Description = e.(map[string]interface{})["description"].(string)
 		}
 		if _, isSet := e.(map[string]interface{})["name"]; isSet {
-			list[i].Name = e.(map[string]interface{})["name"].(string)
+			list[i].Name = SnapshotName(e.(map[string]interface{})["name"].(string))
 		}
 		if _, isSet := e.(map[string]interface{})["parent"]; isSet {
-			list[i].Parent = e.(map[string]interface{})["parent"].(string)
+			list[i].Parent = SnapshotName(e.(map[string]interface{})["parent"].(string))
 		}
 		if _, isSet := e.(map[string]interface{})["snaptime"]; isSet {
 			list[i].SnapTime = uint(e.(map[string]interface{})["snaptime"].(float64))
@@ -122,3 +141,33 @@ func (raw rawSnapshots) FormatSnapshotsTree() (tree []*Snapshot) {
 	}
 	return
 }
+
+// Minimum length of 3 characters
+// Maximum length of 40 characters
+// First character must be a letter
+// Must only contain the following characters: abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_
+type SnapshotName string
+
+const (
+	SnapshotName_Error_IllegalCharacters string = "SnapshotName must only contain the following characters: abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_"
+	SnapshotName_Error_MaxLength         string = "SnapshotName must be at most 40 characters long"
+	SnapshotName_Error_MinLength         string = "SnapshotName must be at least 3 characters long"
+	SnapshotName_Error_StartNoLetter     string = "SnapshotName must start with a letter"
+)
+
+func (name SnapshotName) Validate() error {
+	regex, _ := regexp.Compile(`^([a-zA-Z])([a-z]|[A-Z]|[0-9]|_|-){2,39}$`)
+	if !regex.Match([]byte(name)) {
+		if len(name) < 3 {
+			return fmt.Errorf(SnapshotName_Error_MinLength)
+		}
+		if len(name) > 40 {
+			return fmt.Errorf(SnapshotName_Error_MaxLength)
+		}
+		if !unicode.IsLetter(rune(name[0])) {
+			return fmt.Errorf(SnapshotName_Error_StartNoLetter)
+		}
+		return fmt.Errorf(SnapshotName_Error_IllegalCharacters)
+	}
+	return nil
+}

proxmox/snapshot_test.go

@@ -2,11 +2,41 @@ package proxmox
 
 import (
 	"encoding/json"
+	"errors"
 	"testing"
 
+	"github.com/Telmate/proxmox-api-go/test/data/test_data_snapshot"
 	"github.com/stretchr/testify/require"
 )
 
+func Test_ConfigSnapshot_Validate(t *testing.T) {
+	tests := []struct {
+		name  string
+		input ConfigSnapshot
+		err   bool
+	}{
+		// Valid
+		{name: "Valid ConfigSnapshot",
+			input: ConfigSnapshot{Name: SnapshotName(test_data_snapshot.SnapshotName_Max_Legal())},
+		},
+		// Invalid
+		{name: "Invalid ConfigSnapshot",
+			input: ConfigSnapshot{Name: SnapshotName(test_data_snapshot.SnapshotName_Max_Illegal())},
+			err:   true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(*testing.T) {
+			if test.err {
+				require.Error(t, test.input.Validate(), test.name)
+			} else {
+				require.NoError(t, test.input.Validate(), test.name)
+			}
+		})
+	}
+}
+
+// TODO rename this test
 // Test the formatting logic to build the tree of snapshots
 func Test_FormatSnapshotsTree(t *testing.T) {
 	input := test_FormatSnapshots_Input()
@@ -17,6 +47,7 @@ func Test_FormatSnapshotsTree(t *testing.T) {
 	}
 }
 
+// TODO rename this test
 // Test the formatting logic to build the list of snapshots
 func Test_FormatSnapshotsList(t *testing.T) {
 	input := test_FormatSnapshots_Input()
@@ -204,3 +235,38 @@ func test_FormatSnapshotsList_Output() []string {
 		"name":"bba","time":1666362071,"parent":"bb"},{
 		"name":"bbb","time":1666362062,"parent":"bb"}]`}
 }
+
+func Test_SnapshotName_Validate(t *testing.T) {
+	tests := []struct {
+		name  string
+		input []string
+		err   error
+	}{
+		// Valid
+		{name: "Valid", input: test_data_snapshot.SnapshotName_Legal()},
+		// Invalid
+		{name: "Invalid SnapshotName_Error_MinLength",
+			input: []string{"", test_data_snapshot.SnapshotName_Min_Illegal()},
+			err:   errors.New(SnapshotName_Error_MinLength),
+		},
+		{name: "Invalid SnapshotName_Error_MaxLength",
+			input: []string{test_data_snapshot.SnapshotName_Max_Illegal()},
+			err:   errors.New(SnapshotName_Error_MaxLength),
+		},
+		{name: "Invalid SnapshotName_Error_StartNoLetter",
+			input: test_data_snapshot.SnapshotName_Start_Illegal(),
+			err:   errors.New(SnapshotName_Error_StartNoLetter),
+		},
+		{name: "Invalid SnapshotName_Error_StartNoLetter",
+			input: test_data_snapshot.SnapshotName_Character_Illegal(),
+			err:   errors.New(SnapshotName_Error_IllegalCharacters),
+		},
+	}
+	for _, test := range tests {
+		for _, snapshot := range test.input {
+			t.Run(test.name+" :"+snapshot, func(*testing.T) {
+				require.Equal(t, SnapshotName(snapshot).Validate(), test.err, test.name+" :"+snapshot)
+			})
+		}
+	}
+}