Skip to content

Commit

Permalink
Update required/ignore_empty behavior to match spec (#46)
Browse files Browse the repository at this point in the history 
Bringing the `required` and `ignore_empty` constraints into conformance.

See bufbuild/protovalidate#115
  • Loading branch information
@rodaine
rodaine authored Nov 7, 2023
1 parent 64da14e commit 6cc9f3e
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 86 deletions.
2 changes: 1 addition & 1 deletion Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.1
PROTOVALIDATE_VERSION ?= v0.5.3

# Set to use a different compiler. For example, `GO=go1.18rc1 make test`.
GO ?= go
Expand Down
6 changes: 3 additions & 3 deletions bazel/deps.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,10 @@ _dependencies = {
},
# NOTE: Keep Version in sync with `/Makefile`.
"com_github_bufbuild_protovalidate": {
"sha256": "67c360e29651a3bf81f7a2394f5d7f5353a67c0f79cb304ba420d27900e30203",
"strip_prefix": "protovalidate-0.5.1",
"sha256": "03ee49c344d350355c7e21b36456d709cbcba493f2cfa2029be99fc065aa7c48",
"strip_prefix": "protovalidate-0.5.3",
"urls": [
"https://github.com/bufbuild/protovalidate/archive/v0.5.1.tar.gz",
"https://github.com/bufbuild/protovalidate/archive/v0.5.3.tar.gz",
],
},
}
Expand Down
120 changes: 39 additions & 81 deletions buf/validate/internal/constraints.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,78 +103,11 @@ absl::Status FieldConstraintRules::Validate(
*violation.mutable_message() = "value is required";
*violation.mutable_field_path() = field_->name();
return absl::OkStatus();
} else if (
ignoreEmpty_ || field_->containing_oneof() != nullptr ||
field_->type() == google::protobuf::FieldDescriptor::TYPE_MESSAGE) {
} else if (ignoreEmpty_) {
return absl::OkStatus();
}
}

if (ignoreEmpty_ && field_->containing_oneof() != nullptr) {
// If the field is in a oneof, we have to manually check if the value is 'empty'.
switch (field_->type()) {
case google::protobuf::FieldDescriptor::TYPE_BYTES: {
if (message.GetReflection()->GetString(message, field_).empty()) {
return absl::OkStatus();
}
break;
}
case google::protobuf::FieldDescriptor::TYPE_STRING: {
if (message.GetReflection()->GetString(message, field_).empty()) {
return absl::OkStatus();
}
break;
}
case google::protobuf::FieldDescriptor::TYPE_SFIXED32:
case google::protobuf::FieldDescriptor::TYPE_SINT32:
case google::protobuf::FieldDescriptor::TYPE_INT32:
if (message.GetReflection()->GetInt32(message, field_) == 0) {
return absl::OkStatus();
}
case google::protobuf::FieldDescriptor::TYPE_SFIXED64:
case google::protobuf::FieldDescriptor::TYPE_SINT64:
case google::protobuf::FieldDescriptor::TYPE_INT64: {
if (message.GetReflection()->GetInt64(message, field_) == 0) {
return absl::OkStatus();
}
break;
}
case google::protobuf::FieldDescriptor::TYPE_FIXED32:
case google::protobuf::FieldDescriptor::TYPE_UINT32:
if (message.GetReflection()->GetUInt32(message, field_) == 0) {
return absl::OkStatus();
}
case google::protobuf::FieldDescriptor::TYPE_FIXED64:
case google::protobuf::FieldDescriptor::TYPE_UINT64: {
if (message.GetReflection()->GetUInt64(message, field_) == 0) {
return absl::OkStatus();
}
break;
}
case google::protobuf::FieldDescriptor::TYPE_DOUBLE:
case google::protobuf::FieldDescriptor::TYPE_FLOAT: {
if (message.GetReflection()->GetDouble(message, field_) == 0) {
return absl::OkStatus();
}
break;
}
case google::protobuf::FieldDescriptor::TYPE_BOOL: {
if (!message.GetReflection()->GetBool(message, field_)) {
return absl::OkStatus();
}
break;
}
case google::protobuf::FieldDescriptor::TYPE_ENUM: {
if (message.GetReflection()->GetEnumValue(message, field_) == 0) {
return absl::OkStatus();
}
break;
}
default:
break;
}
}

if (anyRules_ != nullptr &&
field_->cpp_type() == google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE) {
const auto& anyMsg = message.GetReflection()->GetMessage(message, field_);
Expand Down Expand Up @@ -210,6 +143,25 @@ 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);
Expand All @@ -220,8 +172,12 @@ absl::Status RepeatedConstraintRules::Validate(
auto& list = *google::protobuf::Arena::Create<cel::runtime::FieldBackedListImpl>(
ctx.arena, &message, field_, ctx.arena);
for (int i = 0; i < list.size(); i++) {
auto item = list[i];
if (itemRules_->getIgnoreEmpty() && isEmptyItem(item)) {
continue;
}
cel::runtime::Activation activation;
activation.InsertValue("this", list[i]);
activation.InsertValue("this", item);
int pos = ctx.violations.violations_size();
status = itemRules_->ValidateCel(ctx, "", activation);
if (itemRules_->getAnyRules() != nullptr) {
Expand Down Expand Up @@ -257,22 +213,24 @@ absl::Status MapConstraintRules::Validate(
int pos = ctx.violations.violations_size();
auto key = keys[i];
if (keyRules_ != nullptr) {
activation.InsertValue("this", key);
status = keyRules_->ValidateCel(ctx, "", activation);
if (!status.ok()) {
return status;
}
if (ctx.violations.violations_size() > pos) {
for (int j = pos; j < ctx.violations.violations_size(); j++) {
ctx.violations.mutable_violations(j)->set_for_key(true);
if (!keyRules_->getIgnoreEmpty() || !isEmptyItem(key)) {
activation.InsertValue("this", key);
status = keyRules_->ValidateCel(ctx, "", activation);
if (!status.ok()) {
return status;
}
if (ctx.violations.violations_size() > pos) {
for (int j = pos; j < ctx.violations.violations_size(); j++) {
ctx.violations.mutable_violations(j)->set_for_key(true);
}
}
activation.RemoveValueEntry("this");
}
activation.RemoveValueEntry("this");
}
if (valueRules_ != nullptr) {
auto value = mapVal[key];
if (value.has_value()) {
activation.InsertValue("this", *value);
auto value = *mapVal[key];
if (!valueRules_->getIgnoreEmpty() || !isEmptyItem(value)) {
activation.InsertValue("this", value);
status = valueRules_->ValidateCel(ctx, "", activation);
if (!status.ok()) {
return status;
Expand Down
4 changes: 3 additions & 1 deletion buf/validate/internal/constraints.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class FieldConstraintRules : public CelConstraintRules {
const FieldConstraints& field,
const AnyRules* anyRules = nullptr)
: field_(desc),
ignoreEmpty_(field.ignore_empty()),
ignoreEmpty_(field.ignore_empty() || desc->has_presence()),
required_(field.required()),
anyRules_(anyRules) {}

Expand All @@ -56,6 +56,8 @@ class FieldConstraintRules : public CelConstraintRules {

[[nodiscard]] const AnyRules* getAnyRules() const { return anyRules_; }

[[nodiscard]] const bool getIgnoreEmpty() const { return ignoreEmpty_; }

protected:
const google::protobuf::FieldDescriptor* field_ = nullptr;
bool ignoreEmpty_ = false;
Expand Down

0 comments on commit 6cc9f3e

Please sign in to comment.