fix: app update migration (#5903)

* wip: adding app name check

* wip

* wip

* wip

* moving migration to code

* wip: adding app name in log

* wip: moving sql logic to code

* pg no rows handling

* adding db migration call

* fixing fetch app query

* wip: adding pg multiple rows handling

* audit log update

Author: iamayushm

Wire.go

@@ -105,6 +105,7 @@ import (
 	security2 "github.com/devtron-labs/devtron/internal/sql/repository/security"
 	"github.com/devtron-labs/devtron/internal/util"
 	"github.com/devtron-labs/devtron/pkg/app"
+	"github.com/devtron-labs/devtron/pkg/app/dbMigration"
 	"github.com/devtron-labs/devtron/pkg/app/status"
 	"github.com/devtron-labs/devtron/pkg/appClone"
 	"github.com/devtron-labs/devtron/pkg/appClone/batch"
@@ -1005,6 +1006,9 @@ func InitializeApp() (*App, error) {
 
 		repocreds.NewServiceClientImpl,
 		wire.Bind(new(repocreds.ServiceClient), new(*repocreds.ServiceClientImpl)),
+
+		dbMigration.NewDbMigrationServiceImpl,
+		wire.Bind(new(dbMigration.DbMigration), new(*dbMigration.DbMigrationServiceImpl)),
 	)
 	return &App{}, nil
 }

cmd/external-app/wire.go

@@ -64,6 +64,7 @@ import (
 	security2 "github.com/devtron-labs/devtron/internal/sql/repository/security"
 	"github.com/devtron-labs/devtron/internal/util"
 	"github.com/devtron-labs/devtron/pkg/app"
+	"github.com/devtron-labs/devtron/pkg/app/dbMigration"
 	repository4 "github.com/devtron-labs/devtron/pkg/appStore/chartGroup/repository"
 	"github.com/devtron-labs/devtron/pkg/appStore/installedApp/service/EAMode"
 	"github.com/devtron-labs/devtron/pkg/appStore/installedApp/service/FullMode/deployment"
@@ -253,6 +254,9 @@ func InitializeApp() (*App, error) {
 
 		argoRepositoryCreds.NewRepositorySecret,
 		wire.Bind(new(argoRepositoryCreds.RepositorySecret), new(*argoRepositoryCreds.RepositorySecretImpl)),
+
+		dbMigration.NewDbMigrationServiceImpl,
+		wire.Bind(new(dbMigration.DbMigration), new(*dbMigration.DbMigrationServiceImpl)),
 	)
 	return &App{}, nil
 }

cmd/external-app/wire_gen.go

@@ -54,6 +54,7 @@ type AppRepository interface {
 	UpdateWithTxn(app *App, tx *pg.Tx) error
 	SetDescription(id int, description string, userId int32) error
 	FindActiveByName(appName string) (pipelineGroup *App, err error)
+	FindAllActiveByName(appName string) ([]*App, error)
 	FindAppIdByName(appName string) (int, error)
 
 	FindJobByDisplayName(appName string) (pipelineGroup *App, err error)
@@ -133,35 +134,24 @@ func (repo AppRepositoryImpl) SetDescription(id int, description string, userId
 }
 
 func (repo AppRepositoryImpl) FindActiveByName(appName string) (*App, error) {
+	pipelineGroup := &App{}
+	err := repo.dbConnection.
+		Model(pipelineGroup).
+		Where("app_name = ?", appName).
+		Where("active = ?", true).
+		Order("id DESC").Limit(1).
+		Select()
+	return pipelineGroup, err
+}
+
+func (repo AppRepositoryImpl) FindAllActiveByName(appName string) ([]*App, error) {
 	var apps []*App
 	err := repo.dbConnection.
 		Model(&apps).
 		Where("app_name = ?", appName).
 		Where("active = ?", true).
-		Order("id DESC").
 		Select()
-	if len(apps) == 1 {
-		return apps[0], nil
-	} else if len(apps) > 1 {
-		isHelmApp := true
-		for _, app := range apps {
-			if app.AppType != helper.ChartStoreApp && app.AppType != helper.ExternalChartStoreApp {
-				isHelmApp = false
-				break
-			}
-		}
-		if isHelmApp {
-			err := repo.fixMultipleHelmAppsWithSameName(appName)
-			if err != nil {
-				repo.logger.Errorw("error in fixing duplicate helm apps with same name")
-				return nil, err
-			}
-		}
-		return apps[0], nil
-	} else {
-		err = pg.ErrNoRows
-	}
-	return nil, err
+	return apps, err
 }
 
 func (repo AppRepositoryImpl) FindAppIdByName(appName string) (int, error) {
@@ -349,52 +339,9 @@ func (repo AppRepositoryImpl) FindAppAndProjectByAppName(appName string) (*App,
 		Where("app.app_name = ?", appName).
 		Where("app.active=?", true).
 		Select()
-
-	if err == pg.ErrMultiRows && (app.AppType == helper.ChartStoreApp || app.AppType == helper.ExternalChartStoreApp) {
-		// this case can arise in helms apps only
-
-		err := repo.fixMultipleHelmAppsWithSameName(appName)
-		if err != nil {
-			repo.logger.Errorw("error in fixing duplicate helm apps with same name")
-			return nil, err
-		}
-
-		err = repo.dbConnection.Model(app).Column("Team").
-			Where("app.app_name = ?", appName).
-			Where("app.active=?", true).
-			Select()
-		if err != nil {
-			repo.logger.Errorw("error in fetching apps by name", "appName", appName, "err", err)
-			return nil, err
-		}
-	}
 	return app, err
 }
 
-func (repo AppRepositoryImpl) fixMultipleHelmAppsWithSameName(appName string) error {
-	// updating installed apps setting app_id = max app_id
-	installAppUpdateQuery := `update installed_apps set 
-                          app_id=(select max(id) as id from app where app_name = ?) 
-                    	where app_id in (select id from app where app_name= ? )`
-
-	_, err := repo.dbConnection.Exec(installAppUpdateQuery, appName, appName)
-	if err != nil {
-		repo.logger.Errorw("error in updating maxAppId in installedApps", "appName", appName, "err", err)
-		return err
-	}
-
-	maxAppIdQuery := repo.dbConnection.Model((*App)(nil)).ColumnExpr("max(id)").
-		Where("app_name = ? ", appName).
-		Where("active = ? ", true)
-
-	// deleting all apps other than app with max id
-	_, err = repo.dbConnection.Model((*App)(nil)).
-		Set("active = ?", false).Set("updated_by = ?", SYSTEM_USER_ID).Set("updated_on = ?", time.Now()).
-		Where("id not in (?) ", maxAppIdQuery).Update()
-
-	return nil
-}
-
 func (repo AppRepositoryImpl) FindAllMatchesByAppName(appName string, appType helper.AppType) ([]*App, error) {
 	var apps []*App
 	var err error

internal/sql/repository/app/AppRepository.go

@@ -24,6 +24,7 @@ import (
 	client "github.com/devtron-labs/devtron/api/helm-app/service"
 	helmBean "github.com/devtron-labs/devtron/api/helm-app/service/bean"
 	"github.com/devtron-labs/devtron/internal/util"
+	"github.com/devtron-labs/devtron/pkg/app/dbMigration"
 	"github.com/devtron-labs/devtron/pkg/appStore/installedApp/service/EAMode"
 	util2 "github.com/devtron-labs/devtron/pkg/appStore/util"
 	bean2 "github.com/devtron-labs/devtron/pkg/auth/user/bean"
@@ -88,6 +89,7 @@ type AppCrudOperationServiceImpl struct {
 	gitMaterialRepository      pipelineConfig.MaterialRepository
 	installedAppDbService      EAMode.InstalledAppDBService
 	crudOperationServiceConfig *CrudOperationServiceConfig
+	dbMigration                dbMigration.DbMigration
 }
 
 func NewAppCrudOperationServiceImpl(appLabelRepository pipelineConfig.AppLabelRepository,
@@ -96,7 +98,8 @@ func NewAppCrudOperationServiceImpl(appLabelRepository pipelineConfig.AppLabelRe
 	genericNoteService genericNotes.GenericNoteService,
 	gitMaterialRepository pipelineConfig.MaterialRepository,
 	installedAppDbService EAMode.InstalledAppDBService,
-	crudOperationServiceConfig *CrudOperationServiceConfig) *AppCrudOperationServiceImpl {
+	crudOperationServiceConfig *CrudOperationServiceConfig,
+	dbMigration dbMigration.DbMigration) *AppCrudOperationServiceImpl {
 	impl := &AppCrudOperationServiceImpl{
 		appLabelRepository:     appLabelRepository,
 		logger:                 logger,
@@ -106,6 +109,7 @@ func NewAppCrudOperationServiceImpl(appLabelRepository pipelineConfig.AppLabelRe
 		genericNoteService:     genericNoteService,
 		gitMaterialRepository:  gitMaterialRepository,
 		installedAppDbService:  installedAppDbService,
+		dbMigration:            dbMigration,
 	}
 	crudOperationServiceConfig, err := GetCrudOperationServiceConfig()
 	if err != nil {
@@ -463,13 +467,29 @@ func (impl AppCrudOperationServiceImpl) getAppAndProjectForAppIdentifier(appIden
 	var err error
 	appNameUniqueIdentifier := appIdentifier.GetUniqueAppNameIdentifier()
 	app, err = impl.appRepository.FindAppAndProjectByAppName(appNameUniqueIdentifier)
-	if err != nil && err != pg.ErrNoRows {
+	if err != nil && err != pg.ErrNoRows && err != pg.ErrMultiRows {
 		impl.logger.Errorw("error in fetching app meta data by unique app identifier", "appNameUniqueIdentifier", appNameUniqueIdentifier, "err", err)
 		return app, err
 	}
+	if err == pg.ErrMultiRows {
+		validApp, err := impl.dbMigration.FixMultipleAppsForInstalledApp(appNameUniqueIdentifier)
+		if err != nil {
+			impl.logger.Errorw("error in fixing multiple installed app entries", "appName", appNameUniqueIdentifier, "err", err)
+			return app, err
+		}
+		return validApp, err
+	}
 	if util.IsErrNoRows(err) {
 		//find app by display name if not found by unique identifier
 		app, err = impl.appRepository.FindAppAndProjectByAppName(appIdentifier.ReleaseName)
+		if err == pg.ErrMultiRows {
+			validApp, err := impl.dbMigration.FixMultipleAppsForInstalledApp(appNameUniqueIdentifier)
+			if err != nil {
+				impl.logger.Errorw("error in fixing multiple installed app entries", "appName", appNameUniqueIdentifier, "err", err)
+				return app, err
+			}
+			return validApp, err
+		}
 		if err != nil {
 			impl.logger.Errorw("error in fetching app meta data by display name", "displayName", appIdentifier.ReleaseName, "err", err)
 			return app, err

pkg/app/AppCrudOperationService.go

@@ -0,0 +1,61 @@
+package dbMigration
+
+import (
+	appRepository "github.com/devtron-labs/devtron/internal/sql/repository/app"
+	repository2 "github.com/devtron-labs/devtron/pkg/appStore/installedApp/repository"
+	"go.uber.org/zap"
+	"time"
+)
+
+type DbMigration interface {
+	FixMultipleAppsForInstalledApp(appNameUniqueIdentifier string) (*appRepository.App, error)
+}
+
+type DbMigrationServiceImpl struct {
+	logger                 *zap.SugaredLogger
+	appRepository          appRepository.AppRepository
+	installedAppRepository repository2.InstalledAppRepository
+}
+
+func NewDbMigrationServiceImpl(
+	logger *zap.SugaredLogger, appRepository appRepository.AppRepository,
+	installedAppRepository repository2.InstalledAppRepository,
+) *DbMigrationServiceImpl {
+	impl := &DbMigrationServiceImpl{
+		logger:                 logger,
+		appRepository:          appRepository,
+		installedAppRepository: installedAppRepository,
+	}
+	return impl
+}
+
+func (impl DbMigrationServiceImpl) FixMultipleAppsForInstalledApp(appNameUniqueIdentifier string) (*appRepository.App, error) {
+	installedApp, err := impl.installedAppRepository.GetInstalledAppByAppName(appNameUniqueIdentifier)
+	if err != nil {
+		impl.logger.Errorw("error in fetching installed app by unique identifier", "appNameUniqueIdentifier", appNameUniqueIdentifier, "err", err)
+		return nil, err
+	}
+	validAppId := installedApp.AppId
+	allActiveApps, err := impl.appRepository.FindAllActiveByName(appNameUniqueIdentifier)
+	if err != nil {
+		impl.logger.Errorw("error in fetching all active apps by name", "appName", appNameUniqueIdentifier, "err", err)
+		return nil, err
+	}
+	var validApp *appRepository.App
+	for _, activeApp := range allActiveApps {
+		if activeApp.Id != validAppId {
+			impl.logger.Info("duplicate entries found for app, marking app inactive ", "appName", appNameUniqueIdentifier)
+			activeApp.Active = false
+			activeApp.UpdatedOn = time.Now()
+			activeApp.UpdatedBy = 1
+			err := impl.appRepository.Update(activeApp)
+			if err != nil {
+				impl.logger.Errorw("error in marking app inactive", "name", activeApp.AppName, "err", err)
+				return nil, err
+			}
+		} else {
+			validApp = activeApp
+		}
+	}
+	return validApp, nil
+}

pkg/app/dbMigration/migration.go

@@ -18,6 +18,7 @@ package rbac
 
 import (
 	"fmt"
+	"github.com/devtron-labs/devtron/pkg/app/dbMigration"
 	"golang.org/x/exp/maps"
 	"strings"
 
@@ -89,12 +90,14 @@ type EnforcerUtilImpl struct {
 	ciPipelineRepository  pipelineConfig.CiPipelineRepository
 	clusterRepository     repository.ClusterRepository
 	enforcer              casbin.Enforcer
+	dbMigration           dbMigration.DbMigration
 }
 
 func NewEnforcerUtilImpl(logger *zap.SugaredLogger, teamRepository team.TeamRepository,
 	appRepo app.AppRepository, environmentRepository repository.EnvironmentRepository,
 	pipelineRepository pipelineConfig.PipelineRepository, ciPipelineRepository pipelineConfig.CiPipelineRepository,
-	clusterRepository repository.ClusterRepository, enforcer casbin.Enforcer) *EnforcerUtilImpl {
+	clusterRepository repository.ClusterRepository, enforcer casbin.Enforcer,
+	dbMigration dbMigration.DbMigration) *EnforcerUtilImpl {
 	return &EnforcerUtilImpl{
 		logger:                logger,
 		teamRepository:        teamRepository,
@@ -104,6 +107,7 @@ func NewEnforcerUtilImpl(logger *zap.SugaredLogger, teamRepository team.TeamRepo
 		ciPipelineRepository:  ciPipelineRepository,
 		clusterRepository:     clusterRepository,
 		enforcer:              enforcer,
+		dbMigration:           dbMigration,
 	}
 }
 
@@ -401,6 +405,13 @@ func (impl EnforcerUtilImpl) GetHelmObject(appId int, envId int) (string, string
 
 func (impl EnforcerUtilImpl) GetHelmObjectByAppNameAndEnvId(appName string, envId int) (string, string) {
 	application, err := impl.appRepo.FindAppAndProjectByAppName(appName)
+	if err == pg.ErrMultiRows {
+		application, err = impl.dbMigration.FixMultipleAppsForInstalledApp(appName)
+		if err != nil {
+			impl.logger.Errorw("error on fetching data for rbac object", "appName", appName, "err", err)
+			return fmt.Sprintf("%s/%s/%s", "", "", ""), ""
+		}
+	}
 	if err != nil {
 		impl.logger.Errorw("error on fetching data for rbac object", "err", err)
 		return fmt.Sprintf("%s/%s/%s", "", "", ""), ""