[ValidationErrors] de-dup error messages (#41198)

Also mark the test as non-polling, so we don't run it multiple times.

Closes #41198

COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/41198 from markdroth:validation_errors_no_dups 2ebe0f9846e9199a9ec1a3bfceeab58b7d3b65f9
PiperOrigin-RevId: 841792148

diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml
index 9c94bceae3b5abf4c46ee60dedd5b21fc6d7b01d..7b4dc7ecbb117ddadcbd5fb830c854cd2d93933f 100644 (file)
--- a/build_autogenerated.yaml
+++ b/build_autogenerated.yaml
@@ -27230,6 +27230,7 @@ targets:
   deps:
   - gtest
   - grpc_test_util
+  uses_polling: false
 - name: varint_test
   gtest: true
   build: test

diff --git a/src/core/util/validation_errors.cc b/src/core/util/validation_errors.cc
index 850ea674a5faece6131d06bc5e4b9d69b6230a6b..5d9f7f1420cbde5fecf6a234d4daebbb2fdefb7e 100644 (file)
--- a/src/core/util/validation_errors.cc
+++ b/src/core/util/validation_errors.cc
@@ -42,7 +42,7 @@ void ValidationErrors::AddError(absl::string_view error) {
             << max_error_count_ << ")";
     return;
   }
-  field_errors_[key].emplace_back(error);
+  field_errors_[key].emplace(error);
 }
 
 bool ValidationErrors::FieldHasErrors() const {
@@ -64,7 +64,7 @@ std::string ValidationErrors::message(absl::string_view prefix) const {
                                        absl::StrJoin(field_errors, "; "), "]"));
     } else {
       errors.emplace_back(
-          absl::StrCat("field:", field, " error:", field_errors[0]));
+          absl::StrCat("field:", field, " error:", *field_errors.begin()));
     }
   }
   return absl::StrCat(prefix, ": [", absl::StrJoin(errors, "; "), "]");

diff --git a/src/core/util/validation_errors.h b/src/core/util/validation_errors.h
index 4038d927c9ae74b139a95c036465e58c303d675a..b9348c3ceaa653096b82c7a7e15b1a2d853b9112 100644 (file)
--- a/src/core/util/validation_errors.h
+++ b/src/core/util/validation_errors.h
@@ -19,6 +19,7 @@
 #include <stddef.h>
 
 #include <map>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -129,9 +130,7 @@ class ValidationErrors {
   void PopField() GPR_ATTRIBUTE_NOINLINE;
 
   // Errors that we have encountered so far, keyed by field name.
-  // TODO(roth): If we don't actually have any fields for which we
-  // report more than one error, simplify this data structure.
-  std::map<std::string /*field_name*/, std::vector<std::string>> field_errors_;
+  std::map<std::string /*field_name*/, std::set<std::string>> field_errors_;
   // Stack of field names indicating the field that we are currently
   // validating.
   std::vector<std::string> fields_;

diff --git a/test/core/util/BUILD b/test/core/util/BUILD
index 5a361cb883ebb704bd8a50b898b5acc181c10cfd..e4eb33c5a9f5dfd340bbf11ad5dd2818a934cdd3 100644 (file)
--- a/test/core/util/BUILD
+++ b/test/core/util/BUILD
@@ -556,6 +556,8 @@ grpc_cc_test(
     external_deps = [
         "gtest",
     ],
+    uses_event_engine = False,
+    uses_polling = False,
     deps = [
         "//src/core:validation_errors",
         "//test/core/test_util:grpc_test_util",

diff --git a/test/core/util/validation_errors_test.cc b/test/core/util/validation_errors_test.cc
index 6df515209470a9f7047285683de8e9889341d20f..861fc33aab3f6f0c1c51d0aaf89812008c4139ff 100644 (file)
--- a/test/core/util/validation_errors_test.cc
+++ b/test/core/util/validation_errors_test.cc
@@ -77,7 +77,27 @@ TEST(ValidationErrors, MultipleErrorsForSameField) {
   EXPECT_EQ(status.code(), absl::StatusCode::kInvalidArgument);
   EXPECT_EQ(status.message(),
             "errors validating config: [field:foo.bar errors:["
-            "value smells funny; value is ugly]]")
+            "value is ugly; value smells funny]]")
+      << status;
+}
+
+TEST(ValidationErrors, DedupsErrors) {
+  ValidationErrors errors;
+  {
+    ValidationErrors::ScopedField field(&errors, "foo");
+    {
+      ValidationErrors::ScopedField field(&errors, ".bar");
+      errors.AddError("value is ugly");
+      errors.AddError("value is ugly");
+    }
+  }
+  EXPECT_FALSE(errors.ok());
+  EXPECT_EQ(errors.size(), 1);
+  absl::Status status = errors.status(absl::StatusCode::kInvalidArgument,
+                                      "errors validating config");
+  EXPECT_EQ(status.code(), absl::StatusCode::kInvalidArgument);
+  EXPECT_EQ(status.message(),
+            "errors validating config: [field:foo.bar error:value is ugly]")
       << status;
 }
 

diff --git a/test/core/xds/xds_listener_resource_type_test.cc b/test/core/xds/xds_listener_resource_type_test.cc
index 3099346cf662f2ce2b24ef98736f4947737959e5..cc92a4ec4724ae9bce93c6c721f21a38422b389f 100644 (file)
--- a/test/core/xds/xds_listener_resource_type_test.cc
+++ b/test/core/xds/xds_listener_resource_type_test.cc
@@ -696,14 +696,14 @@ TEST_P(HttpConnectionManagerTest, TerminalFilterNotLast) {
                    ".value["
                    "envoy.extensions.filters.network.http_connection_manager.v3"
                    ".HttpConnectionManager].http_filters errors:["
-                   "terminal filter for config type "
-                   "envoy.extensions.filters.http.router.v3.Router must be the "
-                   "last filter in the chain; "
                    "non-terminal filter for config type ",
                    (GetParam().in_api_listener
                         ? "envoy.extensions.filters.http.fault.v3.HTTPFault"
                         : "envoy.extensions.filters.http.rbac.v3.RBAC"),
-                   " is the last filter in the chain]]"))
+                   " is the last filter in the chain; "
+                   "terminal filter for config type "
+                   "envoy.extensions.filters.http.router.v3.Router must be the "
+                   "last filter in the chain]]"))
       << decode_result.resource.status();
 }
 

diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json
index bf78c0c5b20253f2cc0cd2e73ceea2024e2caa8c..7c5aed8e1744843d15596dc023890369a8d24578 100644 (file)
--- a/tools/run_tests/generated/tests.json
+++ b/tools/run_tests/generated/tests.json
@@ -10303,7 +10303,7 @@
       "posix",
       "windows"
     ],
-    "uses_polling": true
+    "uses_polling": false
   },
   {
     "args": [],