From 0c7fc3fdf668e507bf60c3c6f22bfb9dcb773e97 Mon Sep 17 00:00:00 2001 From: mayank-devtron Date: Mon, 16 Mar 2026 13:17:23 +0530 Subject: [PATCH 1/6] feat: update app listing tag filter payload and operators (cherry picked from commit a2866a1b5f54f7a5e6f94d73c6e3011ffabd789d) --- .../app/appList/AppListingRestHandler.go | 7 + .../AppListingRepositoryQueryBuilder.go | 128 +++++++++++++++++- ...RepositoryQueryBuilder_tag_filters_test.go | 75 ++++++++++ pkg/app/AppListingService.go | 65 +++++++-- pkg/app/AppListingService_tag_filter_test.go | 85 ++++++++++++ .../35604400_app_label_filter_index.down.sql | 5 + .../35604400_app_label_filter_index.up.sql | 9 ++ 7 files changed, 358 insertions(+), 16 deletions(-) create mode 100644 internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go create mode 100644 pkg/app/AppListingService_tag_filter_test.go create mode 100644 scripts/sql/35604400_app_label_filter_index.down.sql create mode 100644 scripts/sql/35604400_app_label_filter_index.up.sql diff --git a/api/restHandler/app/appList/AppListingRestHandler.go b/api/restHandler/app/appList/AppListingRestHandler.go index 8c15bee342..8b1249932e 100644 --- a/api/restHandler/app/appList/AppListingRestHandler.go +++ b/api/restHandler/app/appList/AppListingRestHandler.go @@ -331,6 +331,13 @@ func (handler AppListingRestHandlerImpl) FetchAppsByEnvironmentV2(w http.Respons common.WriteJsonResp(w, err, nil, http.StatusBadRequest) return } + normalizedTagFilters, err := app.NormalizeAndValidateTagFilters(fetchAppListingRequest.TagFilters) + if err != nil { + handler.logger.Errorw("request err, ValidateTagFilters", "err", err, "payload", fetchAppListingRequest.TagFilters) + common.WriteJsonResp(w, err, nil, http.StatusBadRequest) + return + } + fetchAppListingRequest.TagFilters = normalizedTagFilters newCtx, span = otel.Tracer("fetchAppListingRequest").Start(newCtx, "GetNamespaceClusterMapping") _, _, err = fetchAppListingRequest.GetNamespaceClusterMapping() span.End() diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go index b6b879a6ad..6dc5ec644d 100644 --- a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go @@ -21,6 +21,7 @@ import ( "github.com/devtron-labs/devtron/util" "github.com/go-pg/pg" "go.uber.org/zap" + "strings" ) type AppType int @@ -44,10 +45,11 @@ func NewAppListingRepositoryQueryBuilder(logger *zap.SugaredLogger) AppListingRe } type AppListingFilter struct { - Environments []int `json:"environments"` - Statuses []string `json:"statutes"` - Teams []int `json:"teams"` - AppStatuses []string `json:"appStatuses"` + Environments []int `json:"environments"` + Statuses []string `json:"statutes"` + Teams []int `json:"teams"` + AppStatuses []string `json:"appStatuses"` + TagFilters []TagFilter AppNameSearch string `json:"appNameSearch"` SortOrder SortOrder `json:"sortOrder"` SortBy SortBy `json:"sortBy"` @@ -59,17 +61,53 @@ type AppListingFilter struct { type SortBy string type SortOrder string +type TagFilterOperator string + +// TagFilter holds one row of label filter sent by UI. +// key is always required. +// value is required for EQUALS/DOES_NOT_EQUAL/CONTAINS/DOES_NOT_CONTAIN. +// value must be absent for EXISTS/DOES_NOT_EXIST. +type TagFilter struct { + Key string `json:"key"` + Operator TagFilterOperator `json:"operator"` + Value *string `json:"value"` +} const ( Asc SortOrder = "ASC" Desc SortOrder = "DESC" ) +const ( + TagFilterOperatorEquals TagFilterOperator = "EQUALS" + TagFilterOperatorDoesNotEqual TagFilterOperator = "DOES_NOT_EQUAL" + TagFilterOperatorContains TagFilterOperator = "CONTAINS" + TagFilterOperatorDoesNotContain TagFilterOperator = "DOES_NOT_CONTAIN" + TagFilterOperatorExists TagFilterOperator = "EXISTS" + TagFilterOperatorDoesNotExist TagFilterOperator = "DOES_NOT_EXIST" +) + const ( AppNameSortBy SortBy = "appNameSort" LastDeployedSortBy = "lastDeployedSort" ) +var likePatternEscaper = strings.NewReplacer("\\", "\\\\", "%", "\\%", "_", "\\_") + +func (operator TagFilterOperator) IsValid() bool { + switch operator { + case TagFilterOperatorEquals, + TagFilterOperatorDoesNotEqual, + TagFilterOperatorContains, + TagFilterOperatorDoesNotContain, + TagFilterOperatorExists, + TagFilterOperatorDoesNotExist: + return true + default: + return false + } +} + func (impl AppListingRepositoryQueryBuilder) BuildJobListingQuery(appIDs []int, statuses []string, environmentIds []int, sortOrder string) (string, []interface{}) { var queryParams []interface{} query := `select ci_pipeline.name as ci_pipeline_name,ci_pipeline.id as ci_pipeline_id,app.id as job_id,app.display_name @@ -273,6 +311,18 @@ func (impl AppListingRepositoryQueryBuilder) buildAppListingWhereCondition(appLi whereCondition += " and aps.status IN (?) " queryParams = append(queryParams, pg.In(appStatusExcludingNotDeployed)) } + + // Tag filters are AND-combined for now as requested by product. + // Each row translates to a correlated EXISTS/NOT EXISTS on app_label. + tagWhereCondition, tagQueryParams := impl.buildTagFiltersWhereConditionAND(appListingFilter.TagFilters) + whereCondition += tagWhereCondition + queryParams = append(queryParams, tagQueryParams...) + + // Future OR support placeholder (intentionally disabled today): + // orTagWhereCondition, orTagQueryParams := impl.buildTagFiltersWhereConditionOR(appListingFilter.TagFilters) + // whereCondition += orTagWhereCondition + // queryParams = append(queryParams, orTagQueryParams...) + if len(appListingFilter.AppIds) > 0 { whereCondition += " and a.id IN (?) " queryParams = append(queryParams, pg.In(appListingFilter.AppIds)) @@ -280,6 +330,76 @@ func (impl AppListingRepositoryQueryBuilder) buildAppListingWhereCondition(appLi return whereCondition, queryParams } +func (impl AppListingRepositoryQueryBuilder) buildTagFiltersWhereConditionAND(tagFilters []TagFilter) (string, []interface{}) { + if len(tagFilters) == 0 { + return "", nil + } + var queryBuilder strings.Builder + queryParams := make([]interface{}, 0, len(tagFilters)*2) + for _, tagFilter := range tagFilters { + predicate, predicateParams := impl.buildTagFilterPredicate(tagFilter) + queryBuilder.WriteString(" and ") + queryBuilder.WriteString(predicate) + queryParams = append(queryParams, predicateParams...) + } + return queryBuilder.String(), queryParams +} + +// buildTagFiltersWhereConditionOR is intentionally unused today. +// It is kept as documented dead code so switching to OR in future is straightforward. +func (impl AppListingRepositoryQueryBuilder) buildTagFiltersWhereConditionOR(tagFilters []TagFilter) (string, []interface{}) { + if len(tagFilters) == 0 { + return "", nil + } + clauses := make([]string, 0, len(tagFilters)) + queryParams := make([]interface{}, 0, len(tagFilters)*2) + for _, tagFilter := range tagFilters { + predicate, predicateParams := impl.buildTagFilterPredicate(tagFilter) + clauses = append(clauses, predicate) + queryParams = append(queryParams, predicateParams...) + } + return " and (" + strings.Join(clauses, " OR ") + ") ", queryParams +} + +func (impl AppListingRepositoryQueryBuilder) buildTagFilterPredicate(tagFilter TagFilter) (string, []interface{}) { + value := "" + if tagFilter.Value != nil { + value = *tagFilter.Value + } + switch tagFilter.Operator { + case TagFilterOperatorEquals: + return "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value = ?)", + []interface{}{tagFilter.Key, value} + case TagFilterOperatorDoesNotEqual: + // NOT EXISTS intentionally includes apps where the key is missing. + return "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value = ?)", + []interface{}{tagFilter.Key, value} + case TagFilterOperatorContains: + return "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value LIKE ? ESCAPE '\\')", + []interface{}{tagFilter.Key, buildContainsPattern(value)} + case TagFilterOperatorDoesNotContain: + // NOT EXISTS intentionally includes apps where the key is missing. + return "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value LIKE ? ESCAPE '\\')", + []interface{}{tagFilter.Key, buildContainsPattern(value)} + case TagFilterOperatorExists: + return "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)", + []interface{}{tagFilter.Key} + case TagFilterOperatorDoesNotExist: + return "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)", + []interface{}{tagFilter.Key} + default: + // Invalid operator should never reach here due request validation. + // Returning false condition keeps query safe if validation is bypassed. + return "1 = 0", nil + } +} + +func buildContainsPattern(value string) string { + // Escape SQL LIKE wildcard chars so "contains" behaves like plain substring search. + escaped := likePatternEscaper.Replace(value) + return "%" + escaped + "%" +} + func GetCommaSepratedString[T int | string](request []T) string { respString := "" for i, item := range request { diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go new file mode 100644 index 0000000000..e6de2ffd52 --- /dev/null +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go @@ -0,0 +1,75 @@ +package helper + +import ( + "fmt" + "go.uber.org/zap" + "testing" + + "github.com/stretchr/testify/require" +) + +func stringPointer(value string) *string { + return &value +} + +func TestBuildAppListingWhereCondition_WithTagFiltersAnd(t *testing.T) { + queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) + whereClause, queryParams := queryBuilder.buildAppListingWhereCondition(AppListingFilter{ + TagFilters: []TagFilter{ + {Key: "owner", Operator: TagFilterOperatorEquals, Value: stringPointer("James")}, + {Key: "env", Operator: TagFilterOperatorDoesNotContain, Value: stringPointer("pro_d%")}, + {Key: "team", Operator: TagFilterOperatorExists, Value: nil}, + {Key: "zone", Operator: TagFilterOperatorDoesNotExist, Value: nil}, + }, + }) + + require.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value = ?)") + require.Contains(t, whereClause, "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value LIKE ? ESCAPE '\\')") + require.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)") + require.Contains(t, whereClause, "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)") + require.Len(t, queryParams, 8) + require.Equal(t, true, queryParams[0]) + require.Equal(t, CustomApp, queryParams[1]) + require.Equal(t, "owner", queryParams[2]) + require.Equal(t, "James", queryParams[3]) + require.Equal(t, "env", queryParams[4]) + require.Equal(t, "%pro\\_d\\%%", queryParams[5]) + require.Equal(t, "team", queryParams[6]) + require.Equal(t, "zone", queryParams[7]) +} + +func TestBuildTagFiltersWhereConditionOR(t *testing.T) { + queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) + whereClause, queryParams := queryBuilder.buildTagFiltersWhereConditionOR([]TagFilter{ + {Key: "owner", Operator: TagFilterOperatorEquals, Value: stringPointer("James")}, + {Key: "cost-center", Operator: TagFilterOperatorContains, Value: stringPointer("ENG")}, + }) + + require.Equal(t, " and (EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value = ?) OR EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value LIKE ? ESCAPE '\\')) ", whereClause) + require.Equal(t, []interface{}{"owner", "James", "cost-center", "%ENG%"}, queryParams) +} + +func BenchmarkBuildAppListingQueryWithTagFilters(b *testing.B) { + queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) + tagFilters := make([]TagFilter, 0, 10) + for i := 0; i < 10; i++ { + value := fmt.Sprintf("value-%d", i) + tagFilters = append(tagFilters, TagFilter{ + Key: fmt.Sprintf("key-%d", i), + Operator: TagFilterOperatorContains, + Value: &value, + }) + } + appListingFilter := AppListingFilter{ + TagFilters: tagFilters, + SortBy: AppNameSortBy, + SortOrder: Asc, + Offset: 0, + Size: 20, + } + + b.ReportAllocs() + for i := 0; i < b.N; i++ { + _, _ = queryBuilder.GetAppIdsQueryWithPaginationForAppNameSearch(appListingFilter) + } +} diff --git a/pkg/app/AppListingService.go b/pkg/app/AppListingService.go index 86f84f2ef3..135132734e 100644 --- a/pkg/app/AppListingService.go +++ b/pkg/app/AppListingService.go @@ -82,18 +82,19 @@ const ( ) type FetchAppListingRequest struct { - Environments []int `json:"environments"` - Statuses []string `json:"statuses"` - Teams []int `json:"teams"` - AppNameSearch string `json:"appNameSearch"` - SortOrder helper.SortOrder `json:"sortOrder"` - SortBy helper.SortBy `json:"sortBy"` - Offset int `json:"offset"` - Size int `json:"size"` - DeploymentGroupId int `json:"deploymentGroupId"` - Namespaces []string `json:"namespaces"` // {clusterId}_{namespace} - AppStatuses []string `json:"appStatuses"` - AppIds []int `json:"-"` // internal use only + Environments []int `json:"environments"` + Statuses []string `json:"statuses"` + Teams []int `json:"teams"` + TagFilters []helper.TagFilter `json:"tagFilters"` + AppNameSearch string `json:"appNameSearch"` + SortOrder helper.SortOrder `json:"sortOrder"` + SortBy helper.SortBy `json:"sortBy"` + Offset int `json:"offset"` + Size int `json:"size"` + DeploymentGroupId int `json:"deploymentGroupId"` + Namespaces []string `json:"namespaces"` // {clusterId}_{namespace} + AppStatuses []string `json:"appStatuses"` + AppIds []int `json:"-"` // internal use only // IsClusterOrNamespaceSelected bool `json:"isClusterOrNamespaceSelected"` } type AppNameTypeIdContainer struct { @@ -102,6 +103,40 @@ type AppNameTypeIdContainer struct { AppId int `json:"appId"` } +// NormalizeAndValidateTagFilters validates each tag filter row and normalizes value handling. +// Rules: +// 1) key must be present. +// 2) operator must be one of the supported backend operators. +// 3) for EXISTS/DOES_NOT_EXIST, value must not be sent. +// 4) for EQUALS/DOES_NOT_EQUAL/CONTAINS/DOES_NOT_CONTAIN, value must be non-empty. +func NormalizeAndValidateTagFilters(tagFilters []helper.TagFilter) ([]helper.TagFilter, error) { + if len(tagFilters) == 0 { + return tagFilters, nil + } + normalizedFilters := make([]helper.TagFilter, 0, len(tagFilters)) + for index, tagFilter := range tagFilters { + tagFilter.Key = strings.TrimSpace(tagFilter.Key) + if len(tagFilter.Key) == 0 { + return nil, fmt.Errorf("tagFilters[%d].key is required", index) + } + if !tagFilter.Operator.IsValid() { + return nil, fmt.Errorf("tagFilters[%d].operator is invalid: %s", index, tagFilter.Operator) + } + switch tagFilter.Operator { + case helper.TagFilterOperatorExists, helper.TagFilterOperatorDoesNotExist: + if tagFilter.Value != nil { + return nil, fmt.Errorf("tagFilters[%d].value must be empty for operator %s", index, tagFilter.Operator) + } + default: + if tagFilter.Value == nil || len(*tagFilter.Value) == 0 { + return nil, fmt.Errorf("tagFilters[%d].value is required for operator %s", index, tagFilter.Operator) + } + } + normalizedFilters = append(normalizedFilters, tagFilter) + } + return normalizedFilters, nil +} + func (req FetchAppListingRequest) GetNamespaceClusterMapping() (namespaceClusterPair []*repository2.ClusterNamespacePair, clusterIds []int, err error) { for _, ns := range req.Namespaces { items := strings.Split(ns, "_") @@ -385,6 +420,11 @@ func (impl AppListingServiceImpl) FetchAppsByEnvironmentV2(fetchAppListingReques if len(fetchAppListingRequest.Namespaces) != 0 && len(fetchAppListingRequest.Environments) == 0 { return []*AppView.AppEnvironmentContainer{}, 0, nil } + normalizedTagFilters, err := NormalizeAndValidateTagFilters(fetchAppListingRequest.TagFilters) + if err != nil { + return []*AppView.AppEnvironmentContainer{}, 0, err + } + fetchAppListingRequest.TagFilters = normalizedTagFilters // Currently AppStatus is available in Db for only ArgoApps // We fetch AppStatus on the fly for Helm Apps from scoop, So AppStatus filter will be applied in last @@ -408,6 +448,7 @@ func (impl AppListingServiceImpl) FetchAppsByEnvironmentV2(fetchAppListingReques Size: fetchAppListingRequest.Size, DeploymentGroupId: fetchAppListingRequest.DeploymentGroupId, AppStatuses: fetchAppListingRequest.AppStatuses, + TagFilters: fetchAppListingRequest.TagFilters, AppIds: fetchAppListingRequest.AppIds, } _, span := otel.Tracer("appListingRepository").Start(r.Context(), "FetchAppsByEnvironment") diff --git a/pkg/app/AppListingService_tag_filter_test.go b/pkg/app/AppListingService_tag_filter_test.go new file mode 100644 index 0000000000..824d935e7d --- /dev/null +++ b/pkg/app/AppListingService_tag_filter_test.go @@ -0,0 +1,85 @@ +package app + +import ( + "testing" + + "github.com/devtron-labs/devtron/internal/sql/repository/helper" + "github.com/stretchr/testify/require" +) + +func strPointer(value string) *string { + return &value +} + +func TestNormalizeAndValidateTagFilters_EqualsRequiresValue(t *testing.T) { + _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ + {Key: "owner", Operator: helper.TagFilterOperatorEquals, Value: nil}, + }) + + require.Error(t, err) + require.Equal(t, "tagFilters[0].value is required for operator EQUALS", err.Error()) +} + +func TestNormalizeAndValidateTagFilters_EqualsRejectsEmptyString(t *testing.T) { + _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ + {Key: "owner", Operator: helper.TagFilterOperatorEquals, Value: strPointer("")}, + }) + + require.Error(t, err) + require.Equal(t, "tagFilters[0].value is required for operator EQUALS", err.Error()) +} + +func TestNormalizeAndValidateTagFilters_ContainsRequiresValue(t *testing.T) { + _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ + {Key: "owner", Operator: helper.TagFilterOperatorContains, Value: nil}, + }) + + require.Error(t, err) + require.Equal(t, "tagFilters[0].value is required for operator CONTAINS", err.Error()) +} + +func TestNormalizeAndValidateTagFilters_EmptyKeyReturnsError(t *testing.T) { + _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ + {Key: " ", Operator: helper.TagFilterOperatorEquals, Value: strPointer("James")}, + }) + + require.Error(t, err) + require.Equal(t, "tagFilters[0].key is required", err.Error()) +} + +func TestNormalizeAndValidateTagFilters_InvalidOperatorReturnsError(t *testing.T) { + _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ + {Key: "owner", Operator: helper.TagFilterOperator("INVALID"), Value: strPointer("James")}, + }) + + require.Error(t, err) + require.Equal(t, "tagFilters[0].operator is invalid: INVALID", err.Error()) +} + +func TestNormalizeAndValidateTagFilters_ExistsAllowsNilValueOnly(t *testing.T) { + normalizedFilters, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ + {Key: "owner", Operator: helper.TagFilterOperatorExists, Value: nil}, + }) + + require.NoError(t, err) + require.Len(t, normalizedFilters, 1) + require.Nil(t, normalizedFilters[0].Value) +} + +func TestNormalizeAndValidateTagFilters_ExistsRejectsProvidedValue(t *testing.T) { + _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ + {Key: "owner", Operator: helper.TagFilterOperatorExists, Value: strPointer("James")}, + }) + + require.Error(t, err) + require.Equal(t, "tagFilters[0].value must be empty for operator EXISTS", err.Error()) +} + +func TestNormalizeAndValidateTagFilters_DoesNotExistRejectsProvidedValue(t *testing.T) { + _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ + {Key: "owner", Operator: helper.TagFilterOperatorDoesNotExist, Value: strPointer("")}, + }) + + require.Error(t, err) + require.Equal(t, "tagFilters[0].value must be empty for operator DOES_NOT_EXIST", err.Error()) +} diff --git a/scripts/sql/35604400_app_label_filter_index.down.sql b/scripts/sql/35604400_app_label_filter_index.down.sql new file mode 100644 index 0000000000..f6be7bb1f3 --- /dev/null +++ b/scripts/sql/35604400_app_label_filter_index.down.sql @@ -0,0 +1,5 @@ +/* + * Copyright (c) 2024. Devtron Inc. + */ + +DROP INDEX IF EXISTS idx_app_label_app_id_key_value; diff --git a/scripts/sql/35604400_app_label_filter_index.up.sql b/scripts/sql/35604400_app_label_filter_index.up.sql new file mode 100644 index 0000000000..a4518f30ce --- /dev/null +++ b/scripts/sql/35604400_app_label_filter_index.up.sql @@ -0,0 +1,9 @@ +/* + * Copyright (c) 2024. Devtron Inc. + */ + +-- Index to support app listing tag filters based on app labels. +-- This keeps EXISTS/NOT EXISTS predicates fast for app_id + key lookups, +-- and also helps equality checks on value. +CREATE INDEX IF NOT EXISTS idx_app_label_app_id_key_value + ON public.app_label USING BTREE (app_id, key, value); From e3f8ffa9dbf11625d2a6d3c72ad4560ee6ec1ac4 Mon Sep 17 00:00:00 2001 From: mayank-devtron Date: Mon, 16 Mar 2026 19:20:21 +0530 Subject: [PATCH 2/6] feat: refine tag negative operators behavior (cherry picked from commit 5a5d201c1072b3e147287967ff99666d3fd35122) --- .../AppListingRepositoryQueryBuilder.go | 18 ++++++++--- ...RepositoryQueryBuilder_tag_filters_test.go | 30 ++++++++++++++++++- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go index 6dc5ec644d..1245942fc9 100644 --- a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go @@ -361,6 +361,14 @@ func (impl AppListingRepositoryQueryBuilder) buildTagFiltersWhereConditionOR(tag return " and (" + strings.Join(clauses, " OR ") + ") ", queryParams } +// buildTagFilterPredicate converts one UI tag filter row into a SQL predicate. +// Operator behavior (all case-sensitive): +// - EQUALS: key exists with exact value match. +// - DOES_NOT_EQUAL: key exists with at least one value different from target. +// - CONTAINS: key exists with at least one value containing target substring. +// - DOES_NOT_CONTAIN: key exists with at least one value not containing target substring. +// - EXISTS: key exists. +// - DOES_NOT_EXIST: key does not exist. func (impl AppListingRepositoryQueryBuilder) buildTagFilterPredicate(tagFilter TagFilter) (string, []interface{}) { value := "" if tagFilter.Value != nil { @@ -371,15 +379,17 @@ func (impl AppListingRepositoryQueryBuilder) buildTagFilterPredicate(tagFilter T return "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value = ?)", []interface{}{tagFilter.Key, value} case TagFilterOperatorDoesNotEqual: - // NOT EXISTS intentionally includes apps where the key is missing. - return "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value = ?)", + // Best-practice semantics for multi-value keys: + // include app when key exists and at least one value is different from target. + return "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value <> ?)", []interface{}{tagFilter.Key, value} case TagFilterOperatorContains: return "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value LIKE ? ESCAPE '\\')", []interface{}{tagFilter.Key, buildContainsPattern(value)} case TagFilterOperatorDoesNotContain: - // NOT EXISTS intentionally includes apps where the key is missing. - return "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value LIKE ? ESCAPE '\\')", + // Best-practice semantics for multi-value keys: + // include app when key exists and at least one value does not contain target. + return "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value NOT LIKE ? ESCAPE '\\')", []interface{}{tagFilter.Key, buildContainsPattern(value)} case TagFilterOperatorExists: return "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)", diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go index e6de2ffd52..ceb378c869 100644 --- a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go @@ -24,7 +24,7 @@ func TestBuildAppListingWhereCondition_WithTagFiltersAnd(t *testing.T) { }) require.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value = ?)") - require.Contains(t, whereClause, "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value LIKE ? ESCAPE '\\')") + require.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value NOT LIKE ? ESCAPE '\\')") require.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)") require.Contains(t, whereClause, "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)") require.Len(t, queryParams, 8) @@ -49,6 +49,34 @@ func TestBuildTagFiltersWhereConditionOR(t *testing.T) { require.Equal(t, []interface{}{"owner", "James", "cost-center", "%ENG%"}, queryParams) } +func TestBuildTagFilterPredicate_DoesNotEqualRequiresKeyAndDifferentValue(t *testing.T) { + queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) + value := "mayank" + + predicate, queryParams := queryBuilder.buildTagFilterPredicate(TagFilter{ + Key: "owner", + Operator: TagFilterOperatorDoesNotEqual, + Value: &value, + }) + + require.Equal(t, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value <> ?)", predicate) + require.Equal(t, []interface{}{"owner", "mayank"}, queryParams) +} + +func TestBuildTagFilterPredicate_DoesNotContainRequiresKeyAndNotLike(t *testing.T) { + queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) + value := "may" + + predicate, queryParams := queryBuilder.buildTagFilterPredicate(TagFilter{ + Key: "owner", + Operator: TagFilterOperatorDoesNotContain, + Value: &value, + }) + + require.Equal(t, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value NOT LIKE ? ESCAPE '\\')", predicate) + require.Equal(t, []interface{}{"owner", "%may%"}, queryParams) +} + func BenchmarkBuildAppListingQueryWithTagFilters(b *testing.B) { queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) tagFilters := make([]TagFilter, 0, 10) From 710cd4cbf35632bdac5faf291c4a4a40cceb405a Mon Sep 17 00:00:00 2001 From: mayank-devtron Date: Wed, 18 Mar 2026 09:04:17 +0530 Subject: [PATCH 3/6] jsontag (cherry picked from commit 81341fccad519cabd5b94c9b26e4f19b8a832c2c) --- .../sql/repository/helper/AppListingRepositoryQueryBuilder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go index 1245942fc9..d76106e9c5 100644 --- a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go @@ -49,7 +49,7 @@ type AppListingFilter struct { Statuses []string `json:"statutes"` Teams []int `json:"teams"` AppStatuses []string `json:"appStatuses"` - TagFilters []TagFilter + TagFilters []TagFilter `json:"tagFilters"` AppNameSearch string `json:"appNameSearch"` SortOrder SortOrder `json:"sortOrder"` SortBy SortBy `json:"sortBy"` From 32a2dcca10b2943adf6597673ad0b9ac29696f2b Mon Sep 17 00:00:00 2001 From: mayank-devtron Date: Wed, 18 Mar 2026 09:36:30 +0530 Subject: [PATCH 4/6] migration renaming (cherry picked from commit c2e625e2ea0aa1feb3d655dd8f28fbf28c96c43a) --- .../AppListingRepositoryQueryBuilder.go | 43 +++++-------------- ...RepositoryQueryBuilder_tag_filters_test.go | 37 ---------------- pkg/app/AppListingService.go | 5 --- ... 35504400_app_label_filter_index.down.sql} | 0 ...=> 35504400_app_label_filter_index.up.sql} | 0 5 files changed, 11 insertions(+), 74 deletions(-) rename scripts/sql/{35604400_app_label_filter_index.down.sql => 35504400_app_label_filter_index.down.sql} (100%) rename scripts/sql/{35604400_app_label_filter_index.up.sql => 35504400_app_label_filter_index.up.sql} (100%) diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go index d76106e9c5..ca29c42b28 100644 --- a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go @@ -45,18 +45,18 @@ func NewAppListingRepositoryQueryBuilder(logger *zap.SugaredLogger) AppListingRe } type AppListingFilter struct { - Environments []int `json:"environments"` - Statuses []string `json:"statutes"` - Teams []int `json:"teams"` - AppStatuses []string `json:"appStatuses"` + Environments []int `json:"environments"` + Statuses []string `json:"statutes"` + Teams []int `json:"teams"` + AppStatuses []string `json:"appStatuses"` TagFilters []TagFilter `json:"tagFilters"` - AppNameSearch string `json:"appNameSearch"` - SortOrder SortOrder `json:"sortOrder"` - SortBy SortBy `json:"sortBy"` - Offset int `json:"offset"` - Size int `json:"size"` - DeploymentGroupId int `json:"deploymentGroupId"` - AppIds []int `json:"-"` // internal use only + AppNameSearch string `json:"appNameSearch"` + SortOrder SortOrder `json:"sortOrder"` + SortBy SortBy `json:"sortBy"` + Offset int `json:"offset"` + Size int `json:"size"` + DeploymentGroupId int `json:"deploymentGroupId"` + AppIds []int `json:"-"` // internal use only } type SortBy string @@ -318,11 +318,6 @@ func (impl AppListingRepositoryQueryBuilder) buildAppListingWhereCondition(appLi whereCondition += tagWhereCondition queryParams = append(queryParams, tagQueryParams...) - // Future OR support placeholder (intentionally disabled today): - // orTagWhereCondition, orTagQueryParams := impl.buildTagFiltersWhereConditionOR(appListingFilter.TagFilters) - // whereCondition += orTagWhereCondition - // queryParams = append(queryParams, orTagQueryParams...) - if len(appListingFilter.AppIds) > 0 { whereCondition += " and a.id IN (?) " queryParams = append(queryParams, pg.In(appListingFilter.AppIds)) @@ -345,22 +340,6 @@ func (impl AppListingRepositoryQueryBuilder) buildTagFiltersWhereConditionAND(ta return queryBuilder.String(), queryParams } -// buildTagFiltersWhereConditionOR is intentionally unused today. -// It is kept as documented dead code so switching to OR in future is straightforward. -func (impl AppListingRepositoryQueryBuilder) buildTagFiltersWhereConditionOR(tagFilters []TagFilter) (string, []interface{}) { - if len(tagFilters) == 0 { - return "", nil - } - clauses := make([]string, 0, len(tagFilters)) - queryParams := make([]interface{}, 0, len(tagFilters)*2) - for _, tagFilter := range tagFilters { - predicate, predicateParams := impl.buildTagFilterPredicate(tagFilter) - clauses = append(clauses, predicate) - queryParams = append(queryParams, predicateParams...) - } - return " and (" + strings.Join(clauses, " OR ") + ") ", queryParams -} - // buildTagFilterPredicate converts one UI tag filter row into a SQL predicate. // Operator behavior (all case-sensitive): // - EQUALS: key exists with exact value match. diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go index ceb378c869..5fd270b5af 100644 --- a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go @@ -1,7 +1,6 @@ package helper import ( - "fmt" "go.uber.org/zap" "testing" @@ -38,17 +37,6 @@ func TestBuildAppListingWhereCondition_WithTagFiltersAnd(t *testing.T) { require.Equal(t, "zone", queryParams[7]) } -func TestBuildTagFiltersWhereConditionOR(t *testing.T) { - queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) - whereClause, queryParams := queryBuilder.buildTagFiltersWhereConditionOR([]TagFilter{ - {Key: "owner", Operator: TagFilterOperatorEquals, Value: stringPointer("James")}, - {Key: "cost-center", Operator: TagFilterOperatorContains, Value: stringPointer("ENG")}, - }) - - require.Equal(t, " and (EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value = ?) OR EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value LIKE ? ESCAPE '\\')) ", whereClause) - require.Equal(t, []interface{}{"owner", "James", "cost-center", "%ENG%"}, queryParams) -} - func TestBuildTagFilterPredicate_DoesNotEqualRequiresKeyAndDifferentValue(t *testing.T) { queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) value := "mayank" @@ -76,28 +64,3 @@ func TestBuildTagFilterPredicate_DoesNotContainRequiresKeyAndNotLike(t *testing. require.Equal(t, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value NOT LIKE ? ESCAPE '\\')", predicate) require.Equal(t, []interface{}{"owner", "%may%"}, queryParams) } - -func BenchmarkBuildAppListingQueryWithTagFilters(b *testing.B) { - queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) - tagFilters := make([]TagFilter, 0, 10) - for i := 0; i < 10; i++ { - value := fmt.Sprintf("value-%d", i) - tagFilters = append(tagFilters, TagFilter{ - Key: fmt.Sprintf("key-%d", i), - Operator: TagFilterOperatorContains, - Value: &value, - }) - } - appListingFilter := AppListingFilter{ - TagFilters: tagFilters, - SortBy: AppNameSortBy, - SortOrder: Asc, - Offset: 0, - Size: 20, - } - - b.ReportAllocs() - for i := 0; i < b.N; i++ { - _, _ = queryBuilder.GetAppIdsQueryWithPaginationForAppNameSearch(appListingFilter) - } -} diff --git a/pkg/app/AppListingService.go b/pkg/app/AppListingService.go index 135132734e..ffe078c0d3 100644 --- a/pkg/app/AppListingService.go +++ b/pkg/app/AppListingService.go @@ -420,11 +420,6 @@ func (impl AppListingServiceImpl) FetchAppsByEnvironmentV2(fetchAppListingReques if len(fetchAppListingRequest.Namespaces) != 0 && len(fetchAppListingRequest.Environments) == 0 { return []*AppView.AppEnvironmentContainer{}, 0, nil } - normalizedTagFilters, err := NormalizeAndValidateTagFilters(fetchAppListingRequest.TagFilters) - if err != nil { - return []*AppView.AppEnvironmentContainer{}, 0, err - } - fetchAppListingRequest.TagFilters = normalizedTagFilters // Currently AppStatus is available in Db for only ArgoApps // We fetch AppStatus on the fly for Helm Apps from scoop, So AppStatus filter will be applied in last diff --git a/scripts/sql/35604400_app_label_filter_index.down.sql b/scripts/sql/35504400_app_label_filter_index.down.sql similarity index 100% rename from scripts/sql/35604400_app_label_filter_index.down.sql rename to scripts/sql/35504400_app_label_filter_index.down.sql diff --git a/scripts/sql/35604400_app_label_filter_index.up.sql b/scripts/sql/35504400_app_label_filter_index.up.sql similarity index 100% rename from scripts/sql/35604400_app_label_filter_index.up.sql rename to scripts/sql/35504400_app_label_filter_index.up.sql From 9a381d9d409244c55589bda46cbb004bb91850c8 Mon Sep 17 00:00:00 2001 From: mayank-devtron Date: Wed, 18 Mar 2026 14:08:02 +0530 Subject: [PATCH 5/6] refactor app list tag-filter validation (cherry picked from commit 60ef7488f49db9b56e2f55323066cc8ea8ba2c55) --- .../app/appList/AppListingRestHandler.go | 28 +++++++++-- .../AppListingRepositoryQueryBuilder.go | 4 +- pkg/app/AppListingService.go | 45 ++++++++++++------ pkg/app/AppListingService_tag_filter_test.go | 47 ++++++++++++------- pkg/app/mocks/AppListingService.go | 11 +++++ 5 files changed, 96 insertions(+), 39 deletions(-) diff --git a/api/restHandler/app/appList/AppListingRestHandler.go b/api/restHandler/app/appList/AppListingRestHandler.go index 8b1249932e..a2a800a170 100644 --- a/api/restHandler/app/appList/AppListingRestHandler.go +++ b/api/restHandler/app/appList/AppListingRestHandler.go @@ -56,6 +56,7 @@ import ( "github.com/gorilla/mux" "go.opentelemetry.io/otel" "go.uber.org/zap" + "gopkg.in/go-playground/validator.v9" "net/http" "strconv" "time" @@ -98,6 +99,7 @@ type AppListingRestHandlerImpl struct { k8sApplicationService k8sApplication.K8sApplicationService deploymentConfigService common2.DeploymentConfigService resourceTreeService resourceTree.Service + validator *validator.Validate } type AppStatus struct { @@ -141,6 +143,7 @@ func NewAppListingRestHandlerImpl(appListingService app.AppListingService, k8sApplicationService: k8sApplicationService, deploymentConfigService: deploymentConfigService, resourceTreeService: resourceTreeService, + validator: validator.New(), } return appListingHandler } @@ -276,6 +279,25 @@ func (handler AppListingRestHandlerImpl) FetchJobOverviewCiPipelines(w http.Resp common.WriteJsonResp(w, err, jobCi, http.StatusOK) } +// validateAndNormalizeFetchAppListingRequest applies request-level validation first, +// then tag-filter business validation, and finally normalization. +func (handler AppListingRestHandlerImpl) validateAndNormalizeFetchAppListingRequest(w http.ResponseWriter, r *http.Request, fetchAppListingRequest *app.FetchAppListingRequest) bool { + err := handler.validator.Struct(*fetchAppListingRequest) + if err != nil { + handler.logger.Errorw("validation err, FetchAppsByEnvironment", "err", err, "payload", fetchAppListingRequest) + common.HandleValidationErrors(w, r, err) + return false + } + err = handler.appListingService.ValidateTagFilters(fetchAppListingRequest.TagFilters) + if err != nil { + handler.logger.Errorw("request err, ValidateTagFilters", "err", err, "payload", fetchAppListingRequest.TagFilters) + common.WriteJsonResp(w, err, nil, http.StatusBadRequest) + return false + } + fetchAppListingRequest.TagFilters = handler.appListingService.NormalizeTagFilters(fetchAppListingRequest.TagFilters) + return true +} + func (handler AppListingRestHandlerImpl) FetchAppsByEnvironmentV2(w http.ResponseWriter, r *http.Request) { //Allow CORS here By * or specific origin util3.SetupCorsOriginHeader(&w) @@ -331,13 +353,9 @@ func (handler AppListingRestHandlerImpl) FetchAppsByEnvironmentV2(w http.Respons common.WriteJsonResp(w, err, nil, http.StatusBadRequest) return } - normalizedTagFilters, err := app.NormalizeAndValidateTagFilters(fetchAppListingRequest.TagFilters) - if err != nil { - handler.logger.Errorw("request err, ValidateTagFilters", "err", err, "payload", fetchAppListingRequest.TagFilters) - common.WriteJsonResp(w, err, nil, http.StatusBadRequest) + if !handler.validateAndNormalizeFetchAppListingRequest(w, r, &fetchAppListingRequest) { return } - fetchAppListingRequest.TagFilters = normalizedTagFilters newCtx, span = otel.Tracer("fetchAppListingRequest").Start(newCtx, "GetNamespaceClusterMapping") _, _, err = fetchAppListingRequest.GetNamespaceClusterMapping() span.End() diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go index ca29c42b28..e0c7704825 100644 --- a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go @@ -68,8 +68,8 @@ type TagFilterOperator string // value is required for EQUALS/DOES_NOT_EQUAL/CONTAINS/DOES_NOT_CONTAIN. // value must be absent for EXISTS/DOES_NOT_EXIST. type TagFilter struct { - Key string `json:"key"` - Operator TagFilterOperator `json:"operator"` + Key string `json:"key" validate:"required"` + Operator TagFilterOperator `json:"operator" validate:"required"` Value *string `json:"value"` } diff --git a/pkg/app/AppListingService.go b/pkg/app/AppListingService.go index ffe078c0d3..4fc3312677 100644 --- a/pkg/app/AppListingService.go +++ b/pkg/app/AppListingService.go @@ -70,6 +70,8 @@ type AppListingService interface { ISLastReleaseStopType(appId, envId int) (bool, error) ISLastReleaseStopTypeV2(pipelineIds []int) (map[int]bool, error) GetReleaseCount(appId, envId int) (int, error) + ValidateTagFilters(tagFilters []helper.TagFilter) error + NormalizeTagFilters(tagFilters []helper.TagFilter) []helper.TagFilter FetchAppsByEnvironmentV2(fetchAppListingRequest FetchAppListingRequest, w http.ResponseWriter, r *http.Request, token string) ([]*AppView.AppEnvironmentContainer, int, error) FetchOverviewAppsByEnvironment(envId, limit, offset int) (*OverviewAppsByEnvironmentBean, error) @@ -85,7 +87,7 @@ type FetchAppListingRequest struct { Environments []int `json:"environments"` Statuses []string `json:"statuses"` Teams []int `json:"teams"` - TagFilters []helper.TagFilter `json:"tagFilters"` + TagFilters []helper.TagFilter `json:"tagFilters" validate:"omitempty,dive"` AppNameSearch string `json:"appNameSearch"` SortOrder helper.SortOrder `json:"sortOrder"` SortBy helper.SortBy `json:"sortBy"` @@ -103,38 +105,53 @@ type AppNameTypeIdContainer struct { AppId int `json:"appId"` } -// NormalizeAndValidateTagFilters validates each tag filter row and normalizes value handling. +// ValidateTagFilters validates each tag filter row. // Rules: // 1) key must be present. // 2) operator must be one of the supported backend operators. // 3) for EXISTS/DOES_NOT_EXIST, value must not be sent. // 4) for EQUALS/DOES_NOT_EQUAL/CONTAINS/DOES_NOT_CONTAIN, value must be non-empty. -func NormalizeAndValidateTagFilters(tagFilters []helper.TagFilter) ([]helper.TagFilter, error) { - if len(tagFilters) == 0 { - return tagFilters, nil - } - normalizedFilters := make([]helper.TagFilter, 0, len(tagFilters)) +func ValidateTagFilters(tagFilters []helper.TagFilter) error { for index, tagFilter := range tagFilters { - tagFilter.Key = strings.TrimSpace(tagFilter.Key) - if len(tagFilter.Key) == 0 { - return nil, fmt.Errorf("tagFilters[%d].key is required", index) + if len(strings.TrimSpace(tagFilter.Key)) == 0 { + return fmt.Errorf("tagFilters[%d].key is required", index) } if !tagFilter.Operator.IsValid() { - return nil, fmt.Errorf("tagFilters[%d].operator is invalid: %s", index, tagFilter.Operator) + return fmt.Errorf("tagFilters[%d].operator is invalid: %s", index, tagFilter.Operator) } switch tagFilter.Operator { case helper.TagFilterOperatorExists, helper.TagFilterOperatorDoesNotExist: if tagFilter.Value != nil { - return nil, fmt.Errorf("tagFilters[%d].value must be empty for operator %s", index, tagFilter.Operator) + return fmt.Errorf("tagFilters[%d].value must be empty for operator %s", index, tagFilter.Operator) } default: if tagFilter.Value == nil || len(*tagFilter.Value) == 0 { - return nil, fmt.Errorf("tagFilters[%d].value is required for operator %s", index, tagFilter.Operator) + return fmt.Errorf("tagFilters[%d].value is required for operator %s", index, tagFilter.Operator) } } + } + return nil +} + +// NormalizeTagFilters normalizes tag filter rows after validation. +func NormalizeTagFilters(tagFilters []helper.TagFilter) []helper.TagFilter { + if len(tagFilters) == 0 { + return tagFilters + } + normalizedFilters := make([]helper.TagFilter, 0, len(tagFilters)) + for _, tagFilter := range tagFilters { + tagFilter.Key = strings.TrimSpace(tagFilter.Key) normalizedFilters = append(normalizedFilters, tagFilter) } - return normalizedFilters, nil + return normalizedFilters +} + +func (impl AppListingServiceImpl) ValidateTagFilters(tagFilters []helper.TagFilter) error { + return ValidateTagFilters(tagFilters) +} + +func (impl AppListingServiceImpl) NormalizeTagFilters(tagFilters []helper.TagFilter) []helper.TagFilter { + return NormalizeTagFilters(tagFilters) } func (req FetchAppListingRequest) GetNamespaceClusterMapping() (namespaceClusterPair []*repository2.ClusterNamespacePair, clusterIds []int, err error) { diff --git a/pkg/app/AppListingService_tag_filter_test.go b/pkg/app/AppListingService_tag_filter_test.go index 824d935e7d..3c4a0dfdd5 100644 --- a/pkg/app/AppListingService_tag_filter_test.go +++ b/pkg/app/AppListingService_tag_filter_test.go @@ -11,8 +11,8 @@ func strPointer(value string) *string { return &value } -func TestNormalizeAndValidateTagFilters_EqualsRequiresValue(t *testing.T) { - _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ +func TestValidateTagFilters_EqualsRequiresValue(t *testing.T) { + err := ValidateTagFilters([]helper.TagFilter{ {Key: "owner", Operator: helper.TagFilterOperatorEquals, Value: nil}, }) @@ -20,8 +20,8 @@ func TestNormalizeAndValidateTagFilters_EqualsRequiresValue(t *testing.T) { require.Equal(t, "tagFilters[0].value is required for operator EQUALS", err.Error()) } -func TestNormalizeAndValidateTagFilters_EqualsRejectsEmptyString(t *testing.T) { - _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ +func TestValidateTagFilters_EqualsRejectsEmptyString(t *testing.T) { + err := ValidateTagFilters([]helper.TagFilter{ {Key: "owner", Operator: helper.TagFilterOperatorEquals, Value: strPointer("")}, }) @@ -29,8 +29,8 @@ func TestNormalizeAndValidateTagFilters_EqualsRejectsEmptyString(t *testing.T) { require.Equal(t, "tagFilters[0].value is required for operator EQUALS", err.Error()) } -func TestNormalizeAndValidateTagFilters_ContainsRequiresValue(t *testing.T) { - _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ +func TestValidateTagFilters_ContainsRequiresValue(t *testing.T) { + err := ValidateTagFilters([]helper.TagFilter{ {Key: "owner", Operator: helper.TagFilterOperatorContains, Value: nil}, }) @@ -38,8 +38,8 @@ func TestNormalizeAndValidateTagFilters_ContainsRequiresValue(t *testing.T) { require.Equal(t, "tagFilters[0].value is required for operator CONTAINS", err.Error()) } -func TestNormalizeAndValidateTagFilters_EmptyKeyReturnsError(t *testing.T) { - _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ +func TestValidateTagFilters_EmptyKeyReturnsError(t *testing.T) { + err := ValidateTagFilters([]helper.TagFilter{ {Key: " ", Operator: helper.TagFilterOperatorEquals, Value: strPointer("James")}, }) @@ -47,8 +47,8 @@ func TestNormalizeAndValidateTagFilters_EmptyKeyReturnsError(t *testing.T) { require.Equal(t, "tagFilters[0].key is required", err.Error()) } -func TestNormalizeAndValidateTagFilters_InvalidOperatorReturnsError(t *testing.T) { - _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ +func TestValidateTagFilters_InvalidOperatorReturnsError(t *testing.T) { + err := ValidateTagFilters([]helper.TagFilter{ {Key: "owner", Operator: helper.TagFilterOperator("INVALID"), Value: strPointer("James")}, }) @@ -56,18 +56,16 @@ func TestNormalizeAndValidateTagFilters_InvalidOperatorReturnsError(t *testing.T require.Equal(t, "tagFilters[0].operator is invalid: INVALID", err.Error()) } -func TestNormalizeAndValidateTagFilters_ExistsAllowsNilValueOnly(t *testing.T) { - normalizedFilters, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ +func TestValidateTagFilters_ExistsAllowsNilValueOnly(t *testing.T) { + err := ValidateTagFilters([]helper.TagFilter{ {Key: "owner", Operator: helper.TagFilterOperatorExists, Value: nil}, }) require.NoError(t, err) - require.Len(t, normalizedFilters, 1) - require.Nil(t, normalizedFilters[0].Value) } -func TestNormalizeAndValidateTagFilters_ExistsRejectsProvidedValue(t *testing.T) { - _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ +func TestValidateTagFilters_ExistsRejectsProvidedValue(t *testing.T) { + err := ValidateTagFilters([]helper.TagFilter{ {Key: "owner", Operator: helper.TagFilterOperatorExists, Value: strPointer("James")}, }) @@ -75,11 +73,24 @@ func TestNormalizeAndValidateTagFilters_ExistsRejectsProvidedValue(t *testing.T) require.Equal(t, "tagFilters[0].value must be empty for operator EXISTS", err.Error()) } -func TestNormalizeAndValidateTagFilters_DoesNotExistRejectsProvidedValue(t *testing.T) { - _, err := NormalizeAndValidateTagFilters([]helper.TagFilter{ +func TestValidateTagFilters_DoesNotExistRejectsProvidedValue(t *testing.T) { + err := ValidateTagFilters([]helper.TagFilter{ {Key: "owner", Operator: helper.TagFilterOperatorDoesNotExist, Value: strPointer("")}, }) require.Error(t, err) require.Equal(t, "tagFilters[0].value must be empty for operator DOES_NOT_EXIST", err.Error()) } + +func TestNormalizeTagFilters_TrimsKey(t *testing.T) { + filters := []helper.TagFilter{ + {Key: " owner ", Operator: helper.TagFilterOperatorEquals, Value: strPointer("James")}, + } + + normalizedFilters := NormalizeTagFilters(filters) + + require.Len(t, normalizedFilters, 1) + require.Equal(t, "owner", normalizedFilters[0].Key) + // Ensure input is not modified by normalization. + require.Equal(t, " owner ", filters[0].Key) +} diff --git a/pkg/app/mocks/AppListingService.go b/pkg/app/mocks/AppListingService.go index b321294fa1..6cab7df9c9 100644 --- a/pkg/app/mocks/AppListingService.go +++ b/pkg/app/mocks/AppListingService.go @@ -4,6 +4,7 @@ package mocks import ( bean "github.com/devtron-labs/devtron/api/bean/AppView" + helper "github.com/devtron-labs/devtron/internal/sql/repository/helper" app "github.com/devtron-labs/devtron/pkg/app" context "context" @@ -313,6 +314,16 @@ func (_m *AppListingService) FetchAppsByEnvironmentV2(fetchAppListingRequest app return r0, r1, r2 } +// NormalizeTagFilters provides a mock function with given fields: tagFilters +func (_m *AppListingService) NormalizeTagFilters(tagFilters []helper.TagFilter) []helper.TagFilter { + return app.NormalizeTagFilters(tagFilters) +} + +// ValidateTagFilters provides a mock function with given fields: tagFilters +func (_m *AppListingService) ValidateTagFilters(tagFilters []helper.TagFilter) error { + return app.ValidateTagFilters(tagFilters) +} + // FetchJobs provides a mock function with given fields: fetchJobListingRequest func (_m *AppListingService) FetchJobs(fetchJobListingRequest app.FetchAppListingRequest) ([]*bean.JobContainer, error) { ret := _m.Called(fetchJobListingRequest) From f9a741a0692acdd6142bbb64f6d8e559bb29ed29 Mon Sep 17 00:00:00 2001 From: mayank-devtron Date: Wed, 18 Mar 2026 15:18:42 +0530 Subject: [PATCH 6/6] test: use assert for tag filter tests --- ...RepositoryQueryBuilder_tag_filters_test.go | 36 +++++++++--------- pkg/app/AppListingService_tag_filter_test.go | 38 +++++++++---------- 2 files changed, 37 insertions(+), 37 deletions(-) diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go index 5fd270b5af..0d768da664 100644 --- a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_tag_filters_test.go @@ -4,7 +4,7 @@ import ( "go.uber.org/zap" "testing" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" ) func stringPointer(value string) *string { @@ -22,19 +22,19 @@ func TestBuildAppListingWhereCondition_WithTagFiltersAnd(t *testing.T) { }, }) - require.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value = ?)") - require.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value NOT LIKE ? ESCAPE '\\')") - require.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)") - require.Contains(t, whereClause, "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)") - require.Len(t, queryParams, 8) - require.Equal(t, true, queryParams[0]) - require.Equal(t, CustomApp, queryParams[1]) - require.Equal(t, "owner", queryParams[2]) - require.Equal(t, "James", queryParams[3]) - require.Equal(t, "env", queryParams[4]) - require.Equal(t, "%pro\\_d\\%%", queryParams[5]) - require.Equal(t, "team", queryParams[6]) - require.Equal(t, "zone", queryParams[7]) + assert.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value = ?)") + assert.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value NOT LIKE ? ESCAPE '\\')") + assert.Contains(t, whereClause, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)") + assert.Contains(t, whereClause, "NOT EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ?)") + assert.Len(t, queryParams, 8) + assert.Equal(t, true, queryParams[0]) + assert.Equal(t, CustomApp, queryParams[1]) + assert.Equal(t, "owner", queryParams[2]) + assert.Equal(t, "James", queryParams[3]) + assert.Equal(t, "env", queryParams[4]) + assert.Equal(t, "%pro\\_d\\%%", queryParams[5]) + assert.Equal(t, "team", queryParams[6]) + assert.Equal(t, "zone", queryParams[7]) } func TestBuildTagFilterPredicate_DoesNotEqualRequiresKeyAndDifferentValue(t *testing.T) { @@ -47,8 +47,8 @@ func TestBuildTagFilterPredicate_DoesNotEqualRequiresKeyAndDifferentValue(t *tes Value: &value, }) - require.Equal(t, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value <> ?)", predicate) - require.Equal(t, []interface{}{"owner", "mayank"}, queryParams) + assert.Equal(t, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value <> ?)", predicate) + assert.Equal(t, []interface{}{"owner", "mayank"}, queryParams) } func TestBuildTagFilterPredicate_DoesNotContainRequiresKeyAndNotLike(t *testing.T) { @@ -61,6 +61,6 @@ func TestBuildTagFilterPredicate_DoesNotContainRequiresKeyAndNotLike(t *testing. Value: &value, }) - require.Equal(t, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value NOT LIKE ? ESCAPE '\\')", predicate) - require.Equal(t, []interface{}{"owner", "%may%"}, queryParams) + assert.Equal(t, "EXISTS (SELECT 1 FROM app_label al WHERE al.app_id = a.id and al.key = ? and al.value NOT LIKE ? ESCAPE '\\')", predicate) + assert.Equal(t, []interface{}{"owner", "%may%"}, queryParams) } diff --git a/pkg/app/AppListingService_tag_filter_test.go b/pkg/app/AppListingService_tag_filter_test.go index 3c4a0dfdd5..59b486a384 100644 --- a/pkg/app/AppListingService_tag_filter_test.go +++ b/pkg/app/AppListingService_tag_filter_test.go @@ -4,7 +4,7 @@ import ( "testing" "github.com/devtron-labs/devtron/internal/sql/repository/helper" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" ) func strPointer(value string) *string { @@ -16,8 +16,8 @@ func TestValidateTagFilters_EqualsRequiresValue(t *testing.T) { {Key: "owner", Operator: helper.TagFilterOperatorEquals, Value: nil}, }) - require.Error(t, err) - require.Equal(t, "tagFilters[0].value is required for operator EQUALS", err.Error()) + assert.Error(t, err) + assert.Equal(t, "tagFilters[0].value is required for operator EQUALS", err.Error()) } func TestValidateTagFilters_EqualsRejectsEmptyString(t *testing.T) { @@ -25,8 +25,8 @@ func TestValidateTagFilters_EqualsRejectsEmptyString(t *testing.T) { {Key: "owner", Operator: helper.TagFilterOperatorEquals, Value: strPointer("")}, }) - require.Error(t, err) - require.Equal(t, "tagFilters[0].value is required for operator EQUALS", err.Error()) + assert.Error(t, err) + assert.Equal(t, "tagFilters[0].value is required for operator EQUALS", err.Error()) } func TestValidateTagFilters_ContainsRequiresValue(t *testing.T) { @@ -34,8 +34,8 @@ func TestValidateTagFilters_ContainsRequiresValue(t *testing.T) { {Key: "owner", Operator: helper.TagFilterOperatorContains, Value: nil}, }) - require.Error(t, err) - require.Equal(t, "tagFilters[0].value is required for operator CONTAINS", err.Error()) + assert.Error(t, err) + assert.Equal(t, "tagFilters[0].value is required for operator CONTAINS", err.Error()) } func TestValidateTagFilters_EmptyKeyReturnsError(t *testing.T) { @@ -43,8 +43,8 @@ func TestValidateTagFilters_EmptyKeyReturnsError(t *testing.T) { {Key: " ", Operator: helper.TagFilterOperatorEquals, Value: strPointer("James")}, }) - require.Error(t, err) - require.Equal(t, "tagFilters[0].key is required", err.Error()) + assert.Error(t, err) + assert.Equal(t, "tagFilters[0].key is required", err.Error()) } func TestValidateTagFilters_InvalidOperatorReturnsError(t *testing.T) { @@ -52,8 +52,8 @@ func TestValidateTagFilters_InvalidOperatorReturnsError(t *testing.T) { {Key: "owner", Operator: helper.TagFilterOperator("INVALID"), Value: strPointer("James")}, }) - require.Error(t, err) - require.Equal(t, "tagFilters[0].operator is invalid: INVALID", err.Error()) + assert.Error(t, err) + assert.Equal(t, "tagFilters[0].operator is invalid: INVALID", err.Error()) } func TestValidateTagFilters_ExistsAllowsNilValueOnly(t *testing.T) { @@ -61,7 +61,7 @@ func TestValidateTagFilters_ExistsAllowsNilValueOnly(t *testing.T) { {Key: "owner", Operator: helper.TagFilterOperatorExists, Value: nil}, }) - require.NoError(t, err) + assert.NoError(t, err) } func TestValidateTagFilters_ExistsRejectsProvidedValue(t *testing.T) { @@ -69,8 +69,8 @@ func TestValidateTagFilters_ExistsRejectsProvidedValue(t *testing.T) { {Key: "owner", Operator: helper.TagFilterOperatorExists, Value: strPointer("James")}, }) - require.Error(t, err) - require.Equal(t, "tagFilters[0].value must be empty for operator EXISTS", err.Error()) + assert.Error(t, err) + assert.Equal(t, "tagFilters[0].value must be empty for operator EXISTS", err.Error()) } func TestValidateTagFilters_DoesNotExistRejectsProvidedValue(t *testing.T) { @@ -78,8 +78,8 @@ func TestValidateTagFilters_DoesNotExistRejectsProvidedValue(t *testing.T) { {Key: "owner", Operator: helper.TagFilterOperatorDoesNotExist, Value: strPointer("")}, }) - require.Error(t, err) - require.Equal(t, "tagFilters[0].value must be empty for operator DOES_NOT_EXIST", err.Error()) + assert.Error(t, err) + assert.Equal(t, "tagFilters[0].value must be empty for operator DOES_NOT_EXIST", err.Error()) } func TestNormalizeTagFilters_TrimsKey(t *testing.T) { @@ -89,8 +89,8 @@ func TestNormalizeTagFilters_TrimsKey(t *testing.T) { normalizedFilters := NormalizeTagFilters(filters) - require.Len(t, normalizedFilters, 1) - require.Equal(t, "owner", normalizedFilters[0].Key) + assert.Len(t, normalizedFilters, 1) + assert.Equal(t, "owner", normalizedFilters[0].Key) // Ensure input is not modified by normalization. - require.Equal(t, " owner ", filters[0].Key) + assert.Equal(t, " owner ", filters[0].Key) }