From 4d0266ecb481816f463c65d29b7d8744229e7731 Mon Sep 17 00:00:00 2001 From: Dainius S Date: Wed, 2 Sep 2020 09:42:39 +0300 Subject: [PATCH 1/7] Improve Roles example for OpenAPI --- govcd/roles.go | 45 +++++++++++++++++++++++++++------------------ govcd/roles_test.go | 35 ++++++++++++++++------------------- 2 files changed, 43 insertions(+), 37 deletions(-) diff --git a/govcd/roles.go b/govcd/roles.go index 25f66cfef..0569f7075 100644 --- a/govcd/roles.go +++ b/govcd/roles.go @@ -11,13 +11,14 @@ import ( "github.com/vmware/go-vcloud-director/v2/types/v56" ) -type OpenApiRole struct { +// Role uses OpenAPI endpoint to operate user roles +type Role struct { Role *types.Role client *Client } // GetOpenApiRoleById retrieves role by given ID -func (adminOrg *AdminOrg) GetOpenApiRoleById(id string) (*OpenApiRole, error) { +func (adminOrg *AdminOrg) GetOpenApiRoleById(id string) (*Role, error) { endpoint := types.OpenApiPathVersion1_0_0 + types.OpenApiEndpointRoles minimumApiVersion, err := adminOrg.client.checkOpenApiEndpointCompatibility(endpoint) if err != nil { @@ -33,7 +34,7 @@ func (adminOrg *AdminOrg) GetOpenApiRoleById(id string) (*OpenApiRole, error) { return nil, err } - role := &OpenApiRole{ + role := &Role{ Role: &types.Role{}, client: adminOrg.client, } @@ -48,7 +49,7 @@ func (adminOrg *AdminOrg) GetOpenApiRoleById(id string) (*OpenApiRole, error) { // GetAllOpenApiRoles retrieves all roles using OpenAPI endpoint. Query parameters can be supplied to perform additional // filtering -func (adminOrg *AdminOrg) GetAllOpenApiRoles(queryParameters url.Values) ([]*types.Role, error) { +func (adminOrg *AdminOrg) GetAllOpenApiRoles(queryParameters url.Values) ([]Role, error) { endpoint := types.OpenApiPathVersion1_0_0 + types.OpenApiEndpointRoles minimumApiVersion, err := adminOrg.client.checkOpenApiEndpointCompatibility(endpoint) if err != nil { @@ -60,35 +61,43 @@ func (adminOrg *AdminOrg) GetAllOpenApiRoles(queryParameters url.Values) ([]*typ return nil, err } - responses := []*types.Role{{}} - - err = adminOrg.client.OpenApiGetAllItems(minimumApiVersion, urlRef, queryParameters, &responses) + typeResponses := []*types.Role{{}} + err = adminOrg.client.OpenApiGetAllItems(minimumApiVersion, urlRef, queryParameters, &typeResponses) if err != nil { return nil, err } - return responses, nil + // Wrap all typeResponses into Role types with client + returnRoles := make([]Role, len(typeResponses)) + for sliceIndex := range typeResponses { + returnRoles[sliceIndex] = Role{ + Role: typeResponses[sliceIndex], + client: adminOrg.client, + } + } + + return returnRoles, nil } -// Create creates a new role using OpenAPI endpoint -func (role *OpenApiRole) Create(newRole *types.Role) (*OpenApiRole, error) { +// CreateRole creates a new role using OpenAPI endpoint +func (adminOrg *AdminOrg) CreateRole(newRole *types.Role) (*Role, error) { endpoint := types.OpenApiPathVersion1_0_0 + types.OpenApiEndpointRoles - minimumApiVersion, err := role.client.checkOpenApiEndpointCompatibility(endpoint) + minimumApiVersion, err := adminOrg.client.checkOpenApiEndpointCompatibility(endpoint) if err != nil { return nil, err } - urlRef, err := role.client.OpenApiBuildEndpoint(endpoint) + urlRef, err := adminOrg.client.OpenApiBuildEndpoint(endpoint) if err != nil { return nil, err } - returnRole := &OpenApiRole{ + returnRole := &Role{ Role: &types.Role{}, - client: role.client, + client: adminOrg.client, } - err = role.client.OpenApiPostItem(minimumApiVersion, urlRef, nil, newRole, returnRole.Role) + err = adminOrg.client.OpenApiPostItem(minimumApiVersion, urlRef, nil, newRole, returnRole.Role) if err != nil { return nil, fmt.Errorf("error creating role: %s", err) } @@ -97,7 +106,7 @@ func (role *OpenApiRole) Create(newRole *types.Role) (*OpenApiRole, error) { } // Update updates existing OpenAPI role -func (role *OpenApiRole) Update() (*OpenApiRole, error) { +func (role *Role) Update() (*Role, error) { endpoint := types.OpenApiPathVersion1_0_0 + types.OpenApiEndpointRoles minimumApiVersion, err := role.client.checkOpenApiEndpointCompatibility(endpoint) if err != nil { @@ -113,7 +122,7 @@ func (role *OpenApiRole) Update() (*OpenApiRole, error) { return nil, err } - returnRole := &OpenApiRole{ + returnRole := &Role{ Role: &types.Role{}, client: role.client, } @@ -127,7 +136,7 @@ func (role *OpenApiRole) Update() (*OpenApiRole, error) { } // Delete deletes OpenAPI role -func (role *OpenApiRole) Delete() error { +func (role *Role) Delete() error { endpoint := types.OpenApiPathVersion1_0_0 + types.OpenApiEndpointRoles minimumApiVersion, err := role.client.checkOpenApiEndpointCompatibility(endpoint) if err != nil { diff --git a/govcd/roles_test.go b/govcd/roles_test.go index d3e81c6d0..f363596e2 100644 --- a/govcd/roles_test.go +++ b/govcd/roles_test.go @@ -28,14 +28,14 @@ func (vcd *TestVCD) Test_Roles(check *C) { // Step 2.1 - retrieve specific role by using FIQL filter queryParams := url.Values{} - queryParams.Add("filter", "id=="+oneRole.ID) + queryParams.Add("filter", "id=="+oneRole.Role.ID) expectOneRoleResultById, err := adminOrg.GetAllOpenApiRoles(queryParams) check.Assert(err, IsNil) check.Assert(len(expectOneRoleResultById) == 1, Equals, true) // Step 2.2 - retrieve specific role by using endpoint - exactItem, err := adminOrg.GetOpenApiRoleById(oneRole.ID) + exactItem, err := adminOrg.GetOpenApiRoleById(oneRole.Role.ID) check.Assert(err, IsNil) check.Assert(err, IsNil) @@ -46,31 +46,28 @@ func (vcd *TestVCD) Test_Roles(check *C) { } - // Step 3 - Create a new role and ensure it is created as specified by doing deep comparison - - newR := &OpenApiRole{ - client: adminOrg.client, - Role: &types.Role{ - Name: check.TestName(), - Description: "Role created by test", - // This BundleKey is being set by VCD even if it is not sent - BundleKey: "com.vmware.vcloud.undefined.key", - ReadOnly: false, - }, + // Step 3 - CreateRole a new role and ensure it is created as specified by doing deep comparison + + newR := &types.Role{ + Name: check.TestName(), + Description: "Role created by test", + // This BundleKey is being set by VCD even if it is not sent + BundleKey: "com.vmware.vcloud.undefined.key", + ReadOnly: false, } - createdRole, err := newR.Create(newR.Role) + createdRole, err := adminOrg.CreateRole(newR) check.Assert(err, IsNil) // Ensure supplied and created structs differ only by ID - newR.Role.ID = createdRole.Role.ID - check.Assert(createdRole.Role, DeepEquals, newR.Role) + newR.ID = createdRole.Role.ID + check.Assert(createdRole.Role, DeepEquals, newR) // Step 4 - updated created role - newR.Role.Description = "Updated description" - updatedRole, err := newR.Update() + createdRole.Role.Description = "Updated description" + updatedRole, err := createdRole.Update() check.Assert(err, IsNil) - check.Assert(updatedRole.Role, DeepEquals, newR.Role) + check.Assert(updatedRole.Role, DeepEquals, createdRole.Role) // Step 5 - delete created role err = updatedRole.Delete() From 68d4872d1b99a6fde667ae133c4a799ae5d816d5 Mon Sep 17 00:00:00 2001 From: Dainius S Date: Wed, 2 Sep 2020 09:47:02 +0300 Subject: [PATCH 2/7] Align types and add changelog detail --- CHANGELOG.md | 2 +- govcd/roles.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4e6b6880..2929932f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ * Introduce low level OpenAPI client functions `OpenApiGetAllItems`,`OpenApiPostItemSync`,`OpenApiPostItemAsync`, `OpenApiPostItem`, `OpenApiGetItem`, `OpenApiPutItem`, `OpenApiPutItemSync`, `OpenApiPutItemAsync`, `OpenApiDeleteItem`, `OpenApiIsSupported`, `OpenApiBuildEndpoints` -[#325](https://github.com/vmware/go-vcloud-director/pull/325) +[#325](https://github.com/vmware/go-vcloud-director/pull/325), [#333](https://github.com/vmware/go-vcloud-director/pull/333) * Add OVF file upload support in UploadOvf function besides OVA. The input should be OVF file path inside the OVF folder. It will check if input file is XML content type, if yes, skip some OVA steps (like unpacking), if not, keep the old logic. [#323](https://github.com/vmware/go-vcloud-director/pull/323) * Dropped support for VMware Cloud Director 9.5 [#330](https://github.com/vmware/go-vcloud-director/pull/330) * Deprecated Vdc.UploadMediaImage because it no longer works with API V32.0+ [#330](https://github.com/vmware/go-vcloud-director/pull/330) diff --git a/govcd/roles.go b/govcd/roles.go index 0569f7075..cebcef061 100644 --- a/govcd/roles.go +++ b/govcd/roles.go @@ -49,7 +49,7 @@ func (adminOrg *AdminOrg) GetOpenApiRoleById(id string) (*Role, error) { // GetAllOpenApiRoles retrieves all roles using OpenAPI endpoint. Query parameters can be supplied to perform additional // filtering -func (adminOrg *AdminOrg) GetAllOpenApiRoles(queryParameters url.Values) ([]Role, error) { +func (adminOrg *AdminOrg) GetAllOpenApiRoles(queryParameters url.Values) ([]*Role, error) { endpoint := types.OpenApiPathVersion1_0_0 + types.OpenApiEndpointRoles minimumApiVersion, err := adminOrg.client.checkOpenApiEndpointCompatibility(endpoint) if err != nil { @@ -68,9 +68,9 @@ func (adminOrg *AdminOrg) GetAllOpenApiRoles(queryParameters url.Values) ([]Role } // Wrap all typeResponses into Role types with client - returnRoles := make([]Role, len(typeResponses)) + returnRoles := make([]*Role, len(typeResponses)) for sliceIndex := range typeResponses { - returnRoles[sliceIndex] = Role{ + returnRoles[sliceIndex] = &Role{ Role: typeResponses[sliceIndex], client: adminOrg.client, } From 08ba3a672af0563d722c5034a052b4670d97d092 Mon Sep 17 00:00:00 2001 From: Dainius S Date: Wed, 2 Sep 2020 10:54:54 +0300 Subject: [PATCH 3/7] Add test and default pageSize option --- govcd/openapi.go | 25 ++++++++++++++++++- govcd/openapi_unit_test.go | 49 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 govcd/openapi_unit_test.go diff --git a/govcd/openapi.go b/govcd/openapi.go index 7c4d77c7e..9ab601adc 100644 --- a/govcd/openapi.go +++ b/govcd/openapi.go @@ -67,6 +67,8 @@ func (client *Client) OpenApiBuildEndpoint(endpoint ...string) (*url.URL, error) // first crawling pages and accumulating all responses into []json.RawMessage (as strings). Because there is no // intermediate unmarshalling to exact `outType` for every page it unmarshals into response struct in one go. 'outType' // must be a slice of object (e.g. []*types.OpenAPIEdgeGateway) because this response contains slice of structs. +// +// Note. Query parameter 'pageSize' is defaulted to 128 (maximum supported) unless it is specified in queryParams func (client *Client) OpenApiGetAllItems(apiVersion string, urlRef *url.URL, queryParams url.Values, outType interface{}) error { util.Logger.Printf("[TRACE] Getting all items from endpoint %s for parsing into %s type\n", urlRef.String(), reflect.TypeOf(outType)) @@ -75,9 +77,15 @@ func (client *Client) OpenApiGetAllItems(apiVersion string, urlRef *url.URL, que return fmt.Errorf("OpenAPI is not supported on this VCD version") } + // Page size is defaulted to 128 (maximum supported number) to reduce HTTP calls and improve performance unless caller + // provides other value + const pageSize = "128" + newQueryParams := defaultPageSize(queryParams, pageSize) + util.Logger.Printf("[TRACE] Will use 'pageSize' %s", newQueryParams.Get("pageSize")) + // Perform API call to initial endpoint. The function call recursively follows pages using Link headers "nextPage" // until it crawls all results - responses, err := client.openApiGetAllPages(apiVersion, urlRef, queryParams, outType, nil) + responses, err := client.openApiGetAllPages(apiVersion, urlRef, newQueryParams, outType, nil) if err != nil { return fmt.Errorf("error getting all pages for endpoint %s: %s", urlRef.String(), err) } @@ -620,3 +628,18 @@ func jsonRawMessagesToStrings(messages []json.RawMessage) []string { return resultString } + +// defaultPageSize allows to set 'pageSize' query parameter to defaultPageSize if one is not already specified in +// url.Values while preserving all other supplied url.Values +func defaultPageSize(queryParams url.Values, defaultPageSize string) url.Values { + newQueryParams := make(map[string][]string) + if queryParams != nil { + newQueryParams = queryParams + } + + if _, ok := newQueryParams["pageSize"]; !ok { + newQueryParams["pageSize"] = []string{defaultPageSize} + } + + return newQueryParams +} diff --git a/govcd/openapi_unit_test.go b/govcd/openapi_unit_test.go new file mode 100644 index 000000000..fefd7ffba --- /dev/null +++ b/govcd/openapi_unit_test.go @@ -0,0 +1,49 @@ +// +build unit ALL + +package govcd + +import ( + "net/url" + "reflect" + "testing" +) + +func Test_defaultPageSize(t *testing.T) { + type args struct { + queryParams url.Values + defaultPageSize string + } + tests := []struct { + name string + args args + want url.Values + }{ + { + name: "NilQueryParams", + args: args{nil, "128"}, + want: map[string][]string{"pageSize": []string{"128"}}, + }, + { + name: "NotNilQueryParams", + args: args{map[string][]string{"otherField": []string{"randomValue"}}, "128"}, + want: map[string][]string{"pageSize": []string{"128"}, "otherField": []string{"randomValue"}}, + }, + { + name: "CustomPageSize", + args: args{map[string][]string{"pageSize": []string{"1"}}, "128"}, + want: map[string][]string{"pageSize": []string{"1"}}, + }, + { + name: "CustomPageSizeWithOtherFields", + args: args{map[string][]string{"pageSize": []string{"1"}, "otherField": []string{"randomValue"}}, "128"}, + want: map[string][]string{"pageSize": []string{"1"}, "otherField": []string{"randomValue"}}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := defaultPageSize(tt.args.queryParams, tt.args.defaultPageSize); !reflect.DeepEqual(got, tt.want) { + t.Errorf("defaultPageSize() = %v, want %v", got, tt.want) + } + }) + } +} From 0ce95bd1e82fbfd73dae13f7b22fc1020522142d Mon Sep 17 00:00:00 2001 From: Dainius S Date: Wed, 16 Sep 2020 13:36:34 +0300 Subject: [PATCH 4/7] Remove constant and insert defaultPageSize into function --- govcd/openapi.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/govcd/openapi.go b/govcd/openapi.go index 9ab601adc..bd182b06e 100644 --- a/govcd/openapi.go +++ b/govcd/openapi.go @@ -79,8 +79,7 @@ func (client *Client) OpenApiGetAllItems(apiVersion string, urlRef *url.URL, que // Page size is defaulted to 128 (maximum supported number) to reduce HTTP calls and improve performance unless caller // provides other value - const pageSize = "128" - newQueryParams := defaultPageSize(queryParams, pageSize) + newQueryParams := defaultPageSize(queryParams, "128") util.Logger.Printf("[TRACE] Will use 'pageSize' %s", newQueryParams.Get("pageSize")) // Perform API call to initial endpoint. The function call recursively follows pages using Link headers "nextPage" From ed1300466df91dc8408de74a8e6c2ea84a558e61 Mon Sep 17 00:00:00 2001 From: Dainius S Date: Thu, 1 Oct 2020 16:36:18 +0300 Subject: [PATCH 5/7] AudiTrail -> AuditTrail --- govcd/openapi_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/govcd/openapi_test.go b/govcd/openapi_test.go index 674d9e099..de00ca7fe 100644 --- a/govcd/openapi_test.go +++ b/govcd/openapi_test.go @@ -19,9 +19,9 @@ import ( . "gopkg.in/check.v1" ) -// Test_OpenApiRawJsonAudiTrail uses low level GET function to test out that pagination really works. It is an example +// Test_OpenApiRawJsonAuditTrail uses low level GET function to test out that pagination really works. It is an example // how to fetch response from multiple pages in RAW json messages without having defined as struct. -func (vcd *TestVCD) Test_OpenApiRawJsonAudiTrail(check *C) { +func (vcd *TestVCD) Test_OpenApiRawJsonAuditTrail(check *C) { minimumRequiredApiVersion := "33.0" skipOpenApiEndpointTest(vcd, check, "1.0.0/auditTrail", minimumRequiredApiVersion) @@ -50,9 +50,9 @@ func (vcd *TestVCD) Test_OpenApiRawJsonAudiTrail(check *C) { check.Assert(len(matches), Equals, len(allResponses)) } -// Test_OpenApiInlineStructAudiTrail uses low level GET function to test out that get function can unmarshal directly +// Test_OpenApiInlineStructAuditTrail uses low level GET function to test out that get function can unmarshal directly // to user defined inline type -func (vcd *TestVCD) Test_OpenApiInlineStructAudiTrail(check *C) { +func (vcd *TestVCD) Test_OpenApiInlineStructAuditTrail(check *C) { minimumRequiredApiVersion := "33.0" skipOpenApiEndpointTest(vcd, check, "1.0.0/auditTrail", minimumRequiredApiVersion) @@ -60,7 +60,7 @@ func (vcd *TestVCD) Test_OpenApiInlineStructAudiTrail(check *C) { check.Assert(err, IsNil) // Inline type - type AudiTrail struct { + type AuditTrail struct { EventID string `json:"eventId"` Description string `json:"description"` OperatingOrg struct { @@ -91,7 +91,7 @@ func (vcd *TestVCD) Test_OpenApiInlineStructAudiTrail(check *C) { } `json:"additionalProperties"` } - allResponses := []*AudiTrail{{}} + allResponses := []*AuditTrail{{}} // Define FIQL query to find events for the last 24 hours queryParams := url.Values{} From ef3fb0af49b7fb68a5c7c779eaba8a671b04d71a Mon Sep 17 00:00:00 2001 From: Dainius S Date: Fri, 2 Oct 2020 10:36:20 +0300 Subject: [PATCH 6/7] Optimize tests and use copies of *url.URL --- govcd/openapi.go | 100 ++++++++++++++++++++++++++++++------------ govcd/openapi_test.go | 54 ++++++++++++++++++++--- 2 files changed, 119 insertions(+), 35 deletions(-) diff --git a/govcd/openapi.go b/govcd/openapi.go index bd182b06e..196a892bc 100644 --- a/govcd/openapi.go +++ b/govcd/openapi.go @@ -35,6 +35,9 @@ import ( // Not all API fields are supported for FIQL filtering and sometimes they return odd errors when filtering is // unsupported. No exact documentation exists so far. // +// Note. All functions accepting URL reference (*url.URL) will make a copy of URL because they may mutate URL reference. +// The parameter is kept as *url.URL for convenience because standard library provides pointer values. +// // OpenAPI versioning. // OpenAPI was introduced in VCD 9.5 (with API version 31.0). Endpoints are being added with each VCD iteration. // Internally hosted documentation (https://HOSTNAME/docs/) can be used to check which endpoints where introduced in @@ -70,8 +73,11 @@ func (client *Client) OpenApiBuildEndpoint(endpoint ...string) (*url.URL, error) // // Note. Query parameter 'pageSize' is defaulted to 128 (maximum supported) unless it is specified in queryParams func (client *Client) OpenApiGetAllItems(apiVersion string, urlRef *url.URL, queryParams url.Values, outType interface{}) error { + // copy passed in URL ref so that it is not mutated + urlRefCopy := copyUrlRef(urlRef) + util.Logger.Printf("[TRACE] Getting all items from endpoint %s for parsing into %s type\n", - urlRef.String(), reflect.TypeOf(outType)) + urlRefCopy.String(), reflect.TypeOf(outType)) if !client.OpenApiIsSupported() { return fmt.Errorf("OpenAPI is not supported on this VCD version") @@ -80,13 +86,13 @@ func (client *Client) OpenApiGetAllItems(apiVersion string, urlRef *url.URL, que // Page size is defaulted to 128 (maximum supported number) to reduce HTTP calls and improve performance unless caller // provides other value newQueryParams := defaultPageSize(queryParams, "128") - util.Logger.Printf("[TRACE] Will use 'pageSize' %s", newQueryParams.Get("pageSize")) + util.Logger.Printf("[TRACE] Will use 'pageSize=%s'", newQueryParams.Get("pageSize")) // Perform API call to initial endpoint. The function call recursively follows pages using Link headers "nextPage" // until it crawls all results - responses, err := client.openApiGetAllPages(apiVersion, urlRef, newQueryParams, outType, nil) + responses, err := client.openApiGetAllPages(apiVersion, urlRefCopy, newQueryParams, outType, nil) if err != nil { - return fmt.Errorf("error getting all pages for endpoint %s: %s", urlRef.String(), err) + return fmt.Errorf("error getting all pages for endpoint %s: %s", urlRefCopy.String(), err) } // Create a slice of raw JSON messages in text so that they can be unmarshalled to specified `outType` after multiple @@ -114,17 +120,20 @@ func (client *Client) OpenApiGetAllItems(apiVersion string, urlRef *url.URL, que // returned this function returns "ErrorEntityNotFound: API_ERROR" so that one can use ContainsNotFound(err) to // differentiate when an objects was not found from any other error. func (client *Client) OpenApiGetItem(apiVersion string, urlRef *url.URL, params url.Values, outType interface{}) error { + // copy passed in URL ref so that it is not mutated + urlRefCopy := copyUrlRef(urlRef) + util.Logger.Printf("[TRACE] Getting item from endpoint %s with expected response of type %s", - urlRef.String(), reflect.TypeOf(outType)) + urlRefCopy.String(), reflect.TypeOf(outType)) if !client.OpenApiIsSupported() { return fmt.Errorf("OpenAPI is not supported on this VCD version") } - req := client.newOpenApiRequest(apiVersion, params, http.MethodGet, urlRef, nil) + req := client.newOpenApiRequest(apiVersion, params, http.MethodGet, urlRefCopy, nil) resp, err := client.Http.Do(req) if err != nil { - return fmt.Errorf("error performing GET request to %s: %s", urlRef.String(), err) + return fmt.Errorf("error performing GET request to %s: %s", urlRefCopy.String(), err) } // Bypassing the regular path using function checkRespWithErrType and returning parsed error directly @@ -163,14 +172,17 @@ func (client *Client) OpenApiGetItem(apiVersion string, urlRef *url.URL, params // Note. Even though it may return error if the item does not support synchronous request - the object may still be // created. OpenApiPostItem would handle both cases and always return created item. func (client *Client) OpenApiPostItemSync(apiVersion string, urlRef *url.URL, params url.Values, payload, outType interface{}) error { + // copy passed in URL ref so that it is not mutated + urlRefCopy := copyUrlRef(urlRef) + util.Logger.Printf("[TRACE] Posting %s item to endpoint %s with expected response of type %s", - reflect.TypeOf(payload), urlRef.String(), reflect.TypeOf(outType)) + reflect.TypeOf(payload), urlRefCopy.String(), reflect.TypeOf(outType)) if !client.OpenApiIsSupported() { return fmt.Errorf("OpenAPI is not supported on this VCD version") } - resp, err := client.openApiPerformPostPut(http.MethodPost, apiVersion, urlRef, params, payload) + resp, err := client.openApiPerformPostPut(http.MethodPost, apiVersion, urlRefCopy, params, payload) if err != nil { return err } @@ -199,14 +211,17 @@ func (client *Client) OpenApiPostItemSync(apiVersion string, urlRef *url.URL, pa // Note. Even though it may return error if the item does not support asynchronous request - the object may still be // created. OpenApiPostItem would handle both cases and always return created item. func (client *Client) OpenApiPostItemAsync(apiVersion string, urlRef *url.URL, params url.Values, payload interface{}) (Task, error) { + // copy passed in URL ref so that it is not mutated + urlRefCopy := copyUrlRef(urlRef) + util.Logger.Printf("[TRACE] Posting async %s item to endpoint %s with expected task response", - reflect.TypeOf(payload), urlRef.String()) + reflect.TypeOf(payload), urlRefCopy.String()) if !client.OpenApiIsSupported() { return Task{}, fmt.Errorf("OpenAPI is not supported on this VCD version") } - resp, err := client.openApiPerformPostPut(http.MethodPost, apiVersion, urlRef, params, payload) + resp, err := client.openApiPerformPostPut(http.MethodPost, apiVersion, urlRefCopy, params, payload) if err != nil { return Task{}, err } @@ -235,14 +250,17 @@ func (client *Client) OpenApiPostItemAsync(apiVersion string, urlRef *url.URL, p // asynchronous requests. The urlRef must point to POST endpoint (e.g. '/1.0.0/edgeGateways'). When a task is // synchronous - it will track task until it is finished and pick reference to marshal outType. func (client *Client) OpenApiPostItem(apiVersion string, urlRef *url.URL, params url.Values, payload, outType interface{}) error { + // copy passed in URL ref so that it is not mutated + urlRefCopy := copyUrlRef(urlRef) + util.Logger.Printf("[TRACE] Posting %s item to endpoint %s with expected response of type %s", - reflect.TypeOf(payload), urlRef.String(), reflect.TypeOf(outType)) + reflect.TypeOf(payload), urlRefCopy.String(), reflect.TypeOf(outType)) if !client.OpenApiIsSupported() { return fmt.Errorf("OpenAPI is not supported on this VCD version") } - resp, err := client.openApiPerformPostPut(http.MethodPost, apiVersion, urlRef, params, payload) + resp, err := client.openApiPerformPostPut(http.MethodPost, apiVersion, urlRefCopy, params, payload) if err != nil { return err } @@ -265,7 +283,7 @@ func (client *Client) OpenApiPostItem(apiVersion string, urlRef *url.URL, params // Task Owner ID is the ID of created object. ID must be used (although HREF exists in task) because HREF points to // old XML API and here we need to pull data from OpenAPI. - newObjectUrl, _ := url.ParseRequestURI(urlRef.String() + "/" + task.Task.Owner.ID) + newObjectUrl, _ := url.ParseRequestURI(urlRefCopy.String() + "/" + task.Task.Owner.ID) err = client.OpenApiGetItem(apiVersion, newObjectUrl, nil, outType) if err != nil { return fmt.Errorf("error retrieving item after creation: %s", err) @@ -295,14 +313,17 @@ func (client *Client) OpenApiPostItem(apiVersion string, urlRef *url.URL, params // Note. Even though it may return error if the item does not support synchronous request - the object may still be // updated. OpenApiPutItem would handle both cases and always return updated item. func (client *Client) OpenApiPutItemSync(apiVersion string, urlRef *url.URL, params url.Values, payload, outType interface{}) error { + // copy passed in URL ref so that it is not mutated + urlRefCopy := copyUrlRef(urlRef) + util.Logger.Printf("[TRACE] Putting %s item to endpoint %s with expected response of type %s", - reflect.TypeOf(payload), urlRef.String(), reflect.TypeOf(outType)) + reflect.TypeOf(payload), urlRefCopy.String(), reflect.TypeOf(outType)) if !client.OpenApiIsSupported() { return fmt.Errorf("OpenAPI is not supported on this VCD version") } - resp, err := client.openApiPerformPostPut(http.MethodPut, apiVersion, urlRef, params, payload) + resp, err := client.openApiPerformPostPut(http.MethodPut, apiVersion, urlRefCopy, params, payload) if err != nil { return err } @@ -331,13 +352,16 @@ func (client *Client) OpenApiPutItemSync(apiVersion string, urlRef *url.URL, par // Note. Even though it may return error if the item does not support asynchronous request - the object may still be // created. OpenApiPutItem would handle both cases and always return created item. func (client *Client) OpenApiPutItemAsync(apiVersion string, urlRef *url.URL, params url.Values, payload interface{}) (Task, error) { + // copy passed in URL ref so that it is not mutated + urlRefCopy := copyUrlRef(urlRef) + util.Logger.Printf("[TRACE] Putting async %s item to endpoint %s with expected task response", - reflect.TypeOf(payload), urlRef.String()) + reflect.TypeOf(payload), urlRefCopy.String()) if !client.OpenApiIsSupported() { return Task{}, fmt.Errorf("OpenAPI is not supported on this VCD version") } - resp, err := client.openApiPerformPostPut(http.MethodPut, apiVersion, urlRef, params, payload) + resp, err := client.openApiPerformPostPut(http.MethodPut, apiVersion, urlRefCopy, params, payload) if err != nil { return Task{}, err } @@ -366,13 +390,16 @@ func (client *Client) OpenApiPutItemAsync(apiVersion string, urlRef *url.URL, pa // The urlRef must point to ID of exact item (e.g. '/1.0.0/edgeGateways/{EDGE_ID}') // It handles synchronous and asynchronous tasks. When a task is synchronous - it will block until it is finished. func (client *Client) OpenApiPutItem(apiVersion string, urlRef *url.URL, params url.Values, payload, outType interface{}) error { + // copy passed in URL ref so that it is not mutated + urlRefCopy := copyUrlRef(urlRef) + util.Logger.Printf("[TRACE] Putting %s item to endpoint %s with expected response of type %s", - reflect.TypeOf(payload), urlRef.String(), reflect.TypeOf(outType)) + reflect.TypeOf(payload), urlRefCopy.String(), reflect.TypeOf(outType)) if !client.OpenApiIsSupported() { return fmt.Errorf("OpenAPI is not supported on this VCD version") } - resp, err := client.openApiPerformPostPut(http.MethodPut, apiVersion, urlRef, params, payload) + resp, err := client.openApiPerformPostPut(http.MethodPut, apiVersion, urlRefCopy, params, payload) if err != nil { return err @@ -393,7 +420,7 @@ func (client *Client) OpenApiPutItem(apiVersion string, urlRef *url.URL, params } // Here we have to find the resource once more to return it populated. Provided params ir ignored for retrieval. - err = client.OpenApiGetItem(apiVersion, urlRef, nil, outType) + err = client.OpenApiGetItem(apiVersion, urlRefCopy, nil, outType) if err != nil { return fmt.Errorf("error retrieving item after updating: %s", err) } @@ -418,14 +445,17 @@ func (client *Client) OpenApiPutItem(apiVersion string, urlRef *url.URL, params // The urlRef must point to ID of exact item (e.g. '/1.0.0/edgeGateways/{EDGE_ID}') // It handles synchronous and asynchronous tasks. When a task is synchronous - it will block until it is finished. func (client *Client) OpenApiDeleteItem(apiVersion string, urlRef *url.URL, params url.Values) error { - util.Logger.Printf("[TRACE] Deleting item at endpoint %s", urlRef.String()) + // copy passed in URL ref so that it is not mutated + urlRefCopy := copyUrlRef(urlRef) + + util.Logger.Printf("[TRACE] Deleting item at endpoint %s", urlRefCopy.String()) if !client.OpenApiIsSupported() { return fmt.Errorf("OpenAPI is not supported on this VCD version") } // Perform request - req := client.newOpenApiRequest(apiVersion, params, http.MethodDelete, urlRef, nil) + req := client.newOpenApiRequest(apiVersion, params, http.MethodDelete, urlRefCopy, nil) resp, err := client.Http.Do(req) if err != nil { @@ -491,12 +521,15 @@ func (client *Client) openApiPerformPostPut(httpMethod string, apiVersion string // no intermediate unmarshalling to exact `outType` for every page it can unmarshal into direct `outType` supplied. // outType must be a slice of object (e.g. []*types.OpenApiRole) because accumulated responses are in JSON list func (client *Client) openApiGetAllPages(apiVersion string, urlRef *url.URL, queryParams url.Values, outType interface{}, responses []json.RawMessage) ([]json.RawMessage, error) { + // copy passed in URL ref so that it is not mutated + urlRefCopy := copyUrlRef(urlRef) + if responses == nil { responses = []json.RawMessage{} } // Perform request - req := client.newOpenApiRequest(apiVersion, queryParams, http.MethodGet, urlRef, nil) + req := client.newOpenApiRequest(apiVersion, queryParams, http.MethodGet, urlRefCopy, nil) resp, err := client.Http.Do(req) if err != nil { @@ -549,9 +582,11 @@ func (client *Client) openApiGetAllPages(apiVersion string, urlRef *url.URL, que // newOpenApiRequest is a low level function used in upstream OpenAPI functions which handles logging and // authentication for each API request func (client *Client) newOpenApiRequest(apiVersion string, params url.Values, method string, reqUrl *url.URL, body io.Reader) *http.Request { + // copy passed in URL ref so that it is not mutated + reqUrlCopy := copyUrlRef(reqUrl) // Add the params to our URL - reqUrl.RawQuery += params.Encode() + reqUrlCopy.RawQuery += params.Encode() // If the body contains data - try to read all contents for logging and re-create another // io.Reader with all contents to use it down the line @@ -564,7 +599,7 @@ func (client *Client) newOpenApiRequest(apiVersion string, params url.Values, me // Build the request, no point in checking for errors here as we're just // passing a string version of an url.URL struct and http.NewRequest returns // error only if can't process an url.ParseRequestURI(). - req, _ := http.NewRequest(method, reqUrl.String(), body) + req, _ := http.NewRequest(method, reqUrlCopy.String(), body) if client.VCDAuthHeader != "" && client.VCDToken != "" { // Add the authorization header @@ -583,7 +618,7 @@ func (client *Client) newOpenApiRequest(apiVersion string, params url.Values, me if req.ContentLength > 0 { payload = string(readBody) } - util.ProcessRequestOutput(util.FuncNameCallStack(), method, reqUrl.String(), payload, req) + util.ProcessRequestOutput(util.FuncNameCallStack(), method, reqUrlCopy.String(), payload, req) debugShowRequest(req, payload) } @@ -631,14 +666,21 @@ func jsonRawMessagesToStrings(messages []json.RawMessage) []string { // defaultPageSize allows to set 'pageSize' query parameter to defaultPageSize if one is not already specified in // url.Values while preserving all other supplied url.Values func defaultPageSize(queryParams url.Values, defaultPageSize string) url.Values { - newQueryParams := make(map[string][]string) + newQueryParams := url.Values{} if queryParams != nil { newQueryParams = queryParams } if _, ok := newQueryParams["pageSize"]; !ok { - newQueryParams["pageSize"] = []string{defaultPageSize} + newQueryParams.Set("pageSize", defaultPageSize) } return newQueryParams } + +// copyUrlRef creates a copy of URL reference by re-parsing it +func copyUrlRef(in *url.URL) *url.URL { + // error is ignored because we expect to have correct URL supplied and this greatly simplifies code inside. + newUrlRef, _ := url.Parse(in.String()) + return newUrlRef +} diff --git a/govcd/openapi_test.go b/govcd/openapi_test.go index de00ca7fe..d7114131e 100644 --- a/govcd/openapi_test.go +++ b/govcd/openapi_test.go @@ -9,6 +9,7 @@ package govcd import ( "encoding/json" "fmt" + "net/http" "net/url" "regexp" "strings" @@ -24,16 +25,18 @@ import ( func (vcd *TestVCD) Test_OpenApiRawJsonAuditTrail(check *C) { minimumRequiredApiVersion := "33.0" skipOpenApiEndpointTest(vcd, check, "1.0.0/auditTrail", minimumRequiredApiVersion) - urlRef, err := vcd.client.Client.OpenApiBuildEndpoint("1.0.0/auditTrail") check.Assert(err, IsNil) + // Get a timestamp after which endpoint contains at least 10 elements + filterTimeStamp := getAuditTrailTimestampWithElements(10, check, vcd, minimumRequiredApiVersion, urlRef, err) + // Limit search of audits trails to the last 12 hours so that it doesn't take too long and set pageSize to be 1 result // to force following pages queryParams := url.Values{} - filterTime := time.Now().Add(-12 * time.Hour).Format(types.FiqlQueryTimestampFormat) - queryParams.Add("filter", "timestamp=gt="+filterTime) - queryParams.Add("pageSize", "1") + queryParams.Add("filter", "timestamp=gt="+filterTimeStamp) + queryParams.Add("pageSize", "1") // pageSize=1 to enforce internal pagination + queryParams.Add("sortDesc", "timestamp") allResponses := []json.RawMessage{{}} err = vcd.vdc.client.OpenApiGetAllItems(minimumRequiredApiVersion, urlRef, queryParams, &allResponses) @@ -93,9 +96,9 @@ func (vcd *TestVCD) Test_OpenApiInlineStructAuditTrail(check *C) { allResponses := []*AuditTrail{{}} - // Define FIQL query to find events for the last 24 hours + // Define FIQL query to find events for the last 6 hours. At least login operations will already be here on test run queryParams := url.Values{} - filterTime := time.Now().Add(-24 * time.Hour).Format(types.FiqlQueryTimestampFormat) + filterTime := time.Now().Add(-6 * time.Hour).Format(types.FiqlQueryTimestampFormat) queryParams.Add("filter", "timestamp=gt="+filterTime) err = vcd.vdc.client.OpenApiGetAllItems(minimumRequiredApiVersion, urlRef, queryParams, &allResponses) @@ -265,3 +268,42 @@ func skipOpenApiEndpointTest(vcd *TestVCD, check *C, endpoint, requiredVersion s check.Skip(skipText) } } + +// getAuditTrailTimestampWithElements helps to pick good timestamp filter so that it doesn't take long time to retrieve +// too many items +func getAuditTrailTimestampWithElements(elementCount int, check *C, vcd *TestVCD, minimumRequiredApiVersion string, urlRef *url.URL, err error) string { + client := vcd.client.Client + qp := url.Values{} + qp.Add("pageSize", "128") + qp.Add("sortDesc", "timestamp") // Need to get the newest + req := client.newOpenApiRequest(minimumRequiredApiVersion, qp, http.MethodGet, urlRef, nil) + + resp, err := client.Http.Do(req) + check.Assert(err, IsNil) + + type AuditTrailTimestamp struct { + Timestamp string `json:"timestamp"` + } + + onePageAuditTrail := make([]AuditTrailTimestamp, 1) + onePageResponse := &types.OpenApiPages{} + err = decodeBody(types.BodyTypeJSON, resp, &onePageResponse) + check.Assert(err, IsNil) + + err = resp.Body.Close() + check.Assert(err, IsNil) + + err = json.Unmarshal(onePageResponse.Values, &onePageAuditTrail) + check.Assert(err, IsNil) + + var singleElement AuditTrailTimestamp + + // Find newest element limited by provided elementCount + if len(onePageAuditTrail) < elementCount { + singleElement = onePageAuditTrail[(len(onePageAuditTrail) - 1)] + } else { + singleElement = onePageAuditTrail[(elementCount - 1)] + } + return singleElement.Timestamp + +} From df1f31983ca1cda4469dfb9aa775906630b0a0d1 Mon Sep 17 00:00:00 2001 From: Dainius S Date: Fri, 2 Oct 2020 10:42:46 +0300 Subject: [PATCH 7/7] Fix incorect refactor --- govcd/openapi_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/govcd/openapi_test.go b/govcd/openapi_test.go index d7114131e..e3a30a65d 100644 --- a/govcd/openapi_test.go +++ b/govcd/openapi_test.go @@ -29,7 +29,7 @@ func (vcd *TestVCD) Test_OpenApiRawJsonAuditTrail(check *C) { check.Assert(err, IsNil) // Get a timestamp after which endpoint contains at least 10 elements - filterTimeStamp := getAuditTrailTimestampWithElements(10, check, vcd, minimumRequiredApiVersion, urlRef, err) + filterTimeStamp := getAuditTrailTimestampWithElements(10, check, vcd, minimumRequiredApiVersion, urlRef) // Limit search of audits trails to the last 12 hours so that it doesn't take too long and set pageSize to be 1 result // to force following pages @@ -271,7 +271,7 @@ func skipOpenApiEndpointTest(vcd *TestVCD, check *C, endpoint, requiredVersion s // getAuditTrailTimestampWithElements helps to pick good timestamp filter so that it doesn't take long time to retrieve // too many items -func getAuditTrailTimestampWithElements(elementCount int, check *C, vcd *TestVCD, minimumRequiredApiVersion string, urlRef *url.URL, err error) string { +func getAuditTrailTimestampWithElements(elementCount int, check *C, vcd *TestVCD, minimumRequiredApiVersion string, urlRef *url.URL) string { client := vcd.client.Client qp := url.Values{} qp.Add("pageSize", "128")