From f5084697b1fd2a3b83ca1c516bf64bccb27b47bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ga=C4=87e=C5=A1a?= Date: Fri, 28 Jul 2023 12:57:16 +0200 Subject: [PATCH] Minor fixes and refactoring --- internal/api/controller/space/delete.go | 2 +- internal/api/controller/space/list_spaces.go | 34 ++++++++---- internal/api/handler/space/delete.go | 7 +-- internal/router/api.go | 6 +-- internal/store/database.go | 2 +- internal/store/database/membership.go | 2 +- internal/store/database/space.go | 54 ++++++++++---------- mocks/mock_store.go | 4 +- types/enum/user_test.go | 2 +- 9 files changed, 61 insertions(+), 52 deletions(-) diff --git a/internal/api/controller/space/delete.go b/internal/api/controller/space/delete.go index 10547d127..5a4e96dbd 100644 --- a/internal/api/controller/space/delete.go +++ b/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, diff --git a/internal/api/controller/space/list_spaces.go b/internal/api/controller/space/list_spaces.go index 2e2d7db83..e2f2d5ea5 100644 --- a/internal/api/controller/space/list_spaces.go +++ b/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, - spaceRef string, filter *types.SpaceFilter) ([]*types.Space, int64, error) { +func (c *Controller) ListSpaces(ctx context.Context, + 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) { - count, err := c.spaceStore.Count(ctx, spaceID, filter) - if err != nil { - return nil, 0, fmt.Errorf("failed to count child spaces: %w", err) - } +) ([]types.Space, int64, error) { + var spaces []types.Space + var count int64 - 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 } /* diff --git a/internal/api/handler/space/delete.go b/internal/api/handler/space/delete.go index c93b2ead4..acad17621 100644 --- a/internal/api/handler/space/delete.go +++ b/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" ) -/* - * Deletes a space. - */ -func HandleDelete(spaceCtrl *space.Controller, repoCtrl *repo.Controller) http.HandlerFunc { +// HandleDelete handles the delete space HTTP API. +func HandleDelete(spaceCtrl *space.Controller) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() session, _ := request.AuthSessionFrom(ctx) diff --git a/internal/router/api.go b/internal/router/api.go index 7a349868d..cfd5d9f6f 100644 --- a/internal/router/api.go +++ b/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)) diff --git a/internal/store/database.go b/internal/store/database.go index 56926837c..43027ea0c 100644 --- a/internal/store/database.go +++ b/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. diff --git a/internal/store/database/membership.go b/internal/store/database/membership.go index 7c393b87f..4aaf54ebc 100644 --- a/internal/store/database/membership.go +++ b/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 } diff --git a/internal/store/database/space.go b/internal/store/database/space.go index 904ca0f2e..36395aa41 100644 --- a/internal/store/database/space.go +++ b/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) - if err != nil { - return fmt.Errorf("failed to map space: %w", err) - } + dbSpace := mapToInternalSpace(space) // 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 { - res := &types.Space{ +func mapToSpace(s *space) 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 { - res := make([]*types.Space, len(spaces)) +func mapToSpaces(spaces []*space) []types.Space { + res := make([]types.Space, len(spaces)) for i := range spaces { res[i] = mapToSpace(spaces[i]) } return res } -func mapToInternalSpace(s *types.Space) (*space, error) { - // space comes from outside. - if s == nil { - return nil, fmt.Errorf("space is nil") - } - - res := &space{ +func mapToInternalSpace(s *types.Space) space { + 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 } diff --git a/mocks/mock_store.go b/mocks/mock_store.go index 34c2db348..0af0bbc34 100644 --- a/mocks/mock_store.go +++ b/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 } diff --git a/types/enum/user_test.go b/types/enum/user_test.go index cc0845499..010ccd5c3 100644 --- a/types/enum/user_test.go +++ b/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},