Skip to content

Commit

Permalink
Fix panic when using a repeated field as a predefined rule (#149)
Browse files Browse the repository at this point in the history
Using a repeated field as a predefined rule value results in a panic, as
CEL cannot convert a `protoreflect.List` value into a valid CEL type.
This results in an error value standing in for the list's value in the
expression. When attempting to optimize the expression via computing
residuals, a type assertion without the guard boolean (i.e., `typed :=
unknown.(T)` instead of `typed, ok := unknown.(T)`) triggers the panic
on this non-list error value.

This patch adds a new helper to the celext package to compute an
appropriate value for any `protoreflect.Value` given its field
descriptor.

Fixes #148
  • Loading branch information
rodaine authored Sep 30, 2024
1 parent ca39053 commit b477f4c
Show file tree
Hide file tree
Showing 8 changed files with 837 additions and 576 deletions.
8 changes: 4 additions & 4 deletions buf.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
version: v2
deps:
- name: buf.build/bufbuild/protovalidate
commit: a6c49f84cc0f4e038680d390392e2ab0
digest: b5:e968392e88ff7915adcbd1635d670b45bff8836ec2415d81fc559ca5470a695dbdc30030bad8bc5764647c731079e9e7bba0023ea25c4e4a1672a7d2561d4a19
commit: 5a7b106cbb87462d9a8c9ffecdbd2e38
digest: b5:0f2dc6c9453e9cc9e9f36807aaa5f94022e837d91fef4dcaeed79a35c0843cc64eba28ff077aab24da3b2cb12639ad256246f9f9a36c033b99d5754b19996b7e
- name: buf.build/bufbuild/protovalidate-testing
commit: 7c0e0340f14c4d6580c200dcaa9b00d0
digest: b5:f0dacc3a02e3fcd277c3516cd0089e69478374b5ad15fe1032853789668d1d660cac4c6e1776799a66b251b9583ddc320e9db91acf6e8681e31d76ccc1f22ea3
commit: 165ebb0f38244fc2a5b23a82cabe812f
digest: b5:4e3e21cabc93277d9069f7ded53c17659fefe47dd8daf9bc534387590feabe971b2d51b88b324835385650f3fa227e8a793a99ca9814cba76ee678f20ba9b4ef
- name: buf.build/envoyproxy/protoc-gen-validate
commit: daf171c6cdb54629b5f51e345a79e4dd
digest: b5:c745e1521879f43740230b1df673d0729f55704efefdcfc489d4a0a2d40c92a26cacfeab62813403040a8b180142d53b398c7ca784a065e43823605ee49681de
11 changes: 11 additions & 0 deletions celext/lookups.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ package celext

import (
"github.com/google/cel-go/cel"
"github.com/google/cel-go/common/types"
"github.com/google/cel-go/common/types/ref"
"google.golang.org/protobuf/reflect/protoreflect"
)

Expand Down Expand Up @@ -58,6 +60,15 @@ func ProtoFieldToCELType(fieldDesc protoreflect.FieldDescriptor, generic, forIte
return protoKindToCELType(fieldDesc.Kind())
}

func ProtoFieldToCELValue(fieldDesc protoreflect.FieldDescriptor, value protoreflect.Value, forItems bool) ref.Val {
switch {
case fieldDesc.IsList() && !forItems:
return types.NewProtoList(types.DefaultTypeAdapter, value.List())
default:
return types.DefaultTypeAdapter.NativeToValue(value.Interface())
}
}

// protoKindToCELType maps a protoreflect.Kind to a compatible cel.Type.
func protoKindToCELType(kind protoreflect.Kind) *cel.Type {
switch kind {
Expand Down
3 changes: 1 addition & 2 deletions internal/constraints/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"github.com/bufbuild/protovalidate-go/internal/expression"
"github.com/bufbuild/protovalidate-go/internal/extensions"
"github.com/google/cel-go/cel"
"github.com/google/cel-go/common/types"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/reflect/protoregistry"
Expand Down Expand Up @@ -77,7 +76,7 @@ func (c *Cache) Build(
cel.Constant(
"rule",
celext.ProtoFieldToCELType(desc, true, forItems),
types.DefaultTypeAdapter.NativeToValue(rule.Interface()),
celext.ProtoFieldToCELValue(desc, rule, forItems),
),
)
if compileErr != nil {
Expand Down
1,085 changes: 543 additions & 542 deletions internal/gen/buf/validate/conformance/cases/numbers.pb.go

Large diffs are not rendered by default.

56 changes: 28 additions & 28 deletions internal/gen/buf/validate/conformance/cases/wkt_wrappers.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

205 changes: 205 additions & 0 deletions internal/gen/tests/example/v1/predefined.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 35 additions & 0 deletions proto/tests/example/v1/predefined.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright 2023-2024 Buf Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

syntax = "proto2";

package tests.example.v1;

import "buf/validate/validate.proto";

// https://github.com/bufbuild/protovalidate-go/issues/148
message Issue148 {
optional int32 test = 1 [
(buf.validate.field).int32.(abs_not_in) = 1,
(buf.validate.field).int32.(abs_not_in) = -2
];
}

extend buf.validate.Int32Rules {
repeated int32 abs_not_in = 1800 [(buf.validate.predefined).cel = {
id: "int32.abs_not_in"
expression: "this in rule || this in rule.map(n, -n)"
message: "value must not be in absolute value of list"
}];
}
10 changes: 10 additions & 0 deletions validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
pb "github.com/bufbuild/protovalidate-go/internal/gen/tests/example/v1"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/anypb"
"google.golang.org/protobuf/types/known/apipb"
"google.golang.org/protobuf/types/known/fieldmaskpb"
Expand Down Expand Up @@ -240,3 +241,12 @@ func TestValidator_Validate_Issue141(t *testing.T) {
require.ErrorAs(t, err, &valErr)
})
}

func TestValidator_Validate_Issue148(t *testing.T) {
t.Parallel()
val, err := New()
require.NoError(t, err)
msg := &pb.Issue148{Test: proto.Int32(1)}
err = val.Validate(msg)
require.NoError(t, err)
}

0 comments on commit b477f4c

Please sign in to comment.