Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update protovalidate-go to work with 0.6.1 protos #101

Merged
merged 3 commits into from
Feb 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ issues:
exclude:
# we will continue to support the deprecated field until it's removed
- "IgnoreEmpty is deprecated"
- "Skipped is deprecated"
exclude-rules:
# Loosen requirements on conformance executor
- path: internal/cmd/
Expand Down Expand Up @@ -84,4 +85,4 @@ issues:
- path: resolver/resolver.go
linters:
# uses deprecated fields on protoimpl.ExtensionInfo but its the only way
- staticcheck
- staticcheck
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ ARGS ?= --strict --strict_message --strict_error
# Set to use a different version of protovalidate-conformance.
# Should be kept in sync with the version referenced in proto/buf.lock and
# 'buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go' in go.mod.
CONFORMANCE_VERSION ?= v0.5.6
CONFORMANCE_VERSION ?= v0.6.1

.PHONY: help
help: ## Describe useful make targets
Expand Down Expand Up @@ -94,7 +94,7 @@ $(BIN)/license-header: $(BIN) Makefile

$(BIN)/golangci-lint: $(BIN) Makefile
GOBIN=$(abspath $(@D)) $(GO) install \
github.com/golangci/golangci-lint/cmd/golangci-lint@v1.52.2
github.com/golangci/golangci-lint/cmd/golangci-lint@v1.56.1

$(BIN)/protovalidate-conformance: $(BIN) Makefile
GOBIN=$(abspath $(BIN)) $(GO) install \
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module github.com/bufbuild/protovalidate-go
go 1.19

require (
buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.32.0-20240212200630-3014d81c3a48.1
buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.32.0-20240221180331-f05a6f4403ce.1
github.com/envoyproxy/protoc-gen-validate v1.0.4
github.com/google/cel-go v0.20.0
github.com/stretchr/testify v1.8.4
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.32.0-20240212200630-3014d81c3a48.1 h1:rOe/itdO7+9cWOnKqvpn1K7VmTDwp3vI4juPz6aNQbc=
buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.32.0-20240212200630-3014d81c3a48.1/go.mod h1:tiTMKD8j6Pd/D2WzREoweufjzaJKHZg35f/VGcZ2v3I=
buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.32.0-20240221180331-f05a6f4403ce.1 h1:AmmAwHbvaeOIxDKG2+aTn5C36HjmFIMkrdTp49rp80Q=
buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.32.0-20240221180331-f05a6f4403ce.1/go.mod h1:tiTMKD8j6Pd/D2WzREoweufjzaJKHZg35f/VGcZ2v3I=
github.com/antlr4-go/antlr/v4 v4.13.0 h1:lxCg3LAv+EUK6t1i0y1V6/SLeUi0eKEKdhQAlS8TVTI=
github.com/antlr4-go/antlr/v4 v4.13.0/go.mod h1:pfChB/xh/Unjila75QW7+VU4TSnWnnk9UTnmpPaOR2g=
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
Expand Down
2 changes: 1 addition & 1 deletion internal/constraints/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (c *Cache) Build(
}

var asts expression.ASTSet
constraints.Range(func(desc protoreflect.FieldDescriptor, val protoreflect.Value) bool {
constraints.Range(func(desc protoreflect.FieldDescriptor, _ protoreflect.Value) bool {
precomputedASTs, compileErr := c.loadOrCompileStandardConstraint(env, desc)
if compileErr != nil {
err = compileErr
Expand Down
6 changes: 3 additions & 3 deletions internal/constraints/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func TestCache_BuildStandardConstraints(t *testing.T) {
if test.exErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
require.NoError(t, err)
assert.Len(t, set, test.exCt)
}
})
Expand All @@ -126,15 +126,15 @@ func TestCache_LoadOrCompileStandardConstraint(t *testing.T) {
assert.False(t, ok)

asts, err := cache.loadOrCompileStandardConstraint(env, desc)
assert.NoError(t, err)
require.NoError(t, err)
assert.NotNil(t, asts)

cached, ok := cache.cache[desc]
assert.True(t, ok)
assert.Equal(t, cached, asts)

asts, err = cache.loadOrCompileStandardConstraint(env, desc)
assert.NoError(t, err)
require.NoError(t, err)
assert.Equal(t, cached, asts)
}

Expand Down
4 changes: 2 additions & 2 deletions internal/errors/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ func TestMerge(t *testing.T) {
t.Run("no errors", func(t *testing.T) {
t.Parallel()
ok, err := Merge(nil, nil, true)
assert.NoError(t, err)
require.NoError(t, err)
assert.True(t, ok)
ok, err = Merge(nil, nil, false)
assert.NoError(t, err)
require.NoError(t, err)
assert.True(t, ok)
})

Expand Down
16 changes: 8 additions & 8 deletions internal/errors/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@ func (err *ValidationError) Error() string {
bldr.WriteString("validation error:")
for _, violation := range err.Violations {
bldr.WriteString("\n - ")
if violation.FieldPath != "" {
bldr.WriteString(violation.FieldPath)
if fieldPath := violation.GetFieldPath(); fieldPath != "" {
bldr.WriteString(fieldPath)
bldr.WriteString(": ")
}
_, _ = fmt.Fprintf(bldr, "%s [%s]",
violation.Message,
violation.ConstraintId)
violation.GetMessage(),
violation.GetConstraintId())
}
return bldr.String()
}
Expand All @@ -52,12 +52,12 @@ func PrefixFieldPaths(err *ValidationError, format string, args ...any) {
prefix := fmt.Sprintf(format, args...)
for _, violation := range err.Violations {
switch {
case violation.FieldPath == "": // no existing field path
case violation.GetFieldPath() == "": // no existing field path
violation.FieldPath = prefix
case violation.FieldPath[0] == '[': // field is a map/list
violation.FieldPath = prefix + violation.FieldPath
case violation.GetFieldPath()[0] == '[': // field is a map/list
violation.FieldPath = prefix + violation.GetFieldPath()
default: // any other field
violation.FieldPath = fmt.Sprintf("%s.%s", prefix, violation.FieldPath)
violation.FieldPath = fmt.Sprintf("%s.%s", prefix, violation.GetFieldPath())
}
}
}
2 changes: 1 addition & 1 deletion internal/errors/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func TestPrefixFieldPaths(t *testing.T) {
}}
PrefixFieldPaths(err, test.format, test.args...)
for _, v := range err.Violations {
assert.Equal(t, test.expected, v.FieldPath)
assert.Equal(t, test.expected, v.GetFieldPath())
}
})
}
Expand Down
36 changes: 28 additions & 8 deletions internal/evaluator/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,21 @@ func (bldr *Builder) buildField(
cache MessageCache,
) (field, error) {
fld := field{
Descriptor: fieldDescriptor,
Required: fieldConstraints.GetRequired(),
IgnoreEmpty: fieldDescriptor.HasPresence() || fieldConstraints.GetIgnoreEmpty(),
Descriptor: fieldDescriptor,
Required: fieldConstraints.GetRequired(),
IgnoreEmpty: fieldDescriptor.HasPresence() ||
fieldConstraints.GetIgnoreEmpty() ||
fieldConstraints.GetIgnore() == validate.Ignore_IGNORE_IF_UNPOPULATED ||
fieldConstraints.GetIgnore() == validate.Ignore_IGNORE_IF_DEFAULT_VALUE,
IgnoreDefault: fieldDescriptor.HasPresence() && fieldConstraints.GetIgnore() == validate.Ignore_IGNORE_IF_DEFAULT_VALUE,
}
if fld.IgnoreDefault {
if fieldDescriptor.Kind() == protoreflect.MessageKind && fieldDescriptor.Cardinality() != protoreflect.Repeated {
msg := dynamicpb.NewMessage(fieldDescriptor.Message())
fld.Zero = protoreflect.ValueOfMessage(msg)
} else {
fld.Zero = fieldDescriptor.Default()
}
}
err := bldr.buildValue(fieldDescriptor, fieldConstraints, false, &fld.Value, cache)
return fld, err
Expand Down Expand Up @@ -254,15 +266,21 @@ func (bldr *Builder) processIgnoreEmpty(
) error {
// the only time we need to ignore empty on a value is if it's evaluating a
// field item (repeated element or map key/value).
val.IgnoreEmpty = forItems && constraints.GetIgnoreEmpty()
if !val.IgnoreEmpty {
val.IgnoreEmpty = forItems && (constraints.GetIgnoreEmpty() ||
constraints.GetIgnore() == validate.Ignore_IGNORE_IF_UNPOPULATED ||
constraints.GetIgnore() == validate.Ignore_IGNORE_IF_DEFAULT_VALUE)
switch {
case !val.IgnoreEmpty:
// only need the zero value for checking ignore_empty constraint
return nil
}
val.Zero = fdesc.Default()
if forItems && fdesc.IsList() {
case fdesc.IsList():
msg := dynamicpb.NewMessage(fdesc.ContainingMessage())
val.Zero = msg.Get(fdesc).List().NewElement()
case fdesc.Kind() == protoreflect.MessageKind:
msg := dynamicpb.NewMessage(fdesc.Message())
val.Zero = protoreflect.ValueOfMessage(msg)
default:
val.Zero = fdesc.Default()
}
return nil
}
Expand Down Expand Up @@ -303,6 +321,7 @@ func (bldr *Builder) processEmbeddedMessage(
) error {
if fdesc.Kind() != protoreflect.MessageKind ||
rules.GetSkipped() ||
rules.GetIgnore() == validate.Ignore_IGNORE_ALWAYS ||
fdesc.IsMap() || (fdesc.IsList() && !forItems) {
return nil
}
Expand All @@ -327,6 +346,7 @@ func (bldr *Builder) processWrapperConstraints(
) error {
if fdesc.Kind() != protoreflect.MessageKind ||
rules.GetSkipped() ||
rules.GetIgnore() == validate.Ignore_IGNORE_ALWAYS ||
fdesc.IsMap() || (fdesc.IsList() && !forItems) {
return nil
}
Expand Down
8 changes: 8 additions & 0 deletions internal/evaluator/field.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ type field struct {
// This field is generally true for nullable fields or fields with the
// ignore_empty constraint explicitly set.
IgnoreEmpty bool
// IgnoreDefault indicates if a field should skip validation on its zero value,
// including for fields which have field presence and are set to the zero value.
IgnoreDefault bool
// Zero is the default or zero-value for this value's type
Zero protoreflect.Value
}

func (f field) Evaluate(val protoreflect.Value, failFast bool) error {
Expand All @@ -53,6 +58,9 @@ func (f field) EvaluateMessage(msg protoreflect.Message, failFast bool) (err err
}

val := msg.Get(f.Descriptor)
if f.IgnoreDefault && val.Equal(f.Zero) {
return nil
}
if err = f.Value.Evaluate(val, failFast); err != nil {
errors.PrefixErrorPaths(err, "%s", f.Descriptor.Name())
}
Expand Down
4 changes: 2 additions & 2 deletions internal/expression/ast_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func TestASTSet_ToProgramSet(t *testing.T) {
empty := ASTSet{}
set, err = empty.ToProgramSet()
assert.Empty(t, set)
assert.NoError(t, err)
require.NoError(t, err)
}

func TestASTSet_ReduceResiduals(t *testing.T) {
Expand All @@ -86,6 +86,6 @@ func TestASTSet_ReduceResiduals(t *testing.T) {
require.NoError(t, err)
assert.Len(t, asts.asts, 1)
set, err := asts.ReduceResiduals(cel.Globals(&Variable{Name: "foo", Val: true}))
assert.NoError(t, err)
require.NoError(t, err)
assert.Empty(t, set)
}
10 changes: 5 additions & 5 deletions internal/expression/compile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestCompile(t *testing.T) {
var exprs []*validate.Constraint
set, err := Compile(exprs, baseEnv)
assert.Nil(t, set)
assert.NoError(t, err)
require.NoError(t, err)
})

t.Run("success", func(t *testing.T) {
Expand All @@ -47,7 +47,7 @@ func TestCompile(t *testing.T) {
}
set, err := Compile(exprs, baseEnv, cel.Variable("this", cel.IntType))
assert.Len(t, set, len(exprs))
assert.NoError(t, err)
require.NoError(t, err)
})

t.Run("env extension err", func(t *testing.T) {
Expand All @@ -58,7 +58,7 @@ func TestCompile(t *testing.T) {
set, err := Compile(exprs, baseEnv, cel.Types(true))
assert.Nil(t, set)
var compErr *errors.CompilationError
assert.ErrorAs(t, err, &compErr)
require.ErrorAs(t, err, &compErr)
})

t.Run("bad syntax", func(t *testing.T) {
Expand All @@ -69,7 +69,7 @@ func TestCompile(t *testing.T) {
set, err := Compile(exprs, baseEnv)
assert.Nil(t, set)
var compErr *errors.CompilationError
assert.ErrorAs(t, err, &compErr)
require.ErrorAs(t, err, &compErr)
})

t.Run("invalid output type", func(t *testing.T) {
Expand All @@ -80,6 +80,6 @@ func TestCompile(t *testing.T) {
set, err := Compile(exprs, baseEnv)
assert.Nil(t, set)
var compErr *errors.CompilationError
assert.ErrorAs(t, err, &compErr)
require.ErrorAs(t, err, &compErr)
})
}
Loading
Loading