repo: refactor into RepoProvider interface

vdye · vdye · commit cadf8b016bea · 2023-02-27T17:41:10.000-08:00
Refactor 'repo.go', changing its standalone functions into methods of a
'RepoProvider' interface. This allows for some simplification of helper
struct (e.g. FileSystem) accesses, sets up for future unit testing work by
making it a mockable interface, and propagates the 'TraceLogger' struct to
its functions. In particular, the logger will be used in later patches to
set up region tracing for various functions in the file.

Signed-off-by: Victoria Dye &lt;vdye@github.com&gt;
diff --git a/cmd/git-bundle-server/delete.go b/cmd/git-bundle-server/delete.go
@@ -37,12 +37,14 @@ func (d *deleteCmd) Run(ctx context.Context, args []string) error {
 	route := parser.PositionalString("route", "the route to delete")
 	parser.Parse(ctx, args)
 
-	repo, err := core.CreateRepository(*route)
+	repoProvider := utils.GetDependency[core.RepositoryProvider](ctx, d.container)
+
+	repo, err := repoProvider.CreateRepository(ctx, *route)
 	if err != nil {
 		return d.logger.Error(ctx, err)
 	}
 
-	err = core.RemoveRoute(*route)
+	err = repoProvider.RemoveRoute(ctx, *route)
 	if err != nil {
 		return d.logger.Error(ctx, err)
 	}
diff --git a/cmd/git-bundle-server/init.go b/cmd/git-bundle-server/init.go
@@ -41,7 +41,9 @@ func (i *initCmd) Run(ctx context.Context, args []string) error {
 	route := parser.PositionalString("route", "the route to host the specified repo")
 	parser.Parse(ctx, args)
 
-	repo, err := core.CreateRepository(*route)
+	repoProvider := utils.GetDependency[core.RepositoryProvider](ctx, i.container)
+
+	repo, err := repoProvider.CreateRepository(ctx, *route)
 	if err != nil {
 		return i.logger.Error(ctx, err)
 	}
diff --git a/cmd/git-bundle-server/start.go b/cmd/git-bundle-server/start.go
@@ -37,8 +37,10 @@ func (s *startCmd) Run(ctx context.Context, args []string) error {
 	route := parser.PositionalString("route", "the route for which bundles should be generated")
 	parser.Parse(ctx, args)
 
+	repoProvider := utils.GetDependency[core.RepositoryProvider](ctx, s.container)
+
 	// CreateRepository registers the route.
-	repo, err := core.CreateRepository(*route)
+	repo, err := repoProvider.CreateRepository(ctx, *route)
 	if err != nil {
 		return s.logger.Error(ctx, err)
 	}
diff --git a/cmd/git-bundle-server/stop.go b/cmd/git-bundle-server/stop.go
@@ -36,7 +36,9 @@ func (s *stopCmd) Run(ctx context.Context, args []string) error {
 	route := parser.PositionalString("route", "the route for which bundles should stop being generated")
 	parser.Parse(ctx, args)
 
-	err := core.RemoveRoute(*route)
+	repoProvider := utils.GetDependency[core.RepositoryProvider](ctx, s.container)
+
+	err := repoProvider.RemoveRoute(ctx, *route)
 	if err != nil {
 		s.logger.Error(ctx, err)
 	}
diff --git a/cmd/git-bundle-server/update-all.go b/cmd/git-bundle-server/update-all.go
@@ -7,7 +7,6 @@ import (
 
 	"github.com/github/git-bundle-server/cmd/utils"
 	"github.com/github/git-bundle-server/internal/argparse"
-	"github.com/github/git-bundle-server/internal/common"
 	"github.com/github/git-bundle-server/internal/core"
 	"github.com/github/git-bundle-server/internal/log"
 )
@@ -34,23 +33,19 @@ For every configured route, run 'git-bundle-server update <options> <route>'.`
 }
 
 func (u *updateAllCmd) Run(ctx context.Context, args []string) error {
-	user, err := utils.GetDependency[common.UserProvider](ctx, u.container).CurrentUser()
-	if err != nil {
-		return u.logger.Error(ctx, err)
-	}
-	fs := utils.GetDependency[common.FileSystem](ctx, u.container)
-
 	parser := argparse.NewArgParser(u.logger, "git-bundle-server update-all")
 	parser.Parse(ctx, args)
 
-	exe, err := os.Executable()
+	repoProvider := utils.GetDependency[core.RepositoryProvider](ctx, u.container)
+
+	repos, err := repoProvider.GetRepositories(ctx)
 	if err != nil {
-		return u.logger.Errorf(ctx, "failed to get path to execuable: %w", err)
+		return u.logger.Error(ctx, err)
 	}
 
-	repos, err := core.GetRepositories(user, fs)
+	exe, err := os.Executable()
 	if err != nil {
-		return u.logger.Error(ctx, err)
+		return u.logger.Errorf(ctx, "failed to get path to execuable: %w", err)
 	}
 
 	subargs := []string{"update", ""}
diff --git a/cmd/git-bundle-server/update.go b/cmd/git-bundle-server/update.go
@@ -39,7 +39,9 @@ func (u *updateCmd) Run(ctx context.Context, args []string) error {
 	route := parser.PositionalString("route", "the route to update")
 	parser.Parse(ctx, args)
 
-	repo, err := core.CreateRepository(*route)
+	repoProvider := utils.GetDependency[core.RepositoryProvider](ctx, u.container)
+
+	repo, err := repoProvider.CreateRepository(ctx, *route)
 	if err != nil {
 		return u.logger.Error(ctx, err)
 	}
diff --git a/cmd/git-bundle-web-server/bundle-server.go b/cmd/git-bundle-web-server/bundle-server.go
@@ -66,13 +66,7 @@ func (b *bundleWebServer) parseRoute(ctx context.Context, path string) (string,
 func (b *bundleWebServer) serve(w http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
 
-	user, err := common.NewUserProvider().CurrentUser()
-	if err != nil {
-		return
-	}
-	fs := common.NewFileSystem()
 	path := r.URL.Path
-
 	owner, repo, file, err := b.parseRoute(ctx, path)
 	if err != nil {
 		w.WriteHeader(http.StatusNotFound)
@@ -82,7 +76,11 @@ func (b *bundleWebServer) serve(w http.ResponseWriter, r *http.Request) {
 
 	route := owner + "/" + repo
 
-	repos, err := core.GetRepositories(user, fs)
+	userProvider := common.NewUserProvider()
+	fileSystem := common.NewFileSystem()
+	repoProvider := core.NewRepositoryProvider(b.logger, userProvider, fileSystem)
+
+	repos, err := repoProvider.GetRepositories(ctx)
 	if err != nil {
 		w.WriteHeader(http.StatusInternalServerError)
 		fmt.Printf("Failed to load routes\n")
diff --git a/cmd/utils/container-helpers.go b/cmd/utils/container-helpers.go
@@ -4,6 +4,7 @@ import (
 	"context"
 
 	"github.com/github/git-bundle-server/internal/common"
+	"github.com/github/git-bundle-server/internal/core"
 	"github.com/github/git-bundle-server/internal/daemon"
 	"github.com/github/git-bundle-server/internal/log"
 )
@@ -19,6 +20,13 @@ func BuildGitBundleServerContainer(logger log.TraceLogger) *DependencyContainer
 	registerDependency(container, func(ctx context.Context) common.FileSystem {
 		return common.NewFileSystem()
 	})
+	registerDependency(container, func(ctx context.Context) core.RepositoryProvider {
+		return core.NewRepositoryProvider(
+			logger,
+			GetDependency[common.UserProvider](ctx, container),
+			GetDependency[common.FileSystem](ctx, container),
+		)
+	})
 	registerDependency(container, func(ctx context.Context) daemon.DaemonProvider {
 		t, err := daemon.NewDaemonProvider(
 			logger,
diff --git a/internal/core/repo.go b/internal/core/repo.go
@@ -1,12 +1,12 @@
 package core
 
 import (
+	"context"
 	"fmt"
-	"log"
 	"os"
-	"os/user"
 
 	"github.com/github/git-bundle-server/internal/common"
+	"github.com/github/git-bundle-server/internal/log"
 )
 
 type Repository struct {
@@ -15,13 +15,36 @@ type Repository struct {
 	WebDir  string
 }
 
-func CreateRepository(route string) (*Repository, error) {
-	user, err := common.NewUserProvider().CurrentUser()
+type RepositoryProvider interface {
+	CreateRepository(ctx context.Context, route string) (*Repository, error)
+	GetRepositories(ctx context.Context) (map[string]Repository, error)
+	RemoveRoute(ctx context.Context, route string) error
+}
+
+type repoProvider struct {
+	logger     log.TraceLogger
+	user       common.UserProvider
+	fileSystem common.FileSystem
+}
+
+func NewRepositoryProvider(logger log.TraceLogger,
+	u common.UserProvider,
+	fs common.FileSystem,
+) RepositoryProvider {
+	return &repoProvider{
+		logger:     logger,
+		user:       u,
+		fileSystem: fs,
+	}
+}
+
+func (r *repoProvider) CreateRepository(ctx context.Context, route string) (*Repository, error) {
+	user, err := r.user.CurrentUser()
 	if err != nil {
 		return nil, err
 	}
-	fs := common.NewFileSystem()
-	repos, err := GetRepositories(user, fs)
+
+	repos, err := r.GetRepositories(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse routes file: %w", err)
 	}
@@ -36,7 +59,7 @@ func CreateRepository(route string) (*Repository, error) {
 
 	mkdirErr := os.MkdirAll(web, os.ModePerm)
 	if mkdirErr != nil {
-		log.Fatal("failed to create web directory: ", mkdirErr)
+		return nil, fmt.Errorf("failed to create web directory: %w", mkdirErr)
 	}
 
 	repo = Repository{
@@ -47,21 +70,16 @@ func CreateRepository(route string) (*Repository, error) {
 
 	repos[route] = repo
 
-	err = WriteRouteFile(repos)
+	err = r.writeRouteFile(repos)
 	if err != nil {
 		return nil, fmt.Errorf("warning: failed to write route file")
 	}
 
 	return &repo, nil
 }
 
-func RemoveRoute(route string) error {
-	user, err := common.NewUserProvider().CurrentUser()
-	if err != nil {
-		return err
-	}
-	fs := common.NewFileSystem()
-	repos, err := GetRepositories(user, fs)
+func (r *repoProvider) RemoveRoute(ctx context.Context, route string) error {
+	repos, err := r.GetRepositories(ctx)
 	if err != nil {
 		return fmt.Errorf("failed to parse routes file: %w", err)
 	}
@@ -73,11 +91,11 @@ func RemoveRoute(route string) error {
 
 	delete(repos, route)
 
-	return WriteRouteFile(repos)
+	return r.writeRouteFile(repos)
 }
 
-func WriteRouteFile(repos map[string]Repository) error {
-	user, err := common.NewUserProvider().CurrentUser()
+func (r *repoProvider) writeRouteFile(repos map[string]Repository) error {
+	user, err := r.user.CurrentUser()
 	if err != nil {
 		return err
 	}
@@ -93,13 +111,18 @@ func WriteRouteFile(repos map[string]Repository) error {
 	return os.WriteFile(routefile, []byte(contents), 0o600)
 }
 
-func GetRepositories(user *user.User, fs common.FileSystem) (map[string]Repository, error) {
+func (r *repoProvider) GetRepositories(ctx context.Context) (map[string]Repository, error) {
+	user, err := r.user.CurrentUser()
+	if err != nil {
+		return nil, err
+	}
+
 	repos := make(map[string]Repository)
 
 	dir := bundleroot(user)
 	routefile := dir + "/routes"
 
-	lines, err := fs.ReadFileLines(routefile)
+	lines, err := r.fileSystem.ReadFileLines(routefile)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/core/repo_test.go b/internal/core/repo_test.go
@@ -1,6 +1,7 @@
 package core_test
 
 import (
+	"context"
 	"errors"
 	"os/user"
 	"testing"
@@ -83,12 +84,16 @@ var getRepositoriesTests = []struct {
 }
 
 func TestRepos_GetRepositories(t *testing.T) {
+	testLogger := &MockTraceLogger{}
 	testFileSystem := &MockFileSystem{}
 	testUser := &user.User{
 		Uid:      "123",
 		Username: "testuser",
 		HomeDir:  "/my/test/dir",
 	}
+	testUserProvider := &MockUserProvider{}
+	testUserProvider.On("CurrentUser").Return(testUser, nil)
+	repoProvider := core.NewRepositoryProvider(testLogger, testUserProvider, testFileSystem)
 
 	for _, tt := range getRepositoriesTests {
 		t.Run(tt.title, func(t *testing.T) {
@@ -97,7 +102,7 @@ func TestRepos_GetRepositories(t *testing.T) {
 				mock.AnythingOfType("string"),
 			).Return(tt.readFileLines.First, tt.readFileLines.Second).Once()
 
-			actual, err := core.GetRepositories(testUser, testFileSystem)
+			actual, err := repoProvider.GetRepositories(context.Background())
 
 			if tt.expectedErr {
 				assert.NotNil(t, err, "Expected error")
