Skip to content

Commit

Permalink
Strict conformance fixes (#212)
Browse files Browse the repository at this point in the history
As it turns out, not specifying `--strict` to the conformance test
runner will cause `--strict_message` and `--strict_error` to do nothing.
The conformance runner will just silently treat any validation errors as
matching. Yikes.

Thankfully, protovalidate-java only had a few issues hidden by this
footgun, and they are easily rectified after a bit of debugging.

Most of these issues are really quite minor and mainly just have to do
with consistency across languages, but notably we were not marking
validation errors from map keys properly, so that will be worth rolling
into a release as soon as possible.
  • Loading branch information
jchadwick-buf authored Nov 26, 2024
1 parent ac7ebb0 commit 54ba1f7
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 7 deletions.
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
protovalidate.version = v0.8.2

# Arguments to the protovalidate-conformance CLI
protovalidate.conformance.args = --strict_message --strict_error
protovalidate.conformance.args = --strict --strict_message --strict_error

# Argument to the license-header CLI
license-header.years = 2023-2024
19 changes: 14 additions & 5 deletions src/main/java/build/buf/protovalidate/internal/celext/Format.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,13 @@

package build.buf.protovalidate.internal.celext;

import static java.time.format.DateTimeFormatter.ISO_INSTANT;

import com.google.protobuf.Duration;
import com.google.protobuf.Timestamp;
import java.nio.charset.StandardCharsets;
import java.text.DecimalFormat;
import java.time.Instant;
import java.util.List;
import org.projectnessie.cel.common.types.Err.ErrException;
import org.projectnessie.cel.common.types.IntT;
Expand Down Expand Up @@ -139,7 +142,10 @@ private static void formatString(StringBuilder builder, Val val) {
if (val.type().typeEnum() == TypeEnum.String) {
builder.append(val.value());
} else if (val.type().typeEnum() == TypeEnum.Bytes) {
builder.append(val.value());
builder.append(new String((byte[]) val.value(), StandardCharsets.UTF_8));
} else if (val.type().typeEnum() == TypeEnum.Double) {
DecimalFormat format = new DecimalFormat();
builder.append(format.format(val.value()));
} else {
formatStringSafe(builder, val, false);
}
Expand All @@ -159,7 +165,11 @@ private static void formatStringSafe(StringBuilder builder, Val val, boolean lis
} else if (type == TypeEnum.Int || type == TypeEnum.Uint) {
formatDecimal(builder, val);
} else if (type == TypeEnum.Double) {
DecimalFormat format = new DecimalFormat("0.#");
// When a double is nested in another type (e.g. a list) it will have a minimum of 6 decimal
// digits. This is to maintain consistency with the Go CEL runtime.
DecimalFormat format = new DecimalFormat();
format.setMaximumFractionDigits(Integer.MAX_VALUE);
format.setMinimumFractionDigits(6);
builder.append(format.format(val.value()));
} else if (type == TypeEnum.String) {
builder.append("\"").append(val.value().toString()).append("\"");
Expand Down Expand Up @@ -205,10 +215,9 @@ private static void formatList(StringBuilder builder, Val val) {
* @param val the value to format.
*/
private static void formatTimestamp(StringBuilder builder, Val val) {
builder.append("timestamp(");
Timestamp timestamp = val.convertToNative(Timestamp.class);
builder.append(timestamp.toString());
builder.append(")");
Instant instant = Instant.ofEpochSecond(timestamp.getSeconds(), timestamp.getNanos());
builder.append(ISO_INSTANT.format(instant));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

/** Performs validation on a map field's key-value pairs. */
class MapEvaluator implements Evaluator {
Expand Down Expand Up @@ -84,7 +85,10 @@ public ValidationResult evaluate(Value val, boolean failFast) throws ExecutionEx

private List<Violation> evalPairs(Value key, Value value, boolean failFast)
throws ExecutionException {
List<Violation> keyViolations = keyEvaluator.evaluate(key, failFast).getViolations();
List<Violation> keyViolations =
keyEvaluator.evaluate(key, failFast).getViolations().stream()
.map(violation -> violation.toBuilder().setForKey(true).build())
.collect(Collectors.toList());
final List<Violation> valueViolations;
if (failFast && !keyViolations.isEmpty()) {
// Don't evaluate value constraints if failFast is enabled and keys failed validation.
Expand Down

0 comments on commit 54ba1f7

Please sign in to comment.