Repository: k1LoW/tbls (go)

Issue:
when expression parsing too simplistic
tbls runs `when` expressions with the [`expr` package](https://expr-lang.org/), but first replaces any `$ENV_VAR` string with the the string, if defined in the environment, with `Env.ENV_VAR`.

There are two problems with this approach:

- If the expression uses the special [`$expr` variable](https://expr-lang.org/docs/language-definition#env), _and_ the environment variable `env` exists, then the special variable is replaced by `Env.env`.
- The text replacement can trivially replace text in string literals or other parts of the expression, even though they are not identifiers. E.g. `env FOO="bar" BAR='$FOO' tbls --when '$BAR == "$FOO"'` should run (`BAR` is equal to the literal string `$FOO`), but fails because the string literal `"$FOO"` in the expression was replaced by `"Env.FOO"`.

The solution is to use [`ast.Patch()`](https://expr-lang.org/docs/patch) after compiling the expression into an AST to walk, and replacing each `$REF` name with `Env.REF` (as a `MemberNode` with Env identifier and `REF` string literal). This can be done for **all** such names (excepting `$env`) so `Env.NONESUCHNAME` properly fails as a `unknown field NONESUCHNAME` error.

I'll create a PR.

Pull request:
Refactor when expression handling
- Use expr.Patch and a visitor to only replace identifiers that start with `$` with a `Env.` member lookup.
- use `expr.AsBool()` to assert the expression produces a boolean
- Clean up envMap parsing to use strings.Cut()
- Expand tests, using testing.T.Run() with test names.

Fixes #599


Patch:
diff --git a/.github/workflows/tagpr.yml b/.github/workflows/tagpr.yml
index 2422188..5c28a23 100644
--- a/.github/workflows/tagpr.yml
+++ b/.github/workflows/tagpr.yml
@@ -46,7 +46,7 @@ jobs:
           brew install sqlite3
 
       - name: Run GoReleaser
-        uses: goreleaser/goreleaser-action@v6
+        uses: goreleaser/goreleaser-action@v5
         with:
           distribution: goreleaser
           version: latest
@@ -89,7 +89,7 @@ jobs:
           sleep 20s
 
       - name: Run GoReleaser
-        uses: goreleaser/goreleaser-action@v6
+        uses: goreleaser/goreleaser-action@v5
         with:
           distribution: goreleaser
           version: latest
@@ -140,7 +140,7 @@ jobs:
         shell: bash
 
       - name: Run GoReleaser
-        uses: goreleaser/goreleaser-action@v6
+        uses: goreleaser/goreleaser-action@v5
         with:
           distribution: goreleaser
           version: latest
diff --git a/.goreleaser/darwin.yml b/.goreleaser/darwin.yml
index c3f0a30..ee8f986 100644
--- a/.goreleaser/darwin.yml
+++ b/.goreleaser/darwin.yml
@@ -1,4 +1,3 @@
-version: 2
 before:
   hooks:
     - go mod download
@@ -35,6 +34,7 @@ checksum:
 snapshot:
   name_template: "{{ .Version }}-next"
 changelog:
+  skip: true
   sort: asc
   filters:
     exclude:
diff --git a/.goreleaser/linux.yml b/.goreleaser/linux.yml
index 6df0ce9..fb8813a 100644
--- a/.goreleaser/linux.yml
+++ b/.goreleaser/linux.yml
@@ -1,4 +1,3 @@
-version: 2
 before:
   hooks:
     - go mod download
@@ -43,6 +42,7 @@ checksum:
 snapshot:
   name_template: "{{ .Version }}-next"
 changelog:
+  skip: true
   sort: asc
   filters:
     exclude:
diff --git a/.goreleaser/windows.yml b/.goreleaser/windows.yml
index 88fca2d..a9a53d5 100644
--- a/.goreleaser/windows.yml
+++ b/.goreleaser/windows.yml
@@ -1,4 +1,3 @@
-version: 2
 before:
   hooks:
     - go mod download
@@ -38,6 +37,7 @@ checksum:
 snapshot:
   name_template: "{{ .Version }}-next"
 changelog:
+  skip: true
   sort: asc
   filters:
     exclude:
diff --git a/cmdutil/when.go b/cmdutil/when.go
index e150779..a79a1e9 100644
--- a/cmdutil/when.go
+++ b/cmdutil/when.go
@@ -1,49 +1,62 @@
 package cmdutil
 
 import (
-	"fmt"
 	"os"
 	"strings"
 
 	"github.com/expr-lang/expr"
+	"github.com/expr-lang/expr/ast"
 	"github.com/pkg/errors"
 )
 
+// AST walker which replaces `$IDENTIFIER` with `Env.IDENTIFIER` member lookup expressions.
+type EnvPatcher struct{}
+
+func (EnvPatcher) Visit(node *ast.Node) {
+	if n, ok := (*node).(*ast.IdentifierNode); ok && n.Value[0] == '$' && n.Value != "$env" {
+		ast.Patch(
+			node,
+			&ast.MemberNode{
+				Node:     &ast.IdentifierNode{Value: "Env"},
+				Property: &ast.StringNode{Value: n.Value[1:]},
+			},
+		)
+	}
+}
+
+// The predefined variables of a when expression
+type WhenEnv struct {
+	Env map[string]string
+}
+
+var NewWhenEnv = func() *WhenEnv {
+	return &WhenEnv{Env: envMap()}
+}
+
 func IsAllowedToExecute(when string) (bool, error) {
 	if when == "" {
 		return true, nil
 	}
-	ropts := []string{}
-	em := envMap()
-	for k := range em {
-		ropts = append(ropts, fmt.Sprintf("$%s", k), fmt.Sprintf("Env.%s", k))
-	}
-	r := strings.NewReplacer(ropts...)
-	when = r.Replace(when)
-	got, err := expr.Eval(fmt.Sprintf("(%s) == true", when), struct {
-		Env map[string]string
-	}{
-		Env: em,
-	})
+
+	whenEnv := NewWhenEnv()
+	// when expressions must produce a boolean result
+	program, err := expr.Compile(when, expr.Patch(&EnvPatcher{}), expr.AsBool(), expr.Env(whenEnv))
 	if err != nil {
 		return false, errors.WithStack(err)
 	}
-	return got.(bool), nil
+	if got, err := expr.Run(program, whenEnv); err != nil {
+		return false, errors.WithStack(err)
+	} else {
+		return got.(bool), nil
+	}
 }
 
 func envMap() map[string]string {
 	m := map[string]string{}
 	for _, kv := range os.Environ() {
-		if !strings.Contains(kv, "=") {
-			continue
-		}
-		parts := strings.SplitN(kv, "=", 2)
-		k := parts[0]
-		if len(parts) < 2 {
-			m[k] = ""
-			continue
+		if k, v, ok := strings.Cut(kv, "="); ok {
+			m[k] = v
 		}
-		m[k] = parts[1]
 	}
 	return m
 }


Test patch:
diff --git a/.github/workflows/release-test.yml b/.github/workflows/release-test.yml
index 943d8b5..e1eda87 100644
--- a/.github/workflows/release-test.yml
+++ b/.github/workflows/release-test.yml
@@ -27,6 +27,7 @@ jobs:
 
       - name: Setup
         run: |
+          brew install goreleaser
           brew install sqlite3
 
       - name: Install
@@ -34,14 +35,9 @@ jobs:
           go install github.com/k1LoW/tbls@latest
           tbls version
 
-      - name: Run GoReleaser (dry-run)
-        uses: goreleaser/goreleaser-action@v6
-        with:
-          distribution: goreleaser
-          version: latest
-          args: --config .goreleaser/darwin.yml --clean --snapshot --skip=publish
-        env:
-          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+      - name: Release (dry-run)
+        run: |
+          goreleaser release --config .goreleaser/darwin.yml --clean --snapshot --skip-publish
 
   linux-release-test:
     runs-on: ubuntu-latest
@@ -70,7 +66,7 @@ jobs:
           tbls version
 
       - name: Run GoReleaser (dry-run)
-        uses: goreleaser/goreleaser-action@v6
+        uses: goreleaser/goreleaser-action@v5
         with:
           distribution: goreleaser
           version: latest
@@ -112,7 +108,7 @@ jobs:
           tbls version
 
       - name: Run GoReleaser (dry-run)
-        uses: goreleaser/goreleaser-action@v6
+        uses: goreleaser/goreleaser-action@v5
         with:
           distribution: goreleaser
           version: latest
diff --git a/cmdutil/when_test.go b/cmdutil/when_test.go
index 8cdb385..9a987a7 100644
--- a/cmdutil/when_test.go
+++ b/cmdutil/when_test.go
@@ -1,48 +1,137 @@
 package cmdutil
 
 import (
-	"os"
+	"strings"
 	"testing"
 )
 
+func TestEnvMap(t *testing.T) {
+	t.Setenv("TEST_ENV_EMPTY", "")
+	t.Setenv("TEST_ENV_SET", "value")
+	result := envMap()
+	if value, ok := result["TEST_ENV_EMPTY"]; !ok {
+		t.Error("Expected TEST_ENV_EMPTY to be set")
+	} else if value != "" {
+		t.Errorf("Expected TEST_ENV_EMPTY to be an empty string, got %v", value)
+	}
+	if value, ok := result["TEST_ENV_SET"]; !ok {
+		t.Error("Expected TEST_ENV_SET to be set")
+	} else if value != "value" {
+		t.Errorf("Expected TEST_ENV_SET to be 'value', got %v", value)
+	}
+}
+
 func TestIsAllowedToExecute(t *testing.T) {
 	tests := []struct {
-		envset map[string]string
-		when   string
-		want   bool
+		name          string
+		envset        map[string]string
+		when          string
+		want          bool
+		errorContains any
 	}{
 		{
+			name:          "Empty expression",
+			envset:        map[string]string{},
+			when:          "",
+			want:          true,
+			errorContains: nil,
+		},
+		{
+			name: "Equality test, true",
+			envset: map[string]string{
+				"TEST_ENV1": "a",
+			},
+			when:          "$TEST_ENV1 == 'a'",
+			want:          true,
+			errorContains: nil,
+		},
+		{
+			name: "Equality test, false",
+			envset: map[string]string{
+				"TEST_ENV1": "a",
+			},
+			when:          "$TEST_ENV1 == 'b'",
+			want:          false,
+			errorContains: nil,
+		},
+		{
+			name: "Containment in $env",
 			envset: map[string]string{
+				"env":       "should not replace $env",
 				"TEST_ENV1": "a",
 			},
-			when: "$TEST_ENV1 == 'a'",
-			want: true,
+			when:          `'TEST_ENV1' not in $env`,
+			want:          true,
+			errorContains: nil,
 		},
 		{
+			name: "Containment in Env",
 			envset: map[string]string{
 				"TEST_ENV1": "a",
 			},
-			when: "$TEST_ENV1 == 'b'",
-			want: false,
+			when:          "'TEST_ENV1' in Env",
+			want:          true,
+			errorContains: nil,
+		},
+		{
+			name: "Env var name is used in string literal",
+			envset: map[string]string{
+				"TEST_ENV1": "foo",
+				"TEST_ENV2": "$TEST_ENV1",
+			},
+			when:          `$TEST_ENV2 == '$TEST_ENV1'`,
+			want:          true,
+			errorContains: nil,
+		},
+		{
+			name:          "Env var not set",
+			envset:        map[string]string{},
+			when:          `$TEST_ENV_NONESUCH == ""`,
+			want:          true,
+			errorContains: nil,
+		},
+		{
+			name:          "Invalid expression",
+			envset:        map[string]string{},
+			when:          `($TEST_ENV1 == "Missing parentheses"`,
+			want:          false,
+			errorContains: "unexpected token EOF",
+		},
+		{
+			name:          "Expression produces a non-boolean result",
+			envset:        map[string]string{},
+			when:          `"String literal expression"`,
+			want:          false,
+			errorContains: "expected bool, but got string",
 		},
 		{
+			name: "Expression references an unknown variable",
 			envset: map[string]string{
 				"TEST_ENV1": "a",
 			},
-			when: `$TEST_ENV1 == "a"`,
-			want: true,
+			when:          `$TEST_ENV1 == NoneSuchVariable`,
+			want:          false,
+			errorContains: "unknown name NoneSuchVariable",
 		},
 	}
 	for _, tt := range tests {
-		for k, v := range tt.envset {
-			os.Setenv(k, v)
-		}
-		got, err := IsAllowedToExecute(tt.when)
-		if err != nil {
-			t.Fatal(err)
-		}
-		if got != tt.want {
-			t.Errorf("got %v\nwant %v", got, tt.want)
-		}
+		t.Run(tt.name, func(t *testing.T) {
+			NewWhenEnv = func() *WhenEnv { return &WhenEnv{Env: tt.envset} }
+			got, err := IsAllowedToExecute(tt.when)
+			if err != nil {
+				if tt.errorContains != nil {
+					if !strings.Contains(err.Error(), tt.errorContains.(string)) {
+						t.Errorf("Error %v does not contain %s", err, tt.errorContains)
+					}
+				} else {
+					t.Error(err)
+				}
+			} else if tt.errorContains != nil {
+				t.Errorf("Expected an error containing %v", tt.errorContains)
+			}
+			if got != tt.want {
+				t.Errorf("got %v\nwant %v", got, tt.want)
+			}
+		})
 	}
 }