Implement restrictions on default column searching for #268 (#274)

* Implement restrictions on default column searching for #268

* Add better docs for config-set excluded-default-search-columns

* Enable debugging

* Clean up server binaries to avoid wasting disk space

* Add tests

* Swap from configuring excluded columns to configuring included columns, to prep for future changes where we may add support for other default columns

* Reduce gotestsum re-runs since tests are less flaky nowadays

* Fix bug in lib.where(...) function that failed to trim the args list and caused DB query correctness issues

* Disable tmate debugging

* Update goldens

Makefile

@@ -15,7 +15,7 @@ forcetest:			## Force running all tests without a test cache
 	make test
 
 test:				## Run all tests
-	TZ='America/Los_Angeles' HISHTORY_TEST=1 HISHTORY_SKIP_INIT_IMPORT=1 gotestsum --packages ./... --rerun-fails=10 --rerun-fails-max-failures=30 --format testname --jsonfile /tmp/testrun.json --post-run-command "go run client/posttest/main.go export" -- -p 1 -timeout 90m
+	TZ='America/Los_Angeles' HISHTORY_TEST=1 HISHTORY_SKIP_INIT_IMPORT=1 gotestsum --packages ./... --rerun-fails=5 --rerun-fails-max-failures=15 --format testname --jsonfile /tmp/testrun.json --post-run-command "go run client/posttest/main.go export" -- -p 1 -timeout 90m
 
 ftest:				## Run a specific test specified via `make ftest FILTER=TestParam/testTui/color` or `make ftest FILTER=TestImportJson`
 	go clean -testcache

client/cmd/configGet.go

@@ -168,6 +168,19 @@ var getAiCompletionEndpoint = &cobra.Command{
 	},
 }
 
+var getDefaultSearchColumns = &cobra.Command{
+	Use:   "default-search-columns",
+	Short: "Get the list of columns that are used for \"default\" search queries that don't use any search atoms",
+	Run: func(cmd *cobra.Command, args []string) {
+		ctx := hctx.MakeContext()
+		config := hctx.GetConf(ctx)
+		for _, col := range config.DefaultSearchColumns {
+			fmt.Print(col + " ")
+		}
+		fmt.Print("\n")
+	},
+}
+
 func init() {
 	rootCmd.AddCommand(configGetCmd)
 	configGetCmd.AddCommand(getEnableControlRCmd)
@@ -185,6 +198,7 @@ func init() {
 	configGetCmd.AddCommand(getCompactMode)
 	configGetCmd.AddCommand(getLogLevelCmd)
 	configGetCmd.AddCommand(getFullScreenCmd)
+	configGetCmd.AddCommand(getDefaultSearchColumns)
 }
 
 var getLogLevelCmd = &cobra.Command{

client/cmd/configSet.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"log"
 	"os"
+	"slices"
 	"strings"
 
 	"github.com/ddworken/hishtory/client/hctx"
@@ -275,6 +276,24 @@ var setFullScreenCmd = &cobra.Command{
 	},
 }
 
+var setDefaultSearchColumns = &cobra.Command{
+	Use:   "default-search-columns",
+	Short: "Get the list of columns that are used for \"default\" search queries that don't use any search atoms",
+	Long:  "By default hishtory queries are checked against `command`, `current_working_directory`, and `hostname`. This option can be used to exclude `current_working_directory` and/or `hostname` from default search queries. E.g. `hishtory config-set default-search-columns hostname command` would exclude `current_working_directory` from default searches.",
+	Args:  cobra.OnlyValidArgs,
+	// Note: If we are ever adding new arguments to this list, we should consider adding support for this config option in configAdd.go and configDelete.go.
+	ValidArgs: []string{"current_working_directory", "hostname", "command"},
+	Run: func(cmd *cobra.Command, args []string) {
+		ctx := hctx.MakeContext()
+		config := hctx.GetConf(ctx)
+		if !slices.Contains(args, "command") {
+			lib.CheckFatalError(fmt.Errorf("command is a required default search column"))
+		}
+		config.DefaultSearchColumns = args
+		lib.CheckFatalError(hctx.SetConfig(config))
+	},
+}
+
 func init() {
 	rootCmd.AddCommand(configSetCmd)
 	configSetCmd.AddCommand(setEnableControlRCmd)
@@ -291,6 +310,7 @@ func init() {
 	configSetCmd.AddCommand(compactMode)
 	configSetCmd.AddCommand(setLogLevelCmd)
 	configSetCmd.AddCommand(setFullScreenCmd)
+	configSetCmd.AddCommand(setDefaultSearchColumns)
 	setColorSchemeCmd.AddCommand(setColorSchemeSelectedText)
 	setColorSchemeCmd.AddCommand(setColorSchemeSelectedBackground)
 	setColorSchemeCmd.AddCommand(setColorSchemeBorderColor)

client/hctx/hctx.go

@@ -223,6 +223,9 @@ type ClientConfig struct {
 	LogLevel logrus.Level `json:"log_level"`
 	// Whether the TUI should render in full-screen mode
 	FullScreenRendering bool `json:"full_screen_rendering"`
+	// Columns that are used for default searches.
+	// See https://github.com/ddworken/hishtory/issues/268 for context on this.
+	DefaultSearchColumns []string `json:"default_search_columns"`
 }
 
 type ColorScheme struct {
@@ -297,6 +300,9 @@ func GetConfig() (ClientConfig, error) {
 	if config.LogLevel == logrus.Level(0) {
 		config.LogLevel = logrus.InfoLevel
 	}
+	if len(config.DefaultSearchColumns) == 0 {
+		config.DefaultSearchColumns = []string{"command", "current_working_directory", "hostname"}
+	}
 	return config, nil
 }
 

client/integration_test.go

@@ -3534,4 +3534,58 @@ func TestOfflineClient(t *testing.T) {
 	require.Contains(t, err.Error(), "panic: Cannot GetHttpClient() from a hishtory client compiled with the offline tag!")
 }
 
+func TestDefaultSearchColumns(t *testing.T) {
+	markTestForSharding(t, 21)
+	defer testutils.BackupAndRestore(t)()
+	tester := zshTester{}
+	installHishtory(t, tester, "")
+
+	// Disable recording so that all our testing commands don't get recorded
+	_, _ = tester.RunInteractiveShellRelaxed(t, ` hishtory disable`)
+	_, _ = tester.RunInteractiveShellRelaxed(t, ` hishtory config-set enable-control-r true`)
+	tester.RunInteractiveShell(t, ` HISHTORY_REDACT_FORCE=true hishtory redact pipefail`)
+	db := hctx.GetDb(hctx.MakeContext())
+	require.NoError(t, db.Where("true").Delete(&data.HistoryEntry{}).Error)
+
+	// Insert a few hishtory entries that we'll use for testing into an empty DB
+	e1 := testutils.MakeFakeHistoryEntry("echo hi")
+	e1.CurrentWorkingDirectory = "/cwd1/"
+	e1.Hostname = "h1"
+	require.NoError(t, db.Create(e1).Error)
+	e2 := testutils.MakeFakeHistoryEntry("ls")
+	e2.CurrentWorkingDirectory = "/echo/"
+	e2.Hostname = "hi"
+	require.NoError(t, db.Create(e2).Error)
+
+	// Check that by default all columns are included
+	out := tester.RunInteractiveShell(t, ` hishtory export echo | grep -v pipefail`)
+	testutils.CompareGoldens(t, out, "TestDefaultSearchColumns-Default-Echo")
+	out = tester.RunInteractiveShell(t, ` hishtory export hi | grep -v pipefail`)
+	testutils.CompareGoldens(t, out, "TestDefaultSearchColumns-Default-Hi")
+
+	// Update the config value to exclude CWD
+	out = tester.RunInteractiveShell(t, ` hishtory config-get default-search-columns`)
+	require.Equal(t, out, "command current_working_directory hostname \n")
+	tester.RunInteractiveShell(t, ` hishtory config-set default-search-columns 'hostname' 'command'`)
+	out = tester.RunInteractiveShell(t, ` hishtory config-get default-search-columns`)
+	require.Equal(t, out, "hostname command \n")
+
+	// Without CWD included
+	out = tester.RunInteractiveShell(t, ` hishtory export echo | grep -v pipefail`)
+	testutils.CompareGoldens(t, out, "TestDefaultSearchColumns-NoCWD-Echo")
+	out = tester.RunInteractiveShell(t, ` hishtory export hi | grep -v pipefail`)
+	testutils.CompareGoldens(t, out, "TestDefaultSearchColumns-NoCWD-Hi")
+
+	// Update the config value to exclude hostname
+	tester.RunInteractiveShell(t, ` hishtory config-set default-search-columns command`)
+	out = tester.RunInteractiveShell(t, ` hishtory config-get default-search-columns`)
+	require.Equal(t, out, "command \n")
+
+	// Without hostname included
+	out = tester.RunInteractiveShell(t, ` hishtory export echo | grep -v pipefail`)
+	testutils.CompareGoldens(t, out, "TestDefaultSearchColumns-NoCWDHostname-Echo")
+	out = tester.RunInteractiveShell(t, ` hishtory export hi | grep -v pipefail`)
+	testutils.CompareGoldens(t, out, "TestDefaultSearchColumns-NoCWDHostname-Hi")
+}
+
 // TODO: somehow test/confirm that hishtory works even if only bash/only zsh is installed

client/lib/lib.go

@@ -750,17 +750,21 @@ func parseTimeGenerously(input string) (time.Time, error) {
 }
 
 // A wrapper around tx.Where(...) that filters out nil-values
-func where(tx *gorm.DB, s string, v1, v2 any) *gorm.DB {
-	if v1 == nil && v2 == nil {
-		return tx.Where(s)
+func where(tx *gorm.DB, s string, args ...any) *gorm.DB {
+	trimmedArgs := make([]any, 0)
+	foundNil := false
+	for _, v := range args {
+		if v == nil {
+			foundNil = true
+		}
+		if foundNil && v != nil {
+			panic(fmt.Sprintf("Illegal state: args=%#v", args))
+		}
+		if v != nil {
+			trimmedArgs = append(trimmedArgs, v)
+		}
 	}
-	if v1 != nil && v2 == nil {
-		return tx.Where(s, v1)
-	}
-	if v1 != nil && v2 != nil {
-		return tx.Where(s, v1, v2)
-	}
-	panic(fmt.Sprintf("Impossible state: v1=%#v, v2=%#v", v1, v2))
+	return tx.Where(s, trimmedArgs...)
 }
 
 func MakeWhereQueryFromSearch(ctx context.Context, db *gorm.DB, query string) (*gorm.DB, error) {
@@ -780,11 +784,11 @@ func MakeWhereQueryFromSearch(ctx context.Context, db *gorm.DB, query string) (*
 				}
 				tx = where(tx, "NOT "+query, v1, v2)
 			} else {
-				query, v1, v2, v3, err := parseNonAtomizedToken(token[1:])
+				query, v1, v2, v3, err := parseNonAtomizedToken(ctx, token[1:])
 				if err != nil {
 					return nil, err
 				}
-				tx = tx.Where("NOT "+query, v1, v2, v3)
+				tx = where(tx, "NOT "+query, v1, v2, v3)
 			}
 		} else if containsUnescaped(token, ":") {
 			query, v1, v2, err := parseAtomizedToken(ctx, token)
@@ -793,11 +797,11 @@ func MakeWhereQueryFromSearch(ctx context.Context, db *gorm.DB, query string) (*
 			}
 			tx = where(tx, query, v1, v2)
 		} else {
-			query, v1, v2, v3, err := parseNonAtomizedToken(token)
+			query, v1, v2, v3, err := parseNonAtomizedToken(ctx, token)
 			if err != nil {
 				return nil, err
 			}
-			tx = tx.Where(query, v1, v2, v3)
+			tx = where(tx, query, v1, v2, v3)
 		}
 	}
 	return tx, nil
@@ -847,9 +851,36 @@ func retryingSearch(ctx context.Context, db *gorm.DB, query string, limit, offse
 	return historyEntries, nil
 }
 
-func parseNonAtomizedToken(token string) (string, any, any, any, error) {
+func parseNonAtomizedToken(ctx context.Context, token string) (string, any, any, any, error) {
 	wildcardedToken := "%" + unescape(token) + "%"
-	return "(command LIKE ? OR hostname LIKE ? OR current_working_directory LIKE ?)", wildcardedToken, wildcardedToken, wildcardedToken, nil
+	query := "(false "
+	numFilters := 0
+	if slices.Contains(hctx.GetConf(ctx).DefaultSearchColumns, "command") {
+		query += "OR command LIKE ? "
+		numFilters += 1
+	}
+	if slices.Contains(hctx.GetConf(ctx).DefaultSearchColumns, "hostname") {
+		query += "OR hostname LIKE ? "
+		numFilters += 1
+	}
+	if slices.Contains(hctx.GetConf(ctx).DefaultSearchColumns, "current_working_directory") {
+		query += "OR current_working_directory LIKE ? "
+		numFilters += 1
+	}
+	query += ")"
+	var t1 any = nil
+	var t2 any = nil
+	var t3 any = nil
+	if numFilters >= 1 {
+		t1 = wildcardedToken
+	}
+	if numFilters >= 2 {
+		t2 = wildcardedToken
+	}
+	if numFilters >= 3 {
+		t3 = wildcardedToken
+	}
+	return query, t1, t2, t3, nil
 }
 
 func parseAtomizedToken(ctx context.Context, token string) (string, any, any, error) {

client/lib/lib_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/ddworken/hishtory/shared/testutils"
 
 	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
 )
 
 func TestMain(m *testing.M) {
@@ -345,3 +346,63 @@ func TestSplitEscaped(t *testing.T) {
 		}
 	}
 }
+
+func TestParseNonAtomizedToken(t *testing.T) {
+	defer testutils.BackupAndRestore(t)()
+	require.NoError(t, hctx.InitConfig())
+	ctx := hctx.MakeContext()
+
+	// Default
+	q, v1, v2, v3, err := parseNonAtomizedToken(ctx, "echo hello")
+	require.NoError(t, err)
+	require.Equal(t, "(false OR command LIKE ? OR hostname LIKE ? OR current_working_directory LIKE ? )", q)
+	require.Equal(t, v1, "%echo hello%")
+	require.Equal(t, v2, "%echo hello%")
+	require.Equal(t, v3, "%echo hello%")
+
+	// Skipping cwd
+	config := hctx.GetConf(ctx)
+	config.DefaultSearchColumns = []string{"hostname", "command"}
+	q, v1, v2, v3, err = parseNonAtomizedToken(ctx, "echo hello")
+	require.NoError(t, err)
+	require.Equal(t, "(false OR command LIKE ? OR hostname LIKE ? )", q)
+	require.Equal(t, v1, "%echo hello%")
+	require.Equal(t, v2, "%echo hello%")
+	require.Nil(t, v3)
+
+	// Skipping cwd and hostname
+	config.DefaultSearchColumns = []string{"command"}
+	q, v1, v2, v3, err = parseNonAtomizedToken(ctx, "echo hello")
+	require.NoError(t, err)
+	require.Equal(t, "(false OR command LIKE ? )", q)
+	require.Equal(t, v1, "%echo hello%")
+	require.Nil(t, v2)
+	require.Nil(t, v3)
+}
+
+func TestWhere(t *testing.T) {
+	defer testutils.BackupAndRestore(t)()
+	require.NoError(t, hctx.InitConfig())
+	ctx := hctx.MakeContext()
+	db := hctx.GetDb(ctx)
+
+	testcases := []struct {
+		in_query       string
+		in_args        []any
+		expected_query string
+	}{
+		{"exit_code = ?", []any{1}, "SELECT * FROM `history_entries` WHERE exit_code = 1"},
+		{"exit_code = ?", []any{1, nil, nil}, "SELECT * FROM `history_entries` WHERE exit_code = 1"},
+		{"exit_code = ? OR exit_code = ?", []any{1, 2, nil}, "SELECT * FROM `history_entries` WHERE exit_code = 1 OR exit_code = 2"},
+	}
+
+	for _, tc := range testcases {
+		tx := where(db, tc.in_query, tc.in_args...)
+		queryString := tx.ToSQL(func(tx *gorm.DB) *gorm.DB {
+			var entries []data.HistoryEntry
+			return tx.Find(&entries)
+		})
+		require.Equal(t, tc.expected_query, queryString)
+
+	}
+}

client/testdata/TestDefaultSearchColumns-Default-Echo

@@ -0,0 +1,2 @@
+echo hi
+ls

client/testdata/TestDefaultSearchColumns-Default-Hi

@@ -0,0 +1,2 @@
+echo hi
+ls

client/testdata/TestDefaultSearchColumns-NoCWD-Echo

@@ -0,0 +1 @@
+echo hi

client/testdata/TestDefaultSearchColumns-NoCWD-Hi

@@ -0,0 +1,2 @@
+echo hi
+ls

client/testdata/TestDefaultSearchColumns-NoCWDHostname-Echo

@@ -0,0 +1 @@
+echo hi

client/testdata/TestDefaultSearchColumns-NoCWDHostname-Hi

@@ -0,0 +1 @@
+echo hi

client/testdata/TestStatusFullConfig

@@ -68,4 +68,8 @@ Full Config:
 	        - ctrl+right
 	loglevel: info
 	fullscreenrendering: false
+	defaultsearchcolumns:
+	    - command
+	    - current_working_directory
+	    - hostname
 	

shared/testutils/testutils.go

@@ -280,10 +280,14 @@ func RunTestServer() func() {
 		panic(fmt.Errorf("expected server stdout to end with %#v, but it doesn't: %#v", expectedSuffix, stdout.String()))
 	}
 	return func() {
+		// Kill the server process to guarantee the next test can run
 		err := cmd.Process.Kill()
 		if err != nil && err.Error() != "os: process already finished" {
 			panic(fmt.Sprintf("failed to kill server process: %v", err))
 		}
+		// Delete the built server binary to avoid wasting disk space
+		_ = os.Remove(fn)
+		// Now that we've cleaned up, check the output to see if the server had any errors
 		allOutput := stdout.String() + stderr.String()
 		if strings.Contains(allOutput, "failed to") && IsOnline() {
 			panic(fmt.Sprintf("server experienced an error\n\n\nstderr=\n%s\n\n\nstdout=%s", stderr.String(), stdout.String()))