Apply changes from Code Review

api/handlers/app.go

@@ -218,19 +218,22 @@ func (h *App) setCurrentDroplet(r *http.Request) (*routing.Response, error) {
 	return routing.NewResponse(http.StatusOK).WithBody(presenter.ForCurrentDroplet(currentDroplet, h.serverURL)), nil
 }
 
-func (h *App) getDroplets(r *http.Request) (*routing.Response, error) {
+func (h *App) listDroplets(r *http.Request) (*routing.Response, error) {
 	authInfo, _ := authorization.InfoFromContext(r.Context())
 	logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.app.get-droplets")
 	appGUID := routing.URLParam(r, "guid")
 
+	appListDroplets := new(payloads.AppListDroplets)
+
 	app, err := h.appRepo.GetApp(r.Context(), authInfo, appGUID)
 	if err != nil {
 		return nil, apierrors.LogAndReturn(logger, apierrors.ForbiddenAsNotFound(err), "Failed to fetch app from Kubernetes", "AppGUID", appGUID)
 	}
 
-	droplets, err := h.dropletRepo.GetDroplets(r.Context(), authInfo, appGUID)
+	dropletListMessage := appListDroplets.ToMessage([]string{appGUID})
+	droplets, err := h.dropletRepo.ListDroplets(r.Context(), authInfo, dropletListMessage)
 	if err != nil {
-		return nil, apierrors.LogAndReturn(logger, apierrors.DropletForbiddenAsNotFound(err), "Failed to fetch droplet from Kubernetes", "dropletGUID", app.DropletGUID)
+		return nil, apierrors.LogAndReturn(logger, apierrors.ForbiddenAsNotFound(err), "Failed to fetch droplet from Kubernetes", "dropletGUID", app.DropletGUID)
 	}
 
 	return routing.NewResponse(http.StatusOK).WithBody(presenter.ForList(presenter.ForDroplet, droplets, h.serverURL, *r.URL)), nil
@@ -726,7 +729,7 @@ func (h *App) AuthenticatedRoutes() []routing.Route {
 		{Method: "GET", Pattern: AppsPath, Handler: h.list},
 		{Method: "POST", Pattern: AppsPath, Handler: h.create},
 		{Method: "PATCH", Pattern: AppCurrentDropletRelationshipPath, Handler: h.setCurrentDroplet},
-		{Method: "GET", Pattern: AppDropletsPath, Handler: h.getDroplets},
+		{Method: "GET", Pattern: AppDropletsPath, Handler: h.listDroplets},
 		{Method: "GET", Pattern: AppCurrentDropletPath, Handler: h.getCurrentDroplet},
 		{Method: "POST", Pattern: AppStartPath, Handler: h.start},
 		{Method: "POST", Pattern: AppStopPath, Handler: h.stop},

api/handlers/app_test.go

@@ -1308,7 +1308,7 @@ var _ = Describe("App", func() {
 
 	Describe("GET /v3/apps/:guid/droplets", func() {
 		BeforeEach(func() {
-			dropletRepo.GetDropletsReturns([]repositories.DropletRecord{{
+			dropletRepo.ListDropletsReturns([]repositories.DropletRecord{{
 				GUID:    dropletGUID,
 				State:   "STAGED",
 				AppGUID: appGUID,
@@ -1318,10 +1318,13 @@ var _ = Describe("App", func() {
 		})
 
 		It("returns the list of droplets", func() {
-			Expect(dropletRepo.GetDropletsCallCount()).To(Equal(1))
-			_, actualAuthInfo, actualAppGUID := dropletRepo.GetDropletsArgsForCall(0)
+			Expect(dropletRepo.ListDropletsCallCount()).To(Equal(1))
+			_, actualAuthInfo, dropletListMessage := dropletRepo.ListDropletsArgsForCall(0)
 			Expect(actualAuthInfo).To(Equal(authInfo))
-			Expect(actualAppGUID).To(Equal(appGUID))
+
+			Expect(dropletListMessage).To(Equal(repositories.ListDropletsMessage{
+				AppGUIDs: []string{appGUID},
+			}))
 
 			Expect(rr).To(HaveHTTPStatus(http.StatusOK))
 			Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json"))
@@ -1337,8 +1340,10 @@ var _ = Describe("App", func() {
 
 		When("the app is not accessible", func() {
 			BeforeEach(func() {
-				toReturnErr := apierrors.NewForbiddenError(nil, repositories.AppResourceType)
-				appRepo.GetAppReturns(repositories.AppRecord{}, toReturnErr)
+				appRepo.GetAppReturns(
+					repositories.AppRecord{},
+					apierrors.NewForbiddenError(nil, repositories.AppResourceType),
+				)
 			})
 
 			It("returns an error", func() {

api/handlers/droplet.go

@@ -23,7 +23,6 @@ const (
 //counterfeiter:generate -o fake -fake-name CFDropletRepository . CFDropletRepository
 type CFDropletRepository interface {
 	GetDroplet(context.Context, authorization.Info, string) (repositories.DropletRecord, error)
-	GetDroplets(context.Context, authorization.Info, string) ([]repositories.DropletRecord, error)
 	ListDroplets(context.Context, authorization.Info, repositories.ListDropletsMessage) ([]repositories.DropletRecord, error)
 	UpdateDroplet(context.Context, authorization.Info, repositories.UpdateDropletMessage) (repositories.DropletRecord, error)
 }

api/payloads/app.go

@@ -204,3 +204,11 @@ func (a *AppPatch) ToMessage(appGUID, spaceGUID string) repositories.PatchAppMes
 
 	return msg
 }
+
+type AppListDroplets struct{}
+
+func (a *AppListDroplets) ToMessage(appGUIDs []string) repositories.ListDropletsMessage {
+	return repositories.ListDropletsMessage{
+		AppGUIDs: appGUIDs,
+	}
+}

api/repositories/droplet_repository.go

@@ -70,6 +70,7 @@ func (r DropletRecord) Relationships() map[string]string {
 
 type ListDropletsMessage struct {
 	PackageGUIDs []string
+	AppGUIDs     []string
 }
 
 func (m *ListDropletsMessage) matches(b korifiv1alpha1.CFBuild) bool {
@@ -87,40 +88,6 @@ func (r *DropletRepo) GetDroplet(ctx context.Context, authInfo authorization.Inf
 	return cfBuildToDroplet(build)
 }
 
-func (r *DropletRepo) GetDroplets(ctx context.Context, authInfo authorization.Info, appGuid string) ([]DropletRecord, error) {
-	buildList := &korifiv1alpha1.CFBuildList{}
-
-	namespaces, err := r.namespacePermissions.GetAuthorizedSpaceNamespaces(ctx, authInfo)
-	if err != nil {
-		return nil, fmt.Errorf("failed to list namespaces for spaces with user role bindings: %w", err)
-	}
-
-	userClient, err := r.userClientFactory.BuildClient(authInfo)
-	if err != nil {
-		return []DropletRecord{}, fmt.Errorf("failed to build user client: %w", err)
-	}
-
-	var allBuilds []korifiv1alpha1.CFBuild
-	for ns := range namespaces {
-		err := userClient.List(ctx, buildList, client.InNamespace(ns))
-		if k8serrors.IsForbidden(err) {
-			continue
-		}
-		if err != nil {
-			return []DropletRecord{}, apierrors.FromK8sError(err, BuildResourceType)
-		}
-		allBuilds = append(allBuilds, buildList.Items...)
-	}
-
-	var filteredDropletRecords []DropletRecord
-	for _, build := range allBuilds {
-		if build.Spec.AppRef.Name == appGuid {
-			filteredDropletRecords = append(filteredDropletRecords, cfBuildToDropletRecord(build))
-		}
-	}
-	return filteredDropletRecords, nil
-}
-
 func (r *DropletRepo) getBuildAssociatedWithDroplet(ctx context.Context, authInfo authorization.Info, dropletGUID string) (*korifiv1alpha1.CFBuild, client.WithWatch, error) {
 	// A droplet is a subset of a build
 	ns, err := r.namespaceRetriever.NamespaceFor(ctx, dropletGUID, DropletResourceType)

api/repositories/droplet_repository_test.go

@@ -313,88 +313,6 @@ var _ = Describe("DropletRepository", func() {
 		})
 	})
 
-	Describe("GetDroplets", func() {
-		var (
-			dropletRecords []repositories.DropletRecord
-			//			appGUID        string
-			listErr error
-		)
-
-		BeforeEach(func() {
-			meta.SetStatusCondition(&build.Status.Conditions, metav1.Condition{
-				Type:    "Staging",
-				Status:  metav1.ConditionFalse,
-				Reason:  "kpack",
-				Message: "kpack",
-			})
-			meta.SetStatusCondition(&build.Status.Conditions, metav1.Condition{
-				Type:    "Succeeded",
-				Status:  metav1.ConditionTrue,
-				Reason:  "Unknown",
-				Message: "Unknown",
-			})
-			build.Status.Droplet = &korifiv1alpha1.BuildDropletStatus{
-				Stack: dropletStack,
-				Registry: korifiv1alpha1.Registry{
-					Image: registryImage,
-					ImagePullSecrets: []corev1.LocalObjectReference{
-						{
-							Name: registryImageSecret,
-						},
-					},
-				},
-				ProcessTypes: []korifiv1alpha1.ProcessType{
-					{
-						Type:    "rake",
-						Command: "bundle exec rake",
-					},
-					{
-						Type:    "web",
-						Command: "bundle exec rackup config.ru -p $PORT",
-					},
-				},
-				Ports: []int32{8080, 443},
-			}
-			// Update Build Status based on changes made to local copy
-			Expect(k8sClient.Status().Update(testCtx, build)).To(Succeed())
-		})
-
-		JustBeforeEach(func() {
-			dropletRecords, listErr = dropletRepo.GetDroplets(testCtx, authInfo, appGUID)
-		})
-
-		When("the user is not authorized to get the droplets", func() {
-			It("returns an empty list to users who lack access", func() {
-				Expect(listErr).NotTo(HaveOccurred())
-				Expect(dropletRecords).To(BeEmpty())
-			})
-		})
-
-		When("the user is a space manager", func() {
-			BeforeEach(func() {
-				createRoleBinding(testCtx, userName, spaceDeveloperRole.Name, space.Name)
-			})
-
-			It("returns a list of droplet records with the appGUID relationship set on them", func() {
-				Expect(listErr).NotTo(HaveOccurred())
-				Expect(dropletRecords).To(HaveLen(1))
-				Expect(dropletRecords[0].Relationships()).To(Equal(map[string]string{"app": build.Spec.AppRef.Name}))
-			})
-
-			When("a space exists with a rolebinding for the user, but without permission to list droplets", func() {
-				BeforeEach(func() {
-					anotherSpace := createSpaceWithCleanup(testCtx, org.Name, "space-without-droplet-space-perm")
-					createRoleBinding(testCtx, userName, rootNamespaceUserRole.Name, anotherSpace.Name)
-				})
-
-				It("returns the droplet", func() {
-					Expect(listErr).NotTo(HaveOccurred())
-					Expect(dropletRecords).To(HaveLen(1))
-				})
-			})
-		})
-	})
-
 	Describe("ListDroplets", func() {
 		var (
 			dropletRecords []repositories.DropletRecord