From df918eaffb6c68f239d45efe8facd5e940b44311 Mon Sep 17 00:00:00 2001
From: Chris Roche <github@rodaine.com>
Date: Tue, 7 May 2024 14:04:50 -0700
Subject: [PATCH] Migrator: properly handle ignore generation on fields (#209)

`ignore` field migration (from `ignore_empty` and `skipped`) was
preserving the field type (e.g., `string` or `repeated`) in the new
format, which is erroneous. `required` was already handling this
correctly, `ignore` just slipped through the cracks here.

Fixes #202

Thanks again @pquerna & @jschaf for the reports!
---
 .../migrate/field_ignore_skip.migrated.proto  | 51 +++++++++++++++++++
 .../migrate/field_ignore_skip.source.proto    | 38 ++++++++++++++
 .../internal/migrator/field.go                | 11 ++--
 .../internal/migrator/migrator_test.go        |  4 ++
 4 files changed, 96 insertions(+), 8 deletions(-)
 create mode 100644 proto/protovalidate-testing/tests/migrate/field_ignore_skip.migrated.proto
 create mode 100644 proto/protovalidate-testing/tests/migrate/field_ignore_skip.source.proto

diff --git a/proto/protovalidate-testing/tests/migrate/field_ignore_skip.migrated.proto b/proto/protovalidate-testing/tests/migrate/field_ignore_skip.migrated.proto
new file mode 100644
index 00000000..e8bf3652
--- /dev/null
+++ b/proto/protovalidate-testing/tests/migrate/field_ignore_skip.migrated.proto
@@ -0,0 +1,51 @@
+// Copyright 2023 Buf Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+syntax = "proto3";
+
+package tests.migrate;
+
+import "validate/validate.proto";
+
+import "buf/validate/validate.proto";
+
+message IgnoreAndSkips {
+  string x = 1 [
+    (validate.rules).string = {
+      ignore_empty: true,
+      pattern: '^a+$',
+    },
+    (buf.validate.field).string = {
+      pattern: '^a+$',
+    }
+    , (buf.validate.field).ignore = IGNORE_IF_UNPOPULATED
+  ];
+  repeated string y = 2 [
+    (validate.rules).repeated = {
+      ignore_empty: true,
+      unique: true,
+      items: { string: { in: ['a', 'b'] } }
+    },
+    (buf.validate.field).repeated = {
+      unique: true,
+      items: { string: { in: ['a', 'b'] } }
+    }
+    , (buf.validate.field).ignore = IGNORE_IF_UNPOPULATED
+  ];
+  IgnoreAndSkips z = 3 [
+    (validate.rules).message = { skip: true },
+    (buf.validate.field) = { }
+    , (buf.validate.field).ignore = IGNORE_ALWAYS
+  ];
+}
diff --git a/proto/protovalidate-testing/tests/migrate/field_ignore_skip.source.proto b/proto/protovalidate-testing/tests/migrate/field_ignore_skip.source.proto
new file mode 100644
index 00000000..294b8cf6
--- /dev/null
+++ b/proto/protovalidate-testing/tests/migrate/field_ignore_skip.source.proto
@@ -0,0 +1,38 @@
+// Copyright 2023 Buf Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+syntax = "proto3";
+
+package tests.migrate;
+
+import "validate/validate.proto";
+
+message IgnoreAndSkips {
+  string x = 1 [
+    (validate.rules).string = {
+      ignore_empty: true,
+      pattern: '^a+$',
+    }
+  ];
+  repeated string y = 2 [
+    (validate.rules).repeated = {
+      ignore_empty: true,
+      unique: true,
+      items: { string: { in: ['a', 'b'] } }
+    }
+  ];
+  IgnoreAndSkips z = 3 [
+    (validate.rules).message = { skip: true }
+  ];
+}
diff --git a/tools/protovalidate-migrate/internal/migrator/field.go b/tools/protovalidate-migrate/internal/migrator/field.go
index b9a29108..043380c8 100644
--- a/tools/protovalidate-migrate/internal/migrator/field.go
+++ b/tools/protovalidate-migrate/internal/migrator/field.go
@@ -16,8 +16,6 @@ package migrator
 
 import (
 	"bytes"
-	"fmt"
-	"strings"
 
 	"github.com/bufbuild/protocompile/ast"
 	"github.com/bufbuild/protovalidate/tools/internal/gen/buf/validate"
@@ -214,14 +212,11 @@ func HandleMessageLiteral(
 	}
 
 	if msgLitVisitor.ignoreNeeded != nil {
+		newName := ", ignore"
 		if prefixed {
-			newName := printer.file.NodeInfo(name).RawText()
-			newName = strings.Replace(newName, "(validate.rules)", "(buf.validate.field)", 1)
-			newName = fmt.Sprintf(", %s.ignore", newName)
-			name = printer.replaceNode(name, newName)
-		} else {
-			name = printer.replaceNode(name, ", ignore")
+			newName = ", (buf.validate.field).ignore"
 		}
+		name = printer.replaceNode(name, newName)
 
 		if err := printer.PrintNodes(false,
 			name,
diff --git a/tools/protovalidate-migrate/internal/migrator/migrator_test.go b/tools/protovalidate-migrate/internal/migrator/migrator_test.go
index c4b2b390..83d60876 100644
--- a/tools/protovalidate-migrate/internal/migrator/migrator_test.go
+++ b/tools/protovalidate-migrate/internal/migrator/migrator_test.go
@@ -131,6 +131,10 @@ func TestMigrator(t *testing.T) {
 			srcPath: "field_ignore_empty.source.proto",
 			dstPath: "field_ignore_empty.migrated.proto",
 		},
+		{
+			srcPath: "field_ignore_skip.source.proto",
+			dstPath: "field_ignore_skip.migrated.proto",
+		},
 		{
 			srcPath: "no_separator.source.proto",
 			dstPath: "no_separator.migrated.proto",