Fixes repository major concurrency issues (#54)

* chore: fixes TestUpdateAccountTokensHandler

* Fixes repository major concurrency issues

Author: petruki

src/controller/account.go

@@ -137,7 +137,7 @@ func (controller *AccountController) UpdateAccountHandler(w http.ResponseWriter,
 		accountRequest.Token = utils.Encrypt(accountRequest.Token, config.GetEnv("GIT_TOKEN_PRIVATE_KEY"))
 	}
 
-	accountUpdated, err := controller.accountRepository.Update(&accountRequest)
+	accountUpdated, err := controller.accountRepository.UpdateByDomainEnvironment(&accountRequest)
 	if err != nil {
 		utils.LogError("Error updating account: %s", err.Error())
 		utils.ResponseJSON(w, ErrorResponse{Error: "Error updating account"}, http.StatusInternalServerError)
@@ -166,7 +166,7 @@ func (controller *AccountController) UpdateAccountTokensHandler(w http.ResponseW
 	}
 
 	// Encrypt token before saving
-	accountTokensRequest.Token = utils.Encrypt(accountTokensRequest.Token, config.GetEnv("GIT_TOKEN_PRIVATE_KEY"))
+	newEncryptedToken := utils.Encrypt(accountTokensRequest.Token, config.GetEnv("GIT_TOKEN_PRIVATE_KEY"))
 
 	// Update account tokens
 	for _, environment := range accountTokensRequest.Environments {
@@ -177,8 +177,8 @@ func (controller *AccountController) UpdateAccountTokensHandler(w http.ResponseW
 			return
 		}
 
-		account.Token = accountTokensRequest.Token
-		controller.accountRepository.Update(account)
+		account.Token = newEncryptedToken
+		controller.accountRepository.UpdateByDomainEnvironment(account)
 	}
 
 	utils.ResponseJSON(w, AccountTokensResponse{

src/controller/account_test.go

@@ -3,16 +3,13 @@ package controller
 import (
 	"bytes"
 	"encoding/json"
-	"io"
 	"net/http"
-	"net/http/httptest"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/switcherapi/switcher-gitops/src/config"
 	"github.com/switcherapi/switcher-gitops/src/model"
-	"github.com/switcherapi/switcher-gitops/src/repository"
 	"github.com/switcherapi/switcher-gitops/src/utils"
 )
 
@@ -26,8 +23,9 @@ func TestCreateAccountHandler(t *testing.T) {
 
 	t.Run("Should create an account", func(t *testing.T) {
 		// Test
-		accountV1.Domain.ID = "123-controller-create-account"
-		payload, _ := json.Marshal(accountV1)
+		account1 := accountV1
+		account1.Domain.ID = "123-controller-create-account"
+		payload, _ := json.Marshal(account1)
 		req, _ := http.NewRequest(http.MethodPost, accountController.routeAccountPath, bytes.NewBuffer(payload))
 		response := executeRequest(req, r, token)
 
@@ -37,7 +35,7 @@ func TestCreateAccountHandler(t *testing.T) {
 
 		assert.Equal(t, http.StatusCreated, response.Code)
 		assert.Nil(t, err)
-		assert.Equal(t, accountV1.Repository, accountResponse.Repository)
+		assert.Equal(t, account1.Repository, accountResponse.Repository)
 		assert.Contains(t, accountResponse.Token, "...")
 	})
 
@@ -54,10 +52,11 @@ func TestCreateAccountHandler(t *testing.T) {
 
 	t.Run("Should not create an account - account already exists", func(t *testing.T) {
 		// Create an account
-		accountController.CreateAccountHandler(givenAccountRequest(accountV1))
+		account1 := accountV1
+		accountController.accountRepository.Create(&account1)
 
 		// Test
-		payload, _ := json.Marshal(accountV1)
+		payload, _ := json.Marshal(account1)
 		req, _ := http.NewRequest(http.MethodPost, accountController.routeAccountPath, bytes.NewBuffer(payload))
 		response := executeRequest(req, r, token)
 
@@ -72,12 +71,13 @@ func TestFetchAccountHandler(t *testing.T) {
 
 	t.Run("Should fetch an account by domain ID / environment", func(t *testing.T) {
 		// Create an account
-		accountV1.Domain.ID = "123-controller-fetch-account"
-		accountController.CreateAccountHandler(givenAccountRequest(accountV1))
+		account1 := accountV1
+		account1.Domain.ID = "123-controller-fetch-account"
+		accountController.accountRepository.Create(&account1)
 
 		// Test
 		payload := []byte("")
-		req, _ := http.NewRequest(http.MethodGet, accountController.routeAccountPath+"/"+accountV1.Domain.ID+"/"+accountV1.Environment, bytes.NewBuffer(payload))
+		req, _ := http.NewRequest(http.MethodGet, accountController.routeAccountPath+"/"+account1.Domain.ID+"/"+account1.Environment, bytes.NewBuffer(payload))
 		response := executeRequest(req, r, token)
 
 		// Assert
@@ -86,7 +86,7 @@ func TestFetchAccountHandler(t *testing.T) {
 
 		assert.Equal(t, http.StatusOK, response.Code)
 		assert.Nil(t, err)
-		assert.Equal(t, accountV1.Repository, accountResponse.Repository)
+		assert.Equal(t, account1.Repository, accountResponse.Repository)
 		assert.Contains(t, accountResponse.Token, "...")
 	})
 
@@ -102,15 +102,22 @@ func TestFetchAccountHandler(t *testing.T) {
 	})
 
 	t.Run("Should fetch all accounts by domain ID", func(t *testing.T) {
-		// Create an account
-		accountV1.Domain.ID = "123-controller-fetch-all-accounts"
-		accountController.CreateAccountHandler(givenAccountRequest(accountV1))
-		accountV1.Environment = "staging"
-		accountController.CreateAccountHandler(givenAccountRequest(accountV1))
+		// Create accounts
+		domainId := "123-controller-fetch-all-accounts"
+
+		account1 := accountV1
+		account1.Domain.ID = domainId
+		account1.Environment = "default"
+		accountController.accountRepository.Create(&account1)
+
+		account2 := accountV1
+		account2.Domain.ID = domainId
+		account2.Environment = "staging"
+		accountController.accountRepository.Create(&account2)
 
 		// Test
 		payload := []byte("")
-		req, _ := http.NewRequest(http.MethodGet, accountController.routeAccountPath+"/"+accountV1.Domain.ID, bytes.NewBuffer(payload))
+		req, _ := http.NewRequest(http.MethodGet, accountController.routeAccountPath+"/"+domainId, bytes.NewBuffer(payload))
 		response := executeRequest(req, r, token)
 
 		// Assert
@@ -141,9 +148,10 @@ func TestUpdateAccountHandler(t *testing.T) {
 
 	t.Run("Should update an account", func(t *testing.T) {
 		// Create an account
-		accountV1.Domain.ID = "123-controller-update-account"
-		accountV1.Environment = "default"
-		accountController.CreateAccountHandler(givenAccountRequest(accountV1))
+		account1 := accountV1
+		account1.Domain.ID = "123-controller-update-account"
+		account1.Environment = "default"
+		accountController.accountRepository.Create(&account1)
 
 		// Update the account
 		accountV2.Domain.ID = "123-controller-update-account"
@@ -162,19 +170,20 @@ func TestUpdateAccountHandler(t *testing.T) {
 		assert.Equal(t, http.StatusOK, response.Code)
 		assert.Nil(t, err)
 		assert.NotEmpty(t, accountResponse.Token)
-		assert.Equal(t, accountV1.Repository, accountResponse.Repository)
+		assert.Equal(t, account1.Repository, accountResponse.Repository)
 		assert.Equal(t, model.StatusSynced, accountResponse.Domain.Status)
 		assert.Equal(t, "Updated successfully", accountResponse.Domain.Message)
 		assert.Equal(t, "5m", accountResponse.Settings.Window)
 	})
 
 	t.Run("Should update account token only", func(t *testing.T) {
 		// Create an account
-		accountV1.Domain.ID = "123-controller-update-account-token"
-		accountController.CreateAccountHandler(givenAccountRequest(accountV1))
+		account1 := accountV1
+		account1.Domain.ID = "123-controller-update-account-token"
+		accountController.accountRepository.Create(&account1)
 
 		// Test
-		accountRequest := accountV1
+		accountRequest := account1
 		accountRequest.Token = "new-token"
 
 		payload, _ := json.Marshal(accountRequest)
@@ -187,11 +196,10 @@ func TestUpdateAccountHandler(t *testing.T) {
 
 		assert.Equal(t, http.StatusOK, response.Code)
 		assert.Nil(t, err)
-		assert.Equal(t, accountV1.Repository, accountResponse.Repository)
+		assert.Equal(t, account1.Repository, accountResponse.Repository)
 		assert.Contains(t, accountResponse.Token, "...")
 
-		accountRepository := repository.NewAccountRepositoryMongo(mongoDb)
-		accountFromDb, _ := accountRepository.FetchByAccountId(accountResponse.ID.Hex())
+		accountFromDb, _ := accountController.accountRepository.FetchByAccountId(accountResponse.ID.Hex())
 
 		decryptedToken, _ := utils.Decrypt(accountFromDb.Token, config.GetEnv("GIT_TOKEN_PRIVATE_KEY"))
 		assert.Equal(t, "new-token", decryptedToken)
@@ -210,14 +218,15 @@ func TestUpdateAccountHandler(t *testing.T) {
 
 	t.Run("Should not update an account - not found", func(t *testing.T) {
 		// Create an account
-		accountV1.Domain.ID = "123-controller-update-account-not-found"
-		accountController.CreateAccountHandler(givenAccountRequest(accountV1))
+		account1 := accountV1
+		account1.Domain.ID = "123-controller-update-account-not-found"
+		accountController.accountRepository.Create(&account1)
 
 		// Replace the domain ID to force an error
-		accountV1.Domain.ID = "111"
+		account1.Domain.ID = "111"
 
 		// Test
-		payload, _ := json.Marshal(accountV1)
+		payload, _ := json.Marshal(account1)
 		req, _ := http.NewRequest(http.MethodPut, accountController.routeAccountPath, bytes.NewBuffer(payload))
 		response := executeRequest(req, r, token)
 
@@ -231,24 +240,26 @@ func TestUpdateAccountTokensHandler(t *testing.T) {
 	token := generateToken("test", time.Minute)
 
 	t.Run("Should update account tokens", func(t *testing.T) {
+		domainId := "123-controller-update-account-tokens"
+
 		// Create accounts
 		account1 := accountV1
-		account1.Domain.ID = "123-controller-update-account-tokens"
+		account1.Domain.ID = domainId
 		account1.Environment = "default"
-		accountController.CreateAccountHandler(givenAccountRequest(account1))
+		accountController.accountRepository.Create(&account1)
 
 		account2 := accountV1
-		account2.Domain.ID = "123-controller-update-account-tokens"
+		account2.Domain.ID = domainId
 		account2.Environment = "staging"
-		accountController.CreateAccountHandler(givenAccountRequest(account2))
+		accountController.accountRepository.Create(&account2)
 
 		// Test
 		payload, _ := json.Marshal(AccountTokensRequest{
 			Environments: []string{account1.Environment, account2.Environment},
 			Token:        "new-token",
 		})
 
-		req, _ := http.NewRequest(http.MethodPut, accountController.routeAccountPath+"/tokens/"+account1.Domain.ID, bytes.NewBuffer(payload))
+		req, _ := http.NewRequest(http.MethodPut, accountController.routeAccountPath+"/tokens/"+domainId, bytes.NewBuffer(payload))
 		response := executeRequest(req, r, token)
 
 		// Assert
@@ -258,11 +269,10 @@ func TestUpdateAccountTokensHandler(t *testing.T) {
 		assert.Equal(t, http.StatusOK, response.Code)
 		assert.Nil(t, err)
 
-		accountRepository := repository.NewAccountRepositoryMongo(mongoDb)
-		accountFromDb1 := accountRepository.FetchAllByDomainId(account1.Domain.ID)
+		accountFromDb := accountController.accountRepository.FetchAllByDomainId(domainId)
+		decryptedToken1, _ := utils.Decrypt(accountFromDb[0].Token, config.GetEnv("GIT_TOKEN_PRIVATE_KEY"))
+		decryptedToken2, _ := utils.Decrypt(accountFromDb[1].Token, config.GetEnv("GIT_TOKEN_PRIVATE_KEY"))
 
-		decryptedToken1, _ := utils.Decrypt(accountFromDb1[0].Token, config.GetEnv("GIT_TOKEN_PRIVATE_KEY"))
-		decryptedToken2, _ := utils.Decrypt(accountFromDb1[1].Token, config.GetEnv("GIT_TOKEN_PRIVATE_KEY"))
 		assert.Equal(t, "new-token", decryptedToken1)
 		assert.Equal(t, "new-token", decryptedToken2)
 	})
@@ -315,11 +325,12 @@ func TestDeleteAccountHandler(t *testing.T) {
 
 	t.Run("Should delete an account by domain ID / environment", func(t *testing.T) {
 		// Create an account
-		accountV1.Domain.ID = "123-controller-delete-account"
-		accountController.CreateAccountHandler(givenAccountRequest(accountV1))
+		account1 := accountV1
+		account1.Domain.ID = "123-controller-delete-account"
+		accountController.accountRepository.Create(&account1)
 
 		// Test
-		req, _ := http.NewRequest(http.MethodDelete, accountController.routeAccountPath+"/"+accountV1.Domain.ID+"/"+accountV1.Environment, nil)
+		req, _ := http.NewRequest(http.MethodDelete, accountController.routeAccountPath+"/"+account1.Domain.ID+"/"+account1.Environment, nil)
 		response := executeRequest(req, r, token)
 
 		// Assert
@@ -381,17 +392,3 @@ func TestUnnauthorizedAccountHandler(t *testing.T) {
 		assert.Equal(t, "{\"error\":\"Invalid token\"}", response.Body.String())
 	})
 }
-
-// Helpers
-
-func givenAccountRequest(data model.Account) (*httptest.ResponseRecorder, *http.Request) {
-	w := httptest.NewRecorder()
-	r := httptest.NewRequest(http.MethodPost, accountController.routeAccountPath, nil)
-
-	// Encode the account request as JSON
-	body, _ := json.Marshal(data)
-	r.Body = io.NopCloser(bytes.NewReader(body))
-	r.Header.Set("Content-Type", "application/json")
-
-	return w, r
-}

src/controller/controller_test.go

@@ -34,6 +34,7 @@ func setup() {
 	os.Setenv("GO_ENV", "test")
 	config.InitEnv()
 	mongoDb = db.InitDb()
+	mongoDb.Drop(context.Background())
 
 	// Init modules
 	accountRepository := repository.NewAccountRepositoryMongo(mongoDb)

src/core/handler.go

@@ -271,5 +271,9 @@ func (c *CoreHandler) updateDomainStatus(account model.Account, status string, m
 	account.Domain.Status = status
 	account.Domain.Message = message
 	account.Domain.LastDate = time.Now().Format(time.ANSIC)
-	c.accountRepository.Update(&account)
+
+	c.accountRepository.UpdateByDomainEnvironment(&model.Account{
+		Environment: account.Environment,
+		Domain:      account.Domain,
+	})
 }

src/repository/account.go

@@ -19,7 +19,7 @@ type AccountRepository interface {
 	FetchByDomainIdEnvironment(domainId string, environment string) (*model.Account, error)
 	FetchAllByDomainId(domainId string) []model.Account
 	FetchAllAccounts() []model.Account
-	Update(account *model.Account) (*model.Account, error)
+	UpdateByDomainEnvironment(account *model.Account) (*model.Account, error)
 	DeleteByAccountId(accountId string) error
 	DeleteByDomainIdEnvironment(domainId string, environment string) error
 }
@@ -49,7 +49,7 @@ func (repo *AccountRepositoryMongo) Create(account *model.Account) (*model.Accou
 	return account, nil
 }
 
-func (repo *AccountRepositoryMongo) Update(account *model.Account) (*model.Account, error) {
+func (repo *AccountRepositoryMongo) UpdateByDomainEnvironment(account *model.Account) (*model.Account, error) {
 	collection, ctx, cancel := getDbContext(repo)
 	defer cancel()
 
@@ -169,24 +169,24 @@ func registerAccountRepositoryValidators(db *mongo.Database) {
 func getUpdateFields(account *model.Account) bson.M {
 	setMap := bson.M{}
 
-	setMap["repository"] = account.Repository
-	setMap["branch"] = account.Branch
-	setMap["path"] = account.Path
-	setMap["environment"] = account.Environment
-	setMap["domain.name"] = account.Domain.Name
-	setMap["settings.active"] = account.Settings.Active
-	setMap["settings.window"] = account.Settings.Window
-	setMap["settings.forcePrune"] = account.Settings.ForcePrune
-
+	setIfNotEmpty(setMap, "environment", account.Environment)
+	setIfNotEmpty(setMap, "repository", account.Repository)
+	setIfNotEmpty(setMap, "branch", account.Branch)
+	setIfNotEmpty(setMap, "path", account.Path)
 	setIfNotEmpty(setMap, "token", account.Token)
+
+	setIfNotEmpty(setMap, "domain.name", account.Domain.Name)
 	setIfNotEmpty(setMap, "domain.version", account.Domain.Version)
 	setIfNotEmpty(setMap, "domain.lastCommit", account.Domain.LastCommit)
 	setIfNotEmpty(setMap, "domain.lastDate", account.Domain.LastDate)
 	setIfNotEmpty(setMap, "domain.status", account.Domain.Status)
 	setIfNotEmpty(setMap, "domain.message", account.Domain.Message)
 
-	update := bson.M{"$set": setMap}
+	setIfNotEmpty(setMap, "settings.window", account.Settings.Window)
+	setIfNotEmpty(setMap, "settings.forcePrune", account.Settings.ForcePrune)
+	setIfNotEmpty(setMap, "settings.active", account.Settings.Active)
 
+	update := bson.M{"$set": setMap}
 	return update
 }