Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix the incorrect parameter order #5918

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Wire.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
4 changes: 4 additions & 0 deletions cmd/external-app/wire.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
6 changes: 4 additions & 2 deletions cmd/external-app/wire_gen.go

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

79 changes: 13 additions & 66 deletions internal/sql/repository/app/AppRepository.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,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)
Expand Down Expand Up @@ -134,35 +135,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) {
Expand Down Expand Up @@ -351,52 +341,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
Expand Down
24 changes: 22 additions & 2 deletions pkg/app/AppCrudOperationService.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -88,6 +89,7 @@ type AppCrudOperationServiceImpl struct {
gitMaterialRepository pipelineConfig.MaterialRepository
installedAppDbService EAMode.InstalledAppDBService
crudOperationServiceConfig *CrudOperationServiceConfig
dbMigration dbMigration.DbMigration
}

func NewAppCrudOperationServiceImpl(appLabelRepository pipelineConfig.AppLabelRepository,
Expand All @@ -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,
Expand All @@ -106,6 +109,7 @@ func NewAppCrudOperationServiceImpl(appLabelRepository pipelineConfig.AppLabelRe
genericNoteService: genericNoteService,
gitMaterialRepository: gitMaterialRepository,
installedAppDbService: installedAppDbService,
dbMigration: dbMigration,
}
crudOperationServiceConfig, err := GetCrudOperationServiceConfig()
if err != nil {
Expand Down Expand Up @@ -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
Expand Down
61 changes: 61 additions & 0 deletions pkg/app/dbMigration/migration.go
Original file line number Diff line number Diff line change
@@ -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
}
2 changes: 1 addition & 1 deletion pkg/auth/user/UserCommonService.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func (impl UserCommonServiceImpl) CreateDefaultPoliciesForAllTypesV2(team, entit
return false, err, nil
}
_, err = impl.userAuthRepository.CreateRole(renderedRole)
if err != nil && strings.Contains("duplicate key value violates unique constraint", err.Error()) {
if err != nil && strings.Contains(err.Error(), "duplicate key value violates unique constraint") {
return false, err, nil
}
return true, nil, renderedPolicyDetails
Expand Down
6 changes: 3 additions & 3 deletions pkg/auth/user/repository/UserAuthRepository.go
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ func (impl UserAuthRepositoryImpl) CreateDefaultPoliciesForAllTypes(team, entity
return false, err, nil
}
_, err = impl.createRole(&roleData, UserId)
if err != nil && strings.Contains("duplicate key value violates unique constraint", err.Error()) {
if err != nil && strings.Contains(err.Error(), "duplicate key value violates unique constraint") {
return false, err, nil
}
err = tx.Commit()
Expand All @@ -494,7 +494,7 @@ func (impl UserAuthRepositoryImpl) CreateRolesWithAccessTypeAndEntity(team, enti
Resource: resource,
}
_, err := impl.createRole(&roleData, UserId)
if err != nil && strings.Contains("duplicate key value violates unique constraint", err.Error()) {
if err != nil && strings.Contains(err.Error(), "duplicate key value violates unique constraint") {
return false, err
}
return true, nil
Expand All @@ -521,7 +521,7 @@ func (impl UserAuthRepositoryImpl) CreateRoleForSuperAdminIfNotExists(tx *pg.Tx,
return false, err
}
_, err = impl.createRole(&roleManagerData, UserId)
if err != nil && strings.Contains("duplicate key value violates unique constraint", err.Error()) {
if err != nil && strings.Contains(err.Error(), "duplicate key value violates unique constraint") {
return false, err
}
}
Expand Down
13 changes: 12 additions & 1 deletion util/rbac/EnforcerUtil.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package rbac

import (
"fmt"
"github.com/devtron-labs/devtron/pkg/app/dbMigration"
"golang.org/x/exp/maps"
"strings"

Expand Down Expand Up @@ -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,
Expand All @@ -104,6 +107,7 @@ func NewEnforcerUtilImpl(logger *zap.SugaredLogger, teamRepository team.TeamRepo
ciPipelineRepository: ciPipelineRepository,
clusterRepository: clusterRepository,
enforcer: enforcer,
dbMigration: dbMigration,
}
}

Expand Down Expand Up @@ -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", "", "", ""), ""
Expand Down
Loading