Editions fixes (#58)

Updates Protobuf to v27 and protovalidate to v0.7.1, and fixes all of the resulting compilation and conformance failures. As one would expect, there was a tremendous amount of troubleshooting involved in this thankfully-relatively-small PR. Here's my log of what happened. I'll try to be succinct, but I want to capture all of the details so my reasoning can be understood in the future. - First, I tried to update protobuf. This led to pulling a newer version of absl. The version of cel-cpp we use did not compile with this version of absl. - Next, I tried to update cel-cpp. However, the latest version of cel-cpp is broken on macOS for two separate reasons <sup>[1](google/cel-cpp#831), [2](https://github.com/google/cel-cpp/issues/832)</sup>. - After taking a break to work on other protovalidate implementations I returned and tried another approach. This time, instead of updating cel-cpp, I just patched it to work with newer absl. Thankfully, this proved surprisingly viable. The `cel_cpp.patch` file now contains this fix too. - Unfortunately, compilation was broken in CI on a non-sense compiler error: ``` error: could not convert template argument 'ptr' from 'const google::protobuf::Struct& (* const)()' to 'const google::protobuf::Struct& (* const)()' ``` It seemed likely to be a compiler issue, thus I was stalled again. - For some reason it finally occurred to me that I probably should just simply update the compiler. In a stroke of accidental rubber-ducking luck, I noticed that GitHub's `ubuntu-latest` had yet to actually move to `ubuntu-24.04`, which has a vastly more up-to-date C++ toolchain than the older `ubuntu-22.04`. This immediately fixed the problem. - E-mail validation is hard. In other languages we fall back on standard library functionality, but C++ puts us at a hard impasse; the C++ standard library hardly concerns itself with application-level functionality like SMTP standards. Anyway, I channeled my frustration at the lack of a consistent validation scheme for e-mail, which culminated into bufbuild/protovalidate#236. For the new failing test cases, we needed to improve the validation of localpart in C++. Lacking any specific reference point, I decided it would be acceptable if the C++ version started adopting ideas from WHATWG HTML email validation. It doesn't move the `localpart` validation to _entirely_ work like WHATWG HTML email validation, as our version still has our specific checks, but now we are a strict subset in protovalidate-cc, so we can remove our additional checks later if we can greenlight adopting the WHATWG HTML standard. - The remaining test failures are all related to ignoring validation rules and presence. The following changes were made: - The algorithm for ignoring empty fields is improved to match the specified behavior closer. - The `ignore` option is now taken into account in addition to the legacy `skipped` and `ignore_empty` options. - Support is added for `IGNORE_IF_DEFAULT_VALUE` - An edge case is added to ignore field presence on synthetic `Map` types. I haven't traced down why, but `has_presence` seems to always be true for fields of synthetic `Map` types in the C++ implementation. (Except in proto3?) And with that I think we will have working Editions support.
bufbuild · Aug 7, 2024 · 0e320b9 · 0e320b9
1 parent 5ba3f45
commit 0e320b9
Show file tree

Hide file tree

Showing 12 changed files with 158 additions and 42 deletions.
diff --git a/.bazelrc b/.bazelrc
@@ -1,6 +1,10 @@
 build --cxxopt=-std=c++17 --host_cxxopt=-std=c++17
 build --cxxopt=-fsized-deallocation
 
+# Ensure we use the protobuf compiler from deps
+build --proto_compiler=@com_google_protobuf//:protoc
+build --proto_toolchain_for_cc=@com_google_protobuf//:cc_toolchain
+
 # Enable matchers in googletest
 build --define absl=1
 

diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -14,7 +14,7 @@ jobs:
   test:
     strategy:
       matrix:
-        os: [ubuntu-latest, macos-latest]
+        os: [ubuntu-24.04, macos-latest]
     name: Unit tests
     runs-on: ${{ matrix.os }}
     steps:

diff --git a/.github/workflows/conformance.yaml b/.github/workflows/conformance.yaml
@@ -13,7 +13,7 @@ permissions:
 jobs:
   conformance:
     name: Conformance Testing
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-24.04
     steps:
       - name: Setup Cache
         uses: actions/cache@v3

diff --git a/Makefile b/Makefile
@@ -11,7 +11,7 @@ COPYRIGHT_YEARS := 2023
 LICENSE_IGNORE := -e internal/testdata/
 LICENSE_HEADER_VERSION := 0294fdbe1ce8649ebaf5e87e8cdd588e33730bbb
 # NOTE: Keep this version in sync with the version in `/bazel/deps.bzl`.
-PROTOVALIDATE_VERSION ?= v0.5.6
+PROTOVALIDATE_VERSION ?= v0.7.1
 
 # Set to use a different compiler. For example, `GO=go1.18rc1 make test`.
 GO ?= go

diff --git a/WORKSPACE b/WORKSPACE
@@ -4,6 +4,10 @@ load("//bazel:deps.bzl", "protovalidate_cc_dependencies")
 
 protovalidate_cc_dependencies()
 
+load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps")
+
+protobuf_deps()
+
 load("@rules_buf//buf:repositories.bzl", "rules_buf_dependencies", "rules_buf_toolchains")
 
 rules_buf_dependencies()
@@ -23,7 +27,7 @@ switched_rules_by_language(
     cc = True,
 )
 
-load("@com_github_protocolbuffers_protobuf//:protobuf_deps.bzl", "protobuf_deps")
+load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps")
 
 protobuf_deps()
 

diff --git a/bazel/cel_cpp.patch b/bazel/cel_cpp.patch
@@ -1,5 +1,35 @@
+diff --git a/base/memory_manager.cc b/base/memory_manager.cc
+index 1b7b3550..13b4ff97 100644
+--- a/base/memory_manager.cc
++++ b/base/memory_manager.cc
+@@ -45,6 +45,7 @@
+ #include "absl/base/config.h"
+ #include "absl/base/dynamic_annotations.h"
+ #include "absl/base/macros.h"
++#include "absl/base/optimization.h"
+ #include "absl/base/thread_annotations.h"
+ #include "absl/numeric/bits.h"
+ #include "absl/synchronization/mutex.h"
+@@ -234,7 +235,7 @@ class GlobalMemoryManager final : public MemoryManager {
+   void* Allocate(size_t size, size_t align) override {
+     static_cast<void>(size);
+     static_cast<void>(align);
+-    ABSL_INTERNAL_UNREACHABLE;
++    ABSL_UNREACHABLE();
+     return nullptr;
+   }
+
+@@ -242,7 +243,7 @@ class GlobalMemoryManager final : public MemoryManager {
+   void OwnDestructor(void* pointer, void (*destructor)(void*)) override {
+     static_cast<void>(pointer);
+     static_cast<void>(destructor);
+-    ABSL_INTERNAL_UNREACHABLE;
++    ABSL_UNREACHABLE();
+   }
+ };
+
 diff --git a/eval/eval/container_access_step.cc b/eval/eval/container_access_step.cc
-index 39c2507..d7962a2 100644
+index 39c2507d..d7962a29 100644
 --- a/eval/eval/container_access_step.cc
 +++ b/eval/eval/container_access_step.cc
 @@ -11,6 +11,8 @@
@@ -8,22 +38,22 @@ index 39c2507..d7962a2 100644
  #include "eval/public/unknown_attribute_set.h"
 +#include "eval/public/structs/legacy_type_adapter.h"
 +#include "eval/public/structs/legacy_type_info_apis.h"
-
+ 
  namespace google::api::expr::runtime {
-
+ 
 @@ -34,6 +36,8 @@ class ContainerAccessStep : public ExpressionStepBase {
                         ExecutionFrame* frame) const;
    CelValue LookupInList(const CelList* cel_list, const CelValue& key,
                          ExecutionFrame* frame) const;
 +  CelValue LookupInMessage(const CelValue::MessageWrapper& msg, const CelValue& key,
 +                           ExecutionFrame* frame) const;
  };
-
+ 
  inline CelValue ContainerAccessStep::LookupInMap(const CelMap* cel_map,
 @@ -109,6 +113,26 @@ inline CelValue ContainerAccessStep::LookupInList(const CelList* cel_list,
                     CelValue::TypeName(key.type())));
  }
-
+ 
 +CelValue ContainerAccessStep::LookupInMessage(const CelValue::MessageWrapper& msg, const CelValue& key,
 +                                              ExecutionFrame* frame) const {
 +  if (!key.IsString()) {

diff --git a/bazel/deps.bzl b/bazel/deps.bzl
@@ -16,19 +16,28 @@ load("@bazel_tools//tools/build_defs/repo:utils.bzl", "maybe")
 load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
 
 _dependencies = {
+    # This is needed due to an unresolved issue with protobuf v27+.
+    # https://github.com/protocolbuffers/protobuf/issues/17200
+    "rules_python": {
+        "sha256": "0a8003b044294d7840ac7d9d73eef05d6ceb682d7516781a4ec62eeb34702578",
+        "strip_prefix": "rules_python-0.24.0",
+        "urls": [
+            "https://github.com/bazelbuild/rules_python/releases/download/0.24.0/rules_python-0.24.0.tar.gz",
+        ],
+    },
     "bazel_skylib": {
         "sha256": "74d544d96f4a5bb630d465ca8bbcfe231e3594e5aae57e1edbf17a6eb3ca2506",
         "urls": [
             "https://github.com/bazelbuild/bazel-skylib/releases/download/1.3.0/bazel-skylib-1.3.0.tar.gz",
             "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/1.3.0/bazel-skylib-1.3.0.tar.gz",
         ],
     },
-    "com_github_protocolbuffers_protobuf": {
-        "sha256": "75be42bd736f4df6d702a0e4e4d30de9ee40eac024c4b845d17ae4cc831fe4ae",
-        "strip_prefix": "protobuf-21.7",
+    "com_google_protobuf": {
+        "sha256": "e4ff2aeb767da6f4f52485c2e72468960ddfe5262483879ef6ad552e52757a77",
+        "strip_prefix": "protobuf-27.2",
         "urls": [
-            "https://mirror.bazel.build/github.com/protocolbuffers/protobuf/archive/v21.7.tar.gz",
-            "https://github.com/protocolbuffers/protobuf/archive/v21.7.tar.gz",
+            "https://mirror.bazel.build/github.com/protocolbuffers/protobuf/archive/v27.2.tar.gz",
+            "https://github.com/protocolbuffers/protobuf/archive/v27.2.tar.gz",
         ],
     },
     "rules_proto": {
@@ -56,10 +65,10 @@ _dependencies = {
     },
     # NOTE: Keep Version in sync with `/Makefile`.
     "com_github_bufbuild_protovalidate": {
-        "sha256": "a6fd142c780c82104198138d609bace9b1b145c99e265aa33de1f651e90047d8",
-        "strip_prefix": "protovalidate-0.5.6",
+        "sha256": "ccb3952c38397d2cb53fe841af66b05fc012dd17fa754cbe35d9abb547cdf92d",
+        "strip_prefix": "protovalidate-0.7.1",
         "urls": [
-            "https://github.com/bufbuild/protovalidate/archive/v0.5.6.tar.gz",
+            "https://github.com/bufbuild/protovalidate/archive/v0.7.1.tar.gz",
         ],
     },
 }

diff --git a/buf/validate/internal/constraints.cc b/buf/validate/internal/constraints.cc
@@ -27,9 +27,70 @@
 #include "eval/public/structs/cel_proto_wrapper.h"
 #include "google/protobuf/any.pb.h"
 #include "google/protobuf/descriptor.pb.h"
+#include "google/protobuf/dynamic_message.h"
+#include "google/protobuf/util/message_differencer.h"
 
 namespace buf::validate::internal {
 namespace cel = google::api::expr;
+namespace {
+
+bool isEmptyItem(cel::runtime::CelValue item) {
+  switch (item.type()) {
+    case ::cel::Kind::kBool:
+      return !item.BoolOrDie();
+    case ::cel::Kind::kInt:
+      return item.Int64OrDie() == 0;
+    case ::cel::Kind::kUint:
+      return item.Uint64OrDie() == 0;
+    case ::cel::Kind::kDouble:
+      return item.DoubleOrDie() == 0;
+    case ::cel::Kind::kString:
+      return item.StringOrDie().value().empty();
+    case ::cel::Kind::kBytes:
+      return item.BytesOrDie().value().empty();
+    default:
+      return false;
+  }
+}
+
+bool isDefaultItem(cel::runtime::CelValue item, const google::protobuf::FieldDescriptor *field) {
+  using google::protobuf::FieldDescriptor;
+  using google::protobuf::DynamicMessageFactory;
+  using google::protobuf::util::MessageDifferencer;
+  switch (field->cpp_type()) {
+    case FieldDescriptor::CPPTYPE_INT32:
+      return item.IsInt64() && item.Int64OrDie() == field->default_value_int32();
+    case FieldDescriptor::CPPTYPE_INT64:
+      return item.IsInt64() && item.Int64OrDie() == field->default_value_int64();
+    case FieldDescriptor::CPPTYPE_UINT32:
+      return item.IsUint64() && item.Uint64OrDie() == field->default_value_uint32();
+    case FieldDescriptor::CPPTYPE_UINT64:
+      return item.IsUint64() && item.Uint64OrDie() == field->default_value_uint64();
+    case FieldDescriptor::CPPTYPE_DOUBLE:
+      return item.IsDouble() && item.DoubleOrDie() == field->default_value_double();
+    case FieldDescriptor::CPPTYPE_FLOAT:
+      return item.IsDouble() && item.DoubleOrDie() == field->default_value_float();
+    case FieldDescriptor::CPPTYPE_BOOL:
+      return item.IsBool() && item.BoolOrDie() == field->default_value_bool();
+    case FieldDescriptor::CPPTYPE_ENUM:
+      return item.IsInt64() && item.Int64OrDie() == field->default_value_enum()->number();
+    case FieldDescriptor::CPPTYPE_STRING:
+      return item.IsString() && item.StringOrDie().value() == field->default_value_string();
+    case FieldDescriptor::CPPTYPE_MESSAGE:
+      if (item.IsMessage()) {
+        DynamicMessageFactory dmf;
+        const auto *message = item.MessageOrDie();
+        auto* empty = dmf.GetPrototype(message->GetDescriptor())->New();
+        return MessageDifferencer::Equals(*message, *empty);
+      }
+      break;
+    default:
+      break;
+  }
+  return false;
+}
+
+}
 
 absl::StatusOr<std::unique_ptr<google::api::expr::runtime::CelExpressionBuilder>>
 NewConstraintBuilder(google::protobuf::Arena* arena) {
@@ -121,6 +182,10 @@ absl::Status FieldConstraintRules::Validate(
     if (!status.ok()) {
       return status;
     }
+
+    if (ignoreDefault_ && isDefaultItem(result, field_)) {
+      return absl::OkStatus();
+    }
   }
   activation.InsertValue("this", result);
   return ValidateCel(ctx, field_->name(), activation);
@@ -143,25 +208,6 @@ absl::Status EnumConstraintRules::Validate(
   return absl::OkStatus();
 }
 
-bool isEmptyItem(cel::runtime::CelValue item) {
-  switch (item.type()) {
-  case ::cel::Kind::kBool:
-    return !item.BoolOrDie();
-  case ::cel::Kind::kInt:
-    return item.Int64OrDie() == 0;
-  case ::cel::Kind::kUint:
-    return item.Uint64OrDie() == 0;
-  case ::cel::Kind::kDouble:
-    return item.DoubleOrDie() == 0;
-  case ::cel::Kind::kString:
-    return item.StringOrDie().value() == "";
-  case ::cel::Kind::kBytes:
-    return item.BytesOrDie().value() == "";
-  default:
-    return false;
-  }
-}
-
 absl::Status RepeatedConstraintRules::Validate(
     ConstraintContext& ctx, const google::protobuf::Message& message) const {
   auto status = Base::Validate(ctx, message);
@@ -176,6 +222,9 @@ absl::Status RepeatedConstraintRules::Validate(
     if (itemRules_->getIgnoreEmpty() && isEmptyItem(item)) {
       continue;
     }
+    if (itemRules_->getIgnoreDefault() && isDefaultItem(item, field_)) {
+      continue;
+    }
     cel::runtime::Activation activation;
     activation.InsertValue("this", item);
     int pos = ctx.violations.violations_size();

diff --git a/buf/validate/internal/constraints.h b/buf/validate/internal/constraints.h
@@ -40,7 +40,13 @@ class FieldConstraintRules : public CelConstraintRules {
       const FieldConstraints& field,
       const AnyRules* anyRules = nullptr)
       : field_(desc),
-        ignoreEmpty_(field.ignore_empty() || desc->has_presence()),
+        mapEntryField_(desc->containing_type()->options().map_entry()),
+        ignoreEmpty_(field.ignore() == IGNORE_IF_DEFAULT_VALUE ||
+                     field.ignore() == IGNORE_IF_UNPOPULATED ||
+                     field.ignore_empty() ||
+                     (desc->has_presence() && !mapEntryField_)),
+        ignoreDefault_(field.ignore() == IGNORE_IF_DEFAULT_VALUE &&
+                       (desc->has_presence() && !mapEntryField_)),
         required_(field.required()),
         anyRules_(anyRules) {}
 
@@ -56,11 +62,15 @@ class FieldConstraintRules : public CelConstraintRules {
 
   [[nodiscard]] const AnyRules* getAnyRules() const { return anyRules_; }
 
-  [[nodiscard]] const bool getIgnoreEmpty() const { return ignoreEmpty_; }
+  [[nodiscard]] bool getIgnoreEmpty() const { return ignoreEmpty_; }
+
+  [[nodiscard]] bool getIgnoreDefault() const { return ignoreDefault_; }
 
  protected:
   const google::protobuf::FieldDescriptor* field_ = nullptr;
+  bool mapEntryField_ = false;
   bool ignoreEmpty_ = false;
+  bool ignoreDefault_ = false;
   bool required_ = false;
   const AnyRules* anyRules_ = nullptr;
 };

diff --git a/buf/validate/internal/extra_func.cc b/buf/validate/internal/extra_func.cc
@@ -189,6 +189,13 @@ cel::CelValue isEmail(google::protobuf::Arena* arena, cel::CelValue::StringHolde
     return cel::CelValue::CreateBool(false);
   }
 
+  // Based on https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address
+  // Note that we are currently _stricter_ than this as we enforce length limits
+  static const re2::RE2 localPart_regex("^[a-zA-Z0-9.!#$%&'*+\\/=?^_`{|}~-]+$");
+  if (!re2::RE2::FullMatch(localPart, localPart_regex)) {
+    return cel::CelValue::CreateBool(false);
+  }
+
   // Validate the hostname
   return cel::CelValue::CreateBool(IsHostname(domainPart));
 }

diff --git a/buf/validate/internal/field_rules.cc b/buf/validate/internal/field_rules.cc
@@ -23,7 +23,7 @@ absl::StatusOr<std::unique_ptr<FieldConstraintRules>> NewFieldRules(
     google::api::expr::runtime::CelExpressionBuilder& builder,
     const google::protobuf::FieldDescriptor* field,
     const FieldConstraints& fieldLvl) {
-  if (fieldLvl.skipped()) {
+  if (fieldLvl.ignore() == IGNORE_ALWAYS || fieldLvl.skipped()) {
     return nullptr;
   }
   absl::StatusOr<std::unique_ptr<FieldConstraintRules>> rules_or;

diff --git a/buf/validate/validator.cc b/buf/validate/validator.cc
@@ -46,9 +46,12 @@ absl::Status Validator::ValidateFields(
     }
     if (field->options().HasExtension(validate::field)) {
       const auto& fieldExt = field->options().GetExtension(validate::field);
-      if (fieldExt.skipped() ||
-          (fieldExt.has_repeated() && fieldExt.repeated().items().skipped()) ||
-          (fieldExt.has_map() && fieldExt.map().values().skipped())) {
+      if (fieldExt.ignore() == IGNORE_ALWAYS ||
+          fieldExt.skipped() ||
+          (fieldExt.has_repeated() && (fieldExt.repeated().items().ignore() == IGNORE_ALWAYS ||
+                                       fieldExt.repeated().items().skipped())) ||
+          (fieldExt.has_map() && (fieldExt.map().values().ignore() == IGNORE_ALWAYS ||
+                                  fieldExt.map().values().skipped()))) {
         continue;
       }
     }