Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Commit

Permalink
Introduce service layer (#456)
Browse files Browse the repository at this point in the history
# Refactoring of application package

- Repositories interface extracted into repository subpackage

This provides a global fix for import cycle issues, solving the problem of having to create a separate repositories interfaces for the service layer

- Services interface introduced.

Provides a convenient way for controllers to access the service layer

# Decouple transaction control from the application package

## Transaction-related functionality moved to application/transaction package

- Allows transactions to be managed without introducing import cycle issues with application package
- Transactional method refactored:
`      func Transactional(tm TransactionManager, todo func(f TransactionalResources) error) error`
      
The `tm` parameter is used by the `Transactional` method to initiate the transaction.  Code executed within the transaction block (i.e. the `todo` function) may interact with transactional resources (e.g. repositories) via the `TransactionalResources` parameter.
      
**Some notes:**

The `Application` object implements `TransactionManager`, so transactions can continue to be conveniently managed inside the controller package:
        
```
err := transaction.Transactional(c.app, func(tr transaction.TransactionalResources) error {
    // transactional code
}
```
        	
`TransactionalResources` implements the `Repositories` interface, providing convenient access to repository types:
        
```
err := transaction.Transactional(c.app, func(tr transaction.TransactionalResources) error {
    tr.ResourceRepository().Delete(ctx, ctx.ResourceID)
}
```
        
The scope of `TransactionalResources` may be easily extended in the future to include other transactional resources, not just repositories.

# Introduction of service layer

- Transaction control moved to service layer

Since the service layer is responsible for the atomic integrity of the operation, transaction control was moved to the service layer where required.

- Extracted db-specific code from service layer to repository layer

This leaves the service layer as a purely business logic layer.  In many cases, this has left the service layer as a thin facade over the repository layer, which is exactly what the Service Layer design pattern recommends.  For example, `RoleManagementService` which was previously over 300 lines of code is now only about 40 lines.

- Fixed Preload issues

Many hours was spent deep inside the Gorm source code to determine why `Preload()` was failing, and I'm pleased to report that I was able to identify the issue and rectify it in our code.  As a result, I was able to do some extensive code refactoring to significantly reduce the number of lines of code required to execute some queries and get the results we required.  For example, I was able to reduce the methods in `RoleManagementService` (some of them around 70 lines of code) significantly (in some cases just over 10 lines of code) in the `IdentityRole` repository.

# Adds `IdentityAssociation` struct

This type is a general purpose DTO for transferring membership and role assignment state from the repository/service layer to the controller layer.  It is designed to replace multiple other DTO types (such as the [`IdentityOrganization`](https://github.com/fabric8-services/fabric8-auth/blob/master/authorization/organization/organization.go) type) with a single reusable type.  Two convenience methods have been added to the `authorization` package to support this struct:

```
func AppendAssociation(associations []IdentityAssociation, resourceID string, resourceName *string, identityID *uuid.UUID, member bool, role *string)
func MergeAssociations(associations []IdentityAssociation, merge []IdentityAssociation) []IdentityAssociation
```

The `AppendAssociation` function can be used to add new state to an `IdentityAssociation` array.  It either merges the state into an existing element (if there is a match) or creates a new element if necessary.  The `MergeAssociations` function is used to correctly merge the state of two separate `IdentityAssociation` arrays into one.


Fixes #446 

Signed-off-by: Shane Bryzak <sbryzak@redhat.com>
  • Loading branch information
sbryzak authored and sbose78 committed Apr 24, 2018
1 parent b5e0fa2 commit 7aa304a
Show file tree
Hide file tree
Showing 77 changed files with 1,630 additions and 1,473 deletions.
14 changes: 9 additions & 5 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Gopkg.toml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ required = [
[[constraint]]
name = "github.com/goadesign/goa"
version = "1.3.0"

[[constraint]]
name = "github.com/jinzhu/gorm"
version = "1.9.1"

[[constraint]]
name = "github.com/magiconair/properties"
Expand Down
19 changes: 10 additions & 9 deletions account/email/verification_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/satori/go.uuid"

"context"
"github.com/fabric8-services/fabric8-auth/application/transaction"
)

type EmailVerificationService interface {
Expand All @@ -20,18 +21,18 @@ type EmailVerificationService interface {
}

type EmailVerificationClient struct {
db application.DB
app application.Application
notification notification.Channel
}

// NewEmailVerificationClient creates a new client for managing email verification.
func NewEmailVerificationClient(db application.DB, notificationChannel notification.Channel) *EmailVerificationClient {
func NewEmailVerificationClient(app application.Application, notificationChannel notification.Channel) *EmailVerificationClient {
n := notificationChannel
if n == nil {
n = &notification.DevNullChannel{}
}
return &EmailVerificationClient{
db: db,
app: app,
notification: n,
}
}
Expand All @@ -49,8 +50,8 @@ func (c *EmailVerificationClient) SendVerificationCode(ctx context.Context, req
"email": identity.User.Email,
}, "verification code to be sent")

err := application.Transactional(c.db, func(appl application.Application) error {
err := appl.VerificationCodes().Create(ctx, &newVerificationCode)
err := transaction.Transactional(c.app, func(tr transaction.TransactionalResources) error {
err := tr.VerificationCodes().Create(ctx, &newVerificationCode)
return err
})
if err != nil {
Expand Down Expand Up @@ -80,9 +81,9 @@ func (c *EmailVerificationClient) VerifyCode(ctx context.Context, code string) (
"code": code,
}, "verification code to be validated")

err := application.Transactional(c.db, func(appl application.Application) error {
err := transaction.Transactional(c.app, func(tr transaction.TransactionalResources) error {

verificationCodeList, err := appl.VerificationCodes().LoadByCode(ctx, code)
verificationCodeList, err := tr.VerificationCodes().LoadByCode(ctx, code)
if err != nil {
log.Error(ctx, map[string]interface{}{
"err": err,
Expand All @@ -97,12 +98,12 @@ func (c *EmailVerificationClient) VerifyCode(ctx context.Context, code string) (

user := verificationCode.User
user.EmailVerified = true
err = appl.Users().Save(ctx, &user)
err = tr.Users().Save(ctx, &user)
if err != nil {
return err
}

err = appl.VerificationCodes().Delete(ctx, verificationCode.ID)
err = tr.VerificationCodes().Delete(ctx, verificationCode.ID)
return err

})
Expand Down
42 changes: 39 additions & 3 deletions account/identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ import (
"strconv"
"time"

"github.com/fabric8-services/fabric8-auth/application/repository"
repository "github.com/fabric8-services/fabric8-auth/application/repository/base"
resource "github.com/fabric8-services/fabric8-auth/authorization/resource/repository"
"github.com/fabric8-services/fabric8-auth/errors"
"github.com/fabric8-services/fabric8-auth/gormsupport"
"github.com/fabric8-services/fabric8-auth/log"

"database/sql"
"github.com/fabric8-services/fabric8-auth/authorization"
"github.com/goadesign/goa"
"github.com/jinzhu/gorm"
errs "github.com/pkg/errors"
Expand Down Expand Up @@ -76,8 +78,8 @@ type Identity struct {
UserID NullUUID `sql:"type:uuid"`
User User
// Link to Resource
IdentityResourceID *string
IdentityResource resource.Resource
IdentityResourceID sql.NullString
IdentityResource resource.Resource `gorm:"foreignkey:IdentityResourceID;association_foreignkey:ResourceID"`
}

// TableName overrides the table name settings in Gorm to force a specific table name
Expand Down Expand Up @@ -125,6 +127,7 @@ type IdentityRepository interface {
List(ctx context.Context) ([]Identity, error)
IsValid(context.Context, uuid.UUID) bool
Search(ctx context.Context, q string, start int, limit int) ([]Identity, int, error)
FindIdentityMemberships(ctx context.Context, identityID uuid.UUID, resourceType *string) ([]authorization.IdentityAssociation, error)
}

// TableName overrides the table name settings in Gorm to force a specific table name
Expand Down Expand Up @@ -444,3 +447,36 @@ WHERE

return result, count, nil
}

// FindIdentityMemberships returns an array of Identity objects with the (optionally) specified resource type in which the specified Identity is a member
func (m *GormIdentityRepository) FindIdentityMemberships(ctx context.Context, identityID uuid.UUID, resourceType *string) ([]authorization.IdentityAssociation, error) {
defer goa.MeasureSince([]string{"goa", "db", "identity", "FindIdentityMemberships"}, time.Now())
associations := []authorization.IdentityAssociation{}

var identities []Identity

// query for identities in which the user is a member
q := m.db.Table(m.TableName()).Preload("IdentityResource")

// with the specified resourceType
if resourceType != nil {
q = q.Joins("JOIN resource r ON r.resource_id = identities.identity_resource_id").
Joins("JOIN resource_type rt ON r.resource_type_id = rt.resource_type_id AND rt.name = ?", resourceType)
}

err := q.Where(`identities.id IN (WITH RECURSIVE m AS (
SELECT member_of FROM membership WHERE member_id = ?
UNION SELECT p.member_of FROM membership p INNER JOIN m ON m.member_of = p.member_id)
SELECT member_of FROM m)`, identityID).
Find(&identities).Error

if err != nil {
return nil, err
}

for _, identity := range identities {
associations = authorization.AppendAssociation(associations, identity.IdentityResourceID.String, &identity.IdentityResource.Name, &identity.ID, true, nil)
}

return associations, nil
}
57 changes: 39 additions & 18 deletions account/identity_blackbox_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (

type identityBlackBoxTest struct {
gormtestsupport.DBTestSuite
repo account.IdentityRepository
}

func TestRunIdentityBlackBoxTest(t *testing.T) {
Expand All @@ -25,7 +24,6 @@ func TestRunIdentityBlackBoxTest(t *testing.T) {

func (s *identityBlackBoxTest) SetupTest() {
s.DBTestSuite.SetupTest()
s.repo = account.NewIdentityRepository(s.DB)
}

func (s *identityBlackBoxTest) TestOKToDelete() {
Expand All @@ -40,15 +38,15 @@ func (s *identityBlackBoxTest) TestOKToDelete() {
Username: "onemoreuserTestIdentity",
ProviderType: account.KeycloakIDP}

err := s.repo.Create(s.Ctx, identity)
err := s.Application.Identities().Create(s.Ctx, identity)
require.Nil(s.T(), err, "Could not create identity")
err = s.repo.Create(s.Ctx, identity2)
err = s.Application.Identities().Create(s.Ctx, identity2)
require.Nil(s.T(), err, "Could not create identity")
// when
err = s.repo.Delete(s.Ctx, identity.ID)
err = s.Application.Identities().Delete(s.Ctx, identity.ID)
// then
assert.Nil(s.T(), err)
identities, err := s.repo.List(s.Ctx)
identities, err := s.Application.Identities().List(s.Ctx)
require.Nil(s.T(), err, "Could not list identities")
require.True(s.T(), len(identities) > 0)
for _, ident := range identities {
Expand All @@ -69,41 +67,40 @@ func (s *identityBlackBoxTest) TestExistsIdentity() {
// given
identity := createAndLoad(s)
// when
err := s.repo.CheckExists(s.Ctx, identity.ID.String())
err := s.Application.Identities().CheckExists(s.Ctx, identity.ID.String())
// then
require.Nil(t, err)
})

t.Run("identity doesn't exist", func(t *testing.T) {
//t.Parallel()
err := s.repo.CheckExists(s.Ctx, uuid.NewV4().String())
err := s.Application.Identities().CheckExists(s.Ctx, uuid.NewV4().String())
// then
require.IsType(t, errors.NotFoundError{}, err)
})

}

func (s *identityBlackBoxTest) TestOKToSave() {
// given
identity := createAndLoad(s)
// when
identity.Username = "newusernameTestIdentity"
err := s.repo.Save(s.Ctx, identity)
err := s.Application.Identities().Save(s.Ctx, identity)
// then
require.Nil(s.T(), err, "Could not update identity")
}

func (s *identityBlackBoxTest) TestLoadIdentityAndUserFailsIfUserOrIdentityDoNotExist() {
// Identity exists but not associated with any user
identity := createAndLoad(s)
_, err := s.repo.LoadWithUser(s.Ctx, identity.ID)
_, err := s.Application.Identities().LoadWithUser(s.Ctx, identity.ID)
require.NotNil(s.T(), err)

assert.Equal(s.T(), errors.NewNotFoundError("user for identity", identity.ID.String()).Error(), err.Error())

// Identity does not exist
id := uuid.NewV4()
_, err = s.repo.LoadWithUser(s.Ctx, id)
_, err = s.Application.Identities().LoadWithUser(s.Ctx, id)
require.NotNil(s.T(), err)
assert.Equal(s.T(), errors.NewNotFoundError("identity", id.String()).Error(), err.Error())
}
Expand All @@ -123,10 +120,10 @@ func (s *identityBlackBoxTest) TestLoadIdentityAndUserOK() {
}
userRepository := account.NewUserRepository(s.DB)
userRepository.Create(s.Ctx, testUser)
s.repo.Create(s.Ctx, testIdentity)
s.Application.Identities().Create(s.Ctx, testIdentity)

// Check load
identity, err := s.repo.LoadWithUser(s.Ctx, testIdentity.ID)
identity, err := s.Application.Identities().LoadWithUser(s.Ctx, testIdentity.ID)
require.NoError(s.T(), err)
require.NotNil(s.T(), identity)
testIdentity.CreatedAt = identity.CreatedAt // Align timestamps
Expand All @@ -153,25 +150,49 @@ func (s *identityBlackBoxTest) TestUserIdentityIsUser() {
}
userRepository := account.NewUserRepository(s.DB)
userRepository.Create(s.Ctx, testUser)
s.repo.Create(s.Ctx, testIdentity)
s.Application.Identities().Create(s.Ctx, testIdentity)

// Load the identity
identity, err := s.repo.LoadWithUser(s.Ctx, testIdentity.ID)
identity, err := s.Application.Identities().LoadWithUser(s.Ctx, testIdentity.ID)
require.NoError(s.T(), err)
require.NotNil(s.T(), identity)
require.True(s.T(), identity.IsUser())
}

func (s *identityBlackBoxTest) TestFindIdentityMemberships() {
// Create an identity
identity := createAndLoad(s)

orgName := "Acme Corporation - identityBlackBoxTest"
// Create an organization
orgID, err := s.Application.OrganizationService().CreateOrganization(s.Ctx, identity.ID, orgName)
require.NoError(s.T(), err)

// Create a record in the membership table
err = s.DB.Unscoped().Exec("INSERT INTO membership (member_id, member_of) VALUES (?, ?)", identity.ID, orgID).Error
require.NoError(s.T(), err)

// Find the identity's memberships
associations, err := s.Application.Identities().FindIdentityMemberships(s.Ctx, identity.ID, nil)
require.NoError(s.T(), err)

// There should be 1 entry
require.Equal(s.T(), 1, len(associations))
require.Equal(s.T(), orgID, associations[0].IdentityID)
require.True(s.T(), associations[0].Member)
require.Equal(s.T(), orgName, associations[0].ResourceName)
}

func createAndLoad(s *identityBlackBoxTest) *account.Identity {
identity := &account.Identity{
ID: uuid.NewV4(),
Username: "someuserTestIdentity2",
ProviderType: account.KeycloakIDP}

err := s.repo.Create(s.Ctx, identity)
err := s.Application.Identities().Create(s.Ctx, identity)
require.Nil(s.T(), err, "Could not create identity")
// when
idnt, err := s.repo.Load(s.Ctx, identity.ID)
idnt, err := s.Application.Identities().Load(s.Ctx, identity.ID)
// then
require.Nil(s.T(), err, "Could not load identity")
require.Equal(s.T(), "someuserTestIdentity2", idnt.Username)
Expand Down
2 changes: 1 addition & 1 deletion account/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"strconv"
"time"

"github.com/fabric8-services/fabric8-auth/application/repository"
repository "github.com/fabric8-services/fabric8-auth/application/repository/base"
"github.com/fabric8-services/fabric8-auth/errors"
"github.com/fabric8-services/fabric8-auth/gormsupport"
"github.com/fabric8-services/fabric8-auth/log"
Expand Down
13 changes: 7 additions & 6 deletions account/userinfo/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,26 @@ import (

"github.com/fabric8-services/fabric8-auth/account"
"github.com/fabric8-services/fabric8-auth/application"
"github.com/fabric8-services/fabric8-auth/application/transaction"
autherrors "github.com/fabric8-services/fabric8-auth/errors"
"github.com/fabric8-services/fabric8-auth/log"
"github.com/fabric8-services/fabric8-auth/token"
)

func NewUserInfoProvider(identities account.IdentityRepository, users account.UserRepository, tokenManager token.Manager, db application.DB) *UserInfoProvider {
func NewUserInfoProvider(identities account.IdentityRepository, users account.UserRepository, tokenManager token.Manager, app application.Application) *UserInfoProvider {
return &UserInfoProvider{
Identities: identities,
Users: users,
TokenManager: tokenManager,
DB: db,
App: app,
}
}

type UserInfoProvider struct {
Identities account.IdentityRepository
Users account.UserRepository
TokenManager token.Manager
DB application.DB
App application.Application
UserInfoService UserInfoService
}

Expand All @@ -44,9 +45,9 @@ func (userInfoProvider *UserInfoProvider) UserInfo(ctx context.Context) (*accoun
}
var user *account.User
var identity *account.Identity
err = application.Transactional(userInfoProvider.DB, func(appl application.Application) error {
err = transaction.Transactional(userInfoProvider.App, func(tr transaction.TransactionalResources) error {

identity, err = appl.Identities().Load(ctx, id)
identity, err = tr.Identities().Load(ctx, id)
if err != nil || identity == nil {
log.Error(ctx, map[string]interface{}{
"identity_id": id,
Expand All @@ -57,7 +58,7 @@ func (userInfoProvider *UserInfoProvider) UserInfo(ctx context.Context) (*accoun

userID := identity.UserID
if userID.Valid {
user, err = appl.Users().Load(ctx, userID.UUID)
user, err = tr.Users().Load(ctx, userID.UUID)
if err != nil {
log.Error(ctx, map[string]interface{}{
"user_id": userID,
Expand Down
Loading

0 comments on commit 7aa304a

Please sign in to comment.