Skip to content

Commit

Permalink
Do not return error on not found for delete operations (#628)
Browse files Browse the repository at this point in the history 
* Do not return error on not found for delete operations

* fix lint

* fix tests

* Add test for not found case

* fix lint
  • Loading branch information
@miguelreiswildlife
miguelreiswildlife committed Jun 28, 2024
1 parent 6daae52 commit faf1c9a
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 48 deletions.
11 changes: 6 additions & 5 deletions internal/core/operations/rooms/remove/executor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ package remove

import (
"context"
"errors"
"fmt"
"sync"

Expand All @@ -32,6 +33,7 @@ import (
"github.com/topfreegames/maestro/internal/core/logs"
"github.com/topfreegames/maestro/internal/core/operations"
"github.com/topfreegames/maestro/internal/core/ports"
porterrors "github.com/topfreegames/maestro/internal/core/ports/errors"
"go.uber.org/zap"
"golang.org/x/sync/errgroup"
)
Expand Down Expand Up @@ -93,11 +95,7 @@ func (e *Executor) Execute(ctx context.Context, op *operation.Operation, definit
func (e *Executor) removeRoomsByIDs(ctx context.Context, schedulerName string, roomsIDs []string, op *operation.Operation) error {
rooms := make([]*game_room.GameRoom, 0, len(roomsIDs))
for _, roomID := range roomsIDs {
gameRoom, err := e.roomStorage.GetRoom(ctx, schedulerName, roomID)
if err != nil {
return err
}

gameRoom := &game_room.GameRoom{ID: roomID, SchedulerID: schedulerName}
rooms = append(rooms, gameRoom)
}

Expand Down Expand Up @@ -134,6 +132,9 @@ func (e *Executor) deleteRooms(ctx context.Context, rooms []*game_room.GameRoom,
msg := fmt.Sprintf("error removing room \"%v\". Reason => %v", room.ID, err.Error())
e.operationManager.AppendOperationEventToExecutionHistory(ctx, op, msg)
}
if errors.Is(err, porterrors.ErrNotFound) {
return nil
}
return err
})
}
Expand Down
64 changes: 21 additions & 43 deletions internal/core/operations/rooms/remove/executor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ import (
"fmt"
"testing"

serviceerrors "github.com/topfreegames/maestro/internal/core/services/errors"

"github.com/golang/mock/gomock"
"github.com/google/uuid"
"github.com/stretchr/testify/require"
porterrors "github.com/topfreegames/maestro/internal/core/ports/errors"
mockports "github.com/topfreegames/maestro/internal/core/ports/mock"
serviceerrors "github.com/topfreegames/maestro/internal/core/services/errors"

"github.com/topfreegames/maestro/internal/core/entities/game_room"
"github.com/topfreegames/maestro/internal/core/entities/operation"
Expand Down Expand Up @@ -151,7 +151,7 @@ func TestExecutor_Execute(t *testing.T) {
})

t.Run("should succeed - rooms are successfully removed => returns without error", func(t *testing.T) {
executor, roomsStorage, roomsManager, _ := testSetup(t)
executor, _, roomsManager, _ := testSetup(t)

firstRoomID := "first-room-id"
secondRoomID := "second-room-id"
Expand All @@ -163,26 +163,20 @@ func TestExecutor_Execute(t *testing.T) {
room := &game_room.GameRoom{
ID: firstRoomID,
SchedulerID: schedulerName,
Status: game_room.GameStatusReady,
Metadata: map[string]interface{}{},
}
secondRoom := &game_room.GameRoom{
ID: secondRoomID,
SchedulerID: schedulerName,
Status: game_room.GameStatusReady,
Metadata: map[string]interface{}{},
}
roomsStorage.EXPECT().GetRoom(gomock.Any(), schedulerName, firstRoomID).Return(room, nil)
roomsStorage.EXPECT().GetRoom(gomock.Any(), schedulerName, secondRoomID).Return(secondRoom, nil)
roomsManager.EXPECT().DeleteRoom(gomock.Any(), room).Return(nil)
roomsManager.EXPECT().DeleteRoom(gomock.Any(), secondRoom).Return(nil)

err := executor.Execute(context.Background(), op, definition)
require.Nil(t, err)
})

t.Run("when failed to get any room it returns with error", func(t *testing.T) {
executor, roomsStorage, _, _ := testSetup(t)
t.Run("when any room failed to delete with unexpected error it returns with error", func(t *testing.T) {
executor, _, roomsManager, operationManager := testSetup(t)

firstRoomID := "first-room-id"
secondRoomID := "second-room-id"
Expand All @@ -194,17 +188,22 @@ func TestExecutor_Execute(t *testing.T) {
room := &game_room.GameRoom{
ID: firstRoomID,
SchedulerID: schedulerName,
Status: game_room.GameStatusReady,
}
roomsStorage.EXPECT().GetRoom(gomock.Any(), schedulerName, firstRoomID).Return(room, nil)
roomsStorage.EXPECT().GetRoom(gomock.Any(), schedulerName, secondRoomID).Return(nil, fmt.Errorf("error on GetRoom"))
secondRoom := &game_room.GameRoom{
ID: secondRoomID,
SchedulerID: schedulerName,
}

roomsManager.EXPECT().DeleteRoom(gomock.Any(), room).Return(nil)
roomsManager.EXPECT().DeleteRoom(gomock.Any(), secondRoom).Return(fmt.Errorf("error on remove room"))
operationManager.EXPECT().AppendOperationEventToExecutionHistory(gomock.Any(), op, gomock.Any())

err := executor.Execute(context.Background(), op, definition)
require.ErrorContains(t, err, "error removing rooms by ids: error on GetRoom")
require.ErrorContains(t, err, "error removing rooms by ids: error on remove room")
})

t.Run("when any room failed to delete with unexpected error it returns with error", func(t *testing.T) {
executor, roomsStorage, roomsManager, operationManager := testSetup(t)
t.Run("when any room returns not found on delete it is ignored and returns without error", func(t *testing.T) {
executor, _, roomsManager, operationManager := testSetup(t)

firstRoomID := "first-room-id"
secondRoomID := "second-room-id"
Expand All @@ -216,26 +215,21 @@ func TestExecutor_Execute(t *testing.T) {
room := &game_room.GameRoom{
ID: firstRoomID,
SchedulerID: schedulerName,
Status: game_room.GameStatusReady,
}
secondRoom := &game_room.GameRoom{
ID: firstRoomID,
ID: secondRoomID,
SchedulerID: schedulerName,
Status: game_room.GameStatusReady,
}
roomsStorage.EXPECT().GetRoom(gomock.Any(), schedulerName, firstRoomID).Return(room, nil)
roomsStorage.EXPECT().GetRoom(gomock.Any(), schedulerName, secondRoomID).Return(secondRoom, nil)
roomsManager.EXPECT().DeleteRoom(gomock.Any(), room).Return(nil)

roomsManager.EXPECT().DeleteRoom(gomock.Any(), secondRoom).Return(fmt.Errorf("error on remove room"))
roomsManager.EXPECT().DeleteRoom(gomock.Any(), secondRoom).Return(porterrors.NewErrNotFound("not found"))
operationManager.EXPECT().AppendOperationEventToExecutionHistory(gomock.Any(), op, gomock.Any())

err := executor.Execute(context.Background(), op, definition)
require.ErrorContains(t, err, "error removing rooms by ids: error on remove room")
require.NoError(t, err)
})

t.Run("when any room failed to delete with timeout error it returns with error", func(t *testing.T) {
executor, roomsStorage, roomsManager, operationManager := testSetup(t)
executor, _, roomsManager, operationManager := testSetup(t)

firstRoomID := "first-room-id"
secondRoomID := "second-room-id"
Expand All @@ -247,15 +241,11 @@ func TestExecutor_Execute(t *testing.T) {
room := &game_room.GameRoom{
ID: firstRoomID,
SchedulerID: schedulerName,
Status: game_room.GameStatusReady,
}
secondRoom := &game_room.GameRoom{
ID: secondRoomID,
SchedulerID: schedulerName,
Status: game_room.GameStatusReady,
}
roomsStorage.EXPECT().GetRoom(gomock.Any(), schedulerName, firstRoomID).Return(room, nil)
roomsStorage.EXPECT().GetRoom(gomock.Any(), schedulerName, secondRoomID).Return(secondRoom, nil)
roomsManager.EXPECT().DeleteRoom(gomock.Any(), room).Return(nil)
roomsManager.EXPECT().DeleteRoom(gomock.Any(), secondRoom).Return(serviceerrors.NewErrGameRoomStatusWaitingTimeout("some error"))
operationManager.EXPECT().AppendOperationEventToExecutionHistory(gomock.Any(), op, gomock.Any())
Expand All @@ -278,7 +268,7 @@ func TestExecutor_Execute(t *testing.T) {
})

t.Run("should succeed - there are ids and amount => return without error", func(t *testing.T) {
executor, roomsStorage, roomsManager, _ := testSetup(t)
executor, _, roomsManager, _ := testSetup(t)

firstRoomID := "first-room-id"
secondRoomID := "second-room-id"
Expand All @@ -292,16 +282,6 @@ func TestExecutor_Execute(t *testing.T) {
}
op := &operation.Operation{ID: "random-uuid", SchedulerName: schedulerName}

room := &game_room.GameRoom{
ID: firstRoomID,
SchedulerID: schedulerName,
Status: game_room.GameStatusReady,
}
secondRoom := &game_room.GameRoom{
ID: secondRoomID,
SchedulerID: schedulerName,
Status: game_room.GameStatusReady,
}
thirdRoom := &game_room.GameRoom{
ID: thirdRoomID,
SchedulerID: schedulerName,
Expand All @@ -312,8 +292,6 @@ func TestExecutor_Execute(t *testing.T) {
SchedulerID: schedulerName,
Status: game_room.GameStatusReady,
}
roomsStorage.EXPECT().GetRoom(gomock.Any(), schedulerName, firstRoomID).Return(room, nil)
roomsStorage.EXPECT().GetRoom(gomock.Any(), schedulerName, secondRoomID).Return(secondRoom, nil)
roomsManager.EXPECT().DeleteRoom(gomock.Any(), gomock.Any()).Return(nil).Times(2)

availableRooms := []*game_room.GameRoom{thirdRoom, fourthRoom}
Expand Down

0 comments on commit faf1c9a

Please sign in to comment.