Minor fixes and refactoring

internal/api/controller/space/delete.go

@@ -36,7 +36,7 @@ func (c *Controller) Delete(ctx context.Context, session *auth.Session, spaceRef
 func (c *Controller) DeleteNoAuth(ctx context.Context, session *auth.Session, spaceID int64) error {
 	filter := &types.SpaceFilter{
 		Page:  1,
-		Size:  int(math.MaxInt),
+		Size:  math.MaxInt,
 		Query: "",
 		Order: enum.OrderAsc,
 		Sort:  enum.SpaceAttrNone,

internal/api/controller/space/list_spaces.go

@@ -10,13 +10,17 @@ import (
 
 	apiauth "github.com/harness/gitness/internal/api/auth"
 	"github.com/harness/gitness/internal/auth"
+	"github.com/harness/gitness/store/database/dbtx"
 	"github.com/harness/gitness/types"
 	"github.com/harness/gitness/types/enum"
 )
 
 // ListSpaces lists the child spaces of a space.
-func (c *Controller) ListSpaces(ctx context.Context, session *auth.Session,
+func (c *Controller) ListSpaces(ctx context.Context,
-	spaceRef string, filter *types.SpaceFilter) ([]*types.Space, int64, error) {
+	session *auth.Session,
+	spaceRef string,
+	filter *types.SpaceFilter,
+) ([]types.Space, int64, error) {
 	space, err := c.spaceStore.FindByRef(ctx, spaceRef)
 	if err != nil {
 		return nil, 0, err
@@ -28,20 +32,30 @@ func (c *Controller) ListSpaces(ctx context.Context, session *auth.Session,
 	return c.ListSpacesNoAuth(ctx, space.ID, filter)
 }
 
-// List spaces WITHOUT checking PermissionSpaceView.
+// ListSpacesNoAuth lists spaces WITHOUT checking PermissionSpaceView.
 func (c *Controller) ListSpacesNoAuth(
 	ctx context.Context,
 	spaceID int64,
 	filter *types.SpaceFilter,
-) ([]*types.Space, int64, error) {
+) ([]types.Space, int64, error) {
-	count, err := c.spaceStore.Count(ctx, spaceID, filter)
+	var spaces []types.Space
-	if err != nil {
+	var count int64
-		return nil, 0, fmt.Errorf("failed to count child spaces: %w", err)
-	}
 
-	spaces, err := c.spaceStore.List(ctx, spaceID, filter)
+	err := dbtx.New(c.db).WithTx(ctx, func(ctx context.Context) (err error) {
+		count, err = c.spaceStore.Count(ctx, spaceID, filter)
+		if err != nil {
+			return fmt.Errorf("failed to count child spaces: %w", err)
+		}
+
+		spaces, err = c.spaceStore.List(ctx, spaceID, filter)
+		if err != nil {
+			return fmt.Errorf("failed to list child spaces: %w", err)
+		}
+
+		return nil
+	}, dbtx.TxDefaultReadOnly)
 	if err != nil {
-		return nil, 0, fmt.Errorf("failed to list child spaces: %w", err)
+		return nil, 0, err
 	}
 
 	/*

internal/api/handler/space/delete.go

@@ -7,16 +7,13 @@ package space
 import (
 	"net/http"
 
-	"github.com/harness/gitness/internal/api/controller/repo"
 	"github.com/harness/gitness/internal/api/controller/space"
 	"github.com/harness/gitness/internal/api/render"
 	"github.com/harness/gitness/internal/api/request"
 )
 
-/*
+// HandleDelete handles the delete space HTTP API.
- * Deletes a space.
+func HandleDelete(spaceCtrl *space.Controller) http.HandlerFunc {
- */
-func HandleDelete(spaceCtrl *space.Controller, repoCtrl *repo.Controller) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		ctx := r.Context()
 		session, _ := request.AuthSessionFrom(ctx)

internal/router/api.go

@@ -122,7 +122,7 @@ func setupRoutesV1(r chi.Router,
 	principalCtrl principal.Controller,
 	checkCtrl *check.Controller,
 ) {
-	setupSpaces(r, spaceCtrl, repoCtrl)
+	setupSpaces(r, spaceCtrl)
 	setupRepos(r, repoCtrl, pullreqCtrl, webhookCtrl, checkCtrl)
 	setupUser(r, userCtrl)
 	setupServiceAccounts(r, saCtrl)
@@ -134,7 +134,7 @@ func setupRoutesV1(r chi.Router,
 	setupResources(r)
 }
 
-func setupSpaces(r chi.Router, spaceCtrl *space.Controller, repoCtrl *repo.Controller) {
+func setupSpaces(r chi.Router, spaceCtrl *space.Controller) {
 	r.Route("/spaces", func(r chi.Router) {
 		// Create takes path and parentId via body, not uri
 		r.Post("/", handlerspace.HandleCreate(spaceCtrl))
@@ -143,7 +143,7 @@ func setupSpaces(r chi.Router, spaceCtrl *space.Controller, repoCtrl *repo.Contr
 			// space operations
 			r.Get("/", handlerspace.HandleFind(spaceCtrl))
 			r.Patch("/", handlerspace.HandleUpdate(spaceCtrl))
-			r.Delete("/", handlerspace.HandleDelete(spaceCtrl, repoCtrl))
+			r.Delete("/", handlerspace.HandleDelete(spaceCtrl))
 
 			r.Post("/move", handlerspace.HandleMove(spaceCtrl))
 			r.Get("/spaces", handlerspace.HandleListSpaces(spaceCtrl))

internal/store/database.go

@@ -187,7 +187,7 @@ type (
 		Count(ctx context.Context, id int64, opts *types.SpaceFilter) (int64, error)
 
 		// List returns a list of child spaces in a space.
-		List(ctx context.Context, id int64, opts *types.SpaceFilter) ([]*types.Space, error)
+		List(ctx context.Context, id int64, opts *types.SpaceFilter) ([]types.Space, error)
 	}
 
 	// RepoStore defines the repository data storage.

internal/store/database/membership.go

@@ -399,7 +399,7 @@ func (s *MembershipStore) mapToMembershipSpaces(ctx context.Context,
 	res := make([]types.MembershipSpace, len(ms))
 	for i, m := range ms {
 		res[i].Membership = mapToMembership(&m.membership)
-		res[i].Space = *mapToSpace(&m.space)
+		res[i].Space = mapToSpace(&m.space)
 		if addedBy, ok := infoMap[m.membership.CreatedBy]; ok {
 			res[i].AddedBy = *addedBy
 		}

internal/store/database/space.go

@@ -86,7 +86,9 @@ func (s *SpaceStore) Find(ctx context.Context, id int64) (*types.Space, error) {
 		return nil, database.ProcessSQLErrorf(err, "Failed to find space")
 	}
 
-	return mapToSpace(dst), nil
+	result := mapToSpace(dst)
+
+	return &result, nil
 }
 
 // FindByRef finds the space using the spaceRef as either the id or the space path.
@@ -113,6 +115,10 @@ func (s *SpaceStore) FindByRef(ctx context.Context, spaceRef string) (*types.Spa
 
 // Create a new space.
 func (s *SpaceStore) Create(ctx context.Context, space *types.Space) error {
+	if space == nil {
+		return errors.New("space is nil")
+	}
+
 	const sqlQuery = `
 		INSERT INTO spaces (
 			space_version
@@ -134,14 +140,9 @@ func (s *SpaceStore) Create(ctx context.Context, space *types.Space) error {
 			,:space_updated
 		) RETURNING space_id`
 
-	dbSpace, err := mapToInternalSpace(space)
-	if err != nil {
-		return fmt.Errorf("failed to map space: %w", err)
-	}
-
 	db := dbtx.GetAccessor(ctx, s.db)
 
-	query, args, err := db.BindNamed(sqlQuery, dbSpace)
+	query, args, err := db.BindNamed(sqlQuery, mapToInternalSpace(space))
 	if err != nil {
 		return database.ProcessSQLErrorf(err, "Failed to bind space object")
 	}
@@ -153,8 +154,12 @@ func (s *SpaceStore) Create(ctx context.Context, space *types.Space) error {
 	return nil
 }
 
-// Updates the space details.
+// Update updates the space details.
 func (s *SpaceStore) Update(ctx context.Context, space *types.Space) error {
+	if space == nil {
+		return errors.New("space is nil")
+	}
+
 	const sqlQuery = `
 		UPDATE spaces
 		SET
@@ -166,10 +171,7 @@ func (s *SpaceStore) Update(ctx context.Context, space *types.Space) error {
 			,space_is_public	= :space_is_public
 		WHERE space_id = :space_id AND space_version = :space_version - 1`
 
-	dbSpace, err := mapToInternalSpace(space)
+	dbSpace := mapToInternalSpace(space)
-	if err != nil {
-		return fmt.Errorf("failed to map space: %w", err)
-	}
 
 	// update Version (used for optimistic locking) and Updated time
 	dbSpace.Version++
@@ -205,7 +207,8 @@ func (s *SpaceStore) Update(ctx context.Context, space *types.Space) error {
 // UpdateOptLock updates the space using the optimistic locking mechanism.
 func (s *SpaceStore) UpdateOptLock(ctx context.Context,
 	space *types.Space,
-	mutateFn func(space *types.Space) error) (*types.Space, error) {
+	mutateFn func(space *types.Space) error,
+) (*types.Space, error) {
 	for {
 		dup := *space
 
@@ -229,7 +232,7 @@ func (s *SpaceStore) UpdateOptLock(ctx context.Context,
 	}
 }
 
-// Deletes the space.
+// Delete deletes a space.
 func (s *SpaceStore) Delete(ctx context.Context, id int64) error {
 	const sqlQuery = `
 		DELETE FROM spaces
@@ -271,7 +274,7 @@ func (s *SpaceStore) Count(ctx context.Context, id int64, opts *types.SpaceFilte
 }
 
 // List returns a list of spaces under the parent space.
-func (s *SpaceStore) List(ctx context.Context, id int64, opts *types.SpaceFilter) ([]*types.Space, error) {
+func (s *SpaceStore) List(ctx context.Context, id int64, opts *types.SpaceFilter) ([]types.Space, error) {
 	stmt := database.Builder.
 		Select(spaceColumnsForJoin).
 		From("spaces").
@@ -310,7 +313,7 @@ func (s *SpaceStore) List(ctx context.Context, id int64, opts *types.SpaceFilter
 
 	db := dbtx.GetAccessor(ctx, s.db)
 
-	dst := []*space{}
+	var dst []*space
 	if err = db.SelectContext(ctx, &dst, sql, args...); err != nil {
 		return nil, database.ProcessSQLErrorf(err, "Failed executing custom list query")
 	}
@@ -318,8 +321,8 @@ func (s *SpaceStore) List(ctx context.Context, id int64, opts *types.SpaceFilter
 	return mapToSpaces(dst), nil
 }
 
-func mapToSpace(s *space) *types.Space {
+func mapToSpace(s *space) types.Space {
-	res := &types.Space{
+	res := types.Space{
 		ID:          s.ID,
 		Version:     s.Version,
 		UID:         s.UID,
@@ -339,21 +342,16 @@ func mapToSpace(s *space) *types.Space {
 	return res
 }
 
-func mapToSpaces(spaces []*space) []*types.Space {
+func mapToSpaces(spaces []*space) []types.Space {
-	res := make([]*types.Space, len(spaces))
+	res := make([]types.Space, len(spaces))
 	for i := range spaces {
 		res[i] = mapToSpace(spaces[i])
 	}
 	return res
 }
 
-func mapToInternalSpace(s *types.Space) (*space, error) {
+func mapToInternalSpace(s *types.Space) space {
-	// space comes from outside.
+	res := space{
-	if s == nil {
-		return nil, fmt.Errorf("space is nil")
-	}
-
-	res := &space{
 		ID:          s.ID,
 		Version:     s.Version,
 		UID:         s.UID,
@@ -371,5 +369,5 @@ func mapToInternalSpace(s *types.Space) (*space, error) {
 		res.ParentID = null.IntFrom(s.ParentID)
 	}
 
-	return res, nil
+	return res
 }

mocks/mock_store.go

@@ -529,10 +529,10 @@ func (mr *MockSpaceStoreMockRecorder) FindByRef(arg0, arg1 interface{}) *gomock.
 }
 
 // List mocks base method.
-func (m *MockSpaceStore) List(arg0 context.Context, arg1 int64, arg2 *types.SpaceFilter) ([]*types.Space, error) {
+func (m *MockSpaceStore) List(arg0 context.Context, arg1 int64, arg2 *types.SpaceFilter) ([]types.Space, error) {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "List", arg0, arg1, arg2)
-	ret0, _ := ret[0].([]*types.Space)
+	ret0, _ := ret[0].([]types.Space)
 	ret1, _ := ret[1].(error)
 	return ret0, ret1
 }

types/enum/user_test.go

@@ -11,7 +11,7 @@ func TestParseUserAttr(t *testing.T) {
 		text string
 		want UserAttr
 	}{
-		{"id", UserAttrUID},
+		{"uid", UserAttrUID},
 		{"name", UserAttrName},
 		{"email", UserAttrEmail},
 		{"created", UserAttrCreated},