From 33f0192040dd58a07a8261e4e77449d3f94cc574 Mon Sep 17 00:00:00 2001 From: "Greg Cobb, Jeff Pak and Liz Dahlstrom" Date: Wed, 17 Jun 2015 11:38:14 -0700 Subject: [PATCH 1/4] Reduce service_access API requests: service offerings - Get all service offerings in one request instead of a request per broker [#96912380] --- cf/actors/broker_builder/broker_builder.go | 31 ++++++-- .../broker_builder/broker_builder_test.go | 4 +- .../fakes/fake_service_builder.go | 33 +++++++- cf/actors/service_builder/service_builder.go | 29 +++++-- .../service_builder/service_builder_test.go | 49 +++++++++++- cf/api/fakes/fake_service_repo.go | 18 +++++ cf/api/services.go | 17 +++++ cf/api/services_test.go | 75 +++++++++++++++++++ 8 files changed, 239 insertions(+), 17 deletions(-) diff --git a/cf/actors/broker_builder/broker_builder.go b/cf/actors/broker_builder/broker_builder.go index d9f9267f445..b8ebec45218 100644 --- a/cf/actors/broker_builder/broker_builder.go +++ b/cf/actors/broker_builder/broker_builder.go @@ -72,25 +72,44 @@ func (builder Builder) AttachSpecificBrokerToServices(brokerName string, service func (builder Builder) GetAllServiceBrokers() ([]models.ServiceBroker, error) { brokers := []models.ServiceBroker{} + brokerGuids := []string{} var err error var services models.ServiceOfferings err = builder.brokerRepo.ListServiceBrokers(func(broker models.ServiceBroker) bool { brokers = append(brokers, broker) + brokerGuids = append(brokerGuids, broker.Guid) return true }) + if err != nil { + return nil, err + } - for index, broker := range brokers { - services, err = builder.serviceBuilder.GetServicesForBroker(broker.Guid) - if err != nil { - return nil, err - } + services, err = builder.serviceBuilder.GetServicesForManyBrokers(brokerGuids) + if err != nil { + return nil, err + } - brokers[index].Services = services + brokers, err = builder.attachServiceOfferingsToBrokers(services, brokers) + if err != nil { + return nil, err } + return brokers, err } +func (builder Builder) attachServiceOfferingsToBrokers(services models.ServiceOfferings, brokers []models.ServiceBroker) ([]models.ServiceBroker, error) { + for _, service := range services { + for index, broker := range brokers { + if broker.Guid == service.BrokerGuid { + brokers[index].Services = append(brokers[index].Services, service) + break + } + } + } + return brokers, nil +} + func (builder Builder) GetBrokerWithAllServices(brokerName string) (models.ServiceBroker, error) { broker, err := builder.brokerRepo.FindByName(brokerName) if err != nil { diff --git a/cf/actors/broker_builder/broker_builder_test.go b/cf/actors/broker_builder/broker_builder_test.go index 1d1856bb230..2e9c63a6f60 100644 --- a/cf/actors/broker_builder/broker_builder_test.go +++ b/cf/actors/broker_builder/broker_builder_test.go @@ -181,7 +181,7 @@ var _ = Describe("Broker Builder", func() { It("returns an error if we cannot list the services for a broker", func() { brokerRepo.ServiceBrokers = []models.ServiceBroker{serviceBroker1} - serviceBuilder.GetServicesForBrokerReturns(nil, errors.New("Cannot find services")) + serviceBuilder.GetServicesForManyBrokersReturns(nil, errors.New("Cannot find services")) _, err := brokerBuilder.GetAllServiceBrokers() Expect(err).To(HaveOccurred()) @@ -189,7 +189,7 @@ var _ = Describe("Broker Builder", func() { It("returns all service brokers populated with their services", func() { brokerRepo.ServiceBrokers = []models.ServiceBroker{serviceBroker1} - serviceBuilder.GetServicesForBrokerReturns(services, nil) + serviceBuilder.GetServicesForManyBrokersReturns(services, nil) brokers, err := brokerBuilder.GetAllServiceBrokers() Expect(err).NotTo(HaveOccurred()) diff --git a/cf/actors/service_builder/fakes/fake_service_builder.go b/cf/actors/service_builder/fakes/fake_service_builder.go index 95fb789a8f7..b1f19ea5862 100644 --- a/cf/actors/service_builder/fakes/fake_service_builder.go +++ b/cf/actors/service_builder/fakes/fake_service_builder.go @@ -2,9 +2,10 @@ package fakes import ( + "sync" + . "github.com/cloudfoundry/cli/cf/actors/service_builder" "github.com/cloudfoundry/cli/cf/models" - "sync" ) type FakeServiceBuilder struct { @@ -80,6 +81,16 @@ type FakeServiceBuilder struct { result1 models.ServiceOffering result2 error } + GetServicesForManyBrokersStub func([]string) ([]models.ServiceOffering, error) + getServicesForManyBrokersMutex sync.RWMutex + getServicesForManyBrokersArgsForCall []struct { + arg1 []string + } + getServicesForManyBrokersReturns struct { + result1 []models.ServiceOffering + result2 error + } + GetServicesForBrokerStub func(string) ([]models.ServiceOffering, error) getServicesForBrokerMutex sync.RWMutex getServicesForBrokerArgsForCall []struct { @@ -372,6 +383,19 @@ func (fake *FakeServiceBuilder) GetServiceByNameForOrgReturns(result1 models.Ser }{result1, result2} } +func (fake *FakeServiceBuilder) GetServicesForManyBrokers(arg1 []string) ([]models.ServiceOffering, error) { + fake.getServicesForManyBrokersMutex.Lock() + defer fake.getServicesForManyBrokersMutex.Unlock() + fake.getServicesForManyBrokersArgsForCall = append(fake.getServicesForManyBrokersArgsForCall, struct { + arg1 []string + }{arg1}) + if fake.GetServicesForManyBrokersStub != nil { + return fake.GetServicesForManyBrokersStub(arg1) + } else { + return fake.getServicesForManyBrokersReturns.result1, fake.getServicesForManyBrokersReturns.result2 + } +} + func (fake *FakeServiceBuilder) GetServicesForBroker(arg1 string) ([]models.ServiceOffering, error) { fake.getServicesForBrokerMutex.Lock() defer fake.getServicesForBrokerMutex.Unlock() @@ -397,6 +421,13 @@ func (fake *FakeServiceBuilder) GetServicesForBrokerArgsForCall(i int) string { return fake.getServicesForBrokerArgsForCall[i].arg1 } +func (fake *FakeServiceBuilder) GetServicesForManyBrokersReturns(result1 []models.ServiceOffering, result2 error) { + fake.getServicesForManyBrokersReturns = struct { + result1 []models.ServiceOffering + result2 error + }{result1, result2} +} + func (fake *FakeServiceBuilder) GetServicesForBrokerReturns(result1 []models.ServiceOffering, result2 error) { fake.getServicesForBrokerReturns = struct { result1 []models.ServiceOffering diff --git a/cf/actors/service_builder/service_builder.go b/cf/actors/service_builder/service_builder.go index 417d7e5f0cd..4dc7d75cd53 100644 --- a/cf/actors/service_builder/service_builder.go +++ b/cf/actors/service_builder/service_builder.go @@ -19,6 +19,7 @@ type ServiceBuilder interface { GetServicesByNameForSpaceWithPlans(string, string) (models.ServiceOfferings, error) GetServiceByNameForOrg(string, string) (models.ServiceOffering, error) + GetServicesForManyBrokers([]string) ([]models.ServiceOffering, error) GetServicesForBroker(string) ([]models.ServiceOffering, error) GetServicesForSpace(string) ([]models.ServiceOffering, error) @@ -168,18 +169,20 @@ func (builder Builder) GetServiceByNameWithPlansWithOrgNames(serviceLabel string return service, nil } +func (builder Builder) GetServicesForManyBrokers(brokerGuids []string) ([]models.ServiceOffering, error) { + services, err := builder.serviceRepo.ListServicesFromManyBrokers(brokerGuids) + if err != nil { + return nil, err + } + return builder.attachPlansToManyServices(services) +} + func (builder Builder) GetServicesForBroker(brokerGuid string) ([]models.ServiceOffering, error) { services, err := builder.serviceRepo.ListServicesFromBroker(brokerGuid) if err != nil { return nil, err } - for index, service := range services { - services[index], err = builder.attachPlansToService(service) - if err != nil { - return nil, err - } - } - return services, nil + return builder.attachPlansToManyServices(services) } func (builder Builder) GetServiceVisibleToOrg(serviceName string, orgName string) (models.ServiceOffering, error) { @@ -218,6 +221,18 @@ func (builder Builder) attachPlansToServiceForOrg(service models.ServiceOffering return service, nil } +func (builder Builder) attachPlansToManyServices(services []models.ServiceOffering) ([]models.ServiceOffering, error) { + var err error + + for index, service := range services { + services[index], err = builder.attachPlansToService(service) + if err != nil { + return nil, err + } + } + return services, nil +} + func (builder Builder) attachPlansToService(service models.ServiceOffering) (models.ServiceOffering, error) { plans, err := builder.planBuilder.GetPlansForServiceWithOrgs(service.Guid) if err != nil { diff --git a/cf/actors/service_builder/service_builder_test.go b/cf/actors/service_builder/service_builder_test.go index 4f3ef5d4a63..12c5fff3c1f 100644 --- a/cf/actors/service_builder/service_builder_test.go +++ b/cf/actors/service_builder/service_builder_test.go @@ -1,6 +1,8 @@ package service_builder_test import ( + "errors" + plan_builder_fakes "github.com/cloudfoundry/cli/cf/actors/plan_builder/fakes" "github.com/cloudfoundry/cli/cf/actors/service_builder" testapi "github.com/cloudfoundry/cli/cf/api/fakes" @@ -17,6 +19,7 @@ var _ = Describe("Service Builder", func() { serviceBuilder service_builder.ServiceBuilder serviceRepo *testapi.FakeServiceRepo service1 models.ServiceOffering + service2 models.ServiceOffering v1Service models.ServiceOffering planWithoutOrgs models.ServicePlanFields plan1 models.ServicePlanFields @@ -36,6 +39,14 @@ var _ = Describe("Service Builder", func() { }, } + service2 = models.ServiceOffering{ + ServiceOfferingFields: models.ServiceOfferingFields{ + Label: "my-service2", + Guid: "service-guid2", + BrokerGuid: "my-service-broker-guid2", + }, + } + v1Service = models.ServiceOffering{ ServiceOfferingFields: models.ServiceOfferingFields{ Label: "v1Service", @@ -46,7 +57,8 @@ var _ = Describe("Service Builder", func() { } serviceRepo.FindServiceOfferingsByLabelName = "my-service1" - serviceRepo.FindServiceOfferingsByLabelServiceOfferings = models.ServiceOfferings([]models.ServiceOffering{service1, v1Service}) + serviceRepo.FindServiceOfferingsByLabelServiceOfferings = + models.ServiceOfferings([]models.ServiceOffering{service1, v1Service}) serviceRepo.GetServiceOfferingByGuidReturns = struct { ServiceOffering models.ServiceOffering @@ -60,6 +72,10 @@ var _ = Describe("Service Builder", func() { "my-service-broker-guid1": []models.ServiceOffering{service1}, } + serviceRepo.ListServicesFromManyBrokersReturns = map[string][]models.ServiceOffering{ + "my-service-broker-guid1,my-service-broker-guid2": []models.ServiceOffering{service1, service2}, + } + plan1 = models.ServicePlanFields{ Name: "service-plan1", Guid: "service-plan1-guid", @@ -374,6 +390,37 @@ var _ = Describe("Service Builder", func() { }) }) + Describe(".GetServicesForManyBrokers", func() { + It("returns all the services for an array of broker guids, fully populated", func() { + brokerGuids := []string{"my-service-broker-guid1", "my-service-broker-guid2"} + services, err := serviceBuilder.GetServicesForManyBrokers(brokerGuids) + Expect(err).NotTo(HaveOccurred()) + + Expect(services).To(HaveLen(2)) + + broker_service := services[0] + Expect(broker_service.Label).To(Equal("my-service1")) + Expect(len(broker_service.Plans)).To(Equal(2)) + Expect(broker_service.Plans[0].Name).To(Equal("service-plan1")) + Expect(broker_service.Plans[1].Name).To(Equal("service-plan2")) + Expect(broker_service.Plans[0].OrgNames).To(Equal([]string{"org1", "org2"})) + + broker_service2 := services[1] + Expect(broker_service2.Label).To(Equal("my-service2")) + Expect(len(broker_service2.Plans)).To(Equal(2)) + Expect(broker_service.Plans[0].Name).To(Equal("service-plan1")) + Expect(broker_service.Plans[1].Name).To(Equal("service-plan2")) + Expect(broker_service.Plans[0].OrgNames).To(Equal([]string{"org1", "org2"})) + }) + + It("raises errors from the service repo", func() { + serviceRepo.ListServicesFromManyBrokersErr = errors.New("error") + brokerGuids := []string{"my-service-broker-guid1", "my-service-broker-guid2"} + _, err := serviceBuilder.GetServicesForManyBrokers(brokerGuids) + Expect(err).To(HaveOccurred()) + }) + }) + Describe(".GetServiceVisibleToOrg", func() { It("Returns a service populated with plans visible to the provided org", func() { service, err := serviceBuilder.GetServiceVisibleToOrg("my-service1", "org1") diff --git a/cf/api/fakes/fake_service_repo.go b/cf/api/fakes/fake_service_repo.go index d0fe6edfa32..bd102f010cd 100644 --- a/cf/api/fakes/fake_service_repo.go +++ b/cf/api/fakes/fake_service_repo.go @@ -1,6 +1,8 @@ package fakes import ( + "strings" + "github.com/cloudfoundry/cli/cf/api/resources" "github.com/cloudfoundry/cli/cf/errors" "github.com/cloudfoundry/cli/cf/models" @@ -82,6 +84,9 @@ type FakeServiceRepo struct { FindServiceOfferingsByLabelApiResponse error FindServiceOfferingsByLabelCalled bool + ListServicesFromManyBrokersReturns map[string][]models.ServiceOffering + ListServicesFromManyBrokersErr error + ListServicesFromBrokerReturns map[string][]models.ServiceOffering ListServicesFromBrokerErr error @@ -213,6 +218,19 @@ func (repo *FakeServiceRepo) FindServicePlanByDescription(planDescription resour return } +func (repo *FakeServiceRepo) ListServicesFromManyBrokers(brokerGuids []string) ([]models.ServiceOffering, error) { + if repo.ListServicesFromManyBrokersErr != nil { + return nil, repo.ListServicesFromManyBrokersErr + } + + key := strings.Join(brokerGuids, ",") + if repo.ListServicesFromManyBrokersReturns[key] != nil { + return repo.ListServicesFromManyBrokersReturns[key], nil + } + + return []models.ServiceOffering{}, nil +} + func (repo *FakeServiceRepo) ListServicesFromBroker(brokerGuid string) ([]models.ServiceOffering, error) { if repo.ListServicesFromBrokerErr != nil { return nil, repo.ListServicesFromBrokerErr diff --git a/cf/api/services.go b/cf/api/services.go index 866dbbceb5c..cb7eca2710f 100644 --- a/cf/api/services.go +++ b/cf/api/services.go @@ -31,6 +31,7 @@ type ServiceRepository interface { DeleteService(instance models.ServiceInstance) (apiErr error) FindServicePlanByDescription(planDescription resources.ServicePlanDescription) (planGuid string, apiErr error) ListServicesFromBroker(brokerGuid string) (services []models.ServiceOffering, err error) + ListServicesFromManyBrokers(brokerGuids []string) (services []models.ServiceOffering, err error) GetServiceInstanceCountForServicePlan(v1PlanGuid string) (count int, apiErr error) MigrateServicePlanFromV1ToV2(v1PlanGuid, v2PlanGuid string) (changedCount int, apiErr error) } @@ -255,6 +256,22 @@ func (repo CloudControllerServiceRepository) FindServicePlanByDescription(planDe return planGuid, apiErr } +func (repo CloudControllerServiceRepository) ListServicesFromManyBrokers(brokerGuids []string) (services []models.ServiceOffering, err error) { + brokerGuidsString := strings.Join(brokerGuids, ",") + + err = repo.gateway.ListPaginatedResources( + repo.config.ApiEndpoint(), + fmt.Sprintf("/v2/services?q=%s", url.QueryEscape("service_broker_guid IN "+brokerGuidsString)), + resources.ServiceOfferingResource{}, + func(resource interface{}) bool { + if service, ok := resource.(resources.ServiceOfferingResource); ok { + services = append(services, service.ToModel()) + } + return true + }) + return +} + func (repo CloudControllerServiceRepository) ListServicesFromBroker(brokerGuid string) (offerings []models.ServiceOffering, err error) { err = repo.gateway.ListPaginatedResources( repo.config.ApiEndpoint(), diff --git a/cf/api/services_test.go b/cf/api/services_test.go index b179a23c090..e7451ac1659 100644 --- a/cf/api/services_test.go +++ b/cf/api/services_test.go @@ -194,6 +194,81 @@ var _ = Describe("Services Repo", func() { }) }) + Describe("returning services for many brokers", func() { + path1 := "/v2/services?q=service_broker_guid%20IN%20my-service-broker-guid,my-service-broker-guid2" + body1 := ` +{ + "total_results": 2, + "total_pages": 2, + "prev_url": null, + "next_url": "/v2/services?q=service_broker_guid%20IN%20my-service-broker-guid,my-service-broker-guid2&page=2", + "resources": [ + { + "metadata": { + "guid": "my-service-guid" + }, + "entity": { + "label": "my-service", + "provider": "androsterone-ensphere", + "description": "Dummy addon that is cool", + "version": "damageableness-preheat", + "documentation_url": "YESWECAN.com" + } + } + ] +}` + path2 := "/v2/services?q=service_broker_guid%20IN%20my-service-broker-guid,my-service-broker-guid2&page=2" + body2 := ` +{ + "total_results": 2, + "total_pages": 2, + "prev_url": "/v2/services?q=service_broker_guid%20IN%20my-service-broker-guid,my-service-broker-guid2", + "next_url": null, + "resources": [ + { + "metadata": { + "guid": "my-service-guid2" + }, + "entity": { + "label": "my-service2", + "provider": "androsterone-ensphere", + "description": "Dummy addon that is cool", + "version": "damageableness-preheat", + "documentation_url": "YESWECAN.com" + } + } + ] +}` + BeforeEach(func() { + setupTestServer( + testapi.NewCloudControllerTestRequest( + testnet.TestRequest{ + Method: "GET", + Path: path1, + Response: testnet.TestResponse{Status: http.StatusOK, Body: body1}, + }), + testapi.NewCloudControllerTestRequest( + testnet.TestRequest{ + Method: "GET", + Path: path2, + Response: testnet.TestResponse{Status: http.StatusOK, Body: body2}, + }), + ) + }) + + It("returns the service brokers services", func() { + brokerGuids := []string{"my-service-broker-guid", "my-service-broker-guid2"} + services, err := repo.ListServicesFromManyBrokers(brokerGuids) + + Expect(err).NotTo(HaveOccurred()) + Expect(testHandler).To(HaveAllRequestsCalled()) + Expect(len(services)).To(Equal(2)) + + Expect(services[0].Guid).To(Equal("my-service-guid")) + Expect(services[1].Guid).To(Equal("my-service-guid2")) + }) + }) + Describe("creating a service instance", func() { It("makes the right request", func() { setupTestServer(testapi.NewCloudControllerTestRequest(testnet.TestRequest{ From d648af1f25f5f17af6f0d7cacb881f257dc5b927 Mon Sep 17 00:00:00 2001 From: Greg Cobb and Jeff Pak Date: Wed, 17 Jun 2015 16:34:38 -0700 Subject: [PATCH 2/4] Reduce service_access API requests: service plans - Get all service plans in one request instead of a request per service offering [#96912380] --- .../plan_builder/fakes/fake_plan_builder.go | 67 ++++++++++++++--- cf/actors/plan_builder/plan_builder.go | 14 ++++ cf/actors/plan_builder/plan_builder_test.go | 27 ++++++- cf/actors/service_builder/service_builder.go | 31 +++++--- .../service_builder/service_builder_test.go | 21 +++++- cf/api/fakes/fake_service_plan_repo.go | 14 ++++ cf/api/service_plan.go | 17 +++++ cf/api/service_plan_test.go | 75 +++++++++++++++++++ 8 files changed, 243 insertions(+), 23 deletions(-) diff --git a/cf/actors/plan_builder/fakes/fake_plan_builder.go b/cf/actors/plan_builder/fakes/fake_plan_builder.go index fa3015f7913..72bb29ea9d7 100644 --- a/cf/actors/plan_builder/fakes/fake_plan_builder.go +++ b/cf/actors/plan_builder/fakes/fake_plan_builder.go @@ -2,9 +2,10 @@ package fakes import ( - . "github.com/cloudfoundry/cli/cf/actors/plan_builder" - "github.com/cloudfoundry/cli/cf/models" "sync" + + "github.com/cloudfoundry/cli/cf/actors/plan_builder" + "github.com/cloudfoundry/cli/cf/models" ) type FakePlanBuilder struct { @@ -46,6 +47,15 @@ type FakePlanBuilder struct { result1 []models.ServicePlanFields result2 error } + GetPlansForManyServicesWithOrgsStub func([]string) ([]models.ServicePlanFields, error) + getPlansForManyServicesWithOrgsMutex sync.RWMutex + getPlansForManyServicesWithOrgsArgsForCall []struct { + arg1 []string + } + getPlansForManyServicesWithOrgsReturns struct { + result1 []models.ServicePlanFields + result2 error + } GetPlansForServiceStub func(string) ([]models.ServicePlanFields, error) getPlansForServiceMutex sync.RWMutex getPlansForServiceArgsForCall []struct { @@ -68,10 +78,10 @@ type FakePlanBuilder struct { func (fake *FakePlanBuilder) AttachOrgsToPlans(arg1 []models.ServicePlanFields) ([]models.ServicePlanFields, error) { fake.attachOrgsToPlansMutex.Lock() - defer fake.attachOrgsToPlansMutex.Unlock() fake.attachOrgsToPlansArgsForCall = append(fake.attachOrgsToPlansArgsForCall, struct { arg1 []models.ServicePlanFields }{arg1}) + fake.attachOrgsToPlansMutex.Unlock() if fake.AttachOrgsToPlansStub != nil { return fake.AttachOrgsToPlansStub(arg1) } else { @@ -92,6 +102,7 @@ func (fake *FakePlanBuilder) AttachOrgsToPlansArgsForCall(i int) []models.Servic } func (fake *FakePlanBuilder) AttachOrgsToPlansReturns(result1 []models.ServicePlanFields, result2 error) { + fake.AttachOrgsToPlansStub = nil fake.attachOrgsToPlansReturns = struct { result1 []models.ServicePlanFields result2 error @@ -100,11 +111,11 @@ func (fake *FakePlanBuilder) AttachOrgsToPlansReturns(result1 []models.ServicePl func (fake *FakePlanBuilder) AttachOrgToPlans(arg1 []models.ServicePlanFields, arg2 string) ([]models.ServicePlanFields, error) { fake.attachOrgToPlansMutex.Lock() - defer fake.attachOrgToPlansMutex.Unlock() fake.attachOrgToPlansArgsForCall = append(fake.attachOrgToPlansArgsForCall, struct { arg1 []models.ServicePlanFields arg2 string }{arg1, arg2}) + fake.attachOrgToPlansMutex.Unlock() if fake.AttachOrgToPlansStub != nil { return fake.AttachOrgToPlansStub(arg1, arg2) } else { @@ -125,6 +136,7 @@ func (fake *FakePlanBuilder) AttachOrgToPlansArgsForCall(i int) ([]models.Servic } func (fake *FakePlanBuilder) AttachOrgToPlansReturns(result1 []models.ServicePlanFields, result2 error) { + fake.AttachOrgToPlansStub = nil fake.attachOrgToPlansReturns = struct { result1 []models.ServicePlanFields result2 error @@ -133,11 +145,11 @@ func (fake *FakePlanBuilder) AttachOrgToPlansReturns(result1 []models.ServicePla func (fake *FakePlanBuilder) GetPlansForServiceForOrg(arg1 string, arg2 string) ([]models.ServicePlanFields, error) { fake.getPlansForServiceForOrgMutex.Lock() - defer fake.getPlansForServiceForOrgMutex.Unlock() fake.getPlansForServiceForOrgArgsForCall = append(fake.getPlansForServiceForOrgArgsForCall, struct { arg1 string arg2 string }{arg1, arg2}) + fake.getPlansForServiceForOrgMutex.Unlock() if fake.GetPlansForServiceForOrgStub != nil { return fake.GetPlansForServiceForOrgStub(arg1, arg2) } else { @@ -158,6 +170,7 @@ func (fake *FakePlanBuilder) GetPlansForServiceForOrgArgsForCall(i int) (string, } func (fake *FakePlanBuilder) GetPlansForServiceForOrgReturns(result1 []models.ServicePlanFields, result2 error) { + fake.GetPlansForServiceForOrgStub = nil fake.getPlansForServiceForOrgReturns = struct { result1 []models.ServicePlanFields result2 error @@ -166,10 +179,10 @@ func (fake *FakePlanBuilder) GetPlansForServiceForOrgReturns(result1 []models.Se func (fake *FakePlanBuilder) GetPlansForServiceWithOrgs(arg1 string) ([]models.ServicePlanFields, error) { fake.getPlansForServiceWithOrgsMutex.Lock() - defer fake.getPlansForServiceWithOrgsMutex.Unlock() fake.getPlansForServiceWithOrgsArgsForCall = append(fake.getPlansForServiceWithOrgsArgsForCall, struct { arg1 string }{arg1}) + fake.getPlansForServiceWithOrgsMutex.Unlock() if fake.GetPlansForServiceWithOrgsStub != nil { return fake.GetPlansForServiceWithOrgsStub(arg1) } else { @@ -190,18 +203,52 @@ func (fake *FakePlanBuilder) GetPlansForServiceWithOrgsArgsForCall(i int) string } func (fake *FakePlanBuilder) GetPlansForServiceWithOrgsReturns(result1 []models.ServicePlanFields, result2 error) { + fake.GetPlansForServiceWithOrgsStub = nil fake.getPlansForServiceWithOrgsReturns = struct { result1 []models.ServicePlanFields result2 error }{result1, result2} } +func (fake *FakePlanBuilder) GetPlansForManyServicesWithOrgs(arg1 []string) ([]models.ServicePlanFields, error) { + fake.getPlansForManyServicesWithOrgsMutex.Lock() + fake.getPlansForManyServicesWithOrgsArgsForCall = append(fake.getPlansForManyServicesWithOrgsArgsForCall, struct { + arg1 []string + }{arg1}) + fake.getPlansForManyServicesWithOrgsMutex.Unlock() + if fake.GetPlansForManyServicesWithOrgsStub != nil { + return fake.GetPlansForManyServicesWithOrgsStub(arg1) + } else { + return fake.getPlansForManyServicesWithOrgsReturns.result1, fake.getPlansForManyServicesWithOrgsReturns.result2 + } +} + +func (fake *FakePlanBuilder) GetPlansForManyServicesWithOrgsCallCount() int { + fake.getPlansForManyServicesWithOrgsMutex.RLock() + defer fake.getPlansForManyServicesWithOrgsMutex.RUnlock() + return len(fake.getPlansForManyServicesWithOrgsArgsForCall) +} + +func (fake *FakePlanBuilder) GetPlansForManyServicesWithOrgsArgsForCall(i int) []string { + fake.getPlansForManyServicesWithOrgsMutex.RLock() + defer fake.getPlansForManyServicesWithOrgsMutex.RUnlock() + return fake.getPlansForManyServicesWithOrgsArgsForCall[i].arg1 +} + +func (fake *FakePlanBuilder) GetPlansForManyServicesWithOrgsReturns(result1 []models.ServicePlanFields, result2 error) { + fake.GetPlansForManyServicesWithOrgsStub = nil + fake.getPlansForManyServicesWithOrgsReturns = struct { + result1 []models.ServicePlanFields + result2 error + }{result1, result2} +} + func (fake *FakePlanBuilder) GetPlansForService(arg1 string) ([]models.ServicePlanFields, error) { fake.getPlansForServiceMutex.Lock() - defer fake.getPlansForServiceMutex.Unlock() fake.getPlansForServiceArgsForCall = append(fake.getPlansForServiceArgsForCall, struct { arg1 string }{arg1}) + fake.getPlansForServiceMutex.Unlock() if fake.GetPlansForServiceStub != nil { return fake.GetPlansForServiceStub(arg1) } else { @@ -222,6 +269,7 @@ func (fake *FakePlanBuilder) GetPlansForServiceArgsForCall(i int) string { } func (fake *FakePlanBuilder) GetPlansForServiceReturns(result1 []models.ServicePlanFields, result2 error) { + fake.GetPlansForServiceStub = nil fake.getPlansForServiceReturns = struct { result1 []models.ServicePlanFields result2 error @@ -230,10 +278,10 @@ func (fake *FakePlanBuilder) GetPlansForServiceReturns(result1 []models.ServiceP func (fake *FakePlanBuilder) GetPlansVisibleToOrg(arg1 string) ([]models.ServicePlanFields, error) { fake.getPlansVisibleToOrgMutex.Lock() - defer fake.getPlansVisibleToOrgMutex.Unlock() fake.getPlansVisibleToOrgArgsForCall = append(fake.getPlansVisibleToOrgArgsForCall, struct { arg1 string }{arg1}) + fake.getPlansVisibleToOrgMutex.Unlock() if fake.GetPlansVisibleToOrgStub != nil { return fake.GetPlansVisibleToOrgStub(arg1) } else { @@ -254,10 +302,11 @@ func (fake *FakePlanBuilder) GetPlansVisibleToOrgArgsForCall(i int) string { } func (fake *FakePlanBuilder) GetPlansVisibleToOrgReturns(result1 []models.ServicePlanFields, result2 error) { + fake.GetPlansVisibleToOrgStub = nil fake.getPlansVisibleToOrgReturns = struct { result1 []models.ServicePlanFields result2 error }{result1, result2} } -var _ PlanBuilder = new(FakePlanBuilder) +var _ plan_builder.PlanBuilder = new(FakePlanBuilder) diff --git a/cf/actors/plan_builder/plan_builder.go b/cf/actors/plan_builder/plan_builder.go index 82c24ae88f6..399c61a8312 100644 --- a/cf/actors/plan_builder/plan_builder.go +++ b/cf/actors/plan_builder/plan_builder.go @@ -11,6 +11,7 @@ type PlanBuilder interface { AttachOrgToPlans([]models.ServicePlanFields, string) ([]models.ServicePlanFields, error) GetPlansForServiceForOrg(string, string) ([]models.ServicePlanFields, error) GetPlansForServiceWithOrgs(string) ([]models.ServicePlanFields, error) + GetPlansForManyServicesWithOrgs([]string) ([]models.ServicePlanFields, error) GetPlansForService(string) ([]models.ServicePlanFields, error) GetPlansVisibleToOrg(string) ([]models.ServicePlanFields, error) } @@ -94,6 +95,19 @@ func (builder Builder) GetPlansForServiceWithOrgs(serviceGuid string) ([]models. return plans, nil } +func (builder Builder) GetPlansForManyServicesWithOrgs(serviceGuids []string) ([]models.ServicePlanFields, error) { + plans, err := builder.servicePlanRepo.ListPlansFromManyServices(serviceGuids) + if err != nil { + return nil, err + } + + plans, err = builder.AttachOrgsToPlans(plans) + if err != nil { + return nil, err + } + return plans, nil +} + func (builder Builder) GetPlansVisibleToOrg(orgName string) ([]models.ServicePlanFields, error) { var plansToReturn []models.ServicePlanFields allPlans, err := builder.servicePlanRepo.Search(nil) diff --git a/cf/actors/plan_builder/plan_builder_test.go b/cf/actors/plan_builder/plan_builder_test.go index 496f98cb2d6..7b317bf33fb 100644 --- a/cf/actors/plan_builder/plan_builder_test.go +++ b/cf/actors/plan_builder/plan_builder_test.go @@ -1,6 +1,8 @@ package plan_builder_test import ( + "errors" + "github.com/cloudfoundry/cli/cf/actors/plan_builder" "github.com/cloudfoundry/cli/cf/api/fakes" testorg "github.com/cloudfoundry/cli/cf/api/organizations/fakes" @@ -35,12 +37,12 @@ var _ = Describe("Plan builder", func() { Guid: "service-plan1-guid", ServiceOfferingGuid: "service-guid1", } - plan2 = models.ServicePlanFields{ Name: "service-plan2", Guid: "service-plan2-guid", ServiceOfferingGuid: "service-guid1", } + planRepo.SearchReturns = map[string][]models.ServicePlanFields{ "service-guid1": []models.ServicePlanFields{plan1, plan2}, } @@ -93,6 +95,29 @@ var _ = Describe("Plan builder", func() { }) }) + Describe(".GetPlansForManyServicesWithOrgs", func() { + It("returns all the plans for all service in a list of guids", func() { + planRepo.ListPlansFromManyServicesReturns = []models.ServicePlanFields{ + plan1, plan2, + } + serviceGuids := []string{"service-guid1", "service-guid2"} + plans, err := builder.GetPlansForManyServicesWithOrgs(serviceGuids) + Expect(err).ToNot(HaveOccurred()) + + Expect(len(plans)).To(Equal(2)) + Expect(plans[0].Name).To(Equal("service-plan1")) + Expect(plans[0].OrgNames).To(Equal([]string{"org1", "org2"})) + Expect(plans[1].Name).To(Equal("service-plan2")) + }) + + It("returns errors from the service plan repo", func() { + planRepo.ListPlansFromManyServicesError = errors.New("Error") + serviceGuids := []string{"service-guid1", "service-guid2"} + _, err := builder.GetPlansForManyServicesWithOrgs(serviceGuids) + Expect(err).To(HaveOccurred()) + }) + }) + Describe(".GetPlansForService", func() { It("returns all the plans for the service with the provided guid", func() { plans, err := builder.GetPlansForService("service-guid1") diff --git a/cf/actors/service_builder/service_builder.go b/cf/actors/service_builder/service_builder.go index 4dc7d75cd53..52e1a2a2b59 100644 --- a/cf/actors/service_builder/service_builder.go +++ b/cf/actors/service_builder/service_builder.go @@ -174,7 +174,7 @@ func (builder Builder) GetServicesForManyBrokers(brokerGuids []string) ([]models if err != nil { return nil, err } - return builder.attachPlansToManyServices(services) + return builder.populateServicesWithPlansAndOrgs(services) } func (builder Builder) GetServicesForBroker(brokerGuid string) ([]models.ServiceOffering, error) { @@ -182,7 +182,20 @@ func (builder Builder) GetServicesForBroker(brokerGuid string) ([]models.Service if err != nil { return nil, err } - return builder.attachPlansToManyServices(services) + return builder.populateServicesWithPlansAndOrgs(services) +} + +func (builder Builder) populateServicesWithPlansAndOrgs(services []models.ServiceOffering) ([]models.ServiceOffering, error) { + serviceGuids := []string{} + for _, service := range services { + serviceGuids = append(serviceGuids, service.Guid) + } + + plans, err := builder.planBuilder.GetPlansForManyServicesWithOrgs(serviceGuids) + if err != nil { + return nil, err + } + return builder.attachPlansToManyServices(services, plans) } func (builder Builder) GetServiceVisibleToOrg(serviceName string, orgName string) (models.ServiceOffering, error) { @@ -221,13 +234,13 @@ func (builder Builder) attachPlansToServiceForOrg(service models.ServiceOffering return service, nil } -func (builder Builder) attachPlansToManyServices(services []models.ServiceOffering) ([]models.ServiceOffering, error) { - var err error - - for index, service := range services { - services[index], err = builder.attachPlansToService(service) - if err != nil { - return nil, err +func (builder Builder) attachPlansToManyServices(services []models.ServiceOffering, plans []models.ServicePlanFields) ([]models.ServiceOffering, error) { + for _, plan := range plans { + for index, service := range services { + if service.Guid == plan.ServiceOfferingGuid { + services[index].Plans = append(service.Plans, plan) + break + } } } return services, nil diff --git a/cf/actors/service_builder/service_builder_test.go b/cf/actors/service_builder/service_builder_test.go index 12c5fff3c1f..2bce689727a 100644 --- a/cf/actors/service_builder/service_builder_test.go +++ b/cf/actors/service_builder/service_builder_test.go @@ -24,6 +24,7 @@ var _ = Describe("Service Builder", func() { planWithoutOrgs models.ServicePlanFields plan1 models.ServicePlanFields plan2 models.ServicePlanFields + plan3 models.ServicePlanFields ) BeforeEach(func() { @@ -89,6 +90,12 @@ var _ = Describe("Service Builder", func() { ServiceOfferingGuid: "service-guid1", } + plan3 = models.ServicePlanFields{ + Name: "service-plan3", + Guid: "service-plan3-guid", + ServiceOfferingGuid: "service-guid2", + } + planWithoutOrgs = models.ServicePlanFields{ Name: "service-plan-without-orgs", Guid: "service-plan-without-orgs-guid", @@ -97,6 +104,7 @@ var _ = Describe("Service Builder", func() { planBuilder.GetPlansVisibleToOrgReturns([]models.ServicePlanFields{plan1, plan2}, nil) planBuilder.GetPlansForServiceWithOrgsReturns([]models.ServicePlanFields{plan1, plan2}, nil) + planBuilder.GetPlansForManyServicesWithOrgsReturns([]models.ServicePlanFields{plan1, plan2, plan3}, nil) planBuilder.GetPlansForServiceForOrgReturns([]models.ServicePlanFields{plan1, plan2}, nil) }) @@ -407,10 +415,8 @@ var _ = Describe("Service Builder", func() { broker_service2 := services[1] Expect(broker_service2.Label).To(Equal("my-service2")) - Expect(len(broker_service2.Plans)).To(Equal(2)) - Expect(broker_service.Plans[0].Name).To(Equal("service-plan1")) - Expect(broker_service.Plans[1].Name).To(Equal("service-plan2")) - Expect(broker_service.Plans[0].OrgNames).To(Equal([]string{"org1", "org2"})) + Expect(len(broker_service2.Plans)).To(Equal(1)) + Expect(broker_service2.Plans[0].Name).To(Equal("service-plan3")) }) It("raises errors from the service repo", func() { @@ -419,6 +425,13 @@ var _ = Describe("Service Builder", func() { _, err := serviceBuilder.GetServicesForManyBrokers(brokerGuids) Expect(err).To(HaveOccurred()) }) + + It("raises errors from the plan builder", func() { + planBuilder.GetPlansForManyServicesWithOrgsReturns(nil, errors.New("error")) + brokerGuids := []string{"my-service-broker-guid1", "my-service-broker-guid2"} + _, err := serviceBuilder.GetServicesForManyBrokers(brokerGuids) + Expect(err).To(HaveOccurred()) + }) }) Describe(".GetServiceVisibleToOrg", func() { diff --git a/cf/api/fakes/fake_service_plan_repo.go b/cf/api/fakes/fake_service_plan_repo.go index d78480ee80e..32b319ad1fc 100644 --- a/cf/api/fakes/fake_service_plan_repo.go +++ b/cf/api/fakes/fake_service_plan_repo.go @@ -22,6 +22,20 @@ type FakeServicePlanRepo struct { updateReturns struct { result1 error } + + ListPlansFromManyServicesReturns []models.ServicePlanFields + ListPlansFromManyServicesError error +} + +func (fake *FakeServicePlanRepo) ListPlansFromManyServices(serviceGuids []string) (plans []models.ServicePlanFields, err error) { + if fake.ListPlansFromManyServicesError != nil { + return nil, fake.ListPlansFromManyServicesError + } + + if fake.ListPlansFromManyServicesReturns != nil { + return fake.ListPlansFromManyServicesReturns, nil + } + return []models.ServicePlanFields{}, nil } func (fake *FakeServicePlanRepo) Search(queryParams map[string]string) ([]models.ServicePlanFields, error) { diff --git a/cf/api/service_plan.go b/cf/api/service_plan.go index 2e5cd62641c..45f0aa17940 100644 --- a/cf/api/service_plan.go +++ b/cf/api/service_plan.go @@ -14,6 +14,7 @@ import ( type ServicePlanRepository interface { Search(searchParameters map[string]string) ([]models.ServicePlanFields, error) Update(models.ServicePlanFields, string, bool) error + ListPlansFromManyServices(serviceGuids []string) ([]models.ServicePlanFields, error) } type CloudControllerServicePlanRepository struct { @@ -43,6 +44,22 @@ func (repo CloudControllerServicePlanRepository) Update(servicePlan models.Servi return repo.gateway.UpdateResource(repo.config.ApiEndpoint(), url, strings.NewReader(body)) } +func (repo CloudControllerServicePlanRepository) ListPlansFromManyServices(serviceGuids []string) (plans []models.ServicePlanFields, err error) { + serviceGuidsString := strings.Join(serviceGuids, ",") + + err = repo.gateway.ListPaginatedResources( + repo.config.ApiEndpoint(), + fmt.Sprintf("/v2/service_plans?q=%s", url.QueryEscape("service_guid IN "+serviceGuidsString)), + resources.ServicePlanResource{}, + func(resource interface{}) bool { + if plan, ok := resource.(resources.ServicePlanResource); ok { + plans = append(plans, plan.ToFields()) + } + return true + }) + return +} + func (repo CloudControllerServicePlanRepository) Search(queryParams map[string]string) (plans []models.ServicePlanFields, err error) { err = repo.gateway.ListPaginatedResources( repo.config.ApiEndpoint(), diff --git a/cf/api/service_plan_test.go b/cf/api/service_plan_test.go index 434f55106f1..607ca0d7959 100644 --- a/cf/api/service_plan_test.go +++ b/cf/api/service_plan_test.go @@ -115,6 +115,28 @@ var _ = Describe("Service Plan Repository", func() { Expect(err).NotTo(HaveOccurred()) }) }) + + Describe(".ListPlansFromManyServices", func() { + BeforeEach(func() { + setupTestServer(manyServiceRequest1, manyServiceRequest2) + }) + + It("returns all service plans for a list of service guids", func() { + serviceGuids := []string{"service-guid1", "service-guid2"} + + servicePlansFields, err := repo.ListPlansFromManyServices(serviceGuids) + Expect(err).NotTo(HaveOccurred()) + + Expect(testHandler).To(HaveAllRequestsCalled()) + Expect(len(servicePlansFields)).To(Equal(2)) + + Expect(servicePlansFields[0].Name).To(Equal("plan one")) + Expect(servicePlansFields[0].Guid).To(Equal("plan1")) + + Expect(servicePlansFields[1].Name).To(Equal("plan two")) + Expect(servicePlansFields[1].Guid).To(Equal("plan2")) + }) + }) }) var firstPlanRequest = testapi.NewCloudControllerTestRequest(testnet.TestRequest{ @@ -218,3 +240,56 @@ var secondPlanRequestWithParams = testapi.NewCloudControllerTestRequest(testnet. }`, }, }) + +var manyServiceRequest1 = testapi.NewCloudControllerTestRequest(testnet.TestRequest{ + Method: "GET", + Path: "/v2/service_plans?q=service_guid+IN+service-guid1,service-guid2", + Response: testnet.TestResponse{ + Status: http.StatusOK, + Body: `{ + "total_results": 2, + "total_pages": 2, + "next_url": "/v2/service_plans?q=service_guid+IN+service-guid1,service-guid2&page=2", + "resources": [ + { + "metadata": { + "guid": "plan1" + }, + "entity": { + "name": "plan one", + "free": true, + "public": true, + "active": true + } + } + ] +}`, + }, +}) + +var manyServiceRequest2 = testapi.NewCloudControllerTestRequest(testnet.TestRequest{ + Method: "GET", + Path: "/v2/service_plans?q=service_guid+IN+service-guid1,service-guid2&page=2", + Response: testnet.TestResponse{ + Status: http.StatusOK, + Body: `{ + "total_results": 2, + "total_pages": 1, + "next_url": null, + "prev_url": "/v2/service_plans?q=service_guid+IN+service-guid1,service-guid2", + "resources": [ + { + "metadata": { + "guid": "plan2" + }, + "entity": { + "name": "plan two", + "free": true, + "public": true, + "active": true + } + } + ] +}`, + }, +}) From 1b84a90c22dd3e2ac3fa9a239a784d71c8d7b62b Mon Sep 17 00:00:00 2001 From: "Greg Cobb, Jeff Pak and Raina Masand" Date: Thu, 18 Jun 2015 15:21:47 -0700 Subject: [PATCH 3/4] Reduce service_access API requests: orgs - To map org guids to org names, we make individual requests for each org instead of requesting all orgs. - This is optimized for the case where there are fewer orgs associated with service_plan_visibilities than the total number of org pages. This seemed to be the case on all environments we checked. - /v2/organizations does not support filtering on a list of org or service_plan_visiblility guids, so we have to make separate GETs - In plan_builder, there are package variables that are used to memoize maps. This causes pollution plan_builder tests, so we nil them in test setup [#96912380] --- cf/actors/plan_builder/plan_builder.go | 30 ++++++++++--- cf/actors/plan_builder/plan_builder_test.go | 9 +++- .../fakes/fake_organization_repository.go | 42 +++++++++++++++++++ cf/api/organizations/organizations.go | 15 +++++++ cf/api/organizations/organizations_test.go | 32 ++++++++++++++ 5 files changed, 121 insertions(+), 7 deletions(-) diff --git a/cf/actors/plan_builder/plan_builder.go b/cf/actors/plan_builder/plan_builder.go index 399c61a8312..5da35497e83 100644 --- a/cf/actors/plan_builder/plan_builder.go +++ b/cf/actors/plan_builder/plan_builder.go @@ -172,29 +172,49 @@ func (builder Builder) buildPlanToOrgsVisibilityMap() (map[string][]string, erro if PlanToOrgsVisibilityMap == nil { orgLookup := make(map[string]string) - orgs, err := builder.orgRepo.ListOrgs() + visibilities, err := builder.servicePlanVisibilityRepo.List() if err != nil { return nil, err } - for _, org := range orgs { - orgLookup[org.Guid] = org.Name - } - visibilities, err := builder.servicePlanVisibilityRepo.List() + orgGuids := builder.getUniqueOrgGuidsFromVisibilities(visibilities) + + orgs, err := builder.orgRepo.GetManyOrgsByGuid(orgGuids) if err != nil { return nil, err } + for _, org := range orgs { + orgLookup[org.Guid] = org.Name + } + visMap := make(map[string][]string) for _, vis := range visibilities { visMap[vis.ServicePlanGuid] = append(visMap[vis.ServicePlanGuid], orgLookup[vis.OrganizationGuid]) } + PlanToOrgsVisibilityMap = &visMap } return *PlanToOrgsVisibilityMap, nil } +func (builder Builder) getUniqueOrgGuidsFromVisibilities(visibilities []models.ServicePlanVisibilityFields) (orgGuids []string) { + for _, visibility := range visibilities { + found := false + for _, orgGuid := range orgGuids { + if orgGuid == visibility.OrganizationGuid { + found = true + break + } + } + if !found { + orgGuids = append(orgGuids, visibility.OrganizationGuid) + } + } + return +} + func (builder Builder) buildOrgToPlansVisibilityMap(planToOrgsMap map[string][]string) map[string][]string { if OrgToPlansVisibilityMap == nil { visMap := make(map[string][]string) diff --git a/cf/actors/plan_builder/plan_builder_test.go b/cf/actors/plan_builder/plan_builder_test.go index 7b317bf33fb..e1e0534a705 100644 --- a/cf/actors/plan_builder/plan_builder_test.go +++ b/cf/actors/plan_builder/plan_builder_test.go @@ -27,6 +27,8 @@ var _ = Describe("Plan builder", func() { ) BeforeEach(func() { + plan_builder.PlanToOrgsVisibilityMap = nil + plan_builder.OrgToPlansVisibilityMap = nil planRepo = &fakes.FakeServicePlanRepo{} visibilityRepo = &fakes.FakeServicePlanVisibilityRepository{} orgRepo = &testorg.FakeOrganizationRepository{} @@ -56,8 +58,9 @@ var _ = Describe("Plan builder", func() { visibilityRepo.ListReturns([]models.ServicePlanVisibilityFields{ {ServicePlanGuid: "service-plan1-guid", OrganizationGuid: "org1-guid"}, {ServicePlanGuid: "service-plan1-guid", OrganizationGuid: "org2-guid"}, + {ServicePlanGuid: "service-plan2-guid", OrganizationGuid: "org1-guid"}, }, nil) - orgRepo.ListOrgsReturns([]models.Organization{org1, org2}, nil) + orgRepo.GetManyOrgsByGuidReturns([]models.Organization{org1, org2}, nil) }) Describe(".AttachOrgsToPlans", func() { @@ -103,6 +106,8 @@ var _ = Describe("Plan builder", func() { serviceGuids := []string{"service-guid1", "service-guid2"} plans, err := builder.GetPlansForManyServicesWithOrgs(serviceGuids) Expect(err).ToNot(HaveOccurred()) + Expect(orgRepo.GetManyOrgsByGuidCallCount()).To(Equal(1)) + Expect(orgRepo.GetManyOrgsByGuidArgsForCall(0)).To(ConsistOf("org1-guid", "org2-guid")) Expect(len(plans)).To(Equal(2)) Expect(plans[0].Name).To(Equal("service-plan1")) @@ -148,7 +153,7 @@ var _ = Describe("Plan builder", func() { plans, err := builder.GetPlansVisibleToOrg("org1") Expect(err).ToNot(HaveOccurred()) - Expect(len(plans)).To(Equal(1)) + Expect(len(plans)).To(Equal(2)) Expect(plans[0].Name).To(Equal("service-plan1")) Expect(plans[0].OrgNames).To(Equal([]string{"org1", "org2"})) }) diff --git a/cf/api/organizations/fakes/fake_organization_repository.go b/cf/api/organizations/fakes/fake_organization_repository.go index 4636061e3eb..475df00dafa 100644 --- a/cf/api/organizations/fakes/fake_organization_repository.go +++ b/cf/api/organizations/fakes/fake_organization_repository.go @@ -16,6 +16,15 @@ type FakeOrganizationRepository struct { result1 []models.Organization result2 error } + GetManyOrgsByGuidStub func(orgGuids []string) (orgs []models.Organization, apiErr error) + getManyOrgsByGuidMutex sync.RWMutex + getManyOrgsByGuidArgsForCall []struct { + orgGuids []string + } + getManyOrgsByGuidReturns struct { + result1 []models.Organization + result2 error + } FindByNameStub func(name string) (org models.Organization, apiErr error) findByNameMutex sync.RWMutex findByNameArgsForCall []struct { @@ -95,6 +104,39 @@ func (fake *FakeOrganizationRepository) ListOrgsReturns(result1 []models.Organiz }{result1, result2} } +func (fake *FakeOrganizationRepository) GetManyOrgsByGuid(orgGuids []string) (orgs []models.Organization, apiErr error) { + fake.getManyOrgsByGuidMutex.Lock() + fake.getManyOrgsByGuidArgsForCall = append(fake.getManyOrgsByGuidArgsForCall, struct { + orgGuids []string + }{orgGuids}) + fake.getManyOrgsByGuidMutex.Unlock() + if fake.GetManyOrgsByGuidStub != nil { + return fake.GetManyOrgsByGuidStub(orgGuids) + } else { + return fake.getManyOrgsByGuidReturns.result1, fake.getManyOrgsByGuidReturns.result2 + } +} + +func (fake *FakeOrganizationRepository) GetManyOrgsByGuidCallCount() int { + fake.getManyOrgsByGuidMutex.RLock() + defer fake.getManyOrgsByGuidMutex.RUnlock() + return len(fake.getManyOrgsByGuidArgsForCall) +} + +func (fake *FakeOrganizationRepository) GetManyOrgsByGuidArgsForCall(i int) []string { + fake.getManyOrgsByGuidMutex.RLock() + defer fake.getManyOrgsByGuidMutex.RUnlock() + return fake.getManyOrgsByGuidArgsForCall[i].orgGuids +} + +func (fake *FakeOrganizationRepository) GetManyOrgsByGuidReturns(result1 []models.Organization, result2 error) { + fake.GetManyOrgsByGuidStub = nil + fake.getManyOrgsByGuidReturns = struct { + result1 []models.Organization + result2 error + }{result1, result2} +} + func (fake *FakeOrganizationRepository) FindByName(name string) (org models.Organization, apiErr error) { fake.findByNameMutex.Lock() fake.findByNameArgsForCall = append(fake.findByNameArgsForCall, struct { diff --git a/cf/api/organizations/organizations.go b/cf/api/organizations/organizations.go index 1815af5132a..d362bf49efd 100644 --- a/cf/api/organizations/organizations.go +++ b/cf/api/organizations/organizations.go @@ -15,6 +15,7 @@ import ( //go:generate counterfeiter -o fakes/fake_organization_repository.go . OrganizationRepository type OrganizationRepository interface { ListOrgs() (orgs []models.Organization, apiErr error) + GetManyOrgsByGuid(orgGuids []string) (orgs []models.Organization, apiErr error) FindByName(name string) (org models.Organization, apiErr error) Create(org models.Organization) (apiErr error) Rename(orgGuid string, name string) (apiErr error) @@ -52,6 +53,20 @@ func (repo CloudControllerOrganizationRepository) ListOrgs() ([]models.Organizat return orgs, err } +func (repo CloudControllerOrganizationRepository) GetManyOrgsByGuid(orgGuids []string) (orgs []models.Organization, err error) { + for _, orgGuid := range orgGuids { + url := fmt.Sprintf("%s/v2/organizations/%s", repo.config.ApiEndpoint(), orgGuid) + orgResource := resources.OrganizationResource{} + err = repo.gateway.GetResource(url, &orgResource) + if err != nil { + return nil, err + } else { + orgs = append(orgs, orgResource.ToModel()) + } + } + return +} + func (repo CloudControllerOrganizationRepository) FindByName(name string) (org models.Organization, apiErr error) { found := false apiErr = repo.gateway.ListPaginatedResources( diff --git a/cf/api/organizations/organizations_test.go b/cf/api/organizations/organizations_test.go index 173c421768a..c9f16efec25 100644 --- a/cf/api/organizations/organizations_test.go +++ b/cf/api/organizations/organizations_test.go @@ -81,6 +81,38 @@ var _ = Describe("Organization Repository", func() { }) }) + Describe(".GetManyOrgsByGuid", func() { + It("requests each org", func() { + firstOrgRequest := testapi.NewCloudControllerTestRequest(testnet.TestRequest{ + Method: "GET", + Path: "/v2/organizations/org1-guid", + Response: testnet.TestResponse{Status: http.StatusOK, Body: `{ + "metadata": { "guid": "org1-guid" }, + "entity": { "name": "Org1" } + }`}, + }) + secondOrgRequest := testapi.NewCloudControllerTestRequest(testnet.TestRequest{ + Method: "GET", + Path: "/v2/organizations/org2-guid", + Response: testnet.TestResponse{Status: http.StatusOK, Body: `{ + "metadata": { "guid": "org2-guid" }, + "entity": { "name": "Org2" } + }`}, + }) + testserver, handler, repo := createOrganizationRepo(firstOrgRequest, secondOrgRequest) + defer testserver.Close() + + orgGuids := []string{"org1-guid", "org2-guid"} + orgs, err := repo.GetManyOrgsByGuid(orgGuids) + Expect(err).NotTo(HaveOccurred()) + + Expect(handler).To(HaveAllRequestsCalled()) + Expect(len(orgs)).To(Equal(2)) + Expect(orgs[0].Guid).To(Equal("org1-guid")) + Expect(orgs[1].Guid).To(Equal("org2-guid")) + }) + }) + Describe("finding organizations by name", func() { It("returns the org with that name", func() { req := testapi.NewCloudControllerTestRequest(testnet.TestRequest{ From 8f35d0f9a5bc0804315f3cd545572b33d854fb90 Mon Sep 17 00:00:00 2001 From: Greg Cobb and Raina Masand Date: Mon, 22 Jun 2015 16:51:10 -0700 Subject: [PATCH 4/4] Declare return vars explicitly in func - And return them by name --- cf/api/service_plan.go | 7 ++++--- cf/api/services.go | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/cf/api/service_plan.go b/cf/api/service_plan.go index 45f0aa17940..8181b8dfd81 100644 --- a/cf/api/service_plan.go +++ b/cf/api/service_plan.go @@ -44,10 +44,11 @@ func (repo CloudControllerServicePlanRepository) Update(servicePlan models.Servi return repo.gateway.UpdateResource(repo.config.ApiEndpoint(), url, strings.NewReader(body)) } -func (repo CloudControllerServicePlanRepository) ListPlansFromManyServices(serviceGuids []string) (plans []models.ServicePlanFields, err error) { +func (repo CloudControllerServicePlanRepository) ListPlansFromManyServices(serviceGuids []string) ([]models.ServicePlanFields, error) { serviceGuidsString := strings.Join(serviceGuids, ",") + plans := []models.ServicePlanFields{} - err = repo.gateway.ListPaginatedResources( + err := repo.gateway.ListPaginatedResources( repo.config.ApiEndpoint(), fmt.Sprintf("/v2/service_plans?q=%s", url.QueryEscape("service_guid IN "+serviceGuidsString)), resources.ServicePlanResource{}, @@ -57,7 +58,7 @@ func (repo CloudControllerServicePlanRepository) ListPlansFromManyServices(servi } return true }) - return + return plans, err } func (repo CloudControllerServicePlanRepository) Search(queryParams map[string]string) (plans []models.ServicePlanFields, err error) { diff --git a/cf/api/services.go b/cf/api/services.go index cb7eca2710f..22301471029 100644 --- a/cf/api/services.go +++ b/cf/api/services.go @@ -256,10 +256,11 @@ func (repo CloudControllerServiceRepository) FindServicePlanByDescription(planDe return planGuid, apiErr } -func (repo CloudControllerServiceRepository) ListServicesFromManyBrokers(brokerGuids []string) (services []models.ServiceOffering, err error) { +func (repo CloudControllerServiceRepository) ListServicesFromManyBrokers(brokerGuids []string) ([]models.ServiceOffering, error) { brokerGuidsString := strings.Join(brokerGuids, ",") + services := []models.ServiceOffering{} - err = repo.gateway.ListPaginatedResources( + err := repo.gateway.ListPaginatedResources( repo.config.ApiEndpoint(), fmt.Sprintf("/v2/services?q=%s", url.QueryEscape("service_broker_guid IN "+brokerGuidsString)), resources.ServiceOfferingResource{}, @@ -269,7 +270,7 @@ func (repo CloudControllerServiceRepository) ListServicesFromManyBrokers(brokerG } return true }) - return + return services, err } func (repo CloudControllerServiceRepository) ListServicesFromBroker(brokerGuid string) (offerings []models.ServiceOffering, err error) {