Skip to content

Commit 1d3b03a

Browse files
moshloopmoshloop
committed
fix(query): address PR review issues and add lenient search mode
- formSeverityQuery returns error instead of "__invalid__" for unknown severity - ApplyClauses returns error and accepts excludeColumns to skip inapplicable columns - FindCatalogInsights/FindCatalogChanges propagate filter parse errors - Remove unsafe type assertion in config_insights clause filtering - timer.Results only called on successful DB query in GetRelatedConfigs - Fix path segment off-by-one in buildConfigTree (use penultimate segment) - Empty config resolution returns empty results instead of global unscoped query - FindConfigAccessByConfigIDs uses large PageSize to avoid truncation - Add Lenient flag to BaseCatalogSearch for global search use cases
1 parent b0f2168 commit 1d3b03a

6 files changed

Lines changed: 150 additions & 82 deletions

File tree

‎query/catalog_search.go‎

Lines changed: 52 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,11 @@ type BaseCatalogSearch struct {
2929
Recursive ChangeRelationDirection `query:"recursive" json:"recursive"`
3030
Soft bool `query:"soft" json:"soft"`
3131

32+
// Lenient silently ignores invalid filters, inapplicable columns,
33+
// and validation errors instead of returning errors.
34+
// Useful for global search where the same filters are applied across different table types.
35+
Lenient bool `query:"lenient" json:"lenient"`
36+
3237
sortOrder string
3338
configIDs []uuid.UUID
3439
FromTime *time.Time `query:"-" json:"-"`
@@ -57,25 +62,39 @@ func (b *BaseCatalogSearch) Validate() error {
5762
if b.From != "" && b.FromTime == nil {
5863
expr, err := datemath.Parse(b.From)
5964
if err != nil {
60-
return fmt.Errorf("invalid 'from' param: %w", err)
65+
if !b.Lenient {
66+
return fmt.Errorf("invalid 'from' param: %w", err)
67+
}
68+
b.From = ""
69+
} else {
70+
t := expr.Time()
71+
b.FromTime = &t
6172
}
62-
t := expr.Time()
63-
b.FromTime = &t
6473
}
6574
if b.To != "" && b.ToTime == nil {
6675
expr, err := datemath.Parse(b.To)
6776
if err != nil {
68-
return fmt.Errorf("invalid 'to' param: %w", err)
77+
if !b.Lenient {
78+
return fmt.Errorf("invalid 'to' param: %w", err)
79+
}
80+
b.To = ""
81+
} else {
82+
t := expr.Time()
83+
b.ToTime = &t
6984
}
70-
t := expr.Time()
71-
b.ToTime = &t
7285
}
7386
if b.FromTime != nil && b.ToTime != nil && !b.FromTime.Before(*b.ToTime) {
74-
return fmt.Errorf("'from' must be before 'to'")
87+
if !b.Lenient {
88+
return fmt.Errorf("'from' must be before 'to'")
89+
}
90+
b.ToTime = nil
7591
}
7692
if b.AgentID != "" {
7793
if _, err := uuid.Parse(b.AgentID); err != nil {
78-
return fmt.Errorf("agent_id(%s) must either be a valid uuid or `local`", b.AgentID)
94+
if !b.Lenient {
95+
return fmt.Errorf("agent_id(%s) must either be a valid uuid or `local`", b.AgentID)
96+
}
97+
b.AgentID = ""
7998
}
8099
}
81100
return nil
@@ -121,31 +140,45 @@ func (b *BaseCatalogSearch) ConfigIDs() []uuid.UUID {
121140
return b.configIDs
122141
}
123142

124-
func (b *BaseCatalogSearch) ApplyClauses() ([]clause.Expression, func(*gorm.DB) *gorm.DB) {
143+
func (b *BaseCatalogSearch) ApplyClauses(excludeColumns ...string) ([]clause.Expression, func(*gorm.DB) *gorm.DB, error) {
144+
excluded := make(map[string]bool, len(excludeColumns))
145+
for _, c := range excludeColumns {
146+
excluded[c] = true
147+
}
148+
125149
var clauses []clause.Expression
126150
var tagsFn func(*gorm.DB) *gorm.DB
127151

128-
if b.AgentID != "" {
129-
if c, err := parseAndBuildFilteringQuery(b.AgentID, "agent_id", false); err == nil {
152+
if b.AgentID != "" && !excluded["agent_id"] {
153+
c, err := parseAndBuildFilteringQuery(b.AgentID, "agent_id", false)
154+
if err != nil && !b.Lenient {
155+
return nil, nil, fmt.Errorf("invalid agent_id filter: %w", err)
156+
} else if err == nil {
130157
clauses = append(clauses, c...)
131158
}
132159
}
133-
if b.ConfigType != "" {
134-
if c, err := parseAndBuildFilteringQuery(b.ConfigType, "type", false); err == nil {
160+
if b.ConfigType != "" && !excluded["type"] {
161+
c, err := parseAndBuildFilteringQuery(b.ConfigType, "type", false)
162+
if err != nil && !b.Lenient {
163+
return nil, nil, fmt.Errorf("invalid config_type filter: %w", err)
164+
} else if err == nil {
135165
clauses = append(clauses, c...)
136166
}
137167
}
138-
if b.FromTime != nil {
168+
if b.FromTime != nil && !excluded["created_at"] {
139169
clauses = append(clauses, clause.Gte{Column: clause.Column{Name: "created_at"}, Value: *b.FromTime})
140170
}
141-
if b.ToTime != nil {
171+
if b.ToTime != nil && !excluded["created_at"] {
142172
clauses = append(clauses, clause.Lte{Column: clause.Column{Name: "created_at"}, Value: *b.ToTime})
143173
}
144-
if !b.IncludeDeletedConfigs {
174+
if !b.IncludeDeletedConfigs && !excluded["deleted_at"] {
145175
clauses = append(clauses, clause.Eq{Column: clause.Column{Name: "deleted_at"}, Value: nil})
146176
}
147-
if b.Tags != "" {
148-
if parsedLabelSelector, err := labels.Parse(b.Tags); err == nil {
177+
if b.Tags != "" && !excluded["tags"] {
178+
parsedLabelSelector, err := labels.Parse(b.Tags)
179+
if err != nil && !b.Lenient {
180+
return nil, nil, fmt.Errorf("invalid tags filter: %w", err)
181+
} else if err == nil {
149182
requirements, _ := parsedLabelSelector.Requirements()
150183
tagsFn = func(db *gorm.DB) *gorm.DB {
151184
for _, r := range requirements {
@@ -155,7 +188,7 @@ func (b *BaseCatalogSearch) ApplyClauses() ([]clause.Expression, func(*gorm.DB)
155188
}
156189
}
157190
}
158-
return clauses, tagsFn
191+
return clauses, tagsFn, nil
159192
}
160193

161194
func (b *BaseCatalogSearch) String() string {

‎query/config_access.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ func FindCatalogAccess(ctx context.Context, req CatalogAccessSearchRequest) (res
2929
if err != nil {
3030
return nil, err
3131
}
32+
if len(configIDs) == 0 && req.CatalogID != "" {
33+
return &CatalogAccessSearchResponse{}, nil
34+
}
3235

3336
var output CatalogAccessSearchResponse
3437
q := ctx.DB().Table("config_access_summary")
@@ -53,7 +56,7 @@ func FindCatalogAccess(ctx context.Context, req CatalogAccessSearchRequest) (res
5356

5457
func FindConfigAccessByConfigIDs(ctx context.Context, configIDs []uuid.UUID) ([]models.ConfigAccessSummary, error) {
5558
resp, err := FindCatalogAccess(ctx, CatalogAccessSearchRequest{
56-
BaseCatalogSearch: BaseCatalogSearch{configIDs: configIDs},
59+
BaseCatalogSearch: BaseCatalogSearch{configIDs: configIDs, PageSize: 10000},
5760
})
5861
if err != nil {
5962
return nil, err

‎query/config_changes.go‎

Lines changed: 52 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -82,27 +82,39 @@ func (t *CatalogChangesSearchRequest) Validate() error {
8282
}
8383

8484
if !lo.Contains(allRecursiveOptions, t.Recursive) {
85-
return fmt.Errorf("'recursive' must be one of %v", allRecursiveOptions)
85+
if !t.Lenient {
86+
return fmt.Errorf("'recursive' must be one of %v", allRecursiveOptions)
87+
}
88+
t.Recursive = CatalogChangeRecursiveDownstream
8689
}
8790

8891
if t.FromInsertedAt != "" {
8992
if expr, err := datemath.Parse(t.FromInsertedAt); err != nil {
90-
return fmt.Errorf("invalid 'from_inserted_at' param: %w", err)
93+
if !t.Lenient {
94+
return fmt.Errorf("invalid 'from_inserted_at' param: %w", err)
95+
}
96+
t.FromInsertedAt = ""
9197
} else {
9298
t.fromInsertedAtParsed = expr.Time()
9399
}
94100
}
95101

96102
if t.ToInsertedAt != "" {
97103
if expr, err := datemath.Parse(t.ToInsertedAt); err != nil {
98-
return fmt.Errorf("invalid 'to_inserted_at' param: %w", err)
104+
if !t.Lenient {
105+
return fmt.Errorf("invalid 'to_inserted_at' param: %w", err)
106+
}
107+
t.ToInsertedAt = ""
99108
} else {
100109
t.toInsertedAtParsed = expr.Time()
101110
}
102111
}
103112

104113
if !t.fromInsertedAtParsed.IsZero() && !t.toInsertedAtParsed.IsZero() && !t.fromInsertedAtParsed.Before(t.toInsertedAtParsed) {
105-
return fmt.Errorf("'from_inserted_at' must be before 'to_inserted_at'")
114+
if !t.Lenient {
115+
return fmt.Errorf("'from_inserted_at' must be before 'to_inserted_at'")
116+
}
117+
t.toInsertedAtParsed = time.Time{}
106118
}
107119

108120
if t.SortBy != "" {
@@ -112,7 +124,10 @@ func (t *CatalogChangesSearchRequest) Validate() error {
112124
}
113125

114126
if !lo.Contains(allowedConfigChangesSortColumns, t.SortBy) {
115-
return fmt.Errorf("invalid 'sort_by' param: %s. allowed sort fields are: %s", t.SortBy, strings.Join(allowedConfigChangesSortColumns, ", "))
127+
if !t.Lenient {
128+
return fmt.Errorf("invalid 'sort_by' param: %s. allowed sort fields are: %s", t.SortBy, strings.Join(allowedConfigChangesSortColumns, ", "))
129+
}
130+
t.SortBy = ""
116131
}
117132
}
118133

@@ -166,9 +181,9 @@ func (t *CatalogChangesSearchResponse) Summarize() {
166181
}
167182
}
168183

169-
func formSeverityQuery(severity string) string {
184+
func formSeverityQuery(severity string) (string, error) {
170185
if strings.HasPrefix(severity, "!") {
171-
return severity
186+
return severity, nil
172187
}
173188

174189
severities := []models.Severity{
@@ -180,20 +195,14 @@ func formSeverityQuery(severity string) string {
180195
}
181196

182197
var applicable []string
183-
found := false
184198
for _, s := range severities {
185199
applicable = append(applicable, string(s))
186200
if string(s) == severity {
187-
found = true
188-
break
201+
return strings.Join(applicable, ","), nil
189202
}
190203
}
191204

192-
if !found {
193-
return "__invalid__"
194-
}
195-
196-
return strings.Join(applicable, ",")
205+
return "", fmt.Errorf("unknown severity %q", severity)
197206
}
198207

199208
func FindCatalogChanges(ctx context.Context, req CatalogChangesSearchRequest) (result *CatalogChangesSearchResponse, err error) {
@@ -209,8 +218,14 @@ func FindCatalogChanges(ctx context.Context, req CatalogChangesSearchRequest) (r
209218
if err != nil {
210219
return nil, err
211220
}
221+
if len(configIDs) == 0 && req.CatalogID != "" {
222+
return &CatalogChangesSearchResponse{}, nil
223+
}
212224

213-
baseClauses, tagsFn := req.ApplyClauses()
225+
baseClauses, tagsFn, err := req.ApplyClauses()
226+
if err != nil {
227+
return nil, api.Errorf(api.EINVALID, "bad request: %v", err)
228+
}
214229
var clauses []clause.Expression
215230
clauses = append(clauses, baseClauses...)
216231

@@ -220,34 +235,39 @@ func FindCatalogChanges(ctx context.Context, req CatalogChangesSearchRequest) (r
220235
}
221236

222237
if req.ChangeType != "" {
223-
if c, parseErr := parseAndBuildFilteringQuery(req.ChangeType, "change_type", false); parseErr == nil {
224-
clauses = append(clauses, c...)
225-
} else {
238+
if c, parseErr := parseAndBuildFilteringQuery(req.ChangeType, "change_type", false); parseErr != nil && !req.Lenient {
226239
return nil, parseErr
240+
} else if parseErr == nil {
241+
clauses = append(clauses, c...)
227242
}
228243
}
229244

230245
if req.Severity != "" {
231-
if c, parseErr := parseAndBuildFilteringQuery(formSeverityQuery(req.Severity), "severity", false); parseErr == nil {
232-
clauses = append(clauses, c...)
233-
} else {
234-
return nil, api.Errorf(api.EINVALID, "failed to parse severity: %v", parseErr)
246+
severityQuery, err := formSeverityQuery(req.Severity)
247+
if err != nil && !req.Lenient {
248+
return nil, api.Errorf(api.EINVALID, "invalid severity: %v", err)
249+
} else if err == nil {
250+
if c, parseErr := parseAndBuildFilteringQuery(severityQuery, "severity", false); parseErr != nil && !req.Lenient {
251+
return nil, api.Errorf(api.EINVALID, "failed to parse severity: %v", parseErr)
252+
} else if parseErr == nil {
253+
clauses = append(clauses, c...)
254+
}
235255
}
236256
}
237257

238258
if req.Summary != "" {
239-
if c, parseErr := parseAndBuildFilteringQuery(req.Summary, "summary", true); parseErr == nil {
240-
clauses = append(clauses, c...)
241-
} else {
259+
if c, parseErr := parseAndBuildFilteringQuery(req.Summary, "summary", true); parseErr != nil && !req.Lenient {
242260
return nil, api.Errorf(api.EINVALID, "failed to parse summary: %v", parseErr)
261+
} else if parseErr == nil {
262+
clauses = append(clauses, c...)
243263
}
244264
}
245265

246266
if req.Source != "" {
247-
if c, parseErr := parseAndBuildFilteringQuery(req.Source, "source", true); parseErr == nil {
248-
clauses = append(clauses, c...)
249-
} else {
267+
if c, parseErr := parseAndBuildFilteringQuery(req.Source, "source", true); parseErr != nil && !req.Lenient {
250268
return nil, api.Errorf(api.EINVALID, "failed to parse source: %v", parseErr)
269+
} else if parseErr == nil {
270+
clauses = append(clauses, c...)
251271
}
252272
}
253273

@@ -264,10 +284,10 @@ func FindCatalogChanges(ctx context.Context, req CatalogChangesSearchRequest) (r
264284
}
265285

266286
if req.externalCreatedBy != "" {
267-
if c, parseErr := parseAndBuildFilteringQuery(req.externalCreatedBy, "external_created_by", true); parseErr == nil {
268-
clauses = append(clauses, c...)
269-
} else {
287+
if c, parseErr := parseAndBuildFilteringQuery(req.externalCreatedBy, "external_created_by", true); parseErr != nil && !req.Lenient {
270288
return nil, api.Errorf(api.EINVALID, "failed to parse external createdby: %v", parseErr)
289+
} else if parseErr == nil {
290+
clauses = append(clauses, c...)
271291
}
272292
}
273293

0 commit comments

Comments
 (0)