From 65d1ebfd072b894095a9c03df07a14d415ab34b0 Mon Sep 17 00:00:00 2001 From: David Dworken Date: Sat, 18 Jan 2025 12:25:07 -0700 Subject: [PATCH] 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 | 2 +- client/cmd/configGet.go | 14 +++++ client/cmd/configSet.go | 20 ++++++ client/hctx/hctx.go | 6 ++ client/integration_test.go | 54 ++++++++++++++++ client/lib/lib.go | 63 ++++++++++++++----- client/lib/lib_test.go | 61 ++++++++++++++++++ .../TestDefaultSearchColumns-Default-Echo | 2 + .../TestDefaultSearchColumns-Default-Hi | 2 + .../TestDefaultSearchColumns-NoCWD-Echo | 1 + .../TestDefaultSearchColumns-NoCWD-Hi | 2 + ...estDefaultSearchColumns-NoCWDHostname-Echo | 1 + .../TestDefaultSearchColumns-NoCWDHostname-Hi | 1 + client/testdata/TestStatusFullConfig | 4 ++ shared/testutils/testutils.go | 4 ++ 15 files changed, 220 insertions(+), 17 deletions(-) create mode 100644 client/testdata/TestDefaultSearchColumns-Default-Echo create mode 100644 client/testdata/TestDefaultSearchColumns-Default-Hi create mode 100644 client/testdata/TestDefaultSearchColumns-NoCWD-Echo create mode 100644 client/testdata/TestDefaultSearchColumns-NoCWD-Hi create mode 100644 client/testdata/TestDefaultSearchColumns-NoCWDHostname-Echo create mode 100644 client/testdata/TestDefaultSearchColumns-NoCWDHostname-Hi diff --git a/Makefile b/Makefile index b99cdff..14b4636 100644 --- a/Makefile +++ b/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 diff --git a/client/cmd/configGet.go b/client/cmd/configGet.go index acd21ca..8a6ebc1 100644 --- a/client/cmd/configGet.go +++ b/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{ diff --git a/client/cmd/configSet.go b/client/cmd/configSet.go index d9e2b31..e3be30c 100644 --- a/client/cmd/configSet.go +++ b/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) diff --git a/client/hctx/hctx.go b/client/hctx/hctx.go index 5df7467..cfd7cff 100644 --- a/client/hctx/hctx.go +++ b/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 } diff --git a/client/integration_test.go b/client/integration_test.go index 9975ea7..4caec13 100644 --- a/client/integration_test.go +++ b/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 diff --git a/client/lib/lib.go b/client/lib/lib.go index 42ee4c9..9f49281 100644 --- a/client/lib/lib.go +++ b/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) { diff --git a/client/lib/lib_test.go b/client/lib/lib_test.go index 56200fc..637ba3a 100644 --- a/client/lib/lib_test.go +++ b/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) + + } +} diff --git a/client/testdata/TestDefaultSearchColumns-Default-Echo b/client/testdata/TestDefaultSearchColumns-Default-Echo new file mode 100644 index 0000000..bca0709 --- /dev/null +++ b/client/testdata/TestDefaultSearchColumns-Default-Echo @@ -0,0 +1,2 @@ +echo hi +ls diff --git a/client/testdata/TestDefaultSearchColumns-Default-Hi b/client/testdata/TestDefaultSearchColumns-Default-Hi new file mode 100644 index 0000000..bca0709 --- /dev/null +++ b/client/testdata/TestDefaultSearchColumns-Default-Hi @@ -0,0 +1,2 @@ +echo hi +ls diff --git a/client/testdata/TestDefaultSearchColumns-NoCWD-Echo b/client/testdata/TestDefaultSearchColumns-NoCWD-Echo new file mode 100644 index 0000000..8b2fe54 --- /dev/null +++ b/client/testdata/TestDefaultSearchColumns-NoCWD-Echo @@ -0,0 +1 @@ +echo hi diff --git a/client/testdata/TestDefaultSearchColumns-NoCWD-Hi b/client/testdata/TestDefaultSearchColumns-NoCWD-Hi new file mode 100644 index 0000000..bca0709 --- /dev/null +++ b/client/testdata/TestDefaultSearchColumns-NoCWD-Hi @@ -0,0 +1,2 @@ +echo hi +ls diff --git a/client/testdata/TestDefaultSearchColumns-NoCWDHostname-Echo b/client/testdata/TestDefaultSearchColumns-NoCWDHostname-Echo new file mode 100644 index 0000000..8b2fe54 --- /dev/null +++ b/client/testdata/TestDefaultSearchColumns-NoCWDHostname-Echo @@ -0,0 +1 @@ +echo hi diff --git a/client/testdata/TestDefaultSearchColumns-NoCWDHostname-Hi b/client/testdata/TestDefaultSearchColumns-NoCWDHostname-Hi new file mode 100644 index 0000000..8b2fe54 --- /dev/null +++ b/client/testdata/TestDefaultSearchColumns-NoCWDHostname-Hi @@ -0,0 +1 @@ +echo hi diff --git a/client/testdata/TestStatusFullConfig b/client/testdata/TestStatusFullConfig index e39572c..8be26f1 100644 --- a/client/testdata/TestStatusFullConfig +++ b/client/testdata/TestStatusFullConfig @@ -68,4 +68,8 @@ Full Config: - ctrl+right loglevel: info fullscreenrendering: false + defaultsearchcolumns: + - command + - current_working_directory + - hostname diff --git a/shared/testutils/testutils.go b/shared/testutils/testutils.go index cf8f0e3..adad836 100644 --- a/shared/testutils/testutils.go +++ b/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()))