applied review suggestions

* enhance rollback errors * use diff for put * use permissions-api mock Signed-off-by: Bailin He <[email protected]>
infratographer · Oct 2, 2024 · 679d897 · 679d897
1 parent 9dca4ff
commit 679d897
Show file tree

Hide file tree

Showing 11 changed files with 431 additions and 330 deletions.
diff --git a/go.mod b/go.mod
@@ -110,6 +110,7 @@ require (
 	github.com/spf13/afero v1.11.0 // indirect
 	github.com/spf13/cast v1.6.0 // indirect
 	github.com/stoewer/go-strcase v1.2.0 // indirect
+	github.com/stretchr/objx v0.5.2 // indirect
 	github.com/subosito/gotenv v1.6.0 // indirect
 	github.com/valyala/bytebufferpool v1.0.0 // indirect
 	github.com/valyala/fasttemplate v1.2.2 // indirect

diff --git a/internal/api/httpsrv/errors.go b/internal/api/httpsrv/errors.go
@@ -22,6 +22,9 @@ var (
 		status:  http.StatusNotFound,
 		message: "not found",
 	}
+
+	// ErrDBRollbackFailed is returned when a database rollback fails
+	ErrDBRollbackFailed = errors.New("failed to rollback database transaction")
 )
 
 func permissionsError(err error) error {

diff --git a/internal/api/httpsrv/handler.go b/internal/api/httpsrv/handler.go
@@ -2,6 +2,7 @@ package httpsrv
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 
 	"github.com/labstack/echo/v4"
@@ -104,7 +105,10 @@ func (h *APIHandler) Routes(rg *echo.Group) {
 
 func (h *apiHandler) rollbackAndReturnError(ctx context.Context, httpcode int, msg string) *echo.HTTPError {
 	if err := h.engine.RollbackContext(ctx); err != nil {
-		return echo.NewHTTPError(http.StatusBadGateway, err)
+		return echo.NewHTTPError(
+			http.StatusInternalServerError,
+			fmt.Errorf("failed to rollback database transaction: %w", err),
+		)
 	}
 
 	return echo.NewHTTPError(httpcode, msg)

diff --git a/internal/api/httpsrv/handler_group.go b/internal/api/httpsrv/handler_group.go
@@ -74,7 +74,7 @@ func (h *apiHandler) CreateGroup(ctx context.Context, req CreateGroupRequestObje
 	}
 
 	if err := h.eventService.CreateGroup(ctx, ownerID, id); err != nil {
-		resperr := h.rollbackAndReturnError(ctx, http.StatusBadGateway, "failed to create group in permissions API")
+		resperr := h.rollbackAndReturnError(ctx, http.StatusInternalServerError, "failed to create group in permissions API")
 		return nil, resperr
 	}
 
@@ -273,7 +273,7 @@ func (h *apiHandler) DeleteGroup(ctx context.Context, req DeleteGroupRequestObje
 	}
 
 	if err := h.eventService.DeleteGroup(ctx, group.OwnerID, group.ID); err != nil {
-		resperr := h.rollbackAndReturnError(ctx, http.StatusBadGateway, "failed to remove group in permissions API")
+		resperr := h.rollbackAndReturnError(ctx, http.StatusInternalServerError, "failed to remove group in permissions API")
 		return nil, resperr
 	}
 

diff --git a/internal/api/httpsrv/handler_group_members.go b/internal/api/httpsrv/handler_group_members.go
@@ -58,7 +58,7 @@ func (h *apiHandler) AddGroupMembers(ctx context.Context, req AddGroupMembersReq
 	}
 
 	if err := h.eventService.AddGroupMembers(ctx, gid, reqbody.MemberIDs...); err != nil {
-		resperr := h.rollbackAndReturnError(ctx, http.StatusBadGateway, "failed to add group members in permissions API")
+		resperr := h.rollbackAndReturnError(ctx, http.StatusInternalServerError, "failed to add group members in permissions API")
 		return nil, resperr
 	}
 
@@ -147,7 +147,7 @@ func (h *apiHandler) RemoveGroupMember(ctx context.Context, req RemoveGroupMembe
 	}
 
 	if err := h.eventService.RemoveGroupMembers(ctx, gid, sid); err != nil {
-		resperr := h.rollbackAndReturnError(ctx, http.StatusBadGateway, "failed to remove group member in permissions API")
+		resperr := h.rollbackAndReturnError(ctx, http.StatusInternalServerError, "failed to remove group member in permissions API")
 		return nil, resperr
 	}
 
@@ -183,7 +183,7 @@ func (h *apiHandler) ReplaceGroupMembers(ctx context.Context, req ReplaceGroupMe
 		return nil, permissionsError(err)
 	}
 
-	current, err := h.engine.ListGroupMembers(ctx, gid, nil)
+	add, rm, err := h.engine.ReplaceGroupMembers(ctx, gid, reqbody.MemberIDs...)
 	if err != nil {
 		if errors.Is(err, types.ErrNotFound) {
 			err = echo.NewHTTPError(http.StatusNotFound, err.Error())
@@ -192,17 +192,13 @@ func (h *apiHandler) ReplaceGroupMembers(ctx context.Context, req ReplaceGroupMe
 		return nil, err
 	}
 
-	if err := h.engine.ReplaceGroupMembers(ctx, gid, reqbody.MemberIDs...); err != nil {
-		return nil, err
-	}
-
-	if err := h.eventService.RemoveGroupMembers(ctx, gid, current...); err != nil {
-		resperr := h.rollbackAndReturnError(ctx, http.StatusBadGateway, "failed to replace group members in permissions API")
+	if err := h.eventService.RemoveGroupMembers(ctx, gid, rm...); err != nil {
+		resperr := h.rollbackAndReturnError(ctx, http.StatusInternalServerError, "failed to replace group members in permissions API")
 		return nil, resperr
 	}
 
-	if err := h.eventService.AddGroupMembers(ctx, gid, reqbody.MemberIDs...); err != nil {
-		resperr := h.rollbackAndReturnError(ctx, http.StatusBadGateway, "failed to replace group members in permissions API")
+	if err := h.eventService.AddGroupMembers(ctx, gid, add...); err != nil {
+		resperr := h.rollbackAndReturnError(ctx, http.StatusInternalServerError, "failed to replace group members in permissions API")
 		return nil, resperr
 	}