fix: delete app also deletes metadata

pkg/admin/client.go

@@ -5,11 +5,13 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"github.com/pyroscope-io/pyroscope/pkg/server"
 	"io"
 	"net/http"
 	"time"
 
+	"github.com/pyroscope-io/pyroscope/pkg/api"
+	"github.com/pyroscope-io/pyroscope/pkg/model/appmetadata"
+
 	"github.com/hashicorp/go-multierror"
 )
 
@@ -57,20 +59,23 @@ func (c *Client) GetAppsNames() (names AppNames, err error) {
 		return names, multierror.Append(ErrStatusCodeNotOK, err)
 	}
 
-	err = json.NewDecoder(resp.Body).Decode(&names)
+	// Strip all data except fqname
+	var apps []appmetadata.ApplicationMetadata
+	err = json.NewDecoder(resp.Body).Decode(&apps)
 	if err != nil {
 		return names, multierror.Append(ErrDecodingResponse, err)
 	}
 
-	return names, nil
+	appNames := make([]string, len(apps))
+	for i, appName := range apps {
+		appNames[i] = appName.FQName
+	}
+
+	return appNames, nil
 }
 
 func (c *Client) DeleteApp(name string) (err error) {
-	// we are kinda robbing here
-	// since the server and client are defined in the same package
-	payload := server.DeleteAppInput{
-		Name: name,
-	}
+	payload := api.DeleteAppInput{Name: name}
 
 	marshalledPayload, err := json.Marshal(payload)
 	if err != nil {

pkg/admin/client_test.go

@@ -60,7 +60,7 @@ var _ = Describe("client", func() {
 				handler = http.NewServeMux()
 				handler.HandleFunc("/v1/apps", func(w http.ResponseWriter, r *http.Request) {
 					w.WriteHeader(200)
-					_, _ = fmt.Fprintf(w, `["name1", "name2"]`)
+					_, _ = fmt.Fprintf(w, `[{ "name": "name1"}, {"name": "name2" }]`)
 				})
 			})
 

pkg/admin/controller.go

@@ -6,26 +6,21 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"github.com/pyroscope-io/pyroscope/pkg/server/httputils"
-	pstorage "github.com/pyroscope-io/pyroscope/pkg/storage"
 	"net/http"
 
+	"github.com/pyroscope-io/pyroscope/pkg/model/appmetadata"
+	"github.com/pyroscope-io/pyroscope/pkg/server/httputils"
+
 	"github.com/gorilla/mux"
 	"github.com/sirupsen/logrus"
 
 	"github.com/pyroscope-io/pyroscope/pkg/model"
 )
 
-type Storage interface {
-	pstorage.AppNameGetter
-	pstorage.AppGetter
-	pstorage.AppDeleter
-}
-
 type Controller struct {
 	log            *logrus.Logger
 	httpUtils      httputils.Utils
-	storage        Storage
+	appService     ApplicationListerAndDeleter
 	userService    UserService
 	storageService StorageService
 }
@@ -38,15 +33,20 @@ type StorageService interface {
 	Cleanup(ctx context.Context) error
 }
 
+type ApplicationListerAndDeleter interface {
+	List(ctx context.Context) (apps []appmetadata.ApplicationMetadata, err error)
+	Delete(ctx context.Context, name string) error
+}
+
 func NewController(
 	log *logrus.Logger,
-	storage Storage,
+	appService ApplicationListerAndDeleter,
 	userService UserService,
 	storageService StorageService) *Controller {
 	return &Controller{
 		log: log,
 
-		storage:        storage,
+		appService:     appService,
 		userService:    userService,
 		storageService: storageService,
 	}

pkg/admin/controller_test.go

@@ -1,16 +1,18 @@
+// TODO: move most of these tests to pkg/api
 package admin_test
 
 import (
 	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
-	"github.com/pyroscope-io/pyroscope/pkg/server"
-	"github.com/pyroscope-io/pyroscope/pkg/storage"
 	"io"
 	"net/http"
 	"net/http/httptest"
 
+	"github.com/pyroscope-io/pyroscope/pkg/api"
+	"github.com/pyroscope-io/pyroscope/pkg/model/appmetadata"
+
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/sirupsen/logrus/hooks/test"
@@ -20,20 +22,15 @@ import (
 )
 
 type mockStorage struct {
-	getAppNamesResult []string
-	getAppsResult     storage.GetAppsOutput
-	deleteResult      error
-}
-
-func (m mockStorage) GetAppNames(ctx context.Context) []string {
-	return m.getAppNamesResult
+	getAppsResult []appmetadata.ApplicationMetadata
+	deleteResult  error
 }
 
-func (m mockStorage) GetApps(ctx context.Context) (storage.GetAppsOutput, error) {
+func (m mockStorage) List(ctx context.Context) ([]appmetadata.ApplicationMetadata, error) {
 	return m.getAppsResult, nil
 }
 
-func (m mockStorage) DeleteApp(ctx context.Context, appname string) error {
+func (m mockStorage) Delete(ctx context.Context, appname string) error {
 	return m.deleteResult
 }
 
@@ -53,21 +50,24 @@ var _ = Describe("controller", func() {
 	Describe("/v1/apps", func() {
 		var svr *admin.Server
 		var response *httptest.ResponseRecorder
-		var storage admin.Storage
+		var appSvc admin.ApplicationListerAndDeleter
 
 		// create a server
 		BeforeEach(func() {
-			storage = mockStorage{
-				getAppNamesResult: []string{"app1", "app2"},
-				deleteResult:      nil,
+			appSvc = mockStorage{
+				getAppsResult: []appmetadata.ApplicationMetadata{
+					{FQName: "app1"},
+					{FQName: "app2"},
+				},
+				deleteResult: nil,
 			}
 		})
 
 		JustBeforeEach(func() {
 			// create a null logger, since we aren't interested
 			logger, _ := test.NewNullLogger()
 
-			ctrl := admin.NewController(logger, storage, mockUserService{}, mockStorageService{})
+			ctrl := admin.NewController(logger, appSvc, mockUserService{}, mockStorageService{})
 			httpServer := &admin.UdsHTTPServer{}
 			server, err := admin.NewServer(logger, ctrl, httpServer)
 
@@ -88,7 +88,7 @@ var _ = Describe("controller", func() {
 					Expect(err).To(BeNil())
 
 					Expect(response.Code).To(Equal(http.StatusOK))
-					Expect(string(body)).To(Equal(`["app1","app2"]
+					Expect(string(body)).To(Equal(`[{"name":"app1"},{"name":"app2"}]
 `))
 				})
 
@@ -100,7 +100,7 @@ var _ = Describe("controller", func() {
 				It("returns StatusOK", func() {
 					// we are kinda robbing here
 					// since the server and client are defined in the same package
-					payload := server.DeleteAppInput{Name: "myapp"}
+					payload := api.DeleteAppInput{Name: "myapp"}
 					marshalledPayload, err := json.Marshal(payload)
 					request, err := http.NewRequest(http.MethodDelete, "/v1/apps", bytes.NewBuffer(marshalledPayload))
 					Expect(err).ToNot(HaveOccurred())
@@ -112,6 +112,12 @@ var _ = Describe("controller", func() {
 
 			Context("when there's an error", func() {
 				Context("with the payload", func() {
+					BeforeEach(func() {
+						appSvc = mockStorage{
+							deleteResult: model.ValidationError{},
+						}
+					})
+
 					It("returns BadRequest", func() {
 						request, err := http.NewRequest(http.MethodDelete, "/v1/apps", bytes.NewBuffer([]byte(``)))
 						Expect(err).ToNot(HaveOccurred())
@@ -123,15 +129,15 @@ var _ = Describe("controller", func() {
 
 				Context("with the server", func() {
 					BeforeEach(func() {
-						storage = mockStorage{
+						appSvc = mockStorage{
 							deleteResult: fmt.Errorf("error"),
 						}
 					})
 
 					It("returns InternalServerError", func() {
 						// we are kinda robbing here
 						// since the server and client are defined in the same package
-						payload := server.DeleteAppInput{Name: "myapp"}
+						payload := api.DeleteAppInput{Name: "myapp"}
 						marshalledPayload, err := json.Marshal(payload)
 						request, err := http.NewRequest(http.MethodDelete, "/v1/apps", bytes.NewBuffer(marshalledPayload))
 						Expect(err).ToNot(HaveOccurred())

pkg/admin/server.go

@@ -1,11 +1,12 @@
 package admin
 
 import (
-	"github.com/pyroscope-io/pyroscope/pkg/server"
-	"github.com/pyroscope-io/pyroscope/pkg/server/httputils"
 	"net/http"
 	"os"
 
+	"github.com/pyroscope-io/pyroscope/pkg/api"
+	"github.com/pyroscope-io/pyroscope/pkg/server/httputils"
+
 	"github.com/gorilla/handlers"
 	"github.com/gorilla/mux"
 	"github.com/sirupsen/logrus"
@@ -42,8 +43,10 @@ func NewServer(logger *logrus.Logger, ctrl *Controller, httpServer HTTPServer) (
 	httpUtils := httputils.NewDefaultHelper(logger)
 	// Routes
 
-	r.HandleFunc("/v1/apps", server.NewGetAppNamesHandler(ctrl.storage, httpUtils)).Methods("GET")
-	r.HandleFunc("/v1/apps", server.NewDeleteAppHandler(ctrl.storage, httpUtils)).Methods("DELETE")
+	applicationsHandler := api.NewApplicationsHandler(ctrl.appService, httpUtils)
+	r.HandleFunc("/v1/apps", applicationsHandler.GetApps).Methods("GET")
+	r.HandleFunc("/v1/apps", applicationsHandler.DeleteApp).Methods("DELETE")
+
 	r.HandleFunc("/v1/users/{username}", ctrl.UpdateUserHandler).Methods("PATCH")
 	r.HandleFunc("/v1/storage/cleanup", ctrl.StorageCleanupHandler).Methods("PUT")
 

pkg/api/applications.go

@@ -0,0 +1,60 @@
+package api
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"net/http"
+
+	"github.com/pyroscope-io/pyroscope/pkg/model/appmetadata"
+	"github.com/pyroscope-io/pyroscope/pkg/server/httputils"
+)
+
+type ApplicationListerAndDeleter interface {
+	List(context.Context) ([]appmetadata.ApplicationMetadata, error)
+	Delete(ctx context.Context, name string) error
+}
+
+type ApplicationsHandler struct {
+	svc       ApplicationListerAndDeleter
+	httpUtils httputils.Utils
+}
+
+func NewApplicationsHandler(svc ApplicationListerAndDeleter, httpUtils httputils.Utils) *ApplicationsHandler {
+	return &ApplicationsHandler{
+		svc:       svc,
+		httpUtils: httpUtils,
+	}
+}
+
+func (h *ApplicationsHandler) GetApps(w http.ResponseWriter, r *http.Request) {
+	apps, err := h.svc.List(r.Context())
+	if err != nil {
+		h.httpUtils.HandleError(r, w, err)
+		return
+	}
+
+	w.WriteHeader(http.StatusOK)
+	h.httpUtils.WriteResponseJSON(r, w, apps)
+}
+
+type DeleteAppInput struct {
+	Name string `json:"name"`
+}
+
+func (h *ApplicationsHandler) DeleteApp(w http.ResponseWriter, r *http.Request) {
+	var payload DeleteAppInput
+
+	fmt.Println("calling delete app")
+	err := json.NewDecoder(r.Body).Decode(&payload)
+	if err != nil {
+		h.httpUtils.HandleError(r, w, httputils.JSONError{Err: err})
+		return
+	}
+
+	err = h.svc.Delete(r.Context(), payload.Name)
+	if err != nil {
+		h.httpUtils.HandleError(r, w, err)
+		return
+	}
+}

pkg/api/router/router.go

@@ -24,6 +24,7 @@ type Services struct {
 	api.APIKeyService
 	api.AnnotationsService
 	api.AdhocService
+	api.ApplicationListerAndDeleter
 }
 
 func New(m *mux.Router, s Services) *Router {
@@ -94,3 +95,12 @@ func (r *Router) RegisterAdhocHandlers() {
 	x.Methods(http.MethodGet).PathPrefix("/diff/{left:[0-9a-f]+}/{right:[0-9a-f]+}").HandlerFunc(h.GetProfileDiff)
 	x.Methods(http.MethodPost).PathPrefix("/upload").HandlerFunc(h.Upload)
 }
+
+func (r *Router) RegisterApplicationHandlers() {
+	h := api.NewApplicationsHandler(r.ApplicationListerAndDeleter, httputils.NewDefaultHelper(r.Logger))
+	authorizer := authz.NewAuthorizer(r.Services.Logger, httputils.NewDefaultHelper(r.Logger))
+
+	x := r.PathPrefix("/apps").Subrouter()
+	x.Methods(http.MethodGet).HandlerFunc(h.GetApps)
+	x.Methods(http.MethodDelete).Handler(authorizer.RequireAdminRole(http.HandlerFunc(h.DeleteApp)))
+}

pkg/cli/app_metadata_migration.go

@@ -4,7 +4,7 @@ import (
 	"context"
 
 	"github.com/hashicorp/go-multierror"
-	"github.com/pyroscope-io/pyroscope/pkg/storage"
+	"github.com/pyroscope-io/pyroscope/pkg/model/appmetadata"
 	"github.com/sirupsen/logrus"
 )
 
@@ -13,8 +13,8 @@ type AppNamesGetter interface {
 }
 
 type AppMetadataSaver interface {
-	CreateOrUpdate(_ context.Context, _ storage.ApplicationMetadata) error
-	List(context.Context) ([]storage.ApplicationMetadata, error)
+	CreateOrUpdate(_ context.Context, _ appmetadata.ApplicationMetadata) error
+	List(context.Context) ([]appmetadata.ApplicationMetadata, error)
 }
 
 type AppMetadataMigrator struct {
@@ -45,7 +45,7 @@ func (m *AppMetadataMigrator) Migrate() (err error) {
 	// TODO skip if not necessary
 
 	// Convert slice -> map
-	appMap := make(map[string]storage.ApplicationMetadata)
+	appMap := make(map[string]appmetadata.ApplicationMetadata)
 	for _, a := range apps {
 		appMap[a.FQName] = a
 	}
@@ -55,7 +55,7 @@ func (m *AppMetadataMigrator) Migrate() (err error) {
 		if _, ok := appMap[a]; !ok {
 			logrus.Info("Migrating app: ", a)
 			// Write to MetadataSaver
-			saveErr := m.appMetadataSaver.CreateOrUpdate(ctx, storage.ApplicationMetadata{
+			saveErr := m.appMetadataSaver.CreateOrUpdate(ctx, appmetadata.ApplicationMetadata{
 				FQName: a,
 			})
 			if err != nil {