Skip to content

Commit

Permalink
Fix broken sorting in list query. Closes #1076.
Browse files Browse the repository at this point in the history
- Replace "%s %s" sprintf substitution in some raw SQL queries with named
  string replace params. Eg: `%order%`.
  • Loading branch information
knadh committed Dec 25, 2022
1 parent 4dee2e9 commit 49f20f3
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 9 deletions.
3 changes: 2 additions & 1 deletion internal/core/bounces.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package core
import (
"fmt"
"net/http"
"strings"

"github.com/knadh/listmonk/models"
"github.com/labstack/echo/v4"
Expand All @@ -22,7 +23,7 @@ func (c *Core) QueryBounces(campID, subID int, source, orderBy, order string, of
}

out := []models.Bounce{}
stmt := fmt.Sprintf(c.q.QueryBounces, orderBy, order)
stmt := strings.ReplaceAll(c.q.QueryBounces, "%order%", orderBy+" "+order)
if err := c.db.Select(&out, stmt, 0, campID, subID, source, offset, limit); err != nil {
c.log.Printf("error fetching bounces: %v", err)
return nil, 0, echo.NewHTTPError(http.StatusInternalServerError,
Expand Down
4 changes: 3 additions & 1 deletion internal/core/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ func makeSearchQuery(searchStr, orderBy, order, query string) (string, string) {
order = SortDesc
}

return searchStr, fmt.Sprintf(query, orderBy, order)
query = strings.ReplaceAll(query, "%order%", orderBy+" "+order)

return searchStr, query
}

// strSliceContains checks if a string is present in the string slice.
Expand Down
4 changes: 3 additions & 1 deletion internal/core/subscribers.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ func (c *Core) QuerySubscribers(query string, listIDs []int, order, orderBy stri

// Run the query again and fetch the actual data. stmt is the raw SQL query.
var out models.Subscribers
stmt = fmt.Sprintf(c.q.QuerySubscribers, cond, orderBy, order)
stmt = strings.ReplaceAll(c.q.QuerySubscribers, "%query%", cond)
stmt = strings.ReplaceAll(stmt, "%order%", orderBy+" "+order)
if err := tx.Select(&out, stmt, pq.Array(listIDs), offset, limit); err != nil {
return nil, 0, echo.NewHTTPError(http.StatusInternalServerError,
c.i18n.Ts("globals.messages.errorFetching", "name", "{globals.terms.subscribers}", "error", pqErrMsg(err)))
Expand Down Expand Up @@ -186,6 +187,7 @@ func (c *Core) ExportSubscribers(query string, subIDs, listIDs []int, batchSize
}

stmt := fmt.Sprintf(c.q.QuerySubscribersForExport, cond)
stmt = strings.ReplaceAll(c.q.QuerySubscribersForExport, "%query%", cond)

// Verify that the arbitrary SQL search expression is read only.
if cond != "" {
Expand Down
13 changes: 7 additions & 6 deletions queries.sql
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,8 @@ SELECT subscribers.* FROM subscribers
AND subscriber_lists.subscriber_id = subscribers.id
)
WHERE (CARDINALITY($1) = 0 OR subscriber_lists.list_id = ANY($1::INT[]))
%s
ORDER BY %s %s OFFSET $2 LIMIT (CASE WHEN $3 < 1 THEN NULL ELSE $3 END);
%query%
ORDER BY %order% OFFSET $2 LIMIT (CASE WHEN $3 < 1 THEN NULL ELSE $3 END);

-- name: query-subscribers-count
-- Replica of query-subscribers for obtaining the results count.
Expand Down Expand Up @@ -331,7 +331,7 @@ SELECT subscribers.id,
)
WHERE sl.list_id = ALL($1::INT[]) AND id > $2
AND (CASE WHEN CARDINALITY($3::INT[]) > 0 THEN id=ANY($3) ELSE true END)
%s
%query%
ORDER BY subscribers.id ASC LIMIT (CASE WHEN $4 < 1 THEN NULL ELSE $4 END);

-- name: query-subscribers-template
Expand Down Expand Up @@ -403,6 +403,7 @@ WITH ls AS (
WHEN $3 != '' THEN to_tsvector(name) @@ to_tsquery ($3)
ELSE true
END
ORDER BY %order%
OFFSET $4 LIMIT (CASE WHEN $5 < 1 THEN NULL ELSE $5 END)
),
counts AS (
Expand All @@ -413,7 +414,7 @@ counts AS (
) row GROUP BY list_id
)
SELECT ls.*, subscriber_statuses FROM ls
LEFT JOIN counts ON (counts.list_id = ls.id) ORDER BY %s %s;
LEFT JOIN counts ON (counts.list_id = ls.id) ORDER BY %order%;


-- name: get-lists-by-optin
Expand Down Expand Up @@ -504,7 +505,7 @@ FROM campaigns c
WHERE ($1 = 0 OR id = $1)
AND status=ANY(CASE WHEN CARDINALITY($2::campaign_status[]) != 0 THEN $2::campaign_status[] ELSE ARRAY[status] END)
AND ($3 = '' OR TO_TSVECTOR(CONCAT(name, ' ', subject)) @@ TO_TSQUERY($3))
ORDER BY %s %s OFFSET $4 LIMIT (CASE WHEN $5 < 1 THEN NULL ELSE $5 END);
ORDER BY %order% OFFSET $4 LIMIT (CASE WHEN $5 < 1 THEN NULL ELSE $5 END);

-- name: get-campaign
SELECT campaigns.*,
Expand Down Expand Up @@ -1018,7 +1019,7 @@ WHERE ($1 = 0 OR bounces.id = $1)
AND ($2 = 0 OR bounces.campaign_id = $2)
AND ($3 = 0 OR bounces.subscriber_id = $3)
AND ($4 = '' OR bounces.source = $4)
ORDER BY %s %s OFFSET $5 LIMIT $6;
ORDER BY %order% OFFSET $5 LIMIT $6;

-- name: delete-bounces
DELETE FROM bounces WHERE CARDINALITY($1::INT[]) = 0 OR id = ANY($1);
Expand Down

0 comments on commit 49f20f3

Please sign in to comment.