Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to pass and override default hard coded api version #274

Merged
merged 12 commits into from
Dec 16, 2019
Merged

Allow to pass and override default hard coded api version #274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ff1b8db
Add change which allows to pass and override default hard coded api v…
vbauzys Dec 6, 2019
1666e87
Add back function to be back compatible
vbauzys Dec 9, 2019
5fd40cb
Improve docs
vbauzys Dec 9, 2019
a4a3f60
Add changelog
vbauzys Dec 9, 2019
398d990
Improve docs and changelog
vbauzys Dec 9, 2019
1ae3d74
Improve docs
vbauzys Dec 9, 2019
4b22e8b
Add test
vbauzys Dec 9, 2019
7d62c82
Merge branch 'master' into api-version-override
vbauzys Dec 9, 2019
448b61f
Add directive
vbauzys Dec 9, 2019
dd83fc4
Improved test
vbauzys Dec 16, 2019
4c4f580
Merge branch 'master' into api-version-override
vbauzys Dec 16, 2019
fdedb01
Fixes
vbauzys Dec 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
`DeleteNsxvIpSetByName` [#269](https://github.com/vmware/go-vcloud-director/pull/269)
* Added `UpdateDhcpRelay`, `GetDhcpRelay` and `ResetDhcpRelay` methods for Edge Gatway DHCP relay
management [#271](https://github.com/vmware/go-vcloud-director/pull/271)
* Added methods which allow override API versions `NewRequestWitNotEncodedParamsWithApiVersion`,
`ExecuteTaskRequestWithApiVersion`, `ExecuteRequestWithoutResponseWithApiVersion`,
`ExecuteRequestWithApiVersion` [#274](https://github.com/vmware/go-vcloud-director/pull/274)
* Moved `VCDClient.supportedVersions` to `VCDClient.Client.supportedVersions` [#274](https://github.com/vmware/go-vcloud-director/pull/274)

BUGS FIXED:
* Remove parentheses from filtering since they weren't treated correctly in some environment [#256]
Expand Down
115 changes: 105 additions & 10 deletions govcd/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ type Client struct {
// where vCloud director may take time to respond and retry mechanism is needed.
// This must be >0 to avoid instant timeout errors.
MaxRetryTimeout int

supportedVersions SupportedVersions // Versions from /api/versions endpoint
}

// The header key used by default to set the authorization token.
Expand Down Expand Up @@ -127,6 +129,17 @@ func ContainsNotFound(err error) bool {

// NewRequestWitNotEncodedParams allows passing complex values params that shouldn't be encoded like for queries. e.g. /query?filter=name=foo
func (cli *Client) NewRequestWitNotEncodedParams(params map[string]string, notEncodedParams map[string]string, method string, reqUrl url.URL, body io.Reader) *http.Request {
return cli.NewRequestWitNotEncodedParamsWithApiVersion(params, notEncodedParams, method, reqUrl, body, cli.APIVersion)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a few tests for the new functions, to make sure that:

  1. They are really using the intended API version.
  2. Subsequent requests using the regular Execute* methods are not affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you share your idea how to test that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a possible way:

  1. Add fields to a type that was enhanced in API versions later than the current one.
  2. Run a GET that retrieves the type with the enhanced fields
  3. Make sure the fields are non-empty
  4. Run the same query without the new API version
  5. Make sure that the enhanced fields are empty

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more ideas:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first option could be valid with on specific vCD version or I have to find valid for all versions, which is lottery
I'll check possibility with response headers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test added

}

// NewRequestWitNotEncodedParamsWithApiVersion allows passing complex values params that shouldn't be encoded like for queries. e.g. /query?filter=name=foo
// * params - request parameters
// * notEncodedParams - request parameters which will be added not encoded
// * method - request type
// * reqUrl - request url
// * body - request body
// * apiVersion - provided Api version overrides default Api version value used in request parameter
func (cli *Client) NewRequestWitNotEncodedParamsWithApiVersion(params map[string]string, notEncodedParams map[string]string, method string, reqUrl url.URL, body io.Reader, apiVersion string) *http.Request {
vbauzys marked this conversation as resolved.
Show resolved Hide resolved
reqValues := url.Values{}

// Build up our request parameters
Expand All @@ -152,7 +165,7 @@ func (cli *Client) NewRequestWitNotEncodedParams(params map[string]string, notEn
// Add the authorization header
req.Header.Add(cli.VCDAuthHeader, cli.VCDToken)
// Add the Accept header for VCD
req.Header.Add("Accept", "application/*+xml;version="+cli.APIVersion)
req.Header.Add("Accept", "application/*+xml;version="+apiVersion)
}

// Avoids passing data if the logging of requests is disabled
Expand Down Expand Up @@ -188,6 +201,12 @@ func (cli *Client) NewRequest(params map[string]string, method string, reqUrl ur
return cli.NewRequestWitNotEncodedParams(params, nil, method, reqUrl, body)
}

// NewRequestWithApiVersion creates a new HTTP request and applies necessary auth headers if set.
// Allows to override default request API Version
func (cli *Client) NewRequestWithApiVersion(params map[string]string, method string, reqUrl url.URL, body io.Reader, apiVersion string) *http.Request {
return cli.NewRequestWitNotEncodedParamsWithApiVersion(params, nil, method, reqUrl, body, apiVersion)
}

// ParseErr takes an error XML resp, error interface for unmarshaling and returns a single string for
// use in error messages.
func ParseErr(resp *http.Response, errType error) error {
Expand Down Expand Up @@ -284,12 +303,36 @@ func checkRespWithErrType(resp *http.Response, err, errType error) (*http.Respon
// payload - XML struct which will be marshalled and added as body/payload
// E.g. client.ExecuteTaskRequest(updateDiskLink.HREF, http.MethodPut, updateDiskLink.Type, "error updating disk: %s", xmlPayload)
func (client *Client) ExecuteTaskRequest(pathURL, requestType, contentType, errorMessage string, payload interface{}) (Task, error) {
return client.executeTaskRequest(pathURL, requestType, contentType, errorMessage, payload, client.APIVersion)
}

// Helper function creates request, runs it, checks response and parses task from response.
// pathURL - request URL
// requestType - HTTP method type
// contentType - value to set for "Content-Type"
// errorMessage - error message to return when error happens
// payload - XML struct which will be marshalled and added as body/payload
// apiVersion - api version which will be used in request
// E.g. client.ExecuteTaskRequest(updateDiskLink.HREF, http.MethodPut, updateDiskLink.Type, "error updating disk: %s", xmlPayload)
func (client *Client) ExecuteTaskRequestWithApiVersion(pathURL, requestType, contentType, errorMessage string, payload interface{}, apiVersion string) (Task, error) {
return client.executeTaskRequest(pathURL, requestType, contentType, errorMessage, payload, apiVersion)
}

// Helper function creates request, runs it, checks response and parses task from response.
// pathURL - request URL
// requestType - HTTP method type
// contentType - value to set for "Content-Type"
// errorMessage - error message to return when error happens
// payload - XML struct which will be marshalled and added as body/payload
// apiVersion - api version which will be used in request
// E.g. client.ExecuteTaskRequest(updateDiskLink.HREF, http.MethodPut, updateDiskLink.Type, "error updating disk: %s", xmlPayload)
func (client *Client) executeTaskRequest(pathURL, requestType, contentType, errorMessage string, payload interface{}, apiVersion string) (Task, error) {

if !isMessageWithPlaceHolder(errorMessage) {
return Task{}, fmt.Errorf("error message has to include place holder for error")
}

resp, err := executeRequest(pathURL, requestType, contentType, payload, client)
resp, err := executeRequestWithApiVersion(pathURL, requestType, contentType, payload, client, apiVersion)
if err != nil {
return Task{}, fmt.Errorf(errorMessage, err)
}
Expand Down Expand Up @@ -317,12 +360,36 @@ func (client *Client) ExecuteTaskRequest(pathURL, requestType, contentType, erro
// payload - XML struct which will be marshalled and added as body/payload
// E.g. client.ExecuteRequestWithoutResponse(catalogItemHREF.String(), http.MethodDelete, "", "error deleting Catalog item: %s", nil)
func (client *Client) ExecuteRequestWithoutResponse(pathURL, requestType, contentType, errorMessage string, payload interface{}) error {
return client.executeRequestWithoutResponse(pathURL, requestType, contentType, errorMessage, payload, client.APIVersion)
}

// Helper function creates request, runs it, checks response and do not expect any values from it.
// pathURL - request URL
// requestType - HTTP method type
// contentType - value to set for "Content-Type"
// errorMessage - error message to return when error happens
// payload - XML struct which will be marshalled and added as body/payload
// apiVersion - api version which will be used in request
// E.g. client.ExecuteRequestWithoutResponse(catalogItemHREF.String(), http.MethodDelete, "", "error deleting Catalog item: %s", nil)
func (client *Client) ExecuteRequestWithoutResponseWithApiVersion(pathURL, requestType, contentType, errorMessage string, payload interface{}, apiVersion string) error {
return client.executeRequestWithoutResponse(pathURL, requestType, contentType, errorMessage, payload, apiVersion)
}

// Helper function creates request, runs it, checks response and do not expect any values from it.
// pathURL - request URL
// requestType - HTTP method type
// contentType - value to set for "Content-Type"
// errorMessage - error message to return when error happens
// payload - XML struct which will be marshalled and added as body/payload
// apiVersion - api version which will be used in request
// E.g. client.ExecuteRequestWithoutResponse(catalogItemHREF.String(), http.MethodDelete, "", "error deleting Catalog item: %s", nil)
func (client *Client) executeRequestWithoutResponse(pathURL, requestType, contentType, errorMessage string, payload interface{}, apiVersion string) error {

if !isMessageWithPlaceHolder(errorMessage) {
return fmt.Errorf("error message has to include place holder for error")
}

resp, err := executeRequest(pathURL, requestType, contentType, payload, client)
resp, err := executeRequestWithApiVersion(pathURL, requestType, contentType, payload, client, apiVersion)
if err != nil {
return fmt.Errorf(errorMessage, err)
}
Expand Down Expand Up @@ -350,12 +417,40 @@ func (client *Client) ExecuteRequestWithoutResponse(pathURL, requestType, conten
// E.g. unmarshalledAdminOrg := &types.AdminOrg{}
// client.ExecuteRequest(adminOrg.AdminOrg.HREF, http.MethodGet, "", "error refreshing organization: %s", nil, unmarshalledAdminOrg)
func (client *Client) ExecuteRequest(pathURL, requestType, contentType, errorMessage string, payload, out interface{}) (*http.Response, error) {
return client.executeRequest(pathURL, requestType, contentType, errorMessage, payload, out, client.APIVersion)
}

// Helper function creates request, runs it, check responses and parses out interface from response.
// pathURL - request URL
// requestType - HTTP method type
// contentType - value to set for "Content-Type"
// errorMessage - error message to return when error happens
// payload - XML struct which will be marshalled and added as body/payload
// out - structure to be used for unmarshalling xml
// apiVersion - api version which will be used in request
// E.g. unmarshalledAdminOrg := &types.AdminOrg{}
// client.ExecuteRequest(adminOrg.AdminOrg.HREF, http.MethodGet, "", "error refreshing organization: %s", nil, unmarshalledAdminOrg)
func (client *Client) ExecuteRequestWithApiVersion(pathURL, requestType, contentType, errorMessage string, payload, out interface{}, apiVersion string) (*http.Response, error) {
return client.executeRequest(pathURL, requestType, contentType, errorMessage, payload, out, apiVersion)
}

// Helper function creates request, runs it, check responses and parses out interface from response.
// pathURL - request URL
// requestType - HTTP method type
// contentType - value to set for "Content-Type"
// errorMessage - error message to return when error happens
// payload - XML struct which will be marshalled and added as body/payload
// out - structure to be used for unmarshalling xml
// apiVersion - api version which will be used in request
// E.g. unmarshalledAdminOrg := &types.AdminOrg{}
// client.ExecuteRequest(adminOrg.AdminOrg.HREF, http.MethodGet, "", "error refreshing organization: %s", nil, unmarshalledAdminOrg)
func (client *Client) executeRequest(pathURL, requestType, contentType, errorMessage string, payload, out interface{}, apiVersion string) (*http.Response, error) {

if !isMessageWithPlaceHolder(errorMessage) {
return &http.Response{}, fmt.Errorf("error message has to include place holder for error")
}

resp, err := executeRequest(pathURL, requestType, contentType, payload, client)
resp, err := executeRequestWithApiVersion(pathURL, requestType, contentType, payload, client, apiVersion)
if err != nil {
return resp, fmt.Errorf(errorMessage, err)
}
Expand Down Expand Up @@ -390,7 +485,7 @@ func (client *Client) ExecuteParamRequestWithCustomError(pathURL string, params
return &http.Response{}, fmt.Errorf("error message has to include place holder for error")
}

resp, err := executeRequestCustomErr(pathURL, params, requestType, contentType, payload, client, errType)
resp, err := executeRequestCustomErr(pathURL, params, requestType, contentType, payload, client, errType, client.APIVersion)
if err != nil {
return &http.Response{}, fmt.Errorf(errorMessage, err)
}
Expand All @@ -413,12 +508,12 @@ func (client *Client) ExecuteParamRequestWithCustomError(pathURL string, params
}

// executeRequest does executeRequestCustomErr and checks for vCD errors in API response
func executeRequest(pathURL, requestType, contentType string, payload interface{}, client *Client) (*http.Response, error) {
return executeRequestCustomErr(pathURL, map[string]string{}, requestType, contentType, payload, client, &types.Error{})
func executeRequestWithApiVersion(pathURL, requestType, contentType string, payload interface{}, client *Client, apiVersion string) (*http.Response, error) {
return executeRequestCustomErr(pathURL, map[string]string{}, requestType, contentType, payload, client, &types.Error{}, apiVersion)
}

// executeRequestCustomErr performs request and unmarshals API error to errType if not 2xx status was returned
func executeRequestCustomErr(pathURL string, params map[string]string, requestType, contentType string, payload interface{}, client *Client, errType error) (*http.Response, error) {
func executeRequestCustomErr(pathURL string, params map[string]string, requestType, contentType string, payload interface{}, client *Client, errType error, apiVersion string) (*http.Response, error) {
url, _ := url.ParseRequestURI(pathURL)

var req *http.Request
Expand All @@ -431,10 +526,10 @@ func executeRequestCustomErr(pathURL string, params map[string]string, requestTy
}
body := bytes.NewBufferString(xml.Header + string(marshaledXml))

req = client.NewRequest(params, requestType, *url, body)
req = client.NewRequestWithApiVersion(params, requestType, *url, body, apiVersion)

default:
req = client.NewRequest(params, requestType, *url, nil)
req = client.NewRequestWithApiVersion(params, requestType, *url, nil, apiVersion)
}

if contentType != "" {
Expand Down
35 changes: 35 additions & 0 deletions govcd/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// +build api functional ALL

/*
* Copyright 2018 VMware, Inc. All rights reserved. Licensed under the Apache v2 License.
*/
Expand All @@ -8,8 +10,11 @@ package govcd

import (
"fmt"
"net/http"
"os"
"testing"

. "gopkg.in/check.v1"
)

var testingTags = make(map[string]string)
Expand Down Expand Up @@ -95,3 +100,33 @@ func TestTags(t *testing.T) {
showTags()
}
}

// Test_NewRequestWitNotEncodedParamsWithApiVersion verifies that api version override works
func (vcd *TestVCD) Test_NewRequestWitNotEncodedParamsWithApiVersion(check *C) {
fmt.Printf("Running: %s\n", check.TestName())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double check the outputs as they are missing new lines. Heres' how it looks.

START: api_test.go:105: TestVCD.Test_NewRequestWitNotEncodedParamsWithApiVersion
Running: TestVCD.Test_NewRequestWitNotEncodedParamsWithApiVersion
Test: TestVCD.Test_NewRequestWitNotEncodedParamsWithApiVersion run with api Version: 33.0
PASS: api_test.go:105: TestVCD.Test_NewRequestWitNotEncodedParamsWithApiVersion 0.191s

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. This is fine as it is as planned.

queryUlr := vcd.client.Client.VCDHREF
queryUlr.Path += "/query"

apiVersion, err := vcd.client.Client.maxSupportedVersion()
check.Assert(err, IsNil)

req := vcd.client.Client.NewRequestWitNotEncodedParamsWithApiVersion(nil, map[string]string{"type": "media",
"filter": "name==any"}, http.MethodGet, queryUlr, nil, apiVersion)

resp, err := checkResp(vcd.client.Client.Http.Do(req))
check.Assert(err, IsNil)

check.Assert(resp.Header.Get("Content-Type"), Equals, "application/vnd.vmware.vcloud.query.records+xml;version="+apiVersion)

// Repeats the call without API version change
req = vcd.client.Client.NewRequestWitNotEncodedParams(nil, map[string]string{"type": "media",
"filter": "name==any"}, http.MethodGet, queryUlr, nil)

resp, err = checkResp(vcd.client.Client.Http.Do(req))
check.Assert(err, IsNil)

// Checks that the regularAPI version was not affected by the previous call
check.Assert(resp.Header.Get("Content-Type"), Equals, "application/vnd.vmware.vcloud.query.records+xml;version="+vcd.client.Client.APIVersion)

fmt.Printf("Test: %s run with api Version: %s\n", check.TestName(), apiVersion)
}
13 changes: 6 additions & 7 deletions govcd/api_vcd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,20 @@ import (
type VCDClientOption func(*VCDClient) error

type VCDClient struct {
Client Client // Client for the underlying VCD instance
sessionHREF url.URL // HREF for the session API
QueryHREF url.URL // HREF for the query API
Mutex sync.Mutex
supportedVersions SupportedVersions // Versions from /api/versions endpoint
Client Client // Client for the underlying VCD instance
sessionHREF url.URL // HREF for the session API
QueryHREF url.URL // HREF for the query API
Mutex sync.Mutex
}

func (vcdCli *VCDClient) vcdloginurl() error {
if err := vcdCli.validateAPIVersion(); err != nil {
if err := vcdCli.Client.validateAPIVersion(); err != nil {
return fmt.Errorf("could not find valid version for login: %s", err)
}

// find login address matching the API version
var neededVersion VersionInfo
for _, versionInfo := range vcdCli.supportedVersions.VersionInfos {
for _, versionInfo := range vcdCli.Client.supportedVersions.VersionInfos {
if versionInfo.Version == vcdCli.Client.APIVersion {
neededVersion = versionInfo
break
Expand Down
Loading