From 54ba1f7b2307d1b30d1c626fc01c55c9c8222844 Mon Sep 17 00:00:00 2001
From: jchadwick-buf <116005195+jchadwick-buf@users.noreply.github.com>
Date: Tue, 26 Nov 2024 15:32:40 -0500
Subject: [PATCH] Strict conformance fixes (#212)

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.
---
 gradle.properties                             |  2 +-
 .../protovalidate/internal/celext/Format.java | 19 ++++++++++++++-----
 .../internal/evaluator/MapEvaluator.java      |  6 +++++-
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/gradle.properties b/gradle.properties
index 0e405393..9e884fbc 100644
--- a/gradle.properties
+++ b/gradle.properties
@@ -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
diff --git a/src/main/java/build/buf/protovalidate/internal/celext/Format.java b/src/main/java/build/buf/protovalidate/internal/celext/Format.java
index df662494..82ea82a0 100644
--- a/src/main/java/build/buf/protovalidate/internal/celext/Format.java
+++ b/src/main/java/build/buf/protovalidate/internal/celext/Format.java
@@ -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;
@@ -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);
     }
@@ -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("\"");
@@ -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));
   }
 
   /**
diff --git a/src/main/java/build/buf/protovalidate/internal/evaluator/MapEvaluator.java b/src/main/java/build/buf/protovalidate/internal/evaluator/MapEvaluator.java
index a2894dfe..3110d390 100644
--- a/src/main/java/build/buf/protovalidate/internal/evaluator/MapEvaluator.java
+++ b/src/main/java/build/buf/protovalidate/internal/evaluator/MapEvaluator.java
@@ -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 {
@@ -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.