From 7a34e14e0810ed32d7a4003d4e3426e02bbc621f Mon Sep 17 00:00:00 2001 From: John Chadwick Date: Wed, 13 Nov 2024 15:20:18 -0500 Subject: [PATCH 1/3] Implement structured field and rule paths in ValidationError --- Makefile | 3 +- bazel/deps.bzl | 7 +- buf/validate/internal/cel_constraint_rules.cc | 18 ++- buf/validate/internal/cel_constraint_rules.h | 7 +- buf/validate/internal/cel_rules.h | 40 +++++- buf/validate/internal/constraint_rules.h | 16 ++- buf/validate/internal/constraints.cc | 81 ++++++++--- buf/validate/internal/constraints.h | 126 ++++++++++++++---- buf/validate/internal/constraints_test.cc | 15 +-- buf/validate/internal/field_rules.cc | 2 +- buf/validate/internal/message_rules.cc | 2 +- buf/validate/validator.cc | 28 +++- 12 files changed, 262 insertions(+), 83 deletions(-) diff --git a/Makefile b/Makefile index e0bb809..e950be5 100644 --- a/Makefile +++ b/Makefile @@ -11,7 +11,8 @@ 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.8.1 +# DO NOT MERGE +PROTOVALIDATE_VERSION ?= 9fbcb20937fb97609f5711d25474213241694294 # Set to use a different compiler. For example, `GO=go1.18rc1 make test`. GO ?= go diff --git a/bazel/deps.bzl b/bazel/deps.bzl index 82a7167..b8a15d2 100644 --- a/bazel/deps.bzl +++ b/bazel/deps.bzl @@ -65,10 +65,11 @@ _dependencies = { }, # NOTE: Keep Version in sync with `/Makefile`. "com_github_bufbuild_protovalidate": { - "sha256": "c637c8cbaf71b6dc38171e47c2c736581b4cfef385984083561480367659d14f", - "strip_prefix": "protovalidate-0.8.1", + # "sha256": "c637c8cbaf71b6dc38171e47c2c736581b4cfef385984083561480367659d14f", + "strip_prefix": "protovalidate-9fbcb20937fb97609f5711d25474213241694294", "urls": [ - "https://github.com/bufbuild/protovalidate/archive/v0.8.1.tar.gz", + # DO NOT MERGE + "https://github.com/bufbuild/protovalidate/archive/9fbcb20937fb97609f5711d25474213241694294.tar.gz", ], }, } diff --git a/buf/validate/internal/cel_constraint_rules.cc b/buf/validate/internal/cel_constraint_rules.cc index bd467d3..33c35d7 100644 --- a/buf/validate/internal/cel_constraint_rules.cc +++ b/buf/validate/internal/cel_constraint_rules.cc @@ -28,7 +28,6 @@ namespace { absl::Status ProcessConstraint( ConstraintContext& ctx, - absl::string_view fieldPath, const google::api::expr::runtime::BaseActivation& activation, const CompiledConstraint& expr) { auto result_or = expr.expr->Evaluate(activation, ctx.arena); @@ -40,17 +39,21 @@ absl::Status ProcessConstraint( if (!result.BoolOrDie()) { // Add violation with the constraint message. Violation& violation = *ctx.violations.add_violations(); - *violation.mutable_field_path() = fieldPath; violation.set_message(expr.constraint.message()); violation.set_constraint_id(expr.constraint.id()); + if (expr.rulePath.has_value()) { + *violation.mutable_rule() = *expr.rulePath; + } } } else if (result.IsString()) { if (!result.StringOrDie().value().empty()) { // Add violation with custom message. Violation& violation = *ctx.violations.add_violations(); - *violation.mutable_field_path() = fieldPath; violation.set_message(std::string(result.StringOrDie().value())); violation.set_constraint_id(expr.constraint.id()); + if (expr.rulePath.has_value()) { + *violation.mutable_rule() = *expr.rulePath; + } } } else if (result.IsError()) { const cel::runtime::CelError& error = *result.ErrorOrDie(); @@ -85,6 +88,7 @@ cel::runtime::CelValue ProtoFieldToCelValue( absl::Status CelConstraintRules::Add( google::api::expr::runtime::CelExpressionBuilder& builder, Constraint constraint, + absl::optional rulePath, const google::protobuf::FieldDescriptor* rule) { auto pexpr_or = cel::parser::Parse(constraint.expression()); if (!pexpr_or.ok()) { @@ -96,7 +100,7 @@ absl::Status CelConstraintRules::Add( return expr_or.status(); } std::unique_ptr expr = std::move(expr_or).value(); - exprs_.emplace_back(CompiledConstraint{std::move(constraint), std::move(expr), rule}); + exprs_.emplace_back(CompiledConstraint{std::move(constraint), std::move(expr), std::move(rulePath), rule}); return absl::OkStatus(); } @@ -105,17 +109,17 @@ absl::Status CelConstraintRules::Add( std::string_view id, std::string_view message, std::string_view expression, + absl::optional rulePath, const google::protobuf::FieldDescriptor* rule) { Constraint constraint; *constraint.mutable_id() = id; *constraint.mutable_message() = message; *constraint.mutable_expression() = expression; - return Add(builder, constraint, rule); + return Add(builder, constraint, std::move(rulePath), rule); } absl::Status CelConstraintRules::ValidateCel( ConstraintContext& ctx, - std::string_view fieldName, google::api::expr::runtime::Activation& activation) const { activation.InsertValue("rules", rules_); activation.InsertValue("now", cel::runtime::CelValue::CreateTimestamp(absl::Now())); @@ -126,7 +130,7 @@ absl::Status CelConstraintRules::ValidateCel( activation.InsertValue( "rule", ProtoFieldToCelValue(rules_.MessageOrDie(), expr.rule, ctx.arena)); } - status = ProcessConstraint(ctx, fieldName, activation, expr); + status = ProcessConstraint(ctx, activation, expr); if (ctx.shouldReturn(status)) { break; } diff --git a/buf/validate/internal/cel_constraint_rules.h b/buf/validate/internal/cel_constraint_rules.h index 8e68baa..79cac39 100644 --- a/buf/validate/internal/cel_constraint_rules.h +++ b/buf/validate/internal/cel_constraint_rules.h @@ -28,6 +28,7 @@ namespace buf::validate::internal { struct CompiledConstraint { buf::validate::Constraint constraint; std::unique_ptr expr; + const absl::optional rulePath; const google::protobuf::FieldDescriptor* rule; }; @@ -41,25 +42,23 @@ class CelConstraintRules : public ConstraintRules { absl::Status Add( google::api::expr::runtime::CelExpressionBuilder& builder, Constraint constraint, + absl::optional rulePath, const google::protobuf::FieldDescriptor* rule); absl::Status Add( google::api::expr::runtime::CelExpressionBuilder& builder, std::string_view id, std::string_view message, std::string_view expression, + absl::optional rulePath, const google::protobuf::FieldDescriptor* rule); - [[nodiscard]] const std::vector& getExprs() const { return exprs_; } // Validate all the cel rules given the activation that already has 'this' bound. absl::Status ValidateCel( ConstraintContext& ctx, - std::string_view fieldName, google::api::expr::runtime::Activation& activation) const; void setRules(google::api::expr::runtime::CelValue rules) { rules_ = rules; } void setRules(const google::protobuf::Message* rules, google::protobuf::Arena* arena); - [[nodiscard]] const google::api::expr::runtime::CelValue& getRules() const { return rules_; } - [[nodiscard]] google::api::expr::runtime::CelValue& getRules() { return rules_; } protected: google::api::expr::runtime::CelValue rules_; diff --git a/buf/validate/internal/cel_rules.h b/buf/validate/internal/cel_rules.h index 1f22ee3..b409505 100644 --- a/buf/validate/internal/cel_rules.h +++ b/buf/validate/internal/cel_rules.h @@ -16,6 +16,7 @@ #include "absl/status/status.h" #include "buf/validate/internal/cel_constraint_rules.h" +#include "buf/validate/internal/constraints.h" #include "buf/validate/internal/message_factory.h" #include "buf/validate/validate.pb.h" #include "google/protobuf/arena.h" @@ -23,6 +24,39 @@ namespace buf::validate::internal { +template +constexpr int ruleFieldNumber() = delete; + +#define MAP_RULES_TO_FIELD_NUMBER(R, fieldNumber) \ + template <> \ + constexpr int ruleFieldNumber() { \ + return fieldNumber; \ + } + +MAP_RULES_TO_FIELD_NUMBER(FloatRules, FieldConstraints::kFloatFieldNumber) +MAP_RULES_TO_FIELD_NUMBER(DoubleRules, FieldConstraints::kDoubleFieldNumber) +MAP_RULES_TO_FIELD_NUMBER(Int32Rules, FieldConstraints::kInt32FieldNumber) +MAP_RULES_TO_FIELD_NUMBER(Int64Rules, FieldConstraints::kInt64FieldNumber) +MAP_RULES_TO_FIELD_NUMBER(UInt32Rules, FieldConstraints::kUint32FieldNumber) +MAP_RULES_TO_FIELD_NUMBER(UInt64Rules, FieldConstraints::kUint64FieldNumber) +MAP_RULES_TO_FIELD_NUMBER(SInt32Rules, FieldConstraints::kSint32FieldNumber) +MAP_RULES_TO_FIELD_NUMBER(SInt64Rules, FieldConstraints::kSint64FieldNumber) +MAP_RULES_TO_FIELD_NUMBER(Fixed32Rules, FieldConstraints::kFixed32FieldNumber) +MAP_RULES_TO_FIELD_NUMBER(Fixed64Rules, FieldConstraints::kFixed64FieldNumber) +MAP_RULES_TO_FIELD_NUMBER(SFixed32Rules, FieldConstraints::kSfixed32FieldNumber) +MAP_RULES_TO_FIELD_NUMBER(SFixed64Rules, FieldConstraints::kSfixed64FieldNumber) +MAP_RULES_TO_FIELD_NUMBER(BoolRules, FieldConstraints::kBoolFieldNumber) +MAP_RULES_TO_FIELD_NUMBER(StringRules, FieldConstraints::kStringFieldNumber) +MAP_RULES_TO_FIELD_NUMBER(BytesRules, FieldConstraints::kBytesFieldNumber) +MAP_RULES_TO_FIELD_NUMBER(EnumRules, FieldConstraints::kEnumFieldNumber) +MAP_RULES_TO_FIELD_NUMBER(RepeatedRules, FieldConstraints::kRepeatedFieldNumber) +MAP_RULES_TO_FIELD_NUMBER(MapRules, FieldConstraints::kMapFieldNumber) +MAP_RULES_TO_FIELD_NUMBER(AnyRules, FieldConstraints::kAnyFieldNumber) +MAP_RULES_TO_FIELD_NUMBER(DurationRules, FieldConstraints::kDurationFieldNumber) +MAP_RULES_TO_FIELD_NUMBER(TimestampRules, FieldConstraints::kTimestampFieldNumber) + +#undef MAP_RULES_TO_FIELD_NUMBER + template absl::Status BuildCelRules( std::unique_ptr& messageFactory, @@ -60,13 +94,17 @@ absl::Status BuildCelRules( R::GetReflection()->ListFields(rules, &fields); } for (const auto* field : fields) { + FieldPath rulePath; + *rulePath.mutable_elements()->Add() = fieldPathElement(field); + *rulePath.mutable_elements()->Add() = + staticFieldPathElement()>(); if (!field->options().HasExtension(buf::validate::predefined)) { continue; } const auto& fieldLvl = field->options().GetExtension(buf::validate::predefined); for (const auto& constraint : fieldLvl.cel()) { auto status = result.Add( - builder, constraint.id(), constraint.message(), constraint.expression(), field); + builder, constraint.id(), constraint.message(), constraint.expression(), rulePath, field); if (!status.ok()) { return status; } diff --git a/buf/validate/internal/constraint_rules.h b/buf/validate/internal/constraint_rules.h index 36bb349..4e57026 100644 --- a/buf/validate/internal/constraint_rules.h +++ b/buf/validate/internal/constraint_rules.h @@ -34,14 +34,18 @@ struct ConstraintContext { return !status.ok() || (failFast && violations.violations_size() > 0); } - void prefixFieldPath(std::string_view prefix, int start) { + void appendFieldPathElement(const FieldPathElement &element, int start) { for (int i = start; i < violations.violations_size(); i++) { auto* violation = violations.mutable_violations(i); - if (violation->field_path().empty()) { - *violation->mutable_field_path() = prefix; - } else { - violation->set_field_path(absl::StrCat(prefix, ".", violation->field_path())); - } + *violation->mutable_field()->mutable_elements()->Add() = element; + } + } + + void appendRulePathElement(std::initializer_list suffix, int start) { + for (int i = start; i < violations.violations_size(); i++) { + auto* violation = violations.mutable_violations(i); + auto* elements = violation->mutable_rule()->mutable_elements(); + std::copy(suffix.begin(), suffix.end(), RepeatedPtrFieldBackInserter(elements)); } } }; diff --git a/buf/validate/internal/constraints.cc b/buf/validate/internal/constraints.cc index d696608..e1bb78c 100644 --- a/buf/validate/internal/constraints.cc +++ b/buf/validate/internal/constraints.cc @@ -116,7 +116,7 @@ absl::Status MessageConstraintRules::Validate( ConstraintContext& ctx, const google::protobuf::Message& message) const { google::api::expr::runtime::Activation activation; activation.InsertValue("this", cel::runtime::CelProtoWrapper::CreateMessage(&message, ctx.arena)); - return ValidateCel(ctx, "", activation); + return ValidateCel(ctx, activation); } absl::Status FieldConstraintRules::Validate( @@ -135,7 +135,9 @@ absl::Status FieldConstraintRules::Validate( auto& violation = *ctx.violations.add_violations(); *violation.mutable_constraint_id() = "required"; *violation.mutable_message() = "value is required"; - *violation.mutable_field_path() = field_->name(); + *violation.mutable_field()->mutable_elements()->Add() = fieldPathElement(field_); + *violation.mutable_rule()->mutable_elements()->Add() = + staticFieldPathElement(); } } } else if (field_->is_repeated()) { @@ -149,7 +151,9 @@ absl::Status FieldConstraintRules::Validate( auto& violation = *ctx.violations.add_violations(); *violation.mutable_constraint_id() = "required"; *violation.mutable_message() = "value is required"; - *violation.mutable_field_path() = field_->name(); + *violation.mutable_field()->mutable_elements()->Add() = fieldPathElement(field_); + *violation.mutable_rule()->mutable_elements()->Add() = + staticFieldPathElement(); } } } else { @@ -158,7 +162,9 @@ absl::Status FieldConstraintRules::Validate( auto& violation = *ctx.violations.add_violations(); *violation.mutable_constraint_id() = "required"; *violation.mutable_message() = "value is required"; - *violation.mutable_field_path() = field_->name(); + *violation.mutable_field()->mutable_elements()->Add() = fieldPathElement(field_); + *violation.mutable_rule()->mutable_elements()->Add() = + staticFieldPathElement(); return absl::OkStatus(); } else if (ignoreEmpty_) { return absl::OkStatus(); @@ -168,7 +174,7 @@ absl::Status FieldConstraintRules::Validate( if (anyRules_ != nullptr && field_->cpp_type() == google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE) { const auto& anyMsg = message.GetReflection()->GetMessage(message, field_); - auto status = ValidateAny(ctx, field_->name(), anyMsg); + auto status = ValidateAny(ctx, field_, anyMsg); if (!status.ok()) { return status; } @@ -184,7 +190,16 @@ absl::Status FieldConstraintRules::Validate( } } activation.InsertValue("this", result); - return ValidateCel(ctx, field_->name(), activation); + int pos = ctx.violations.violations_size(); + auto status = ValidateCel(ctx, activation); + if (!status.ok()) { + return status; + } + if (ctx.violations.violations_size() > pos) { + auto element = fieldPathElement(field_); + ctx.appendFieldPathElement(element, pos); + } + return status; } absl::Status EnumConstraintRules::Validate( @@ -198,7 +213,9 @@ absl::Status EnumConstraintRules::Validate( auto& violation = *ctx.violations.add_violations(); *violation.mutable_constraint_id() = "enum.defined_only"; *violation.mutable_message() = "enum value must be defined"; - *violation.mutable_field_path() = field_->name(); + *violation.mutable_field()->mutable_elements()->Add() = fieldPathElement(field_); + *violation.mutable_rule()->mutable_elements()->Add() = fieldPathElement(EnumRules::descriptor()->FindFieldByNumber(EnumRules::kDefinedOnlyFieldNumber)); + *violation.mutable_rule()->mutable_elements()->Add() = fieldPathElement(FieldConstraints::descriptor()->FindFieldByNumber(FieldConstraints::kEnumFieldNumber)); } } return absl::OkStatus(); @@ -224,13 +241,19 @@ absl::Status RepeatedConstraintRules::Validate( cel::runtime::Activation activation; activation.InsertValue("this", item); int pos = ctx.violations.violations_size(); - status = itemRules_->ValidateCel(ctx, "", activation); + status = itemRules_->ValidateCel(ctx, activation); if (itemRules_->getAnyRules() != nullptr) { const auto& anyMsg = message.GetReflection()->GetRepeatedMessage(message, field_, i); - status = itemRules_->ValidateAny(ctx, "", anyMsg); + status = itemRules_->ValidateAny(ctx, nullptr, anyMsg); } if (ctx.violations.violations_size() > pos) { - ctx.prefixFieldPath(absl::StrCat(field_->name(), "[", i, "]"), pos); + FieldPathElement element = fieldPathElement(field_); + element.set_index(i); + ctx.appendFieldPathElement(element, pos); + ctx.appendRulePathElement({ + fieldPathElement(RepeatedRules::descriptor()->FindFieldByNumber(RepeatedRules::kItemsFieldNumber)), + fieldPathElement(FieldConstraints::descriptor()->FindFieldByNumber(FieldConstraints::kRepeatedFieldNumber)), + }, pos); } if (ctx.shouldReturn(status)) { return status; @@ -260,11 +283,15 @@ absl::Status MapConstraintRules::Validate( if (keyRules_ != nullptr) { if (!keyRules_->getIgnoreEmpty() || !isEmptyItem(key)) { activation.InsertValue("this", key); - status = keyRules_->ValidateCel(ctx, "", activation); + status = keyRules_->ValidateCel(ctx, activation); if (!status.ok()) { return status; } if (ctx.violations.violations_size() > pos) { + ctx.appendRulePathElement({ + fieldPathElement(MapRules::descriptor()->FindFieldByNumber(MapRules::kKeysFieldNumber)), + fieldPathElement(FieldConstraints::descriptor()->FindFieldByNumber(FieldConstraints::kMapFieldNumber)), + }, pos); for (int j = pos; j < ctx.violations.violations_size(); j++) { ctx.violations.mutable_violations(j)->set_for_key(true); } @@ -276,16 +303,26 @@ absl::Status MapConstraintRules::Validate( auto value = *mapVal[key]; if (!valueRules_->getIgnoreEmpty() || !isEmptyItem(value)) { activation.InsertValue("this", value); - status = valueRules_->ValidateCel(ctx, "", activation); + int valuePos = ctx.violations.violations_size(); + status = valueRules_->ValidateCel(ctx, activation); if (!status.ok()) { return status; } + if (ctx.violations.violations_size() > valuePos) { + ctx.appendRulePathElement({ + fieldPathElement(MapRules::descriptor()->FindFieldByNumber(MapRules::kValuesFieldNumber)), + fieldPathElement(FieldConstraints::descriptor()->FindFieldByNumber(FieldConstraints::kMapFieldNumber)), + }, valuePos); + } activation.RemoveValueEntry("this"); } } if (ctx.violations.violations_size() > pos) { - ctx.prefixFieldPath( - absl::StrCat(field_->name(), "[", makeMapKeyString(elemMsg, keyField), "]"), pos); + FieldPathElement element = fieldPathElement(field_); + if (auto status = setPathElementMapKey(&element, elemMsg, keyField); !status.ok()) { + return status; + } + ctx.appendFieldPathElement(element, pos); if (ctx.failFast) { return absl::OkStatus(); } @@ -296,7 +333,7 @@ absl::Status MapConstraintRules::Validate( absl::Status FieldConstraintRules::ValidateAny( ConstraintContext& ctx, - std::string_view fieldPath, + const google::protobuf::FieldDescriptor* field, const google::protobuf::Message& anyMsg) const { const auto* typeUriField = anyMsg.GetDescriptor()->FindFieldByName("type_url"); if (typeUriField == nullptr || @@ -317,7 +354,11 @@ absl::Status FieldConstraintRules::ValidateAny( auto& violation = *ctx.violations.add_violations(); *violation.mutable_constraint_id() = "any.in"; *violation.mutable_message() = "type URL must be in the allow list"; - *violation.mutable_field_path() = fieldPath; + if (field != nullptr) { + *violation.mutable_field()->mutable_elements()->Add() = fieldPathElement(field); + } + *violation.mutable_rule()->mutable_elements()->Add() = fieldPathElement(AnyRules::descriptor()->FindFieldByNumber(AnyRules::kInFieldNumber)); + *violation.mutable_rule()->mutable_elements()->Add() = fieldPathElement(FieldConstraints::descriptor()->FindFieldByNumber(FieldConstraints::kAnyFieldNumber)); } } for (const auto& block : anyRules_->not_in()) { @@ -325,7 +366,11 @@ absl::Status FieldConstraintRules::ValidateAny( auto& violation = *ctx.violations.add_violations(); *violation.mutable_constraint_id() = "any.not_in"; *violation.mutable_message() = "type URL must not be in the block list"; - *violation.mutable_field_path() = fieldPath; + if (field != nullptr) { + *violation.mutable_field()->mutable_elements()->Add() = fieldPathElement(field); + } + *violation.mutable_rule()->mutable_elements()->Add() = fieldPathElement(AnyRules::descriptor()->FindFieldByNumber(AnyRules::kNotInFieldNumber)); + *violation.mutable_rule()->mutable_elements()->Add() = fieldPathElement(FieldConstraints::descriptor()->FindFieldByNumber(FieldConstraints::kAnyFieldNumber)); break; } } @@ -340,7 +385,7 @@ absl::Status OneofConstraintRules::Validate( auto& violation = *ctx.violations.add_violations(); *violation.mutable_constraint_id() = "required"; *violation.mutable_message() = "exactly one field is required in oneof"; - *violation.mutable_field_path() = oneof_->name(); + *violation.mutable_field()->mutable_elements()->Add() = oneofPathElement(*oneof_); } } return absl::OkStatus(); diff --git a/buf/validate/internal/constraints.h b/buf/validate/internal/constraints.h index b960f6b..e70dadb 100644 --- a/buf/validate/internal/constraints.h +++ b/buf/validate/internal/constraints.h @@ -16,6 +16,7 @@ #include #include +#include #include "buf/validate/internal/cel_constraint_rules.h" #include "buf/validate/validate.pb.h" @@ -50,14 +51,12 @@ class FieldConstraintRules : public CelConstraintRules { required_(field.required()), anyRules_(anyRules) {} - [[nodiscard]] const google::protobuf::FieldDescriptor* getField() const { return field_; } - absl::Status Validate( ConstraintContext& ctx, const google::protobuf::Message& message) const override; absl::Status ValidateAny( ConstraintContext& ctx, - std::string_view fieldName, + const google::protobuf::FieldDescriptor* field, const google::protobuf::Message& anyMsg) const; [[nodiscard]] const AnyRules* getAnyRules() const { return anyRules_; } @@ -145,32 +144,105 @@ class OneofConstraintRules : public ConstraintRules { absl::StatusOr> NewConstraintBuilder(google::protobuf::Arena* arena); -inline std::string makeMapKeyString( - const google::protobuf::Message& message, const google::protobuf::FieldDescriptor* keyField) { - switch (keyField->cpp_type()) { - case google::protobuf::FieldDescriptor::CPPTYPE_INT32: - return std::to_string(message.GetReflection()->GetInt32(message, keyField)); - case google::protobuf::FieldDescriptor::CPPTYPE_INT64: - return std::to_string(message.GetReflection()->GetInt64(message, keyField)); - case google::protobuf::FieldDescriptor::CPPTYPE_UINT32: - return std::to_string(message.GetReflection()->GetUInt32(message, keyField)); - case google::protobuf::FieldDescriptor::CPPTYPE_UINT64: - return std::to_string(message.GetReflection()->GetUInt64(message, keyField)); - case google::protobuf::FieldDescriptor::CPPTYPE_DOUBLE: - return std::to_string(message.GetReflection()->GetDouble(message, keyField)); - case google::protobuf::FieldDescriptor::CPPTYPE_FLOAT: - return std::to_string(message.GetReflection()->GetFloat(message, keyField)); - case google::protobuf::FieldDescriptor::CPPTYPE_BOOL: - return message.GetReflection()->GetBool(message, keyField) ? "true" : "false"; - case google::protobuf::FieldDescriptor::CPPTYPE_ENUM: - return message.GetReflection()->GetEnum(message, keyField)->name(); - case google::protobuf::FieldDescriptor::CPPTYPE_STRING: - // Quote and escape the string. - return absl::StrCat( - "\"", absl::CEscape(message.GetReflection()->GetString(message, keyField)), "\""); +inline auto fieldPathElement(const google::protobuf::FieldDescriptor* fieldDescriptor) -> FieldPathElement { + FieldPathElement element; + element.set_field_number(fieldDescriptor->number()); + element.set_field_type( + static_cast<::google::protobuf::FieldDescriptorProto_Type>(fieldDescriptor->type())); + if (fieldDescriptor->is_extension()) { + *element.mutable_field_name() = absl::StrCat("[", fieldDescriptor->full_name(), "]"); + } else { + *element.mutable_field_name() = fieldDescriptor->name(); + } + return element; +} + +template +auto staticFieldPathElement() -> FieldPathElement { + // This is thread-safe: C++11 guarantees static local initialization to be done only once and is + // synchronized automatically. + static const FieldPathElement element = + fieldPathElement(ProtoMessage::descriptor()->FindFieldByNumber(number)); + return element; +} + +inline auto oneofPathElement(const google::protobuf::OneofDescriptor& oneofDescriptor) + -> FieldPathElement { + FieldPathElement element; + *element.mutable_field_name() = oneofDescriptor.name(); + return element; +} + +inline absl::Status setPathElementMapKey( + FieldPathElement* element, + const google::protobuf::Message& message, + const google::protobuf::FieldDescriptor* keyField) { + using Type = google::protobuf::FieldDescriptor::Type; + switch (keyField->type()) { + case Type::TYPE_BOOL: + element->set_bool_key(message.GetReflection()->GetBool(message, keyField)); + break; + case Type::TYPE_SINT32: + element->set_sint_key(message.GetReflection()->GetInt32(message, keyField)); + break; + case Type::TYPE_SINT64: + element->set_sint_key(message.GetReflection()->GetInt64(message, keyField)); + break; + case Type::TYPE_INT32: + case Type::TYPE_SFIXED32: + element->set_int_key(message.GetReflection()->GetInt32(message, keyField)); + break; + case Type::TYPE_INT64: + case Type::TYPE_SFIXED64: + element->set_int_key(message.GetReflection()->GetInt64(message, keyField)); + break; + case Type::TYPE_UINT32: + case Type::TYPE_FIXED32: + element->set_uint_key(message.GetReflection()->GetUInt32(message, keyField)); + break; + case Type::TYPE_UINT64: + case Type::TYPE_FIXED64: + element->set_uint_key(message.GetReflection()->GetUInt64(message, keyField)); + break; + case Type::TYPE_STRING: + *element->mutable_string_key() = message.GetReflection()->GetString(message, keyField); + break; default: - return "?"; + return absl::InternalError(absl::StrCat("unexpected map key type ", keyField->type_name())); + } + return {}; +} + +inline std::string fieldPathString(const FieldPath &path) { + std::string result; + for (const FieldPathElement& element : path.elements()) { + if (!result.empty()) { + result += '.'; + } + switch (element.subscript_case()) { + case FieldPathElement::kIndex: + absl::StrAppend(&result, element.field_name(), "[", std::to_string(element.index()), "]"); + break; + case FieldPathElement::kBoolKey: + absl::StrAppend(&result, element.field_name(), element.bool_key() ? "[true]" : "[false]"); + break; + case FieldPathElement::kIntKey: + absl::StrAppend(&result, element.field_name(), "[", std::to_string(element.int_key()), "]"); + break; + case FieldPathElement::kSintKey: + absl::StrAppend(&result, element.field_name(), "[", std::to_string(element.sint_key()), "]"); + break; + case FieldPathElement::kUintKey: + absl::StrAppend(&result, element.field_name(), "[", std::to_string(element.uint_key()), "]"); + break; + case FieldPathElement::kStringKey: + absl::StrAppend(&result, element.field_name(), "[\"", absl::CEscape(element.string_key()), "\"]"); + break; + case FieldPathElement::SUBSCRIPT_NOT_SET: + absl::StrAppend(&result, element.field_name()); + } } + return result; } } // namespace buf::validate::internal diff --git a/buf/validate/internal/constraints_test.cc b/buf/validate/internal/constraints_test.cc index 0365b16..96f12c1 100644 --- a/buf/validate/internal/constraints_test.cc +++ b/buf/validate/internal/constraints_test.cc @@ -48,16 +48,15 @@ class ExpressionTest : public testing::Test { constraint.set_expression(std::move(expr)); constraint.set_message(std::move(message)); constraint.set_id(std::move(id)); - return constraints_->Add(*builder_, constraint, nullptr); + return constraints_->Add(*builder_, constraint, absl::nullopt, nullptr); } absl::Status Validate( - const std::string& fieldPath, google::api::expr::runtime::Activation& activation, std::vector& violations) { ConstraintContext ctx; ctx.arena = &arena_; - auto status = constraints_->ValidateCel(ctx, fieldPath, activation); + auto status = constraints_->ValidateCel(ctx, activation); if (!status.ok()) { return status; } @@ -74,10 +73,9 @@ TEST_F(ExpressionTest, BoolResult) { cel::runtime::Activation ctx; std::vector violations; - auto status = Validate("a.b", ctx, violations); + auto status = Validate(ctx, violations); ASSERT_TRUE(status.ok()) << status; ASSERT_EQ(violations.size(), 1); - EXPECT_EQ(violations[0].field_path(), "a.b"); EXPECT_EQ(violations[0].message(), "always fails"); EXPECT_EQ(violations[0].constraint_id(), "always-fails"); } @@ -88,10 +86,9 @@ TEST_F(ExpressionTest, StringResult) { cel::runtime::Activation ctx; std::vector violations; - auto status = Validate("a.b", ctx, violations); + auto status = Validate(ctx, violations); ASSERT_TRUE(status.ok()) << status; ASSERT_EQ(violations.size(), 1); - EXPECT_EQ(violations[0].field_path(), "a.b"); EXPECT_EQ(violations[0].message(), "error"); EXPECT_EQ(violations[0].constraint_id(), "always-fails"); } @@ -100,7 +97,7 @@ TEST_F(ExpressionTest, Error) { ASSERT_TRUE(AddConstraint("1/0", "always fails", "always-fails").ok()); cel::runtime::Activation ctx; std::vector violations; - auto status = Validate("a.b", ctx, violations); + auto status = Validate(ctx, violations); ASSERT_FALSE(status.ok()) << status; EXPECT_EQ(status.code(), absl::StatusCode::kInvalidArgument); EXPECT_EQ(status.message(), "divide by zero"); @@ -110,7 +107,7 @@ TEST_F(ExpressionTest, BadType) { ASSERT_TRUE(AddConstraint("1", "always fails", "always-fails").ok()); cel::runtime::Activation ctx; std::vector violations; - auto status = Validate("a.b", ctx, violations); + auto status = Validate(ctx, violations); ASSERT_FALSE(status.ok()) << status; EXPECT_EQ(status.code(), absl::StatusCode::kInvalidArgument); EXPECT_EQ(status.message(), "invalid result type"); diff --git a/buf/validate/internal/field_rules.cc b/buf/validate/internal/field_rules.cc index 85c96c0..df102c1 100644 --- a/buf/validate/internal/field_rules.cc +++ b/buf/validate/internal/field_rules.cc @@ -342,7 +342,7 @@ absl::StatusOr> NewFieldRules( } if (rules_or.ok()) { for (const auto& constraint : fieldLvl.cel()) { - auto status = rules_or.value()->Add(builder, constraint, nullptr); + auto status = rules_or.value()->Add(builder, constraint, absl::nullopt, nullptr); if (!status.ok()) { return status; } diff --git a/buf/validate/internal/message_rules.cc b/buf/validate/internal/message_rules.cc index 34364d4..95e01c1 100644 --- a/buf/validate/internal/message_rules.cc +++ b/buf/validate/internal/message_rules.cc @@ -23,7 +23,7 @@ absl::StatusOr> BuildMessageRules( const MessageConstraints& constraints) { auto result = std::make_unique(); for (const auto& constraint : constraints.cel()) { - if (auto status = result->Add(builder, constraint, nullptr); !status.ok()) { + if (auto status = result->Add(builder, constraint, absl::nullopt, nullptr); !status.ok()) { return status; } } diff --git a/buf/validate/validator.cc b/buf/validate/validator.cc index cef4a75..9481eba 100644 --- a/buf/validate/validator.cc +++ b/buf/validate/validator.cc @@ -73,9 +73,12 @@ absl::Status Validator::ValidateFields( int pos = ctx.violations.violations_size(); auto status = ValidateMessage(ctx, valueMsg); if (pos < ctx.violations.violations_size()) { - ctx.prefixFieldPath( - absl::StrCat(field->name(), "[", internal::makeMapKeyString(elemMsg, keyField), "]"), - pos); + FieldPathElement element = internal::fieldPathElement(field); + if (auto status = internal::setPathElementMapKey(&element, elemMsg, keyField); + !status.ok()) { + return status; + } + ctx.appendFieldPathElement(element, pos); } if (ctx.shouldReturn(status)) { return status; @@ -88,7 +91,9 @@ absl::Status Validator::ValidateFields( const auto& subMsg = message.GetReflection()->GetRepeatedMessage(message, field, i); auto status = ValidateMessage(ctx, subMsg); if (pos < ctx.violations.violations_size()) { - ctx.prefixFieldPath(absl::StrCat(field->name(), "[", i, "]"), pos); + FieldPathElement element = internal::fieldPathElement(field); + element.set_index(i); + ctx.appendFieldPathElement(element, pos); } if (ctx.shouldReturn(status)) { return status; @@ -99,7 +104,7 @@ absl::Status Validator::ValidateFields( int pos = ctx.violations.violations_size(); auto status = ValidateMessage(ctx, subMsg); if (pos < ctx.violations.violations_size()) { - ctx.prefixFieldPath(field->name(), pos); + ctx.appendFieldPathElement(internal::fieldPathElement(field), pos); } if (ctx.shouldReturn(status)) { return status; @@ -117,6 +122,19 @@ absl::StatusOr Validator::Validate(const google::protobuf::Message& if (!status.ok()) { return status; } + for (Violation& violation : *ctx.violations.mutable_violations()) { + if (violation.has_field()) { + std::reverse( + violation.mutable_field()->mutable_elements()->begin(), + violation.mutable_field()->mutable_elements()->end()); + *violation.mutable_field_path() = internal::fieldPathString(violation.field()); + } + if (violation.has_rule()) { + std::reverse( + violation.mutable_rule()->mutable_elements()->begin(), + violation.mutable_rule()->mutable_elements()->end()); + } + } return std::move(ctx.violations); } From bb2612209d0c056c10c738e77328e8c825aadc72 Mon Sep 17 00:00:00 2001 From: John Chadwick Date: Mon, 25 Nov 2024 16:59:06 -0500 Subject: [PATCH 2/3] Update to pass latest conformance --- Makefile | 2 +- bazel/deps.bzl | 4 ++-- buf/validate/internal/constraints.cc | 4 +++- buf/validate/internal/constraints.h | 18 ++++++++---------- buf/validate/internal/field_rules.cc | 10 ++++++++-- buf/validate/validator.cc | 2 +- 6 files changed, 23 insertions(+), 17 deletions(-) diff --git a/Makefile b/Makefile index e950be5..882a969 100644 --- a/Makefile +++ b/Makefile @@ -12,7 +12,7 @@ LICENSE_IGNORE := -e internal/testdata/ LICENSE_HEADER_VERSION := 0294fdbe1ce8649ebaf5e87e8cdd588e33730bbb # NOTE: Keep this version in sync with the version in `/bazel/deps.bzl`. # DO NOT MERGE -PROTOVALIDATE_VERSION ?= 9fbcb20937fb97609f5711d25474213241694294 +PROTOVALIDATE_VERSION ?= a4d7e113cc9e8944fb22098123eddc5413500381 # Set to use a different compiler. For example, `GO=go1.18rc1 make test`. GO ?= go diff --git a/bazel/deps.bzl b/bazel/deps.bzl index b8a15d2..a9247a0 100644 --- a/bazel/deps.bzl +++ b/bazel/deps.bzl @@ -66,10 +66,10 @@ _dependencies = { # NOTE: Keep Version in sync with `/Makefile`. "com_github_bufbuild_protovalidate": { # "sha256": "c637c8cbaf71b6dc38171e47c2c736581b4cfef385984083561480367659d14f", - "strip_prefix": "protovalidate-9fbcb20937fb97609f5711d25474213241694294", + "strip_prefix": "protovalidate-a4d7e113cc9e8944fb22098123eddc5413500381", "urls": [ # DO NOT MERGE - "https://github.com/bufbuild/protovalidate/archive/9fbcb20937fb97609f5711d25474213241694294.tar.gz", + "https://github.com/bufbuild/protovalidate/archive/a4d7e113cc9e8944fb22098123eddc5413500381.tar.gz", ], }, } diff --git a/buf/validate/internal/constraints.cc b/buf/validate/internal/constraints.cc index e1bb78c..34ef104 100644 --- a/buf/validate/internal/constraints.cc +++ b/buf/validate/internal/constraints.cc @@ -271,6 +271,7 @@ absl::Status MapConstraintRules::Validate( cel::runtime::FieldBackedMapImpl mapVal(&message, field_, ctx.arena); cel::runtime::Activation activation; const auto* keyField = field_->message_type()->FindFieldByName("key"); + const auto* valueField = field_->message_type()->FindFieldByName("value"); auto keys_or = mapVal.ListKeys(); if (!keys_or.ok()) { return keys_or.status(); @@ -319,7 +320,8 @@ absl::Status MapConstraintRules::Validate( } if (ctx.violations.violations_size() > pos) { FieldPathElement element = fieldPathElement(field_); - if (auto status = setPathElementMapKey(&element, elemMsg, keyField); !status.ok()) { + if (auto status = setPathElementMapKey(&element, elemMsg, keyField, valueField); + !status.ok()) { return status; } ctx.appendFieldPathElement(element, pos); diff --git a/buf/validate/internal/constraints.h b/buf/validate/internal/constraints.h index e70dadb..05f4df0 100644 --- a/buf/validate/internal/constraints.h +++ b/buf/validate/internal/constraints.h @@ -176,24 +176,25 @@ inline auto oneofPathElement(const google::protobuf::OneofDescriptor& oneofDescr inline absl::Status setPathElementMapKey( FieldPathElement* element, const google::protobuf::Message& message, - const google::protobuf::FieldDescriptor* keyField) { + const google::protobuf::FieldDescriptor* keyField, + const google::protobuf::FieldDescriptor* valueField) { using Type = google::protobuf::FieldDescriptor::Type; + element->set_key_type( + static_cast<::google::protobuf::FieldDescriptorProto_Type>(keyField->type())); + element->set_value_type( + static_cast<::google::protobuf::FieldDescriptorProto_Type>(valueField->type())); switch (keyField->type()) { case Type::TYPE_BOOL: element->set_bool_key(message.GetReflection()->GetBool(message, keyField)); break; - case Type::TYPE_SINT32: - element->set_sint_key(message.GetReflection()->GetInt32(message, keyField)); - break; - case Type::TYPE_SINT64: - element->set_sint_key(message.GetReflection()->GetInt64(message, keyField)); - break; case Type::TYPE_INT32: case Type::TYPE_SFIXED32: + case Type::TYPE_SINT32: element->set_int_key(message.GetReflection()->GetInt32(message, keyField)); break; case Type::TYPE_INT64: case Type::TYPE_SFIXED64: + case Type::TYPE_SINT64: element->set_int_key(message.GetReflection()->GetInt64(message, keyField)); break; case Type::TYPE_UINT32: @@ -229,9 +230,6 @@ inline std::string fieldPathString(const FieldPath &path) { case FieldPathElement::kIntKey: absl::StrAppend(&result, element.field_name(), "[", std::to_string(element.int_key()), "]"); break; - case FieldPathElement::kSintKey: - absl::StrAppend(&result, element.field_name(), "[", std::to_string(element.sint_key()), "]"); - break; case FieldPathElement::kUintKey: absl::StrAppend(&result, element.field_name(), "[", std::to_string(element.uint_key()), "]"); break; diff --git a/buf/validate/internal/field_rules.cc b/buf/validate/internal/field_rules.cc index df102c1..5004c03 100644 --- a/buf/validate/internal/field_rules.cc +++ b/buf/validate/internal/field_rules.cc @@ -341,8 +341,14 @@ absl::StatusOr> NewFieldRules( absl::StrFormat("unknown field validator type %d", fieldLvl.type_case())); } if (rules_or.ok()) { - for (const auto& constraint : fieldLvl.cel()) { - auto status = rules_or.value()->Add(builder, constraint, absl::nullopt, nullptr); + for (int i = 0; i < fieldLvl.cel_size(); i++) { + const auto& constraint = fieldLvl.cel(i); + FieldPathElement celElement = + staticFieldPathElement(); + celElement.set_index(i); + FieldPath rulePath; + *rulePath.mutable_elements()->Add() = celElement; + auto status = rules_or.value()->Add(builder, constraint, rulePath, nullptr); if (!status.ok()) { return status; } diff --git a/buf/validate/validator.cc b/buf/validate/validator.cc index 9481eba..093a2fb 100644 --- a/buf/validate/validator.cc +++ b/buf/validate/validator.cc @@ -74,7 +74,7 @@ absl::Status Validator::ValidateFields( auto status = ValidateMessage(ctx, valueMsg); if (pos < ctx.violations.violations_size()) { FieldPathElement element = internal::fieldPathElement(field); - if (auto status = internal::setPathElementMapKey(&element, elemMsg, keyField); + if (auto status = internal::setPathElementMapKey(&element, elemMsg, keyField, valueField); !status.ok()) { return status; } From 8435e6c2889dbbc44fdf1b18a5cecd57a18367ee Mon Sep 17 00:00:00 2001 From: John Chadwick Date: Tue, 26 Nov 2024 16:46:37 -0500 Subject: [PATCH 3/3] Update protovalidate to v0.9.0 --- Makefile | 3 +-- bazel/deps.bzl | 7 +++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 882a969..c32fb4f 100644 --- a/Makefile +++ b/Makefile @@ -11,8 +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`. -# DO NOT MERGE -PROTOVALIDATE_VERSION ?= a4d7e113cc9e8944fb22098123eddc5413500381 +PROTOVALIDATE_VERSION ?= v0.9.0 # Set to use a different compiler. For example, `GO=go1.18rc1 make test`. GO ?= go diff --git a/bazel/deps.bzl b/bazel/deps.bzl index a9247a0..4d9f4e5 100644 --- a/bazel/deps.bzl +++ b/bazel/deps.bzl @@ -65,11 +65,10 @@ _dependencies = { }, # NOTE: Keep Version in sync with `/Makefile`. "com_github_bufbuild_protovalidate": { - # "sha256": "c637c8cbaf71b6dc38171e47c2c736581b4cfef385984083561480367659d14f", - "strip_prefix": "protovalidate-a4d7e113cc9e8944fb22098123eddc5413500381", + "sha256": "fca6143d820e9575f3aec328918fa25acc8eeb6e706050127d3a36cfdede4610", + "strip_prefix": "protovalidate-0.9.0", "urls": [ - # DO NOT MERGE - "https://github.com/bufbuild/protovalidate/archive/a4d7e113cc9e8944fb22098123eddc5413500381.tar.gz", + "https://github.com/bufbuild/protovalidate/archive/v0.9.0.tar.gz", ], }, }