Skip to content

Commit

Permalink
Fix repeated field resolution in CEL for buf lint protovalidate rule (#…
Browse files Browse the repository at this point in the history
…3162)

This fixes #3161.

`buf lint` for protovalidate should work correctly for both

```
repeated int32 ints = 1 [(buf.validate.field).repeated.items.cel = {
  id: "..."
  message: "..."
  expression: "this > 2"
}];
```

and 

```
repeated int32 ints = 1 [(buf.validate.field).cel = {
  id: "..."
  message: "..."
  expression: "size(this) > 2"
}];
```

This PR fixes the bug in second case where `buf lint` thought `this` is
an `int32`, while it should be a _list_ of `int32`.
  • Loading branch information
oliversun9 authored Jul 16, 2024
1 parent 2f39a05 commit b13657b
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 47 deletions.
1 change: 1 addition & 0 deletions private/bufpkg/bufcheck/buflint/buflint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,7 @@ func TestRunProtovalidate(t *testing.T) {
bufanalysistesting.NewFileAnnotation(t, "cel_field.proto", 88, 5, 92, 6, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "cel_field.proto", 106, 5, 110, 6, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "cel_field.proto", 116, 5, 120, 6, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "cel_field.proto", 156, 5, 160, 6, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "cel_message.proto", 22, 3, 26, 5, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "cel_message.proto", 28, 3, 32, 5, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "cel_message.proto", 34, 3, 38, 5, "PROTOVALIDATE"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ func checkCELForField(
adder *adder,
fieldConstraints *validate.FieldConstraints,
fieldDescriptor protoreflect.FieldDescriptor,
// forItems is true if the CEL rule is defined on a non-repeated field or on each item of a repeated field.
forItems bool,
) error {
if len(fieldConstraints.GetCel()) == 0 {
return nil
Expand All @@ -84,7 +86,7 @@ func checkCELForField(
celEnv, err = celEnv.Extend(
append(
celext.RequiredCELEnvOptions(fieldDescriptor),
cel.Variable("this", celext.ProtoFieldToCELType(fieldDescriptor, false, fieldDescriptor.IsList())),
cel.Variable("this", celext.ProtoFieldToCELType(fieldDescriptor, false, forItems)),
)...,
)
if err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ func checkConstraintsForField(
adder,
fieldConstraints,
fieldDescriptor,
!expectRepeatedRule,
); err != nil {
return err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,140 +8,155 @@ import "google/protobuf/timestamp.proto";

message Foo {
required double compile_fail = 1 [(buf.validate.field).cel = {
id: "Foo.compile_fail",
message: "compile_fail_message",
id: "Foo.compile_fail"
message: "compile_fail_message"
expression: "this > '1'"
}];
optional string some_expressions_fail = 2 [
// this one fails
(buf.validate.field).cel = {
id: "Foo.some_expressions_fail_1",
message: "some_expressions_fail_1_message",
id: "Foo.some_expressions_fail_1"
message: "some_expressions_fail_1_message"
expression: "this > 5"
},
// this one succeeds
(buf.validate.field).cel = {
id: "Foo.some_expressions_fail_2",
message: "some_expressions_fail_2_message",
id: "Foo.some_expressions_fail_2"
message: "some_expressions_fail_2_message"
expression: "size(this) > 5"
},
// this one fails
(buf.validate.field).cel = {
id: "Foo.some_expressions_fail_3",
message: "some_expressions_fail_3_message",
id: "Foo.some_expressions_fail_3"
message: "some_expressions_fail_3_message"
expression: "this * 2"
}
];
required int32 bad_return_type = 3 [(buf.validate.field).cel = {
id: "Foo.bad_return_type",
message: "some_expressions_fail_3_message",
id: "Foo.bad_return_type"
message: "some_expressions_fail_3_message"
expression: "this + 1"
}];
optional string no_validate = 4;
optional string no_cel = 5 [(buf.validate.field).string.email = true];
required string expressions_succeed = 6 [
(buf.validate.field).cel = {
id: "Foo.expressions_succeed",
message: "expression_succeed_message",
id: "Foo.expressions_succeed"
message: "expression_succeed_message"
expression: "this.startsWith('foo') && this.endsWith('bar')"
},
(buf.validate.field).cel = {
id: "Foo.expressions_succeed_2",
message: "expression_succeed_message_2",
id: "Foo.expressions_succeed_2"
message: "expression_succeed_message_2"
expression: "'bar' < this && this < 'foo'"
}
];
required string bar = 8;
option (buf.validate.message).cel = {
id: "Foo.bar.1",
message: "bar",
id: "Foo.bar.1"
message: "bar"
expression: "this.bar.matches('foo')"
};
option (buf.validate.message).cel = {
id: "Foo.bar.2",
message: "bar",
id: "Foo.bar.2"
message: "bar"
expression: "size(this.bar)"
};
required google.protobuf.Api transitive_dependency_succeed = 9 [(buf.validate.field).cel = {
id: "Foo.api",
message: "api",
id: "Foo.api"
message: "api"
expression: "this.source_context.file_name.contains('foo')"
}];
required google.protobuf.Duration duration = 10 [
(buf.validate.field).cel = {
id: "Foo.duration",
message: "duration",
id: "Foo.duration"
message: "duration"
expression: "this <= duration('23h59m59s')"
},
(buf.validate.field).cel = {
id: "Foo.duration.fail",
message: "duration_fail",
id: "Foo.duration.fail"
message: "duration_fail"
expression: "this <= 1"
}
];
required google.protobuf.Timestamp timestamp = 11 [
(buf.validate.field).cel = {
id: "Foo.timestamp",
message: "timestamp",
id: "Foo.timestamp"
message: "timestamp"
expression: "this < timestamp('1900-01-01T00:00:00+00:00')"
},
(buf.validate.field).cel = {
id: "Foo.timestamp.fail",
message: "duration_fail",
id: "Foo.timestamp.fail"
message: "duration_fail"
expression: "this < 1"
}
];
required google.protobuf.Any any = 12 [(buf.validate.field).cel = {
id: "Foo.any",
message: "any fail",
id: "Foo.any"
message: "any fail"
// This is valid.
expression: "this == this"
}];
map<int32, string> invalid_map = 13 [
(buf.validate.field).map.keys.cel = {
id: "map_keys_valid",
id: "map_keys_valid"
message: "foo"
expression: "this > 5"
},
(buf.validate.field).map.keys.cel = {
id: "map_keys_invalid",
id: "map_keys_invalid"
message: "foo"
expression: "this"
},
(buf.validate.field).map.values.cel = {
id: "map_values_valid",
id: "map_values_valid"
message: "foo"
expression: "size(this) < 100"
},
(buf.validate.field).map.values.cel = {
id: "map_values_invalid",
message: "foo",
id: "map_values_invalid"
message: "foo"
expression: "this * this"
}
];
// Valid CEL expressions for repeated fields.
repeated string allow_cidr = 14 [(buf.validate.field).repeated = {
min_items: 1,
min_items: 1
items: {
cel: [
{
id: "ip_prefix",
message: "value must be IPv4 prefix",
expression: "this.isIpPrefix(4, true)",
id: "ip_prefix"
message: "value must be IPv4 prefix"
expression: "this.isIpPrefix(4, true)"
}
],
},
]
}
}];
// Valid CEL expressions for map fields.
map<string, bool> allow_cidr_map = 15 [(buf.validate.field).map = {
keys: {
cel: [
{
id: "ip_prefix",
message: "key must be IPv4 prefix",
expression: "this.isIpPrefix(4, true)",
id: "ip_prefix"
message: "key must be IPv4 prefix"
expression: "this.isIpPrefix(4, true)"
}
],
},
]
}
}];
// CEL expression defined on the field itself (not each element) should treat
// "this" as a list, not the element type.
repeated int32 values = 16 [
(buf.validate.field).cel = {
id: "min_len_and_min_value"
message: "must have at least one value and all values must be greater than 2"
expression: "size(this) > 0 && this.all(x, x >= 2)"
},
// This is incorrect because "this" is not an int32 but a list of int32.
(buf.validate.field).cel = {
id: "min_value"
message: "each value must be greater than 2"
expression: "this > 2"
}
];
}

0 comments on commit b13657b

Please sign in to comment.