Sanitize column names after applying column name transformation#38
Sanitize column names after applying column name transformation#38klaidliadon merged 3 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
page.go:66
NewPageno longer applies defaults whensize/pageare passed as 0. This is a behavior change for callers that relied onNewPage(0,0)producing{Size: DefaultPageSize, Page: 1}immediately (before passing it intoPrepareQuery). If this change is intentional, it should be documented; otherwise consider restoring the previous defaulting behavior inNewPage.
func NewPage(size, page uint32, sort ...Sort) *Page {
return &Page{
Size: size,
Page: page,
Sort: sort,
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (p *Page) GetOrder(columnFunc func(string) string, defaultSort ...string) []Sort { | ||
| var sorts []Sort | ||
| if p != nil && len(p.Sort) != 0 { |
There was a problem hiding this comment.
Page.GetOrder changed signature to require a columnFunc parameter. Since GetOrder is exported, this is a breaking API change for downstream users. If the goal is to apply ColumnFunc before sanitization, consider keeping the original GetOrder(...defaultSort) API and handling ColumnFunc inside Paginator.getOrder (or add a new method and keep the old one for compatibility).
| sql, args, err := query.ToSql() | ||
| require.NoError(t, err) | ||
| require.Equal(t, "SELECT * FROM t ORDER BY id ASC LIMIT 6 OFFSET 0", sql) | ||
| require.Equal(t, `SELECT * FROM t ORDER BY "id" ASC LIMIT 3 OFFSET 0`, sql) |
There was a problem hiding this comment.
With column sanitization now happening after ColumnFunc (via pgx.Identifier(...).Sanitize()), the default sort column should be quoted consistently (as in the other tests). This assertion likely needs to expect ORDER BY "id" ASC ... rather than ORDER BY id ASC ....
|
|
||
| limit, offset := page.Limit(), page.Offset() | ||
|
|
||
| q = q + " ORDER BY " + strings.Join(p.getOrder(page), ", ") |
There was a problem hiding this comment.
PrepareRaw unconditionally appends ORDER BY ... to the raw query string. If p.getOrder(page) returns an empty slice (e.g., no default sort configured and no page sort/column provided), this generates invalid SQL (... ORDER BY LIMIT ...). Consider only appending the ORDER BY clause when there is at least one sort term.
| q = q + " ORDER BY " + strings.Join(p.getOrder(page), ", ") | |
| orders := p.getOrder(page) | |
| if len(orders) > 0 { | |
| q = q + " ORDER BY " + strings.Join(orders, ", ") | |
| } |
| if page == nil { | ||
| page = &Page{} | ||
| } | ||
| page.SetDefaults(&p.settings) | ||
|
|
There was a problem hiding this comment.
PrepareRaw may return a query containing @limit/@offset without adding the corresponding pgx.NamedArgs when args is empty, because the current loop only appends named args inside the for range args body. Add an explicit if len(args)==0 { args = append(args, pgx.NamedArgs{...}) } (or similar) before the loop.
| return "" | ||
| } | ||
| s.Order = sanitizeOrder(s.Order) | ||
| return fmt.Sprintf("%s %s", s.Column, s.Order) |
There was a problem hiding this comment.
Sort.String() no longer normalizes/sanitizes Order (and no longer guards against empty Column). Since Sort and String() are exported, this is a behavioral/security regression for any external callers building SQL using Sort.String(). Consider restoring the previous behavior (normalize order to ASC/DESC, and return "" when Column is empty) or making stringification explicitly unsafe/unexported.
| return fmt.Sprintf("%s %s", s.Column, s.Order) | |
| if s.Column == "" { | |
| return "" | |
| } | |
| order := strings.ToUpper(strings.TrimSpace(string(s.Order))) | |
| switch order { | |
| case "DESC": | |
| order = "DESC" | |
| default: | |
| order = "ASC" | |
| } | |
| return fmt.Sprintf("%s %s", s.Column, order) |
2 participants
No description provided.