Fix Prevent event-watcher JSON/HCL replacements from truncating files #7394

.gitignore

@@ -67,3 +67,5 @@ gomock_reflect_*/
 
 # hack
 hack/oidc/realm.local.json
+
+WORKABLE.md

docs/content/en/docs-v1.0.x/user-guide/managing-application/configuration-reference.md

@@ -214,14 +214,12 @@ At least one of `name`, `kind`, or `labels` must be set.
 
 ## EventWatcherReplacement
 
-Only one of `yamlField`, `jsonField`, `HCLField`, or `regex` may be set alongside `file`.
+Only one of `yamlField` or `regex` may be set alongside `file`.
 
 | Field | Type | Description | Required |
 | --- | --- | --- | --- |
 | `file` | string | Path to the file to update. | Yes |
 | `yamlField` | string | YAML path to the field to update. Must start with `$`. e.g. `$.foo.bar[0].baz`. | No |
-| `jsonField` | string | JSON path to the field to update. | No |
-| `HCLField` | string | HCL path to the field to update. | No |
 | `regex` | string | Regular expression specifying what to replace. Only the first capturing group `()` is replaced. e.g. `host.xz/foo/bar:(v[0-9].[0-9].[0-9])`. | No |
 
 ## DriftDetection

pkg/app/piped/eventwatcher/eventwatcher.go

@@ -689,9 +689,9 @@ func (w *watcher) commitFiles(ctx context.Context, latestEvent *model.Event, eve
 		case r.YAMLField != "":
 			newContent, upToDate, err = modifyYAML(path, r.YAMLField, latestEvent.Data)
 		case r.JSONField != "":
-			// TODO: Empower Event watcher to parse JSON format
+			return "", fmt.Errorf("jsonField replacements are not supported")
 		case r.HCLField != "":
-			// TODO: Empower Event watcher to parse HCL format
+			return "", fmt.Errorf("HCLField replacements are not supported")
 		case r.Regex != "":
 			newContent, upToDate, err = modifyText(path, r.Regex, latestEvent.Data)
 		}
@@ -703,10 +703,6 @@ func (w *watcher) commitFiles(ctx context.Context, latestEvent *model.Event, eve
 			continue
 		}
 
-		if err := os.WriteFile(path, newContent, os.ModePerm); err != nil {
-			w.logger.Error("failed to write file", zap.Error(err))
-			return "", err
-		}
 		changes[filePath] = newContent
 	}
 	if len(changes) == 0 {

pkg/app/piped/eventwatcher/eventwatcher_test.go

@@ -15,9 +15,17 @@
 package eventwatcher
 
 import (
+	"context"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"go.uber.org/mock/gomock"
+
+	config "github.com/pipe-cd/pipecd/pkg/config"
+	"github.com/pipe-cd/pipecd/pkg/git/gittest"
+	"github.com/pipe-cd/pipecd/pkg/model"
 )
 
 func TestConvertStr(t *testing.T) {
@@ -81,6 +89,63 @@ func TestConvertStr(t *testing.T) {
 	}
 }
 
+func TestCommitFilesDoesNotTruncateUnsupportedReplacementFiles(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		name        string
+		replacement config.EventWatcherReplacement
+		wantError   string
+	}{
+		{
+			name: "JSON field",
+			replacement: config.EventWatcherReplacement{
+				File:      "version.json",
+				JSONField: "$.image",
+			},
+			wantError: "jsonField replacements are not supported",
+		},
+		{
+			name: "HCL field",
+			replacement: config.EventWatcherReplacement{
+				File:     "version.hcl",
+				HCLField: "image",
+			},
+			wantError: "HCLField replacements are not supported",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			dir := t.TempDir()
+			path := filepath.Join(dir, tc.replacement.File)
+			original := []byte("must not be truncated\n")
+			assert.NoError(t, os.WriteFile(path, original, 0o600))
+
+			ctrl := gomock.NewController(t)
+			repo := gittest.NewMockRepo(ctrl)
+			repo.EXPECT().GetPath().Return(dir)
+
+			w := &watcher{}
+			_, err := w.commitFiles(
+				context.Background(),
+				&model.Event{Data: "new-value"},
+				"image-update",
+				"",
+				"",
+				[]config.EventWatcherReplacement{tc.replacement},
+				repo,
+				false,
+			)
+			assert.EqualError(t, err, tc.wantError)
+
+			actual, readErr := os.ReadFile(path)
+			assert.NoError(t, readErr)
+			assert.Equal(t, original, actual)
+		})
+	}
+}
+
 func TestModifyYAML(t *testing.T) {
 	t.Parallel()
 

pkg/app/pipedv1/eventwatcher/eventwatcher.go

@@ -625,9 +625,9 @@ func (w *watcher) commitFiles(ctx context.Context, latestEvent *model.Event, eve
 		case r.YAMLField != "":
 			newContent, upToDate, err = modifyYAML(path, r.YAMLField, latestEvent.Data)
 		case r.JSONField != "":
-			// TODO: Empower Event watcher to parse JSON format
+			return "", fmt.Errorf("jsonField replacements are not supported")
 		case r.HCLField != "":
-			// TODO: Empower Event watcher to parse HCL format
+			return "", fmt.Errorf("HCLField replacements are not supported")
 		case r.Regex != "":
 			newContent, upToDate, err = modifyText(path, r.Regex, latestEvent.Data)
 		}
@@ -638,9 +638,6 @@ func (w *watcher) commitFiles(ctx context.Context, latestEvent *model.Event, eve
 			continue
 		}
 
-		if err := os.WriteFile(path, newContent, os.ModePerm); err != nil {
-			return "", fmt.Errorf("failed to write file: %w", err)
-		}
 		changes[filePath] = newContent
 	}
 	if len(changes) == 0 {

pkg/app/pipedv1/eventwatcher/eventwatcher_test.go

@@ -15,9 +15,17 @@
 package eventwatcher
 
 import (
+	"context"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"go.uber.org/mock/gomock"
+
+	config "github.com/pipe-cd/pipecd/pkg/configv1"
+	"github.com/pipe-cd/pipecd/pkg/git/gittest"
+	"github.com/pipe-cd/pipecd/pkg/model"
 )
 
 func TestConvertStr(t *testing.T) {
@@ -81,6 +89,63 @@ func TestConvertStr(t *testing.T) {
 	}
 }
 
+func TestCommitFilesDoesNotTruncateUnsupportedReplacementFiles(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		name        string
+		replacement config.EventWatcherReplacement
+		wantError   string
+	}{
+		{
+			name: "JSON field",
+			replacement: config.EventWatcherReplacement{
+				File:      "version.json",
+				JSONField: "$.image",
+			},
+			wantError: "jsonField replacements are not supported",
+		},
+		{
+			name: "HCL field",
+			replacement: config.EventWatcherReplacement{
+				File:     "version.hcl",
+				HCLField: "image",
+			},
+			wantError: "HCLField replacements are not supported",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			dir := t.TempDir()
+			path := filepath.Join(dir, tc.replacement.File)
+			original := []byte("must not be truncated\n")
+			assert.NoError(t, os.WriteFile(path, original, 0o600))
+
+			ctrl := gomock.NewController(t)
+			repo := gittest.NewMockRepo(ctrl)
+			repo.EXPECT().GetPath().Return(dir)
+
+			w := &watcher{}
+			_, err := w.commitFiles(
+				context.Background(),
+				&model.Event{Data: "new-value"},
+				"image-update",
+				"",
+				"",
+				[]config.EventWatcherReplacement{tc.replacement},
+				repo,
+				false,
+			)
+			assert.EqualError(t, err, tc.wantError)
+
+			actual, readErr := os.ReadFile(path)
+			assert.NoError(t, readErr)
+			assert.Equal(t, original, actual)
+		})
+	}
+}
+
 func TestModifyYAML(t *testing.T) {
 	t.Parallel()
 

pkg/configv1/application.go

@@ -157,6 +157,12 @@ func (s *GenericApplicationSpec) Validate() error {
 		}
 	}
 
+	for _, ew := range s.EventWatcher {
+		if err := ew.Validate(); err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 

pkg/configv1/application_test.go

@@ -545,6 +545,29 @@ func TestGenericPostSyncConfiguration(t *testing.T) {
 	}
 }
 
+func TestGenericApplicationSpecValidatesEventWatcher(t *testing.T) {
+	t.Parallel()
+
+	spec := GenericApplicationSpec{
+		EventWatcher: []EventWatcherConfig{
+			{
+				Handler: EventWatcherHandler{
+					Config: EventWatcherHandlerConfig{
+						Replacements: []EventWatcherReplacement{
+							{
+								File:      "version.json",
+								JSONField: "$.image",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	assert.Error(t, spec.Validate())
+}
+
 func TestGetStageConfigByte(t *testing.T) {
 	testcases := []struct {
 		name   string

pkg/configv1/event_watcher.go

@@ -70,6 +70,22 @@ type EventWatcherHandlerConfig struct {
 	Replacements []EventWatcherReplacement `json:"replacements"`
 }
 
+func (c EventWatcherConfig) Validate() error {
+	if err := c.Handler.Config.Validate(); err != nil {
+		return fmt.Errorf("invalid event watcher handler config: %w", err)
+	}
+	return nil
+}
+
+func (c EventWatcherHandlerConfig) Validate() error {
+	for _, r := range c.Replacements {
+		if err := r.Validate(); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 type EventWatcherReplacement struct {
 	// The path to the file to be updated.
 	File string `json:"file"`
@@ -88,6 +104,33 @@ type EventWatcherReplacement struct {
 	Regex string `json:"regex"`
 }
 
+func (r EventWatcherReplacement) Validate() error {
+	if r.File == "" {
+		return fmt.Errorf("replacement has no file name")
+	}
+	if r.JSONField != "" {
+		return fmt.Errorf("replacement has an unsupported jsonField")
+	}
+	if r.HCLField != "" {
+		return fmt.Errorf("replacement has an unsupported HCLField")
+	}
+
+	count := 0
+	if r.YAMLField != "" {
+		count++
+	}
+	if r.Regex != "" {
+		count++
+	}
+	if count == 0 {
+		return fmt.Errorf("replacement has no field")
+	}
+	if count > 1 {
+		return fmt.Errorf("replacement has multiple fields")
+	}
+	return nil
+}
+
 // EventWatcherHandlerType represents the type of an event watcher handler.
 type EventWatcherHandlerType string
 
@@ -202,28 +245,8 @@ func (e *EventWatcherEvent) Validate() error {
 		return fmt.Errorf("there must be at least one replacement to an event")
 	}
 	for _, r := range e.Replacements {
-		if r.File == "" {
-			return fmt.Errorf("event %q has a replacement with no file name", e.Name)
-		}
-
-		var count int
-		if r.YAMLField != "" {
-			count++
-		}
-		if r.JSONField != "" {
-			count++
-		}
-		if r.HCLField != "" {
-			count++
-		}
-		if r.Regex != "" {
-			count++
-		}
-		if count == 0 {
-			return fmt.Errorf("event %q has a replacement with no field", e.Name)
-		}
-		if count > 2 {
-			return fmt.Errorf("event %q has multiple fields", e.Name)
+		if err := r.Validate(); err != nil {
+			return fmt.Errorf("event %q: %w", e.Name, err)
 		}
 	}
 	return nil