-
Notifications
You must be signed in to change notification settings - Fork 44
RHINENG-25944: add search to the tags list endpoint #2204
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| package controllers | ||
|
|
||
| import ( | ||
| "app/base/database" | ||
| "app/base/utils" | ||
| "errors" | ||
| "net/http" | ||
|
|
||
| "app/base/database" | ||
| "app/base/utils" | ||
|
|
||
| "app/manager/middlewares" | ||
|
|
||
| "github.com/gin-gonic/gin" | ||
|
|
@@ -48,16 +49,22 @@ var SystemTagsOpts = ListOpts{ | |
| }, | ||
| StableSort: "tag", | ||
| DefaultSort: "tag", | ||
| SearchFields: []string{ | ||
| "sq.tag->>'namespace'", | ||
| "sq.tag->>'key'", | ||
| "sq.tag->>'value'", | ||
|
Comment on lines
+52
to
+55
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a question for my understanding, how did you determine these 3 fields should be searched?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, this is based on how tag searching works (or is intended to work) in Insights 😅 |
||
| }, | ||
| } | ||
|
|
||
| // @Summary Show me systems tags applicable to this application | ||
| // @Description Show me systems tags applicable to this application | ||
| // @ID listSystemTags | ||
| // @Security RhIdentity | ||
| // @Produce json | ||
| // @Param sort query string false "Sort field" Enums(tag, count) | ||
| // @Param limit query int fals "Limit for paging" | ||
| // @Param offset query int false "Offset for paging" | ||
| // @Param sort query string false "Sort field" Enums(tag, count) | ||
| // @Param limit query int false "Limit for paging" | ||
| // @Param search query string false "Find matching text" | ||
| // @Param offset query int false "Offset for paging" | ||
| // @Success 200 {object} SystemTagsResponse | ||
| // @Failure 400 {object} utils.ErrorResponse | ||
| // @Failure 500 {object} utils.ErrorResponse | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Avoid hard-coding the SQL alias in
SearchFieldsto reduce coupling and future breakage riskThese
SearchFieldsare tightly coupled to thesqalias (for example,"sq.tag->>'namespace'"). If the table/CTE alias changes, search will silently break. Either make these expressions independent of the outer alias (if supported), or centralize them in a shared constant/helper used by both the query builder andSearchFieldsso alias updates are done in one place.Suggested implementation:
This change assumes that the query builder / underlying SQL does not require an explicit table/CTE alias prefix for these expressions. If the current implementation does require the alias (e.g. because multiple tables expose a
tagcolumn), you should instead:const systemTagAlias = "sq"or a function that formats search fields for a given alias) in this file or a shared package.SearchFields, so alias changes are centralized.