Split out common add/remove code

This way there will be less duplication among storage implementations.

Just goes to show how it can be difficult to abstract before actual
implementing that abstraction.

Author: code-asher

api/api_test.go

@@ -26,12 +26,12 @@ import (
 
 type fakeStorage struct{}
 
-func (s *fakeStorage) AddExtension(ctx context.Context, vsix []byte) (*storage.Extension, error) {
-	return nil, errors.New("not implemented")
+func (s *fakeStorage) AddExtension(ctx context.Context, manifest *storage.VSIXManifest, vsix []byte) (string, error) {
+	return "", errors.New("not implemented")
 }
 
-func (s *fakeStorage) RemoveExtension(ctx context.Context, id string, all bool) ([]string, error) {
-	return nil, errors.New("not implemented")
+func (s *fakeStorage) RemoveExtension(ctx context.Context, publisher, extension, version string) error {
+	return errors.New("not implemented")
 }
 
 func (s *fakeStorage) FileServer() http.Handler {
@@ -48,6 +48,10 @@ func (s *fakeStorage) Manifest(ctx context.Context, publisher, extension, versio
 	return nil, errors.New("not implemented")
 }
 
+func (s *fakeStorage) Versions(ctx context.Context, publisher, name string) ([]string, error) {
+	return nil, errors.New("not implemented")
+}
+
 func (s *fakeStorage) WalkExtensions(ctx context.Context, fn func(manifest *storage.VSIXManifest, versions []string) error) error {
 	return errors.New("not implemented")
 }

cli/add.go

@@ -52,33 +52,55 @@ func add() *cobra.Command {
 				return err
 			}
 
+			// The manifest is required to know where to place the extension since it
+			// is unsafe to rely on the file name or URI.
+			manifest, err := storage.ReadVSIXManifest(vsix)
+			if err != nil {
+				return err
+			}
+
 			// Always local storage for now.
-			store := storage.NewLocalStorage(ctx, extdir, logger)
-			ext, err := store.AddExtension(ctx, vsix)
+			store := storage.NewLocalStorage(extdir, logger)
+			location, err := store.AddExtension(ctx, manifest, vsix)
 			if err != nil {
 				return err
 			}
 
-			depCount := len(ext.Dependencies)
+			deps := []string{}
+			pack := []string{}
+			for _, prop := range manifest.Metadata.Properties.Property {
+				if prop.Value == "" {
+					continue
+				}
+				switch prop.ID {
+				case storage.DependencyPropertyType:
+					deps = append(deps, strings.Split(prop.Value, ",")...)
+				case storage.PackPropertyType:
+					pack = append(pack, strings.Split(prop.Value, ",")...)
+				}
+			}
+
+			depCount := len(deps)
+			id := storage.ExtensionID(manifest)
 			summary := []string{
-				fmt.Sprintf("Unpacked %s to %s", ext.ID, ext.Location),
-				fmt.Sprintf("%s has %s", ext.ID, util.Plural(depCount, "dependency", "dependencies")),
+				fmt.Sprintf("Unpacked %s to %s", id, location),
+				fmt.Sprintf("%s has %s", id, util.Plural(depCount, "dependency", "dependencies")),
 			}
 
 			if depCount > 0 {
-				for _, id := range ext.Dependencies {
+				for _, id := range deps {
 					summary = append(summary, fmt.Sprintf("  - %s", id))
 				}
 			}
 
-			packCount := len(ext.Pack)
+			packCount := len(pack)
 			if packCount > 0 {
-				summary = append(summary, fmt.Sprintf("%s is in a pack with %s", ext.ID, util.Plural(packCount, "other extension", "")))
-				for _, id := range ext.Pack {
+				summary = append(summary, fmt.Sprintf("%s is in a pack with %s", id, util.Plural(packCount, "other extension", "")))
+				for _, id := range pack {
 					summary = append(summary, fmt.Sprintf("  - %s", id))
 				}
 			} else {
-				summary = append(summary, fmt.Sprintf("%s is not in a pack", ext.ID))
+				summary = append(summary, fmt.Sprintf("%s is not in a pack", id))
 			}
 
 			_, err = fmt.Fprintln(cmd.OutOrStdout(), strings.Join(summary, "\n"))

cli/remove.go

@@ -2,16 +2,20 @@ package cli
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"os"
 	"path/filepath"
 	"strings"
 
 	"github.com/spf13/cobra"
+	"golang.org/x/xerrors"
 
 	"cdr.dev/slog"
 	"cdr.dev/slog/sloggers/sloghuman"
 
 	"github.com/coder/code-marketplace/storage"
+	"github.com/coder/code-marketplace/util"
 )
 
 func remove() *cobra.Command {
@@ -46,23 +50,51 @@ func remove() *cobra.Command {
 				return err
 			}
 
-			// Always local storage for now.
-			store := storage.NewLocalStorage(ctx, extdir, logger)
-			removed, err := store.RemoveExtension(ctx, args[0], all)
+			id := args[0]
+			publisher, name, version, err := storage.ParseExtensionID(id)
 			if err != nil {
 				return err
 			}
 
-			removedCount := len(removed)
-			pluralVersions := "versions"
-			if removedCount == 1 {
-				pluralVersions = "version"
+			if version != "" && all {
+				return xerrors.Errorf("cannot specify both --all and version %s", version)
+			}
+
+			// Always local storage for now.
+			store := storage.NewLocalStorage(extdir, logger)
+
+			allVersions, err := store.Versions(ctx, publisher, name)
+			if err != nil && !errors.Is(err, os.ErrNotExist) {
+				return err
+			}
+
+			versionCount := len(allVersions)
+			if !all && version != "" && !contains(allVersions, version) {
+				return xerrors.Errorf("%s does not exist", id)
+			} else if versionCount == 0 {
+				return xerrors.Errorf("%s.%s has no versions to delete", publisher, name)
+			} else if version == "" && !all {
+				return xerrors.Errorf(
+					"use %s-<version> to target a specific version or pass --all to delete %s of %s",
+					id,
+					util.Plural(versionCount, "version", ""),
+					id,
+				)
 			}
-			summary := []string{
-				fmt.Sprintf("Removed %d %s", removedCount, pluralVersions),
+			err = store.RemoveExtension(ctx, publisher, name, version)
+			if err != nil {
+				return err
 			}
-			for _, id := range removed {
-				summary = append(summary, fmt.Sprintf("  - %s", id))
+
+			summary := []string{}
+			if all {
+				removedCount := len(allVersions)
+				summary = append(summary, fmt.Sprintf("Removed %s", util.Plural(removedCount, "version", "")))
+				for _, version := range allVersions {
+					summary = append(summary, fmt.Sprintf("  - %s", version))
+				}
+			} else {
+				summary = append(summary, fmt.Sprintf("Removed %s", version))
 			}
 
 			_, err = fmt.Fprintln(cmd.OutOrStdout(), strings.Join(summary, "\n"))
@@ -76,3 +108,12 @@ func remove() *cobra.Command {
 
 	return cmd
 }
+
+func contains(a []string, b string) bool {
+	for _, astr := range a {
+		if astr == b {
+			return true
+		}
+	}
+	return false
+}

cli/remove_test.go

@@ -2,14 +2,16 @@ package cli_test
 
 import (
 	"bytes"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/code-marketplace/cli"
+	"github.com/coder/code-marketplace/testutil"
 )
 
-func TestRemove(t *testing.T) {
+func TestRemoveHelp(t *testing.T) {
 	t.Parallel()
 
 	cmd := cli.Root()
@@ -23,3 +25,112 @@ func TestRemove(t *testing.T) {
 	output := buf.String()
 	require.Contains(t, output, "Remove an extension", "has help")
 }
+
+func TestRemove(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		// all means to pass --all.
+		all bool
+		// error is the expected error.
+		error string
+		// extension is the extension to remove.  testutil.Extensions[0] will be
+		// added with versions a, b, and c before each test.
+		extension testutil.Extension
+		// name is the name of the test.
+		name string
+		// version is the version to remove.
+		version string
+	}{
+		{
+			name:      "RemoveOne",
+			extension: testutil.Extensions[0],
+			version:   "a",
+		},
+		{
+			name:      "All",
+			extension: testutil.Extensions[0],
+			all:       true,
+		},
+		{
+			name:      "MissingTarget",
+			error:     "target a specific version or pass --all",
+			extension: testutil.Extensions[0],
+		},
+		{
+			name:      "MissingTargetNoVersions",
+			error:     "has no versions",
+			extension: testutil.Extensions[1],
+		},
+		{
+			name:      "AllWithVersion",
+			error:     "cannot specify both",
+			extension: testutil.Extensions[0],
+			all:       true,
+			version:   "a",
+		},
+		{
+			name:      "NoVersion",
+			error:     "does not exist",
+			extension: testutil.Extensions[0],
+			version:   "d",
+		},
+		{
+			name:      "NoVersions",
+			error:     "does not exist",
+			extension: testutil.Extensions[1],
+			version:   "a",
+		},
+		{
+			name:      "AllNoVersions",
+			error:     "has no versions",
+			extension: testutil.Extensions[1],
+			all:       true,
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			extdir := t.TempDir()
+			ext := testutil.Extensions[0]
+			testutil.AddExtension(t, ext, extdir, "a")
+			testutil.AddExtension(t, ext, extdir, "b")
+			testutil.AddExtension(t, ext, extdir, "c")
+
+			id := fmt.Sprintf("%s.%s", test.extension.Publisher, test.extension.Name)
+			if test.version != "" {
+				id = fmt.Sprintf("%s-%s", id, test.version)
+			}
+
+			cmd := cli.Root()
+			args := []string{"remove", id, "--extensions-dir", extdir}
+			if test.all {
+				args = append(args, "--all")
+			}
+			cmd.SetArgs(args)
+			buf := new(bytes.Buffer)
+			cmd.SetOutput(buf)
+
+			err := cmd.Execute()
+			output := buf.String()
+
+			if test.error != "" {
+				require.Error(t, err)
+				require.Regexp(t, test.error, err.Error())
+			} else {
+				require.NoError(t, err)
+				if test.all {
+					require.Contains(t, output, "Removed 3 versions")
+					require.Contains(t, output, "  - a")
+					require.Contains(t, output, "  - b")
+					require.Contains(t, output, "  - c")
+				} else {
+					require.Contains(t, output, fmt.Sprintf("Removed %s", test.version))
+				}
+			}
+		})
+	}
+}

cli/server.go

@@ -69,7 +69,7 @@ func server() *cobra.Command {
 			}
 
 			// Always local storage for now.
-			store := storage.NewLocalStorage(ctx, extdir, logger)
+			store := storage.NewLocalStorage(extdir, logger)
 
 			// Always no database for now.
 			database := &database.NoDB{

database/database_test.go

@@ -20,12 +20,12 @@ import (
 
 type memoryStorage struct{}
 
-func (s *memoryStorage) AddExtension(ctx context.Context, vsix []byte) (*storage.Extension, error) {
-	return nil, errors.New("not implemented")
+func (s *memoryStorage) AddExtension(ctx context.Context, manifest *storage.VSIXManifest, vsix []byte) (string, error) {
+	return "", errors.New("not implemented")
 }
 
-func (s *memoryStorage) RemoveExtension(ctx context.Context, id string, all bool) ([]string, error) {
-	return nil, errors.New("not implemented")
+func (s *memoryStorage) RemoveExtension(ctx context.Context, publisher, extension, version string) error {
+	return errors.New("not implemented")
 }
 
 func (s *memoryStorage) FileServer() http.Handler {
@@ -34,6 +34,10 @@ func (s *memoryStorage) FileServer() http.Handler {
 	})
 }
 
+func (s *memoryStorage) Versions(ctx context.Context, publisher, name string) ([]string, error) {
+	return nil, errors.New("not implemented")
+}
+
 func (s *memoryStorage) Manifest(ctx context.Context, publisher, extension, version string) (*storage.VSIXManifest, error) {
 	for _, ext := range testutil.Extensions {
 		if ext.Publisher == publisher && ext.Name == extension {