From 933d213e9e4481081de9047cdfddc34d96e4df74 Mon Sep 17 00:00:00 2001
From: vkhoroz <vkhoroz@users.noreply.github.com>
Date: Thu, 9 May 2024 23:04:28 +0300
Subject: [PATCH] A set of device list cleanups (#397)

* Cleanup: pass device filter options as a map

This allows to fully move an argument parsing logic from the client into the command.
Before the change, an argument parsing logic was split between the two.

* Cleanup: use name instead of name_ilike to filter device list

A new API has a newer name argument for few months already.
Use it instead of the obsolete name_ilike.

* Cleanup: use net/url to escape special characters in device list

Currently, we onlhy special-cased the URL escaping of a ? for device name pattern.
But, other characters are not escaped, and uuid pattern is not escaped at all.

Most of the time this just works, but may produce unexpected results.
For example `fioctl devices list 'a*&'` will ignore the trailing ampersand,
and behave just like `fioctl device list 'a*'`, matching unexpected devices.

Using the `net/url` to URL escape the query makes filtering more predictable for such edge cases.

* Cleanup: only pass non-empty filters to device list API

This shortens the URL, making it easier to debug.

Signed-off-by: Volodymyr Khoroz <volodymyr.khoroz@foundries.io>
---
 client/foundries.go         | 33 +++++++++++------------
 subcommands/devices/list.go | 52 ++++++++++++++-----------------------
 2 files changed, 34 insertions(+), 51 deletions(-)

diff --git a/client/foundries.go b/client/foundries.go
index 2ebd0e94..6b9b5a10 100644
--- a/client/foundries.go
+++ b/client/foundries.go
@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	netUrl "net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -780,26 +781,22 @@ func (a *Api) DeviceGet(factory, device string) (*Device, error) {
 	return &d, nil
 }
 
-func (a *Api) DeviceList(
-	mine, prod, nonProd bool, matchTag, byFactory, byGroup, nameIlike, uuid, byTarget, sortBy string, page, limit uint64,
-) (*DeviceList, error) {
-	var (
-		mineInt int
-		prodStr string
-	)
-	if mine {
-		mineInt = 1
+func (a *Api) DeviceList(filterBy map[string]string, sortBy string, page, limit uint64) (*DeviceList, error) {
+	url := a.serverUrl + "/ota/devices/?"
+	query := netUrl.Values{}
+	for key, val := range filterBy {
+		if len(val) > 0 {
+			query.Set(key, val)
+		}
 	}
-	switch {
-	case prod:
-		prodStr = "1"
-	case nonProd:
-		prodStr = "0"
+	if len(sortBy) > 0 {
+		query.Set("sortby", sortBy)
 	}
-	url := a.serverUrl + "/ota/devices/?"
-	url += fmt.Sprintf(
-		"mine=%d&prod=%s&match_tag=%s&name_ilike=%s&factory=%s&uuid=%s&group=%s&target_name=%s&sortby=%s&page=%d&limit=%d",
-		mineInt, prodStr, matchTag, nameIlike, byFactory, uuid, byGroup, byTarget, sortBy, page, limit)
+	if page > 1 {
+		query.Set("page", strconv.FormatUint(page, 10))
+	}
+	query.Set("limit", strconv.FormatUint(limit, 10))
+	url += query.Encode()
 	logrus.Debugf("DeviceList with url: %s", url)
 	return a.DeviceListCont(url)
 }
diff --git a/subcommands/devices/list.go b/subcommands/devices/list.go
index ea3d0d62..6639ab57 100644
--- a/subcommands/devices/list.go
+++ b/subcommands/devices/list.go
@@ -159,22 +159,6 @@ func init() {
 	listCmd.MarkFlagsMutuallyExclusive("sort-by-name", "sort-by-last-seen")
 }
 
-// We allow pattern matching using filepath.Match type * and ?
-// ie * matches everything and ? matches a single character.
-// In sql we need * and ? to maps to % and _ respectively
-// Since _ is a special character we need to escape that. And
-//
-// Soo... a pattern like: H?st_b* would become: H_st\_b%
-// and would match stuff like host_b and hast_FOOO
-func sqlLikeIfy(filePathLike string) string {
-	// %25 = urlencode("%")
-	sql := strings.Replace(filePathLike, "*", "%25", -1)
-	sql = strings.Replace(sql, "_", "\\_", -1)
-	sql = strings.Replace(sql, "?", "_", -1)
-	logrus.Debugf("Converted query(%s) -> %s", filePathLike, sql)
-	return sql
-}
-
 func assertPagination() {
 	// hack until: https://github.com/spf13/pflag/issues/236
 	for _, x := range paginationLimits {
@@ -220,24 +204,26 @@ func doList(cmd *cobra.Command, args []string) {
 	sortBy = appendSortFlagValue(sortBy, cmd, "sort-by-last-seen", "last_seen")
 	sortBy = appendSortFlagValue(sortBy, cmd, "sort-by-name", "name")
 
-	name_ilike := ""
+	filterBy := map[string]string{
+		"factory":     factory,
+		"group":       deviceByGroup,
+		"match_tag":   deviceByTag,
+		"target_name": deviceByTarget,
+		"uuid":        deviceUuid,
+	}
 	if len(args) == 1 {
-		name_ilike = sqlLikeIfy(args[0])
-	}
-	dl, err := api.DeviceList(
-		deviceMine,
-		deviceOnlyProd,
-		deviceOnlyNonProd,
-		deviceByTag,
-		factory,
-		deviceByGroup,
-		name_ilike,
-		deviceUuid,
-		deviceByTarget,
-		strings.Join(sortBy, ","),
-		showPage,
-		paginationLimit,
-	)
+		filterBy["name"] = args[0]
+	}
+	if deviceMine {
+		filterBy["mine"] = "1"
+	}
+	if deviceOnlyProd {
+		filterBy["prod"] = "1"
+	} else if deviceOnlyNonProd {
+		filterBy["prod"] = "0"
+	}
+
+	dl, err := api.DeviceList(filterBy, strings.Join(sortBy, ","), showPage, paginationLimit)
 	subcommands.DieNotNil(err)
 	showDeviceList(dl, showColumns)
 }