Fix validation error message bug

[lyang]

Symptom

Some standard validation error messages have Integer Overflow bug on large decimal values.

Example

message LargeValue {
  uint64 count = 1 [(buf.validate.field).uint64 = {gte: 0, lte: 999999999999}];
}

@Test
public void validate_count() {
  LargeValue value = LargeValue.newBuilder().setCount(9999999999999).build();
  ValidationResult result = new Validator().validate(value);
  assertThat(result.getViolations().get(0).getMessage()).isEqualTo("value must be greater than or equal to 0 and less than or equal to 999999999999");
  // Above fails with: "value must be greater than or equal to 0 and less than or equal to -727379969"
}

Cause

This is due to %s interpreted as String in Format.java:

// ...
        case 's':
          formatString(builder, arg);
          break;
// ...

which eventually calls:

// ...
    } else if (type == TypeEnum.Int || type == TypeEnum.Uint) {
      formatInteger(builder, Long.valueOf(val.intValue()).intValue());
// ...

Change

Changed those %s placeholders for decimal values to %d, so that they can be properly formatted for large values.

Note

I've only tested those change with protovalidate-java, not sure if it breaks/fixes other languages.

[CLAassistant]

All committers have signed the CLA.

[lyang]

If I understand the CI error correctly, it says I need to run buf generate and check in the updated tools/internal/gen/buf/validate/validate.pb.go ?

[nicksnyder]

Thanks for reporting this! This problem looks more to me like a bug in the code you quoted in Format.java. It is taking a uint and jamming it into a regular int. I think the right solve is to open a PR on protovalidate-java to introduce a failing test, and then to fix the code so it doesn't fail.

I suspect the solution would look something like

• Updating formatInteger to be formatLong (with a long type as the second arg)
• Updating call site to be formatLong(builder, val.intValue());

[lyang]

Hi @nicksnyder,

I agree that protovalidate-java also needs a fix there, as it unnecessarily coerces large values. I can create a separate PR there.

But at the same time, the semantic / intention of those lte / len etc are actually better expressed in %d rather than %s, except maybe a few places for floaty types. Not sure why protovalidate-java chose not to support %f (and %e, %b, %o) though.

Using a reference from https://github.com/google/cel-go/blob/master/ext/strings.go:

// Passing an incorrect type (an integer to %s) is considered an error, as well as attempting
// to use more formatting clauses than there are arguments (%d %d %d while passing two ints, for instance).

Overall, replacing some of those %s to %d should apply to all languages, while protovalidate-java changes only apply to java.

[nicksnyder]

I don't think the change you are proposing here is necessarily wrong, but I would expect %s to work (in other words, keeping it %s everywhere seems like it is also not wrong).

The docs in cel-go seem mostly to support the fact that it is valid to use %s to format an int

• The definition of %s clearly states that int is a valid type.

  // %s - substitutes a string. This can also be used on bools, lists, maps, bytes,
  // Duration and Timestamp, in addition to all numerical types (int, uint, and double).

• The examples also show %s used with many non-string types.

  // Examples:
  //
  // "this is a string: %s\nand an integer: %d".format(["str", 42]) // returns "this is a string: str\nand an integer: 42"
  // "a double substituted with %%s: %s".format([64.2]) // returns "a double substituted with %s: 64.2"
  // "string type: %s".format([type(string)]) // returns "string type: string"
  // "timestamp: %s".format([timestamp("2023-02-03T23:31:20+00:00")]) // returns "timestamp: 2023-02-03T23:31:20Z"
  // "duration: %s".format([duration("1h45m47s")]) // returns "duration: 6347s"
  // "%f".format([3.14]) // returns "3.140000"
  // "scientific notation: %e".format([2.71828]) // returns "scientific notation: 2.718280\u202f\u00d7\u202f10\u2070\u2070"
  // "5 in binary: %b".format([5]), // returns "5 in binary; 101"
  // "26 in hex: %x".format([26]), // returns "26 in hex: 1a"
  // "26 in hex (uppercase): %X".format([26]) // returns "26 in hex (uppercase): 1A"
  // "30 in octal: %o".format([30]) // returns "30 in octal: 36"
  // "a map inside a list: %s".format([[1, 2, 3, {"a": "x", "b": "y", "c": "z"}]]) // returns "a map inside a list: [1, 2, 3, {"a":"x", "b":"y", "c":"d"}]"
  // "true bool: %s - false bool: %s\nbinary bool: %b".format([true, false, true]) // returns "true bool: true - false bool: false\nbinary bool: 1"

I didn't find an exact test case for an int being passed into a string, but I added a test case locally and it passes

diff --git a/ext/strings_test.go b/ext/strings_test.go
index fc0ade1..564fd10 100644
--- a/ext/strings_test.go
+++ b/ext/strings_test.go
@@ -713,6 +713,14 @@ func TestStringFormat(t *testing.T) {
 			expectedRuntimeCost:   11,
 			expectedEstimatedCost: checker.CostEstimate{Min: 11, Max: 11},
 		},
+		{
+			name:                  "int support for string",
+			format:                "%s",
+			formatArgs:            `999999999999`,
+			expectedOutput:        "999999999999",
+			expectedRuntimeCost:   11,
+			expectedEstimatedCost: checker.CostEstimate{Min: 11, Max: 11},
+		},
 		{
 			name:                  "bytes support for string",
 			format:                "some bytes: %s",

This leads me to believe that the parenthetical example in this docstring is just wrong:

Passing an incorrect type (an integer to %s) is considered an error

[lyang]

Alright, I think that makes sense.

I'll make a PR in protovalidate-java then. That seems a safer change and works for my current needs. 😄

Thanks!

[nicksnyder]

I opened an upstream PR to fix docs: google/cel-go#873

[lyang]

PR in bufbuild/protovalidate-java#75