Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions contracts/binding/binding.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,7 @@ var (
Cache,
Config,
Orm,
Process,
Route,
Session,
},
Expand Down
1 change: 1 addition & 0 deletions errors/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ var (
JSONParserNotSet = New("JSON parser is not initialized")
LogFacadeNotSet = New("log facade is not initialized")
OrmFacadeNotSet = New("orm facade is not initialized")
ProcessFacadeNotSet = New("process facade is not initialized")
QueueFacadeNotSet = New("queue facade is not initialized")
RateLimiterFacadeNotSet = New("rate limiter facade is not initialized")
ScheduleFacadeNotSet = New("schedule facade is not initialized")
Expand Down
1 change: 1 addition & 0 deletions process/pipeline.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ type Pipe struct {
}

func (r *Pipe) Command(name string, args ...string) contractsprocess.PipeCommand {
name, args = formatCommand(name, args)
command := NewPipeCommand(strconv.Itoa(len(r.commands)), name, args)
r.commands = append(r.commands, command)
return command
Expand Down
1 change: 1 addition & 0 deletions process/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ type Pool struct {
}

func (r *Pool) Command(name string, args ...string) contractsprocess.PoolCommand {
name, args = formatCommand(name, args)
command := NewPoolCommand(strconv.Itoa(len(r.commands)), name, args)
r.commands = append(r.commands, command)
return command
Expand Down
17 changes: 17 additions & 0 deletions process/process.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"time"

contractsprocess "github.com/goravel/framework/contracts/process"
"github.com/goravel/framework/support/env"
"github.com/goravel/framework/support/str"
)

var _ contractsprocess.Process = (*Process)(nil)
Expand Down Expand Up @@ -78,6 +80,7 @@ func (r *Process) Quietly() contractsprocess.Process {
}

func (r *Process) Run(name string, args ...string) contractsprocess.Result {
name, args = formatCommand(name, args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we make this explicit by adding a RunShell method. The auto-detection is ambiguous. For instance, on Windows, running a binary inside c:\Program Files\App.exe would fail because the code treats the space as a shell separator.

It also creates inconsistent behavior. For instance, facades.Process().Run("ls | grep foo", "-la") will fail. Because a second argument is present, the logic skips the shell detection and tries to find an executable literally named 'ls | grep foo'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also considered adding a new function like RunShell, but I'm thinking if it's necessary to add a single function to support running string commands. I just want to omit /bin/sh -c, it's a bit heavy to add such a function.

c:\Program Files\App.exe is a good case, space may be contained in the route.

facades.Process().Run("ls | grep foo", "-la") doesn't trigger the formatCommand, so it's not related to this PR.

We can optimize the condition to if len(args) == 0 && str.Of(name).Contains("&", "|"), except space. Then it's simpler to run such commands:

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you are concerned about the verbosity of manually writing /bin/sh -c but the default Run method should remain raw and strict to avoid 'magic' inside it.

Go's standard library actually warns against this implicit shell invocation:

Unlike the "system" library call from C and other languages, the os/exec package intentionally does not invoke the system shell and does not expand any glob patterns or handle other expansions, pipelines, or redirections typically done by shells. The package behaves more like C's "exec" family of functions. To expand glob patterns, either call the shell directly, taking care to escape any dangerous input, or use the path/filepath package's Glob function. To expand environment variables, use package os's ExpandEnv.

https://pkg.go.dev/os/exec

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, if we need to provide this functionality, let’s add an annotation for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Go has own standard, and our function is following that as default. I just want to provide an additional way to run string commands directly. It's very convenient in some situations.

run, err := r.start(name, args...)
if err != nil {
return NewResult(err, 1, "", "", "")
Expand Down Expand Up @@ -185,3 +188,17 @@ func (r *Process) start(name string, args ...string) (contractsprocess.Running,

return NewRunning(ctx, cmd, cancel, stdoutBuffer, stderrBuffer, r.loading, r.loadingMessage), nil
}

func formatCommand(name string, args []string) (string, []string) {
if len(args) == 0 && str.Of(name).Contains(" ", "&", "|") {
if env.IsWindows() {
args = []string{"/c", name}
name = "cmd"
} else {
args = []string{"-c", name}
name = "/bin/sh"
}
}

return name, args
}
103 changes: 103 additions & 0 deletions process/process_unix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,3 +257,106 @@ func TestProcess_Pipe_Unix(t *testing.T) {
assert.ErrorIs(t, result.Error(), errors.ProcessPipeNilConfigurer)
})
}

func TestFormatCommand_Unix(t *testing.T) {
tests := []struct {
name string
inputName string
inputArgs []string
expectedName string
expectedArgs []string
}{
{
name: "command with args - not wrapped",
inputName: "echo",
inputArgs: []string{"hello", "world"},
expectedName: "echo",
expectedArgs: []string{"hello", "world"},
},
{
name: "simple command - not wrapped",
inputName: "ls",
inputArgs: []string{},
expectedName: "ls",
expectedArgs: []string{},
},
{
name: "command with space only - wrapped",
inputName: "echo hello",
inputArgs: []string{},
expectedName: "/bin/sh",
expectedArgs: []string{"-c", "echo hello"},
},
{
name: "command with space and ampersand but has args - not wrapped",
inputName: "sleep 5 &",
inputArgs: []string{"-v"},
expectedName: "sleep 5 &",
expectedArgs: []string{"-v"},
},
{
name: "background command - wrapped",
inputName: "sleep 5 &",
inputArgs: []string{},
expectedName: "/bin/sh",
expectedArgs: []string{"-c", "sleep 5 &"},
},
{
name: "piped command - wrapped",
inputName: "cat file.txt | grep test",
inputArgs: []string{},
expectedName: "/bin/sh",
expectedArgs: []string{"-c", "cat file.txt | grep test"},
},
{
name: "piped background command - wrapped",
inputName: "cat file.txt | grep test &",
inputArgs: []string{},
expectedName: "/bin/sh",
expectedArgs: []string{"-c", "cat file.txt | grep test &"},
},
{
name: "logical AND operators - wrapped",
inputName: "echo hello && echo world",
inputArgs: []string{},
expectedName: "/bin/sh",
expectedArgs: []string{"-c", "echo hello && echo world"},
},
{
name: "multiple pipes - wrapped",
inputName: "cat file | sort | uniq",
inputArgs: []string{},
expectedName: "/bin/sh",
expectedArgs: []string{"-c", "cat file | sort | uniq"},
},
{
name: "command with ampersand only - wrapped",
inputName: "test&",
inputArgs: []string{},
expectedName: "/bin/sh",
expectedArgs: []string{"-c", "test&"},
},
{
name: "command with pipe only - wrapped",
inputName: "test|",
inputArgs: []string{},
expectedName: "/bin/sh",
expectedArgs: []string{"-c", "test|"},
},
{
name: "single ampersand - wrapped",
inputName: "&",
inputArgs: []string{},
expectedName: "/bin/sh",
expectedArgs: []string{"-c", "&"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
gotName, gotArgs := formatCommand(tt.inputName, tt.inputArgs)
assert.Equal(t, tt.expectedName, gotName)
assert.Equal(t, tt.expectedArgs, gotArgs)
})
}
}
103 changes: 103 additions & 0 deletions process/process_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,106 @@ func TestProcess_Pipe_Windows(t *testing.T) {
assert.Equal(t, errors.ProcessPipeNilConfigurer, res.Error())
})
}

func TestFormatCommand_Windows(t *testing.T) {
tests := []struct {
name string
inputName string
inputArgs []string
expectedName string
expectedArgs []string
}{
{
name: "command with args - not wrapped",
inputName: "echo",
inputArgs: []string{"hello", "world"},
expectedName: "echo",
expectedArgs: []string{"hello", "world"},
},
{
name: "simple command - not wrapped",
inputName: "dir",
inputArgs: []string{},
expectedName: "dir",
expectedArgs: []string{},
},
{
name: "command with space only - wrapped",
inputName: "echo hello",
inputArgs: []string{},
expectedName: "cmd",
expectedArgs: []string{"/c", "echo hello"},
},
{
name: "command with space and ampersand but has args - not wrapped",
inputName: "timeout 5 &",
inputArgs: []string{"/nobreak"},
expectedName: "timeout 5 &",
expectedArgs: []string{"/nobreak"},
},
{
name: "background command - wrapped",
inputName: "timeout 5 &",
inputArgs: []string{},
expectedName: "cmd",
expectedArgs: []string{"/c", "timeout 5 &"},
},
{
name: "piped command - wrapped",
inputName: "type file.txt | findstr test",
inputArgs: []string{},
expectedName: "cmd",
expectedArgs: []string{"/c", "type file.txt | findstr test"},
},
{
name: "piped background command - wrapped",
inputName: "type file.txt | findstr test &",
inputArgs: []string{},
expectedName: "cmd",
expectedArgs: []string{"/c", "type file.txt | findstr test &"},
},
{
name: "logical AND operators - wrapped",
inputName: "echo hello && echo world",
inputArgs: []string{},
expectedName: "cmd",
expectedArgs: []string{"/c", "echo hello && echo world"},
},
{
name: "multiple pipes - wrapped",
inputName: "type file | sort | findstr test",
inputArgs: []string{},
expectedName: "cmd",
expectedArgs: []string{"/c", "type file | sort | findstr test"},
},
{
name: "command with ampersand only - wrapped",
inputName: "test&",
inputArgs: []string{},
expectedName: "cmd",
expectedArgs: []string{"/c", "test&"},
},
{
name: "command with pipe only - wrapped",
inputName: "test|",
inputArgs: []string{},
expectedName: "cmd",
expectedArgs: []string{"/c", "test|"},
},
{
name: "single ampersand - wrapped",
inputName: "&",
inputArgs: []string{},
expectedName: "cmd",
expectedArgs: []string{"/c", "&"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
gotName, gotArgs := formatCommand(tt.inputName, tt.inputArgs)
assert.Equal(t, tt.expectedName, gotName)
assert.Equal(t, tt.expectedArgs, gotArgs)
})
}
}
33 changes: 31 additions & 2 deletions support/docker/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package docker

import (
"fmt"
"math/rand"
"net"
"strings"

"github.com/goravel/framework/contracts/testing/docker"
"github.com/goravel/framework/support/process"
"github.com/goravel/framework/errors"
)

func ExposedPort(exposedPorts []string, port string) string {
Expand Down Expand Up @@ -41,7 +43,7 @@ func ImageToCommand(image *docker.Image) (command string, exposedPorts []string)
if len(image.ExposedPorts) > 0 {
for _, port := range image.ExposedPorts {
if !strings.Contains(port, ":") {
port = fmt.Sprintf("%d:%s", process.ValidPort(), port)
port = fmt.Sprintf("%d:%s", ValidPort(), port)
}
ports = append(ports, port)
commands = append(commands, "-p", port)
Expand All @@ -60,3 +62,30 @@ func ImageToCommand(image *docker.Image) (command string, exposedPorts []string)

return strings.Join(commands, " "), ports
}

// Used by TestContainer, to simulate the port is using.
var TestPortUsing = false

func IsPortUsing(port int) bool {
if TestPortUsing {
return true
}

l, err := net.Listen("tcp", fmt.Sprintf(":%d", port))
if l != nil {
errors.Ignore(l.Close)
}

return err != nil
}

func ValidPort() int {
for range 60 {
random := rand.Intn(10000) + 10000
if !IsPortUsing(random) {
return random
}
}

return 0
}
4 changes: 4 additions & 0 deletions support/docker/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,7 @@ func TestImageToCommand(t *testing.T) {
})
assert.Equal(t, "docker run --rm -d -e a=b -p 1234:6379 redis:latest --a=b sleep 1000", command)
}

func TestValidPort(t *testing.T) {
assert.True(t, ValidPort() > 0)
}
25 changes: 0 additions & 25 deletions support/process/process.go

This file was deleted.

18 changes: 0 additions & 18 deletions support/process/process_test.go

This file was deleted.

Loading
Loading