From 2a5725fe119739ca223ebb408d32b256aabf9861 Mon Sep 17 00:00:00 2001 From: Danail Branekov Date: Thu, 8 Aug 2024 14:55:09 +0000 Subject: [PATCH] Include service offerings when listing plans fixes #3279 Co-authored-by: Georgi Sabev --- api/handlers/service_plan.go | 56 +++++++- api/handlers/service_plan_test.go | 52 ++++++- api/main.go | 1 + api/payloads/service_plan.go | 8 ++ api/payloads/service_plan_test.go | 2 + .../service_offering_repository.go | 4 +- .../service_offering_repository_test.go | 129 ++++++++++-------- 7 files changed, 185 insertions(+), 67 deletions(-) diff --git a/api/handlers/service_plan.go b/api/handlers/service_plan.go index 35f838bc7..bf6e82e69 100644 --- a/api/handlers/service_plan.go +++ b/api/handlers/service_plan.go @@ -5,6 +5,7 @@ import ( "context" "net/http" "net/url" + "slices" "code.cloudfoundry.org/korifi/api/authorization" apierrors "code.cloudfoundry.org/korifi/api/errors" @@ -12,6 +13,9 @@ import ( "code.cloudfoundry.org/korifi/api/presenter" "code.cloudfoundry.org/korifi/api/repositories" "code.cloudfoundry.org/korifi/api/routing" + "code.cloudfoundry.org/korifi/model" + "code.cloudfoundry.org/korifi/tools" + "github.com/BooleanCat/go-functional/iter" "github.com/go-logr/logr" ) @@ -29,20 +33,23 @@ type CFServicePlanRepository interface { } type ServicePlan struct { - serverURL url.URL - requestValidator RequestValidator - servicePlanRepo CFServicePlanRepository + serverURL url.URL + requestValidator RequestValidator + servicePlanRepo CFServicePlanRepository + serviceOfferingRepo CFServiceOfferingRepository } func NewServicePlan( serverURL url.URL, requestValidator RequestValidator, servicePlanRepo CFServicePlanRepository, + serviceOfferingRepo CFServiceOfferingRepository, ) *ServicePlan { return &ServicePlan{ - serverURL: serverURL, - requestValidator: requestValidator, - servicePlanRepo: servicePlanRepo, + serverURL: serverURL, + requestValidator: requestValidator, + servicePlanRepo: servicePlanRepo, + serviceOfferingRepo: serviceOfferingRepo, } } @@ -60,7 +67,42 @@ func (h *ServicePlan) list(r *http.Request) (*routing.Response, error) { return nil, apierrors.LogAndReturn(logger, err, "failed to list service plans") } - return routing.NewResponse(http.StatusOK).WithBody(presenter.ForList(presenter.ForServicePlan, servicePlanList, h.serverURL, *r.URL)), nil + includedResources := []model.IncludedResource{} + + if slices.Contains(payload.IncludeResources, "service_offering") { + includedOfferings, err := h.getIncludedServiceOfferings(r.Context(), authInfo, servicePlanList, h.serverURL) + if err != nil { + return nil, apierrors.LogAndReturn(logger, err, "failed to get included service offerings") + } + includedResources = append(includedResources, includedOfferings...) + } + + return routing.NewResponse(http.StatusOK).WithBody(presenter.ForList(presenter.ForServicePlan, servicePlanList, h.serverURL, *r.URL, includedResources...)), nil +} + +func (h *ServicePlan) getIncludedServiceOfferings( + ctx context.Context, + authInfo authorization.Info, + servicePlans []repositories.ServicePlanRecord, + baseUrl url.URL, +) ([]model.IncludedResource, error) { + offeringGUIDs := iter.Map(iter.Lift(servicePlans), func(o repositories.ServicePlanRecord) string { + return o.ServiceOfferingGUID + }).Collect() + + offerings, err := h.serviceOfferingRepo.ListOfferings(ctx, authInfo, repositories.ListServiceOfferingMessage{ + GUIDs: tools.Uniq(offeringGUIDs), + }) + if err != nil { + return nil, err + } + + return iter.Map(iter.Lift(offerings), func(o repositories.ServiceOfferingRecord) model.IncludedResource { + return model.IncludedResource{ + Type: "service_offerings", + Resource: presenter.ForServiceOffering(o, baseUrl), + } + }).Collect(), nil } func (h *ServicePlan) getPlanVisibility(r *http.Request) (*routing.Response, error) { diff --git a/api/handlers/service_plan_test.go b/api/handlers/service_plan_test.go index 9317aacd0..51e317662 100644 --- a/api/handlers/service_plan_test.go +++ b/api/handlers/service_plan_test.go @@ -10,6 +10,7 @@ import ( "code.cloudfoundry.org/korifi/api/payloads" "code.cloudfoundry.org/korifi/api/repositories" "code.cloudfoundry.org/korifi/model" + "code.cloudfoundry.org/korifi/model/services" . "code.cloudfoundry.org/korifi/tests/matchers" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" @@ -19,18 +20,21 @@ import ( var _ = Describe("ServicePlan", func() { var ( - servicePlanRepo *fake.CFServicePlanRepository - requestValidator *fake.RequestValidator + servicePlanRepo *fake.CFServicePlanRepository + serviceOfferingRepo *fake.CFServiceOfferingRepository + requestValidator *fake.RequestValidator ) BeforeEach(func() { requestValidator = new(fake.RequestValidator) servicePlanRepo = new(fake.CFServicePlanRepository) + serviceOfferingRepo = new(fake.CFServiceOfferingRepository) apiHandler := NewServicePlan( *serverURL, requestValidator, servicePlanRepo, + serviceOfferingRepo, ) routerBuilder.LoadRoutes(apiHandler) }) @@ -43,6 +47,15 @@ var _ = Describe("ServicePlan", func() { }, ServiceOfferingGUID: "service-offering-guid", }}, nil) + + serviceOfferingRepo.ListOfferingsReturns([]repositories.ServiceOfferingRecord{{ + ServiceOffering: services.ServiceOffering{ + Name: "service-offering-name", + }, + CFResource: model.CFResource{ + GUID: "service-offering-guid", + }, + }}, nil) }) JustBeforeEach(func() { @@ -65,6 +78,7 @@ var _ = Describe("ServicePlan", func() { MatchJSONPath("$.resources[0].guid", "plan-guid"), MatchJSONPath("$.resources[0].links.self.href", "https://api.example.org/v3/service_plans/plan-guid"), MatchJSONPath("$.resources[0].links.service_offering.href", "https://api.example.org/v3/service_offerings/service-offering-guid"), + Not(ContainSubstring("included")), ))) }) @@ -82,6 +96,40 @@ var _ = Describe("ServicePlan", func() { }) }) + Describe("include service_offering", func() { + BeforeEach(func() { + requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.ServicePlanList{ + IncludeResources: []string{"service_offering"}, + }) + }) + + It("lists the offerings", func() { + Expect(serviceOfferingRepo.ListOfferingsCallCount()).To(Equal(1)) + _, _, actualListMessage := serviceOfferingRepo.ListOfferingsArgsForCall(0) + Expect(actualListMessage).To(Equal(repositories.ListServiceOfferingMessage{ + GUIDs: []string{"service-offering-guid"}, + })) + }) + + When("listing offerings fails", func() { + BeforeEach(func() { + serviceOfferingRepo.ListOfferingsReturns([]repositories.ServiceOfferingRecord{}, errors.New("list-offering-err")) + }) + + It("returns an error", func() { + expectUnknownError() + }) + }) + + It("includes broker fields in the response", func() { + Expect(rr).Should(HaveHTTPStatus(http.StatusOK)) + Expect(rr).To(HaveHTTPBody(SatisfyAll( + MatchJSONPath("$.included.service_offerings[0].guid", "service-offering-guid"), + MatchJSONPath("$.included.service_offerings[0].name", "service-offering-name"), + ))) + }) + }) + When("the request is invalid", func() { BeforeEach(func() { requestValidator.DecodeAndValidateURLValuesReturns(errors.New("invalid-request")) diff --git a/api/main.go b/api/main.go index e967ea7aa..0aa8ffb0d 100644 --- a/api/main.go +++ b/api/main.go @@ -431,6 +431,7 @@ func main() { *serverURL, requestValidator, servicePlanRepo, + serviceOfferingRepo, ), } for _, handler := range apiHandlers { diff --git a/api/payloads/service_plan.go b/api/payloads/service_plan.go index 5d6edc013..d97ff00b3 100644 --- a/api/payloads/service_plan.go +++ b/api/payloads/service_plan.go @@ -20,6 +20,13 @@ type ServicePlanList struct { ServiceOfferingGUIDs string Names string Available *bool + IncludeResources []string +} + +func (l ServicePlanList) Validate() error { + return jellidation.ValidateStruct(&l, + jellidation.Field(&l.IncludeResources, jellidation.Each(validation.OneOf("service_offering"))), + ) } func (l *ServicePlanList) ToMessage() repositories.ListServicePlanMessage { @@ -47,6 +54,7 @@ func (l *ServicePlanList) DecodeFromURLValues(values url.Values) error { return fmt.Errorf("failed to parse 'available' query parameter: %w", err) } l.Available = available + l.IncludeResources = parse.ArrayParam(values.Get("include")) return nil } diff --git a/api/payloads/service_plan_test.go b/api/payloads/service_plan_test.go index 855ef3016..9a973b6de 100644 --- a/api/payloads/service_plan_test.go +++ b/api/payloads/service_plan_test.go @@ -24,6 +24,7 @@ var _ = Describe("ServicePlan", func() { Entry("names", "names=b1,b2", payloads.ServicePlanList{Names: "b1,b2"}), Entry("available", "available=true", payloads.ServicePlanList{Available: tools.PtrTo(true)}), Entry("not available", "available=false", payloads.ServicePlanList{Available: tools.PtrTo(false)}), + Entry("include", "include=service_offering", payloads.ServicePlanList{IncludeResources: []string{"service_offering"}}), ) DescribeTable("invalid query", @@ -32,6 +33,7 @@ var _ = Describe("ServicePlan", func() { Expect(decodeErr).To(errMatcher) }, Entry("invalid available", "available=invalid", MatchError(ContainSubstring("failed to parse"))), + Entry("invalid include", "include=foo", MatchError(ContainSubstring("value must be one of: service_offering"))), ) Describe("ToMessage", func() { diff --git a/api/repositories/service_offering_repository.go b/api/repositories/service_offering_repository.go index 08ac63919..2fa5b1716 100644 --- a/api/repositories/service_offering_repository.go +++ b/api/repositories/service_offering_repository.go @@ -32,11 +32,13 @@ type ServiceOfferingRepo struct { type ListServiceOfferingMessage struct { Names []string + GUIDs []string BrokerNames []string } func (m *ListServiceOfferingMessage) matchesName(cfServiceOffering korifiv1alpha1.CFServiceOffering) bool { - return tools.EmptyOrContains(m.Names, cfServiceOffering.Spec.Name) + return tools.EmptyOrContains(m.Names, cfServiceOffering.Spec.Name) && + tools.EmptyOrContains(m.GUIDs, cfServiceOffering.Name) } func NewServiceOfferingRepo( diff --git a/api/repositories/service_offering_repository_test.go b/api/repositories/service_offering_repository_test.go index ca89fd47d..c787a3d87 100644 --- a/api/repositories/service_offering_repository_test.go +++ b/api/repositories/service_offering_repository_test.go @@ -30,15 +30,17 @@ var _ = Describe("ServiceOfferingRepo", func() { Describe("List", func() { var ( - offeringGUID string - broker *korifiv1alpha1.CFServiceBroker - listedOfferings []repositories.ServiceOfferingRecord - message repositories.ListServiceOfferingMessage - listErr error + offeringGUID string + anotherOfferingGUID string + broker *korifiv1alpha1.CFServiceBroker + listedOfferings []repositories.ServiceOfferingRecord + message repositories.ListServiceOfferingMessage + listErr error ) BeforeEach(func() { offeringGUID = uuid.NewString() + anotherOfferingGUID = uuid.NewString() broker = &korifiv1alpha1.CFServiceBroker{ ObjectMeta: metav1.ObjectMeta{ @@ -88,6 +90,21 @@ var _ = Describe("ServiceOfferingRepo", func() { }, })).To(Succeed()) + Expect(k8sClient.Create(ctx, &korifiv1alpha1.CFServiceOffering{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: rootNamespace, + Name: anotherOfferingGUID, + Labels: map[string]string{ + korifiv1alpha1.RelServiceBrokerLabel: "another-broker", + }, + }, + Spec: korifiv1alpha1.CFServiceOfferingSpec{ + ServiceOffering: services.ServiceOffering{ + Name: "another-offering", + }, + }, + })).To(Succeed()) + message = repositories.ListServiceOfferingMessage{} }) @@ -97,55 +114,50 @@ var _ = Describe("ServiceOfferingRepo", func() { It("lists service offerings", func() { Expect(listErr).NotTo(HaveOccurred()) - Expect(listedOfferings).To(ConsistOf(MatchFields(IgnoreExtras, Fields{ - "ServiceOffering": MatchFields(IgnoreExtras, Fields{ - "Name": Equal("my-offering"), - "Description": Equal("my offering description"), - "Tags": ConsistOf("t1"), - "Requires": ConsistOf("r1"), - "DocumentationURL": PointTo(Equal("https://my.offering.com")), - "BrokerCatalog": MatchFields(IgnoreExtras, Fields{ - "Id": Equal("offering-catalog-guid"), - "Metadata": PointTo(MatchFields(IgnoreExtras, Fields{ - "Raw": MatchJSON(`{"offering-md": "offering-md-value"}`), - })), - - "Features": MatchFields(IgnoreExtras, Fields{ - "PlanUpdateable": BeTrue(), - "Bindable": BeTrue(), - "InstancesRetrievable": BeTrue(), - "BindingsRetrievable": BeTrue(), - "AllowContextUpdates": BeTrue(), + Expect(listedOfferings).To(ConsistOf( + MatchFields(IgnoreExtras, Fields{ + "ServiceOffering": MatchFields(IgnoreExtras, Fields{ + "Name": Equal("my-offering"), + "Description": Equal("my offering description"), + "Tags": ConsistOf("t1"), + "Requires": ConsistOf("r1"), + "DocumentationURL": PointTo(Equal("https://my.offering.com")), + "BrokerCatalog": MatchFields(IgnoreExtras, Fields{ + "Id": Equal("offering-catalog-guid"), + "Metadata": PointTo(MatchFields(IgnoreExtras, Fields{ + "Raw": MatchJSON(`{"offering-md": "offering-md-value"}`), + })), + + "Features": MatchFields(IgnoreExtras, Fields{ + "PlanUpdateable": BeTrue(), + "Bindable": BeTrue(), + "InstancesRetrievable": BeTrue(), + "BindingsRetrievable": BeTrue(), + "AllowContextUpdates": BeTrue(), + }), }), }), + "CFResource": MatchFields(IgnoreExtras, Fields{ + "GUID": Equal(offeringGUID), + "CreatedAt": Not(BeZero()), + "UpdatedAt": BeNil(), + "Metadata": MatchAllFields(Fields{ + "Labels": HaveKeyWithValue(korifiv1alpha1.RelServiceBrokerLabel, broker.Name), + "Annotations": HaveKeyWithValue("annotation", "annotation-value"), + }), + }), + "ServiceBrokerGUID": Equal(broker.Name), }), - "CFResource": MatchFields(IgnoreExtras, Fields{ - "GUID": Equal(offeringGUID), - "CreatedAt": Not(BeZero()), - "UpdatedAt": BeNil(), - "Metadata": MatchAllFields(Fields{ - "Labels": HaveKeyWithValue(korifiv1alpha1.RelServiceBrokerLabel, broker.Name), - "Annotations": HaveKeyWithValue("annotation", "annotation-value"), + MatchFields(IgnoreExtras, Fields{ + "CFResource": MatchFields(IgnoreExtras, Fields{ + "GUID": Equal(anotherOfferingGUID), }), }), - "ServiceBrokerGUID": Equal(broker.Name), - }))) + )) }) When("filtering by name", func() { BeforeEach(func() { - Expect(k8sClient.Create(ctx, &korifiv1alpha1.CFServiceOffering{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: rootNamespace, - Name: uuid.NewString(), - }, - Spec: korifiv1alpha1.CFServiceOfferingSpec{ - ServiceOffering: services.ServiceOffering{ - Name: "my-other-offering", - }, - }, - })).To(Succeed()) - message.Names = []string{"my-offering"} }) @@ -161,24 +173,27 @@ var _ = Describe("ServiceOfferingRepo", func() { When("filtering by broker name", func() { BeforeEach(func() { - Expect(k8sClient.Create(ctx, &korifiv1alpha1.CFServiceOffering{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: rootNamespace, - Name: uuid.NewString(), - Labels: map[string]string{ - korifiv1alpha1.RelServiceBrokerLabel: uuid.NewString(), - }, - }, - })).To(Succeed()) - message.BrokerNames = []string{broker.Spec.Name} }) It("returns the matching offerings", func() { Expect(listErr).NotTo(HaveOccurred()) Expect(listedOfferings).To(ConsistOf(MatchFields(IgnoreExtras, Fields{ - "ServiceOffering": MatchFields(IgnoreExtras, Fields{ - "Name": Equal("my-offering"), + "ServiceBrokerGUID": Equal(broker.Name), + }))) + }) + }) + + When("filtering by guid", func() { + BeforeEach(func() { + message.GUIDs = []string{offeringGUID} + }) + + It("returns the matching offerings", func() { + Expect(listErr).NotTo(HaveOccurred()) + Expect(listedOfferings).To(ConsistOf(MatchFields(IgnoreExtras, Fields{ + "CFResource": MatchFields(IgnoreExtras, Fields{ + "GUID": Equal(offeringGUID), }), }))) })