From 7dfdefd29d70088832237e6c575f5b19a7035edb Mon Sep 17 00:00:00 2001 From: Fred Date: Wed, 23 Oct 2024 21:23:40 +0100 Subject: [PATCH 1/3] fix command line arguments broken on launchd after #420 --- schedule/command_arguments.go | 48 ++++++++++++++++++++++++++++++ schedule/command_arguments_test.go | 41 +++++++++++++++++++++++++ schedule/config.go | 4 +-- schedule/config_test.go | 4 +-- schedule/handler_crond.go | 6 ++-- schedule/handler_darwin.go | 3 +- schedule/handler_systemd.go | 2 +- schedule/handler_test.go | 6 ++-- schedule/handler_windows.go | 2 +- schedule_jobs.go | 4 +-- schtasks/config.go | 2 +- schtasks/taskscheduler.go | 12 ++++---- schtasks/taskscheduler_test.go | 2 +- 13 files changed, 111 insertions(+), 25 deletions(-) create mode 100644 schedule/command_arguments.go create mode 100644 schedule/command_arguments_test.go diff --git a/schedule/command_arguments.go b/schedule/command_arguments.go new file mode 100644 index 00000000..b9137296 --- /dev/null +++ b/schedule/command_arguments.go @@ -0,0 +1,48 @@ +package schedule + +import "strings" + +type CommandArguments struct { + args []string +} + +func NewCommandArguments(args []string) CommandArguments { + return CommandArguments{ + args: args, + } +} + +func (ca CommandArguments) RawArgs() []string { + return ca.args +} + +// String returns the arguments as a string, with quotes around arguments that contain spaces +func (ca CommandArguments) String() string { + if len(ca.args) == 0 { + return "" + } + + var n int + for _, elem := range ca.args { + n += len(elem) + 2 // add 2 if quotes are needed + } + + b := new(strings.Builder) + b.Grow(n) + ca.writeString(b, ca.args[0]) + for _, s := range ca.args[1:] { + b.WriteString(" ") + ca.writeString(b, s) + } + return b.String() +} + +func (ca CommandArguments) writeString(b *strings.Builder, str string) { + if strings.Contains(str, " ") { + b.WriteString(`"`) + b.WriteString(str) + b.WriteString(`"`) + } else { + b.WriteString(str) + } +} diff --git a/schedule/command_arguments_test.go b/schedule/command_arguments_test.go new file mode 100644 index 00000000..ac0443d6 --- /dev/null +++ b/schedule/command_arguments_test.go @@ -0,0 +1,41 @@ +package schedule + +import ( + "testing" +) + +func TestRawArgs(t *testing.T) { + args := []string{"arg1", "arg2"} + ca := NewCommandArguments(args) + rawArgs := ca.RawArgs() + if len(rawArgs) != len(args) { + t.Errorf("expected %d raw arguments, got %d", len(args), len(rawArgs)) + } + for i, arg := range args { + if rawArgs[i] != arg { + t.Errorf("expected raw argument %d to be %s, got %s", i, arg, rawArgs[i]) + } + } +} + +func TestString(t *testing.T) { + tests := []struct { + args []string + expected string + }{ + {[]string{}, ""}, + {[]string{"arg1"}, "arg1"}, + {[]string{"arg1 with space"}, `"arg1 with space"`}, + {[]string{"arg1", "arg2"}, "arg1 arg2"}, + {[]string{"arg1", "arg with spaces"}, `arg1 "arg with spaces"`}, + {[]string{"arg1", "arg with spaces", "anotherArg"}, `arg1 "arg with spaces" anotherArg`}, + } + + for _, test := range tests { + ca := NewCommandArguments(test.args) + result := ca.String() + if result != test.expected { + t.Errorf("expected %s, got %s", test.expected, result) + } + } +} diff --git a/schedule/config.go b/schedule/config.go index d1526506..6c5a2ad7 100644 --- a/schedule/config.go +++ b/schedule/config.go @@ -14,7 +14,7 @@ type Config struct { Permission string WorkingDirectory string Command string // path to resticprofile executable - Arguments []string + Arguments CommandArguments Environment []string JobDescription string TimerDescription string @@ -40,7 +40,7 @@ func NewRemoveOnlyConfig(profileName, commandName string) *Config { func (s *Config) SetCommand(wd, command string, args []string) { s.WorkingDirectory = wd s.Command = command - s.Arguments = args + s.Arguments = NewCommandArguments(args) } // Priority is either "background" or "standard" diff --git a/schedule/config_test.go b/schedule/config_test.go index 7bf78661..38240ceb 100644 --- a/schedule/config_test.go +++ b/schedule/config_test.go @@ -14,7 +14,7 @@ func TestScheduleProperties(t *testing.T) { Permission: "admin", WorkingDirectory: "home", Command: "command", - Arguments: []string{"1", "2"}, + Arguments: NewCommandArguments([]string{"1", "2"}), Environment: []string{"test=dev"}, JobDescription: "job", TimerDescription: "timer", @@ -35,7 +35,7 @@ func TestScheduleProperties(t *testing.T) { assert.Equal(t, "admin", schedule.Permission) assert.Equal(t, "command", schedule.Command) assert.Equal(t, "home", schedule.WorkingDirectory) - assert.ElementsMatch(t, []string{"1", "2"}, schedule.Arguments) + assert.ElementsMatch(t, []string{"1", "2"}, schedule.Arguments.RawArgs()) assert.Equal(t, []string{"test=dev"}, schedule.Environment) assert.Equal(t, "background", schedule.GetPriority()) // default value } diff --git a/schedule/handler_crond.go b/schedule/handler_crond.go index cf1f77bc..2d69f103 100644 --- a/schedule/handler_crond.go +++ b/schedule/handler_crond.go @@ -1,8 +1,6 @@ package schedule import ( - "strings" - "github.com/creativeprojects/resticprofile/calendar" "github.com/creativeprojects/resticprofile/constants" "github.com/creativeprojects/resticprofile/crond" @@ -68,7 +66,7 @@ func (h *HandlerCrond) CreateJob(job *Config, schedules []*calendar.Event, permi job.ConfigFile, job.ProfileName, job.CommandName, - job.Command+" "+strings.Join(job.Arguments, " "), + job.Command+" "+job.Arguments.String(), job.WorkingDirectory, ) if h.config.Username != "" { @@ -92,7 +90,7 @@ func (h *HandlerCrond) RemoveJob(job *Config, permission string) error { job.ConfigFile, job.ProfileName, job.CommandName, - job.Command+" "+strings.Join(job.Arguments, " "), + job.Command+" "+job.Arguments.String(), job.WorkingDirectory, ), } diff --git a/schedule/handler_darwin.go b/schedule/handler_darwin.go index 9aa49e1e..d5604d0e 100644 --- a/schedule/handler_darwin.go +++ b/schedule/handler_darwin.go @@ -144,7 +144,6 @@ Do you want to start it now?` func (h *HandlerLaunchd) getLaunchdJob(job *Config, schedules []*calendar.Event) *LaunchdJob { name := getJobName(job.ProfileName, job.CommandName) - args := job.Arguments // we always set the log file in the job settings as a default // if changed in the configuration via schedule-log the standard output will be empty anyway logfile := name + ".log" @@ -165,7 +164,7 @@ func (h *HandlerLaunchd) getLaunchdJob(job *Config, schedules []*calendar.Event) launchdJob := &LaunchdJob{ Label: name, Program: job.Command, - ProgramArguments: append([]string{job.Command, "--no-prio"}, args...), + ProgramArguments: append([]string{job.Command, "--no-prio"}, job.Arguments.RawArgs()...), StandardOutPath: logfile, StandardErrorPath: logfile, WorkingDirectory: job.WorkingDirectory, diff --git a/schedule/handler_systemd.go b/schedule/handler_systemd.go index 55023407..bc6b1eb9 100644 --- a/schedule/handler_systemd.go +++ b/schedule/handler_systemd.go @@ -107,7 +107,7 @@ func (h *HandlerSystemd) CreateJob(job *Config, schedules []*calendar.Event, per } err := systemd.Generate(systemd.Config{ - CommandLine: job.Command + " " + strings.Join(append([]string{"--no-prio"}, job.Arguments...), " "), + CommandLine: job.Command + " " + strings.Join(append([]string{"--no-prio"}, job.Arguments.RawArgs()...), " "), Environment: job.Environment, WorkingDirectory: job.WorkingDirectory, Title: job.ProfileName, diff --git a/schedule/handler_test.go b/schedule/handler_test.go index eba1f44d..83ac9e63 100644 --- a/schedule/handler_test.go +++ b/schedule/handler_test.go @@ -1,14 +1,14 @@ package schedule import ( - "runtime" "testing" + "github.com/creativeprojects/resticprofile/platform" "github.com/stretchr/testify/assert" ) func TestLookupExistingBinary(t *testing.T) { - if runtime.GOOS == "windows" { + if platform.IsWindows() { t.Skip() } err := lookupBinary("sh", "sh") @@ -16,7 +16,7 @@ func TestLookupExistingBinary(t *testing.T) { } func TestLookupNonExistingBinary(t *testing.T) { - if runtime.GOOS == "windows" { + if platform.IsWindows() { t.Skip() } err := lookupBinary("something", "almost_certain_not_to_be_available") diff --git a/schedule/handler_windows.go b/schedule/handler_windows.go index cc98c8ce..cc3af507 100644 --- a/schedule/handler_windows.go +++ b/schedule/handler_windows.go @@ -58,7 +58,7 @@ func (h *HandlerWindows) CreateJob(job *Config, schedules []*calendar.Event, per ProfileName: job.ProfileName, CommandName: job.CommandName, Command: job.Command, - Arguments: job.Arguments, + Arguments: job.Arguments.String(), WorkingDirectory: job.WorkingDirectory, JobDescription: job.JobDescription, } diff --git a/schedule_jobs.go b/schedule_jobs.go index 892e5104..2ca0440a 100644 --- a/schedule_jobs.go +++ b/schedule_jobs.go @@ -33,7 +33,7 @@ func scheduleJobs(handler schedule.Handler, profileName string, configs []*confi args := []string{ "--no-ansi", "--config", - `"` + scheduleConfig.ConfigFile + `"`, + scheduleConfig.ConfigFile, "run-schedule", scheduleName, } @@ -141,7 +141,7 @@ func scheduleToConfig(sched *config.Schedule) *schedule.Config { Permission: sched.Permission, WorkingDirectory: "", Command: "", - Arguments: []string{}, + Arguments: schedule.NewCommandArguments(nil), Environment: sched.Environment, JobDescription: "", TimerDescription: "", diff --git a/schtasks/config.go b/schtasks/config.go index eebb417c..5356016e 100644 --- a/schtasks/config.go +++ b/schtasks/config.go @@ -4,7 +4,7 @@ type Config struct { ProfileName string CommandName string Command string - Arguments []string + Arguments string WorkingDirectory string JobDescription string } diff --git a/schtasks/taskscheduler.go b/schtasks/taskscheduler.go index 71415133..5de4dfb0 100644 --- a/schtasks/taskscheduler.go +++ b/schtasks/taskscheduler.go @@ -114,7 +114,7 @@ func createUserTask(config *Config, schedules []*calendar.Event) error { task := taskService.NewTaskDefinition() task.AddAction(taskmaster.ExecAction{ Path: config.Command, - Args: strings.Join(config.Arguments, " "), + Args: config.Arguments, WorkingDir: config.WorkingDirectory, }) task.Principal.LogonType = taskmaster.TASK_LOGON_PASSWORD @@ -154,7 +154,7 @@ func updateUserTask(task taskmaster.RegisteredTask, config *Config, schedules [] task.Definition.Actions = make([]taskmaster.Action, 0, 1) task.Definition.AddAction(taskmaster.ExecAction{ Path: config.Command, - Args: strings.Join(config.Arguments, " "), + Args: config.Arguments, WorkingDir: config.WorkingDirectory, }) task.Definition.Principal.LogonType = taskmaster.TASK_LOGON_PASSWORD @@ -210,7 +210,7 @@ func createUserLoggedOnTask(config *Config, schedules []*calendar.Event) error { task := taskService.NewTaskDefinition() task.AddAction(taskmaster.ExecAction{ Path: config.Command, - Args: strings.Join(config.Arguments, " "), + Args: config.Arguments, WorkingDir: config.WorkingDirectory, }) task.Principal.LogonType = taskmaster.TASK_LOGON_INTERACTIVE_TOKEN @@ -244,7 +244,7 @@ func updateUserLoggedOnTask(task taskmaster.RegisteredTask, config *Config, sche task.Definition.Actions = make([]taskmaster.Action, 0, 1) task.Definition.AddAction(taskmaster.ExecAction{ Path: config.Command, - Args: strings.Join(config.Arguments, " "), + Args: config.Arguments, WorkingDir: config.WorkingDirectory, }) task.Definition.Principal.LogonType = taskmaster.TASK_LOGON_INTERACTIVE_TOKEN @@ -278,7 +278,7 @@ func createSystemTask(config *Config, schedules []*calendar.Event) error { task := taskService.NewTaskDefinition() task.AddAction(taskmaster.ExecAction{ Path: config.Command, - Args: strings.Join(config.Arguments, " "), + Args: config.Arguments, WorkingDir: config.WorkingDirectory, }) task.Principal.LogonType = taskmaster.TASK_LOGON_SERVICE_ACCOUNT @@ -307,7 +307,7 @@ func updateSystemTask(task taskmaster.RegisteredTask, config *Config, schedules task.Definition.Actions = make([]taskmaster.Action, 0, 1) task.Definition.AddAction(taskmaster.ExecAction{ Path: config.Command, - Args: strings.Join(config.Arguments, " "), + Args: config.Arguments, WorkingDir: config.WorkingDirectory, }) task.Definition.Principal.LogonType = taskmaster.TASK_LOGON_SERVICE_ACCOUNT diff --git a/schtasks/taskscheduler_test.go b/schtasks/taskscheduler_test.go index 65857ced..02f32b82 100644 --- a/schtasks/taskscheduler_test.go +++ b/schtasks/taskscheduler_test.go @@ -349,7 +349,7 @@ func TestCreationOfTasks(t *testing.T) { ProfileName: "test", CommandName: strconv.Itoa(count), Command: "echo", - Arguments: []string{"hello"}, + Arguments: "hello", WorkingDirectory: "C:\\", JobDescription: fixture.description, } From 64127e007457a0aa7907e03f9b1572db008ae45b Mon Sep 17 00:00:00 2001 From: Fred Date: Wed, 23 Oct 2024 21:30:05 +0100 Subject: [PATCH 2/3] fix test on mock handler --- config/mocks/NamedPropertySet.go | 2 +- config/mocks/ProfileInfo.go | 2 +- config/mocks/PropertyInfo.go | 2 +- config/mocks/SectionInfo.go | 2 +- monitor/mocks/OutputAnalysis.go | 2 +- schedule/mocks/Handler.go | 2 +- schedule_jobs_test.go | 5 +++-- 7 files changed, 9 insertions(+), 8 deletions(-) diff --git a/config/mocks/NamedPropertySet.go b/config/mocks/NamedPropertySet.go index c606d31f..48b38516 100644 --- a/config/mocks/NamedPropertySet.go +++ b/config/mocks/NamedPropertySet.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.46.2. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package mocks diff --git a/config/mocks/ProfileInfo.go b/config/mocks/ProfileInfo.go index 8bafba01..55438596 100644 --- a/config/mocks/ProfileInfo.go +++ b/config/mocks/ProfileInfo.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.46.2. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package mocks diff --git a/config/mocks/PropertyInfo.go b/config/mocks/PropertyInfo.go index 98e43991..b6a3b4c3 100644 --- a/config/mocks/PropertyInfo.go +++ b/config/mocks/PropertyInfo.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.46.2. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package mocks diff --git a/config/mocks/SectionInfo.go b/config/mocks/SectionInfo.go index 695def83..929bfb56 100644 --- a/config/mocks/SectionInfo.go +++ b/config/mocks/SectionInfo.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.46.2. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package mocks diff --git a/monitor/mocks/OutputAnalysis.go b/monitor/mocks/OutputAnalysis.go index a65cd599..1aa6f1cd 100644 --- a/monitor/mocks/OutputAnalysis.go +++ b/monitor/mocks/OutputAnalysis.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.46.2. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package mocks diff --git a/schedule/mocks/Handler.go b/schedule/mocks/Handler.go index 75c4ba37..37af71ec 100644 --- a/schedule/mocks/Handler.go +++ b/schedule/mocks/Handler.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.46.2. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package mocks diff --git a/schedule_jobs_test.go b/schedule_jobs_test.go index 98ceb852..a6e26829 100644 --- a/schedule_jobs_test.go +++ b/schedule_jobs_test.go @@ -42,12 +42,13 @@ func TestSimpleScheduleJob(t *testing.T) { mock.AnythingOfType("[]*calendar.Event"), mock.AnythingOfType("string")). RunAndReturn(func(scheduleConfig *schedule.Config, events []*calendar.Event, permission string) error { - assert.Equal(t, []string{"--no-ansi", "--config", `"config.file"`, "run-schedule", "backup@profile"}, scheduleConfig.Arguments) + assert.Equal(t, []string{"--no-ansi", "--config", `config file`, "run-schedule", "backup@profile"}, scheduleConfig.Arguments.RawArgs()) + assert.Equal(t, `--no-ansi --config "config file" run-schedule backup@profile`, scheduleConfig.Arguments.String()) return nil }) scheduleConfig := configForJob("backup", "sched") - scheduleConfig.ConfigFile = "config.file" + scheduleConfig.ConfigFile = "config file" err := scheduleJobs(handler, "profile", []*config.Schedule{scheduleConfig}) assert.NoError(t, err) } From 2dd98100615915c22fd688c772d8ad33df72e2b2 Mon Sep 17 00:00:00 2001 From: Fred Date: Wed, 23 Oct 2024 21:54:30 +0100 Subject: [PATCH 3/3] addressing code review comments --- schedule/command_arguments.go | 6 +++-- schedule/command_arguments_test.go | 36 ++++++++++++++++++++++-------- schedule/config.go | 2 ++ schedule/config_test.go | 1 + schtasks/taskscheduler_test.go | 2 +- 5 files changed, 35 insertions(+), 12 deletions(-) diff --git a/schedule/command_arguments.go b/schedule/command_arguments.go index b9137296..9ad618f8 100644 --- a/schedule/command_arguments.go +++ b/schedule/command_arguments.go @@ -13,7 +13,9 @@ func NewCommandArguments(args []string) CommandArguments { } func (ca CommandArguments) RawArgs() []string { - return ca.args + result := make([]string, len(ca.args)) + copy(result, ca.args) + return result } // String returns the arguments as a string, with quotes around arguments that contain spaces @@ -24,7 +26,7 @@ func (ca CommandArguments) String() string { var n int for _, elem := range ca.args { - n += len(elem) + 2 // add 2 if quotes are needed + n += len(elem) + 3 // add 2 if quotes are needed, plus 1 for the space } b := new(strings.Builder) diff --git a/schedule/command_arguments_test.go b/schedule/command_arguments_test.go index ac0443d6..c6c226f0 100644 --- a/schedule/command_arguments_test.go +++ b/schedule/command_arguments_test.go @@ -5,16 +5,29 @@ import ( ) func TestRawArgs(t *testing.T) { - args := []string{"arg1", "arg2"} - ca := NewCommandArguments(args) - rawArgs := ca.RawArgs() - if len(rawArgs) != len(args) { - t.Errorf("expected %d raw arguments, got %d", len(args), len(rawArgs)) + tests := []struct { + name string + args []string + }{ + {"empty args", []string{}}, + {"simple args", []string{"arg1", "arg2"}}, + {"args with spaces", []string{"C:\\Program Files\\app.exe", "--config", "C:\\My Documents\\config.toml"}}, + {"args with special chars", []string{"--name", "my-task!", "--config=test.conf"}}, } - for i, arg := range args { - if rawArgs[i] != arg { - t.Errorf("expected raw argument %d to be %s, got %s", i, arg, rawArgs[i]) - } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ca := NewCommandArguments(tt.args) + rawArgs := ca.RawArgs() + if len(rawArgs) != len(tt.args) { + t.Errorf("expected %d raw arguments, got %d", len(tt.args), len(rawArgs)) + } + for i, arg := range tt.args { + if rawArgs[i] != arg { + t.Errorf("expected raw argument %d to be %s, got %s", i, arg, rawArgs[i]) + } + } + }) } } @@ -29,6 +42,11 @@ func TestString(t *testing.T) { {[]string{"arg1", "arg2"}, "arg1 arg2"}, {[]string{"arg1", "arg with spaces"}, `arg1 "arg with spaces"`}, {[]string{"arg1", "arg with spaces", "anotherArg"}, `arg1 "arg with spaces" anotherArg`}, + {[]string{"--config", "C:\\Program Files\\config.toml"}, `--config "C:\Program Files\config.toml"`}, + {[]string{"--config", "C:\\Users\\John Doe\\Documents\\config.toml", "--name", "backup task"}, + `--config "C:\Users\John Doe\Documents\config.toml" --name "backup task"`}, + {[]string{"--config", "C:\\My Files\\config.toml", "--no-ansi"}, + `--config "C:\My Files\config.toml" --no-ansi`}, } for _, test := range tests { diff --git a/schedule/config.go b/schedule/config.go index 6c5a2ad7..434d6fc6 100644 --- a/schedule/config.go +++ b/schedule/config.go @@ -37,6 +37,8 @@ func NewRemoveOnlyConfig(profileName, commandName string) *Config { } } +// SetCommand sets the command details for scheduling. Arguments are automatically +// processed to ensure proper handling of paths with spaces and special characters. func (s *Config) SetCommand(wd, command string, args []string) { s.WorkingDirectory = wd s.Command = command diff --git a/schedule/config_test.go b/schedule/config_test.go index 38240ceb..b0fc5d6b 100644 --- a/schedule/config_test.go +++ b/schedule/config_test.go @@ -36,6 +36,7 @@ func TestScheduleProperties(t *testing.T) { assert.Equal(t, "command", schedule.Command) assert.Equal(t, "home", schedule.WorkingDirectory) assert.ElementsMatch(t, []string{"1", "2"}, schedule.Arguments.RawArgs()) + assert.Equal(t, `1 2`, schedule.Arguments.String()) assert.Equal(t, []string{"test=dev"}, schedule.Environment) assert.Equal(t, "background", schedule.GetPriority()) // default value } diff --git a/schtasks/taskscheduler_test.go b/schtasks/taskscheduler_test.go index 02f32b82..0bedfcb9 100644 --- a/schtasks/taskscheduler_test.go +++ b/schtasks/taskscheduler_test.go @@ -349,7 +349,7 @@ func TestCreationOfTasks(t *testing.T) { ProfileName: "test", CommandName: strconv.Itoa(count), Command: "echo", - Arguments: "hello", + Arguments: "hello there", WorkingDirectory: "C:\\", JobDescription: fixture.description, }