Patch GetSpecificApiVersionOnCondition to respect minimum API version…

… (client.APIVersion) (#658)

.changes/v2.25.0/658-improvements.md

@@ -0,0 +1,2 @@
+* Fix bug in `Client.GetSpecificApiVersionOnCondition` that could result in using unsupported API
+  version [GH-658]

govcd/api_vcd_versions.go

@@ -231,13 +231,25 @@ func (client *Client) validateAPIVersion() error {
 	return nil
 }
 
-// GetSpecificApiVersionOnCondition returns default version or wantedApiVersion if it is connected to version
-// described in vcdApiVersionCondition
-// f.e. values ">= 32.0", "32.0" returns 32.0 if vCD version is above or 9.7
-func (client *Client) GetSpecificApiVersionOnCondition(vcdApiVersionCondition, wantedApiVersion string) string {
+// GetSpecificApiVersionOnCondition returns default version or wantedApiVersion if it is connected
+// to version described in vcdApiVersionCondition e.g. values ">= 32.0", "32.0" returns 32.0 if vCD
+// version is above or 9.7
+// Note. This function will always respect minimum supported API version which is defined in
+// client.APIVersion. If the wantedApiVersionOrMinimumRequired is less than minimum supported
+// version, this function will return the minimum supported version. This means that it must be be
+// well tested when client.APIVersion is bumped to avoid unexpected errors due to newer API version
+// being used.
+func (client *Client) GetSpecificApiVersionOnCondition(vcdApiVersionCondition, wantedApiVersionOrMinimumRequired string) string {
 	apiVersion := client.APIVersion
 	if client.APIVCDMaxVersionIs(vcdApiVersionCondition) {
-		apiVersion = wantedApiVersion
+		versionConstraint := fmt.Sprintf(">= %s", apiVersion)
+		// only if the version is not less than minimum supported version 'client.APIVersion' we can
+		// specify 'wantedApiVersionOrMinimumRequired'
+		if matches, err := client.apiVersionMatchesConstraint(wantedApiVersionOrMinimumRequired, versionConstraint); err == nil && matches {
+			return wantedApiVersionOrMinimumRequired
+		}
+		util.Logger.Printf("[TRACE] API version %s does not satisfy constraints '%s'. Will use minimum supported version '%s'.",
+			wantedApiVersionOrMinimumRequired, versionConstraint, apiVersion)
 	}
 	return apiVersion
 }

govcd/api_vcd_versions_test.go

@@ -184,3 +184,37 @@ func (vcd *TestVCD) Test_GetVcdVersion(check *C) {
 	check.Assert(err, IsNil)
 	check.Assert(result, Equals, false)
 }
+
+func (vcd *TestVCD) TestClient_GetSpecificApiVersionOnCondition(check *C) {
+	clientApiVersion := vcd.client.Client.APIVersion
+	maxApiSupportVersion, err := vcd.client.Client.MaxSupportedVersion()
+	check.Assert(err, IsNil)
+
+	fmt.Println("# API minimum required:" + vcd.client.Client.APIVersion)
+	fmt.Println("# API maximum:" + maxApiSupportVersion)
+
+	type args struct {
+		versionCondition string
+		wantedVersion    string
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{name: "ClientHigherThanRequired", args: args{versionCondition: ">=32", wantedVersion: "32"}, want: clientApiVersion},
+		{name: "ClientLowerThanRequired", args: args{versionCondition: ">=72.0", wantedVersion: "72.0"}, want: clientApiVersion},
+		{name: "ElevateToMaximumSupported", args: args{versionCondition: ">= " + maxApiSupportVersion, wantedVersion: maxApiSupportVersion}, want: maxApiSupportVersion},
+	}
+
+	for _, tt := range tests {
+		fmt.Printf("## " + tt.name + ": ")
+
+		if got := vcd.client.Client.GetSpecificApiVersionOnCondition(tt.args.versionCondition, tt.args.wantedVersion); got != tt.want {
+			check.Errorf("Client.GetSpecificApiVersionOnCondition() = %v, want %v", got, tt.want)
+		} else {
+			fmt.Printf("Got %s from GetSpecificApiVersionOnCondition(\"%s\", \"%s\")\n",
+				got, tt.args.versionCondition, tt.args.wantedVersion)
+		}
+	}
+}

govcd/disk.go

@@ -83,9 +83,8 @@ func (vdc *Vdc) CreateDisk(diskCreateParams *types.DiskCreateParams) (Task, erro
 
 	disk := NewDisk(vdc.client)
 
-	_, err = vdc.client.ExecuteRequestWithApiVersion(createDiskLink.HREF, http.MethodPost,
-		createDiskLink.Type, "error create disk: %s", diskCreateParams, disk.Disk,
-		vdc.client.GetSpecificApiVersionOnCondition(">= 36.0", "36.0"))
+	_, err = vdc.client.ExecuteRequest(createDiskLink.HREF, http.MethodPost,
+		createDiskLink.Type, "error create disk: %s", diskCreateParams, disk.Disk)
 	if err != nil {
 		return Task{}, err
 	}
@@ -165,9 +164,8 @@ func (disk *Disk) Update(newDiskInfo *types.Disk) (Task, error) {
 	}
 
 	// Return the task
-	return disk.client.ExecuteTaskRequestWithApiVersion(updateDiskLink.HREF, http.MethodPut,
-		updateDiskLink.Type, "error updating disk: %s", xmlPayload,
-		disk.client.GetSpecificApiVersionOnCondition(">= 36.0", "36.0"))
+	return disk.client.ExecuteTaskRequest(updateDiskLink.HREF, http.MethodPut,
+		updateDiskLink.Type, "error updating disk: %s", xmlPayload)
 }
 
 // Remove an independent disk
@@ -226,9 +224,8 @@ func (disk *Disk) Refresh() error {
 
 	unmarshalledDisk := &types.Disk{}
 
-	_, err := disk.client.ExecuteRequestWithApiVersion(disk.Disk.HREF, http.MethodGet,
-		"", "error refreshing independent disk: %s", nil, unmarshalledDisk,
-		disk.client.GetSpecificApiVersionOnCondition(">= 36.0", "36.0"))
+	_, err := disk.client.ExecuteRequest(disk.Disk.HREF, http.MethodGet,
+		"", "error refreshing independent disk: %s", nil, unmarshalledDisk)
 	if err != nil {
 		return err
 	}
@@ -322,9 +319,8 @@ func (vdc *Vdc) QueryDisk(diskName string) (DiskRecord, error) {
 		typeMedia = "adminDisk"
 	}
 
-	results, err := vdc.QueryWithNotEncodedParamsWithApiVersion(nil, map[string]string{"type": typeMedia,
-		"filter": "name==" + url.QueryEscape(diskName) + ";vdc==" + vdc.vdcId(), "filterEncoded": "true"},
-		vdc.client.GetSpecificApiVersionOnCondition(">= 36.0", "36.0"))
+	results, err := vdc.QueryWithNotEncodedParams(nil, map[string]string{"type": typeMedia,
+		"filter": "name==" + url.QueryEscape(diskName) + ";vdc==" + vdc.vdcId(), "filterEncoded": "true"})
 	if err != nil {
 		return DiskRecord{}, fmt.Errorf("error querying disk %s", err)
 	}
@@ -357,9 +353,8 @@ func (vdc *Vdc) QueryDisks(diskName string) (*[]*types.DiskRecordType, error) {
 		typeMedia = "adminDisk"
 	}
 
-	results, err := vdc.QueryWithNotEncodedParamsWithApiVersion(nil, map[string]string{"type": typeMedia,
-		"filter": "name==" + url.QueryEscape(diskName) + ";vdc==" + vdc.vdcId(), "filterEncoded": "true"},
-		vdc.client.GetSpecificApiVersionOnCondition(">= 36.0", "36.0"))
+	results, err := vdc.QueryWithNotEncodedParams(nil, map[string]string{"type": typeMedia,
+		"filter": "name==" + url.QueryEscape(diskName) + ";vdc==" + vdc.vdcId(), "filterEncoded": "true"})
 	if err != nil {
 		return nil, fmt.Errorf("error querying disks %s", err)
 	}
@@ -379,9 +374,8 @@ func (vdc *Vdc) GetDiskByHref(diskHref string) (*Disk, error) {
 	util.Logger.Printf("[TRACE] Get Disk By Href: %s\n", diskHref)
 	Disk := NewDisk(vdc.client)
 
-	_, err := vdc.client.ExecuteRequestWithApiVersion(diskHref, http.MethodGet,
-		"", "error retrieving Disk: %s", nil, Disk.Disk,
-		vdc.client.GetSpecificApiVersionOnCondition(">= 36.0", "36.0"))
+	_, err := vdc.client.ExecuteRequest(diskHref, http.MethodGet,
+		"", "error retrieving Disk: %s", nil, Disk.Disk)
 	if err != nil && (strings.Contains(err.Error(), "MajorErrorCode:403") || strings.Contains(err.Error(), "does not exist")) {
 		return nil, ErrorEntityNotFound
 	}