Skip to content

Commit

Permalink
update method names to be more descriptive
Browse files Browse the repository at this point in the history 
Updated CreateRole, UpdateRole and DeleteRole to CreateRoleTransaction,
UpdateRoleTransaction and DeleteRoleTransaction to make it more clear
that a transaction is being started.

Additionally, the comments on these methods have been updated to include
statements that Commit or Rollback must be called to ensure the database
lifts all locks on rows which are affected.

Previously, the method names made it appear as though the action was
taken and completed. However this could lead to hung connections and
rows.

The new names make it clear that a new transaction is being started.
The returning struct only has two methods Commit and Rollback in
addition to the Record attribute resulting in a simple structure.

Signed-off-by: Mike Mason <[email protected]>
  • Loading branch information
@mikemrm
mikemrm committed Nov 30, 2023
1 parent 1d28c85 commit 80dde1f
Show file tree
Hide file tree
Showing 5 changed files with 151 additions and 134 deletions.
6 changes: 3 additions & 3 deletions internal/database/db.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ type Database interface {
GetRoleByID(ctx context.Context, id gidx.PrefixedID) (*Role, error)
GetResourceRoleByName(ctx context.Context, resourceID gidx.PrefixedID, name string) (*Role, error)
ListResourceRoles(ctx context.Context, resourceID gidx.PrefixedID) ([]*Role, error)
CreateRole(ctx context.Context, actorID gidx.PrefixedID, roleID gidx.PrefixedID, name string, resourceID gidx.PrefixedID) (*Role, error)
UpdateRole(ctx context.Context, actorID gidx.PrefixedID, roleID gidx.PrefixedID, name string, resourceID gidx.PrefixedID) (*Role, error)
DeleteRole(ctx context.Context, roleID gidx.PrefixedID) (*Role, error)
CreateRoleTransaction(ctx context.Context, actorID gidx.PrefixedID, roleID gidx.PrefixedID, name string, resourceID gidx.PrefixedID) (*Transaction[*Role], error)
UpdateRoleTransaction(ctx context.Context, actorID gidx.PrefixedID, roleID gidx.PrefixedID, name string, resourceID gidx.PrefixedID) (*Transaction[*Role], error)
DeleteRoleTransaction(ctx context.Context, roleID gidx.PrefixedID) (*Transaction[*Role], error)
HealthCheck(ctx context.Context) error
}

Expand Down
82 changes: 27 additions & 55 deletions internal/database/roles.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import (
"time"

"go.infratographer.com/x/gidx"
"go.uber.org/zap"
)

// TxRole defines a Role Transaction.
type TxRole = *Transaction[*Role]

// Role represents a role in the database.
type Role struct {
ID gidx.PrefixedID
Expand All @@ -19,37 +21,6 @@ type Role struct {
CreatorID gidx.PrefixedID
CreatedAt time.Time
UpdatedAt time.Time

logger *zap.SugaredLogger
commit func() error
rollback func() error
}

// Commit calls commit on the transaction if the role has been created within a transaction.
// If not the method returns an ErrMethodUnavailable error.
func (r *Role) Commit() error {
if r.commit == nil {
return ErrMethodUnavailable
}

return r.commit()
}

// Rollback calls rollback on the transaction if the role has been created within a transaction.
// If not the method returns an ErrMethodUnavailable error.
//
// To simplify rollbacks, logging has automatically been setup to log any errors produced if a rollback fails.
func (r *Role) Rollback() error {
if r.rollback == nil {
return ErrMethodUnavailable
}

err := r.rollback()
if err != nil && !errors.Is(err, sql.ErrTxDone) {
r.logger.Errorw("failed to rollback role", "role_id", r.ID, zap.Error(err))
}

return err
}

// GetRoleByID retrieves a role from the database by the provided prefixed ID.
Expand Down Expand Up @@ -165,10 +136,13 @@ func (db *database) ListResourceRoles(ctx context.Context, resourceID gidx.Prefi
return roles, nil
}

// CreateRole creates a role with the provided details returning the new Role entry.
// CreateRoleTransaction creates a role with the provided details in a new transaction which must be committed.
// If a role already exists with the given roleID an ErrRoleAlreadyExists error is returned.
// If a role already exists with the same name under the given resource ID then an ErrRoleNameTaken error is returned.
func (db *database) CreateRole(ctx context.Context, actorID, roleID gidx.PrefixedID, name string, resourceID gidx.PrefixedID) (*Role, error) {
//
// Transaction.Commit() or Transaction.Rollback() should be called if error is nil otherwise the database will hold
// the indexes waiting for the transaction to complete.
func (db *database) CreateRoleTransaction(ctx context.Context, actorID, roleID gidx.PrefixedID, name string, resourceID gidx.PrefixedID) (TxRole, error) {
var role Role

tx, err := db.BeginTx(ctx, nil)
Expand Down Expand Up @@ -203,17 +177,16 @@ func (db *database) CreateRole(ctx context.Context, actorID, roleID gidx.Prefixe
return nil, err
}

role.logger = db.logger.Named("role")
role.commit = tx.Commit
role.rollback = tx.Rollback

return &role, nil
return newTransaction(db.logger.With("role_id", role.ID), tx, &role), nil
}

// UpdateRole updates an existing role if one exists.
// If no role already exists, a new role is created in the same way as CreateRole.
// UpdateRoleTransaction starts a new transaction to update an existing role if one exists.
// If no role already exists, a new role is created in the same way as CreateRoleTransaction.
// If changing the name and the new name results in a duplicate name error, an ErrRoleNameTaken error is returned.
func (db *database) UpdateRole(ctx context.Context, actorID, roleID gidx.PrefixedID, name string, resourceID gidx.PrefixedID) (*Role, error) {
//
// Transaction.Commit() or Transaction.Rollback() should be called if error is nil otherwise the database will hold
// the indexes waiting for the transaction to complete.
func (db *database) UpdateRoleTransaction(ctx context.Context, actorID, roleID gidx.PrefixedID, name string, resourceID gidx.PrefixedID) (TxRole, error) {
var role Role

tx, err := db.BeginTx(ctx, nil)
Expand Down Expand Up @@ -245,15 +218,15 @@ func (db *database) UpdateRole(ctx context.Context, actorID, roleID gidx.Prefixe
return nil, err
}

role.logger = db.logger.Named("role")
role.commit = tx.Commit
role.rollback = tx.Rollback

return &role, nil
return newTransaction(db.logger.With("role_id", role.ID), tx, &role), nil
}

// DeleteRole deletes the role id provided, if no rows are affected an ErrNoRoleFound error is returned.
func (db *database) DeleteRole(ctx context.Context, roleID gidx.PrefixedID) (*Role, error) {
// DeleteRoleTransaction starts a new transaction to delete the role for the id provided.
// If no rows are affected an ErrNoRoleFound error is returned.
//
// Transaction.Commit() or Transaction.Rollback() should be called if error is nil otherwise the database will hold
// the indexes waiting for the transaction to complete.
func (db *database) DeleteRoleTransaction(ctx context.Context, roleID gidx.PrefixedID) (TxRole, error) {
tx, err := db.BeginTx(ctx, nil)
if err != nil {
return nil, err
Expand All @@ -273,10 +246,9 @@ func (db *database) DeleteRole(ctx context.Context, roleID gidx.PrefixedID) (*Ro
return nil, ErrNoRoleFound
}

return &Role{
ID: roleID,
logger: db.logger.Named("role"),
commit: tx.Commit,
rollback: tx.Rollback,
}, nil
role := Role{
ID: roleID,
}

return newTransaction(db.logger.With("role_id", role.ID), tx, &role), nil
}
106 changes: 54 additions & 52 deletions internal/database/roles_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ func TestGetRoleByID(t *testing.T) {
assert.ErrorIs(t, err, database.ErrNoRoleFound)
require.Nil(t, role, "no role expected to be returned")

createdRole, err := db.CreateRole(ctx, actorID, roleID, roleName, resourceID)
tx, err := db.CreateRoleTransaction(ctx, actorID, roleID, roleName, resourceID)

require.NoError(t, err, "no error expected while seeding database role")

err = createdRole.Commit()
err = tx.Commit()

require.NoError(t, err, "no error expected while committing role creation")

Expand All @@ -47,8 +47,8 @@ func TestGetRoleByID(t *testing.T) {
assert.Equal(t, roleName, role.Name)
assert.Equal(t, resourceID, role.ResourceID)
assert.Equal(t, actorID, role.CreatorID)
assert.Equal(t, createdRole.CreatedAt, role.CreatedAt)
assert.Equal(t, createdRole.UpdatedAt, role.UpdatedAt)
assert.Equal(t, tx.Record.CreatedAt, role.CreatedAt)
assert.Equal(t, tx.Record.UpdatedAt, role.UpdatedAt)
}

func TestGetResourceRoleByName(t *testing.T) {
Expand All @@ -68,11 +68,11 @@ func TestGetResourceRoleByName(t *testing.T) {
assert.ErrorIs(t, err, database.ErrNoRoleFound)
require.Nil(t, role, "role expected to be returned")

createdRole, err := db.CreateRole(ctx, actorID, roleID, roleName, resourceID)
roleTx, err := db.CreateRoleTransaction(ctx, actorID, roleID, roleName, resourceID)

require.NoError(t, err, "no error expected while seeding database role")

err = createdRole.Commit()
err = roleTx.Commit()

require.NoError(t, err, "no error expected while committing role creation")

Expand All @@ -86,8 +86,8 @@ func TestGetResourceRoleByName(t *testing.T) {
assert.Equal(t, roleName, role.Name)
assert.Equal(t, resourceID, role.ResourceID)
assert.Equal(t, actorID, role.CreatorID)
assert.Equal(t, createdRole.CreatedAt, role.CreatedAt)
assert.Equal(t, createdRole.UpdatedAt, role.UpdatedAt)
assert.Equal(t, roleTx.Record.CreatedAt, role.CreatedAt)
assert.Equal(t, roleTx.Record.UpdatedAt, role.UpdatedAt)
}

func TestListResourceRoles(t *testing.T) {
Expand All @@ -112,11 +112,11 @@ func TestListResourceRoles(t *testing.T) {
}

for roleName, roleID := range groups {
role, err := db.CreateRole(ctx, actorID, roleID, roleName, resourceID)
roleTx, err := db.CreateRoleTransaction(ctx, actorID, roleID, roleName, resourceID)

require.NoError(t, err, "no error expected creating role", roleName)
require.NoError(t, err, "no error expected creating role transaction", roleName)

err = role.Commit()
err = roleTx.Commit()

require.NoError(t, err, "no error expected while committing role", roleName)
}
Expand All @@ -139,7 +139,7 @@ func TestListResourceRoles(t *testing.T) {
}
}

func TestCreateRole(t *testing.T) {
func TestCreateRoleTransaction(t *testing.T) {
db, dbClose := testdb.NewTestDatabase(t)
defer dbClose()

Expand All @@ -150,28 +150,28 @@ func TestCreateRole(t *testing.T) {
roleName := "admins"
resourceID := gidx.PrefixedID("testten-jkl789")

role, err := db.CreateRole(ctx, actorID, roleID, roleName, resourceID)
roleTx, err := db.CreateRoleTransaction(ctx, actorID, roleID, roleName, resourceID)

require.NoError(t, err, "no error expected while creating role")

err = role.Commit()
err = roleTx.Commit()

require.NoError(t, err, "no error expected while committing role creation")

assert.Equal(t, roleID, role.ID)
assert.Equal(t, roleName, role.Name)
assert.Equal(t, resourceID, role.ResourceID)
assert.Equal(t, actorID, role.CreatorID)
assert.False(t, role.CreatedAt.IsZero())
assert.False(t, role.UpdatedAt.IsZero())
assert.Equal(t, roleID, roleTx.Record.ID)
assert.Equal(t, roleName, roleTx.Record.Name)
assert.Equal(t, resourceID, roleTx.Record.ResourceID)
assert.Equal(t, actorID, roleTx.Record.CreatorID)
assert.False(t, roleTx.Record.CreatedAt.IsZero())
assert.False(t, roleTx.Record.UpdatedAt.IsZero())

dupeRole, err := db.CreateRole(ctx, actorID, roleID, roleName, resourceID)
dupeRole, err := db.CreateRoleTransaction(ctx, actorID, roleID, roleName, resourceID)

assert.Error(t, err, "expected error for duplicate index")
assert.ErrorIs(t, err, database.ErrRoleAlreadyExists, "expected error to be for role already exists")
require.Nil(t, dupeRole, "expected role to be nil")

takenNameRole, err := db.CreateRole(ctx, actorID, roleID2, roleName, resourceID)
takenNameRole, err := db.CreateRoleTransaction(ctx, actorID, roleID2, roleName, resourceID)

assert.Error(t, err, "expected error for already taken name")
assert.ErrorIs(t, err, database.ErrRoleNameTaken, "expected error to be for already taken name")
Expand All @@ -191,73 +191,75 @@ func TestUpdateRole(t *testing.T) {
roleName2 := "temps"
resourceID := gidx.PrefixedID("testten-jkl789")

createdRole, err := db.CreateRole(ctx, createActorID, roleID1, roleName, resourceID)
createdRoleTx, err := db.CreateRoleTransaction(ctx, createActorID, roleID1, roleName, resourceID)
require.NoError(t, err, "no error expected while seeding database role")

err = createdRole.Commit()
err = createdRoleTx.Commit()

require.NoError(t, err, "no error expected while committing role creation")

createdRole2, err := db.CreateRole(ctx, createActorID, roleID2, roleName2, resourceID)
createdRole2Tx, err := db.CreateRoleTransaction(ctx, createActorID, roleID2, roleName2, resourceID)
require.NoError(t, err, "no error expected while seeding database role 2")

err = createdRole2.Commit()
err = createdRole2Tx.Commit()

require.NoError(t, err, "no error expected while committing role 2 creation")

updateActorID := gidx.PrefixedID("idntusr-abc456")

t.Run("update error", func(t *testing.T) {
role, err := db.UpdateRole(ctx, updateActorID, roleID2, roleName, resourceID)
roleTx, err := db.UpdateRoleTransaction(ctx, updateActorID, roleID2, roleName, resourceID)

assert.Error(t, err, "expected error updating role name to an already taken role name")
assert.ErrorIs(t, err, database.ErrRoleNameTaken, "expected error to be role name taken error")
assert.Nil(t, role, "expected role to be nil")
assert.Nil(t, roleTx, "expected role to be nil")
})

updateRoleName := "new-admins"
updateResourceID := gidx.PrefixedID("testten-mno101")

t.Run("existing role", func(t *testing.T) {
role, err := db.UpdateRole(ctx, updateActorID, roleID1, updateRoleName, updateResourceID)
updateTx, err := db.UpdateRoleTransaction(ctx, updateActorID, roleID1, updateRoleName, updateResourceID)

require.NoError(t, err, "no error expected while updating role")

require.NotNil(t, role, "role expected to be returned")
require.NotNil(t, updateTx, "transaction expected to be returned")
require.NotNil(t, updateTx.Record, "role expected to be returned")

err = role.Commit()
err = updateTx.Commit()

require.NoError(t, err, "no error expected while committing role update")

assert.Equal(t, roleID1, role.ID)
assert.Equal(t, updateRoleName, role.Name)
assert.Equal(t, updateResourceID, role.ResourceID)
assert.Equal(t, createActorID, role.CreatorID)
assert.Equal(t, createdRole.CreatedAt, role.CreatedAt)
assert.NotEqual(t, createdRole.UpdatedAt, role.UpdatedAt)
assert.Equal(t, roleID1, updateTx.Record.ID)
assert.Equal(t, updateRoleName, updateTx.Record.Name)
assert.Equal(t, updateResourceID, updateTx.Record.ResourceID)
assert.Equal(t, createActorID, updateTx.Record.CreatorID)
assert.Equal(t, createdRoleTx.Record.CreatedAt, updateTx.Record.CreatedAt)
assert.NotEqual(t, createdRoleTx.Record.UpdatedAt, updateTx.Record.UpdatedAt)
})

t.Run("new role", func(t *testing.T) {
newRoleID := gidx.PrefixedID("permrol-xyz789")
newRoleName := "users"
newResourceID := gidx.PrefixedID("testten-lmn159")

role, err := db.UpdateRole(ctx, updateActorID, newRoleID, newRoleName, newResourceID)
updateTx, err := db.UpdateRoleTransaction(ctx, updateActorID, newRoleID, newRoleName, newResourceID)

require.NoError(t, err, "no error expected while updating role")

require.NotNil(t, role, "role expected to be returned")
require.NotNil(t, updateTx, "transaction expected to be returned")
require.NotNil(t, updateTx.Record, "role expected to be returned")

err = role.Commit()
err = updateTx.Commit()

require.NoError(t, err, "no error expected while committing new role from update")

assert.Equal(t, newRoleID, role.ID)
assert.Equal(t, newRoleName, role.Name)
assert.Equal(t, newResourceID, role.ResourceID)
assert.Equal(t, updateActorID, role.CreatorID)
assert.False(t, createdRole.CreatedAt.IsZero())
assert.False(t, createdRole.UpdatedAt.IsZero())
assert.Equal(t, newRoleID, updateTx.Record.ID)
assert.Equal(t, newRoleName, updateTx.Record.Name)
assert.Equal(t, newResourceID, updateTx.Record.ResourceID)
assert.Equal(t, updateActorID, updateTx.Record.CreatorID)
assert.False(t, createdRoleTx.Record.CreatedAt.IsZero())
assert.False(t, createdRoleTx.Record.UpdatedAt.IsZero())
})
}

Expand All @@ -271,28 +273,28 @@ func TestDeleteRole(t *testing.T) {
roleName := "admins"
resourceID := gidx.PrefixedID("testten-jkl789")

_, err := db.DeleteRole(ctx, roleID)
_, err := db.DeleteRoleTransaction(ctx, roleID)

require.Error(t, err, "error expected while deleting role which doesn't exist")
require.ErrorIs(t, err, database.ErrNoRoleFound, "expected no role found error for missing role")

role, err := db.CreateRole(ctx, actorID, roleID, roleName, resourceID)
createTx, err := db.CreateRoleTransaction(ctx, actorID, roleID, roleName, resourceID)

require.NoError(t, err, "no error expected while seeding database role")

err = role.Commit()
err = createTx.Commit()

require.NoError(t, err, "no error expected while committing role creation")

role, err = db.DeleteRole(ctx, roleID)
deleteTx, err := db.DeleteRoleTransaction(ctx, roleID)

require.NoError(t, err, "no error expected while deleting role")

err = role.Commit()
err = deleteTx.Commit()

require.NoError(t, err, "no error expected while committing role deletion")

role, err = db.GetRoleByID(ctx, roleID)
role, err := db.GetRoleByID(ctx, roleID)

require.Error(t, err, "expected error retrieving role")
assert.ErrorIs(t, err, database.ErrNoRoleFound, "expected no rows error")
Expand Down
Loading

0 comments on commit 80dde1f

Please sign in to comment.