- Notifications
You must be signed in to change notification settings - Fork 16
Fix some issues with custom format function #277
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
Changes from all commits
e4f1916 3cab97f ea5ff72 5558c85 4dbc4f3 26665e1 aefec2e 7b75aab 1453f41 File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -139,52 +139,25 @@ private static String bytesToHex(byte[] bytes, char[] digits) { | |
| * @param val the value to format. | ||
| */ | ||
| 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(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); | ||
| } | ||
| } | ||
| | ||
| /** | ||
| * Formats a string value safely for other value types. | ||
| * | ||
| * @param builder the StringBuilder to append the formatted string to. | ||
| * @param val the value to format. | ||
| * @param listType indicates if the value type is a list. | ||
| */ | ||
| private static void formatStringSafe(StringBuilder builder, Val val, boolean listType) { | ||
| TypeEnum type = val.type().typeEnum(); | ||
| if (type == TypeEnum.Bool) { | ||
| builder.append(val.booleanValue()); | ||
| } else if (type == TypeEnum.Int || type == TypeEnum.Uint) { | ||
| formatDecimal(builder, val); | ||
| } else if (type == TypeEnum.Double) { | ||
| // 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("\""); | ||
| } else if (type == TypeEnum.String || type == TypeEnum.Int || type == TypeEnum.Uint) { | ||
| builder.append(val.value()); | ||
| } else if (type == TypeEnum.Bytes) { | ||
| formatBytes(builder, val); | ||
| builder.append(new String((byte[]) val.value(), StandardCharsets.UTF_8)); | ||
| Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mainly a question on CEL in general but can we always assume bytes can be converted to a valid UTF-8 string? Is there some fallback where it would display invalid UTF-8 strings as hex or something like that? Contributor Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our formatting is pretty inconsistent right now and we need to add more comprehensive tests to make sure it behaves the same. Right now, CEL-Go's built-in formatting function will throw a runtime error with invalid UTF-8, but our Java implementation will print placeholders �� when using | ||
| } else if (type == TypeEnum.Double) { | ||
| formatDecimal(builder, val); | ||
| } else if (type == TypeEnum.Duration) { | ||
| formatDuration(builder, val, listType); | ||
| formatDuration(builder, val); | ||
| } else if (type == TypeEnum.Timestamp) { | ||
| formatTimestamp(builder, val); | ||
| } else if (type == TypeEnum.List) { | ||
| formatList(builder, val); | ||
| } else if (type == TypeEnum.Map) { | ||
| throw new ErrException("unimplemented stringSafe map type"); | ||
| throw new ErrException("unimplemented string map type"); | ||
| } else if (type == TypeEnum.Null) { | ||
| throw new ErrException("unimplemented stringSafe null type"); | ||
| throw new ErrException("unimplemented string null type"); | ||
| } | ||
| } | ||
| | ||
| | @@ -200,7 +173,7 @@ private static void formatList(StringBuilder builder, Val val) { | |
| List list = val.convertToNative(List.class); | ||
| for (int i = 0; i < list.size(); i++) { | ||
| Object obj = list.get(i); | ||
| formatStringSafe(builder, DefaultTypeAdapter.nativeToValue(Db.newDb(), null, obj), true); | ||
| formatString(builder, DefaultTypeAdapter.nativeToValue(Db.newDb(), null, obj)); | ||
| if (i != list.size() - 1) { | ||
| builder.append(", "); | ||
| } | ||
| | @@ -225,35 +198,15 @@ private static void formatTimestamp(StringBuilder builder, Val val) { | |
| * | ||
| * @param builder the StringBuilder to append the formatted duration value to. | ||
| * @param val the value to format. | ||
| * @param listType indicates if the value type is a list. | ||
| */ | ||
| private static void formatDuration(StringBuilder builder, Val val, boolean listType) { | ||
| if (listType) { | ||
| builder.append("duration(\""); | ||
| } | ||
| private static void formatDuration(StringBuilder builder, Val val) { | ||
| Duration duration = val.convertToNative(Duration.class); | ||
| | ||
| double totalSeconds = duration.getSeconds() + (duration.getNanos() / 1_000_000_000.0); | ||
| | ||
| DecimalFormat format = new DecimalFormat("0.#########"); | ||
| builder.append(format.format(totalSeconds)); | ||
| DecimalFormat formatter = new DecimalFormat("0.#########"); | ||
| builder.append(formatter.format(totalSeconds)); | ||
| builder.append("s"); | ||
| if (listType) { | ||
| builder.append("\")"); | ||
| } | ||
| } | ||
| | ||
| /** | ||
| * Formats a byte array value. | ||
| * | ||
| * @param builder the StringBuilder to append the formatted byte array value to. | ||
| * @param val the value to format. | ||
| */ | ||
| private static void formatBytes(StringBuilder builder, Val val) { | ||
| builder | ||
| .append("\"") | ||
| .append(new String((byte[]) val.value(), StandardCharsets.UTF_8)) | ||
| .append("\""); | ||
| } | ||
| | ||
| /** | ||
| | @@ -283,9 +236,10 @@ private static void formatHex(StringBuilder builder, Val val, char[] digits) { | |
| * Formats a decimal value. | ||
| * | ||
| * @param builder the StringBuilder to append the formatted decimal value to. | ||
| * @param arg the value to format. | ||
| * @param val the value to format. | ||
| */ | ||
| private static void formatDecimal(StringBuilder builder, Val arg) { | ||
| builder.append(arg.value()); | ||
| private static void formatDecimal(StringBuilder builder, Val val) { | ||
| DecimalFormat formatter = new DecimalFormat("0.#########"); | ||
| builder.append(formatter.format(val.value())); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -15,18 +15,107 @@ | |
| package build.buf.protovalidate; | ||
| | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
| | ||
| import com.google.protobuf.Duration; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.projectnessie.cel.common.types.DoubleT; | ||
| import org.projectnessie.cel.common.types.Err.ErrException; | ||
| import org.projectnessie.cel.common.types.ListT; | ||
| import org.projectnessie.cel.common.types.StringT; | ||
| import org.projectnessie.cel.common.types.UintT; | ||
| import org.projectnessie.cel.common.types.pb.DefaultTypeAdapter; | ||
| import org.projectnessie.cel.common.types.ref.Val; | ||
| | ||
| class FormatTest { | ||
| @Test | ||
| void largeDecimalValuesAreProperlyFormatted() { | ||
| void testNotEnoughArgumentsThrows() { | ||
| StringT one = StringT.stringOf("one"); | ||
| ListT val = (ListT) ListT.newValArrayList(null, new Val[] {one}); | ||
| | ||
| assertThatThrownBy( | ||
| () -> { | ||
| Format.format("first value: %s and %s", val); | ||
| }) | ||
| .isInstanceOf(ErrException.class) | ||
| .hasMessageContaining("format: not enough arguments"); | ||
| } | ||
| | ||
| @Test | ||
| void testDouble() { | ||
| Contributor Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jchadwick-buf this is what I meant by some nuance in testing. We could move these double tests to the conformance tests I guess, but not sure about things like testing the above thrown exception. Wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I'm not overly concerned about handling all of the error cases identically as long as the error cases do fail as-expected, so I don't see this as a huge problem for adding tests to conformance per-se. | ||
| ListT val = | ||
| (ListT) | ||
| ListT.newValArrayList( | ||
| null, | ||
| new Val[] { | ||
| DoubleT.doubleOf(-1.20000000000), | ||
| DoubleT.doubleOf(-1.2), | ||
| DoubleT.doubleOf(-1.230), | ||
| DoubleT.doubleOf(-1.002), | ||
| DoubleT.doubleOf(-0.1), | ||
| DoubleT.doubleOf(-.1), | ||
| DoubleT.doubleOf(-1), | ||
| DoubleT.doubleOf(-0.0), | ||
| DoubleT.doubleOf(0), | ||
| DoubleT.doubleOf(0.0), | ||
| DoubleT.doubleOf(1), | ||
| DoubleT.doubleOf(0.1), | ||
| DoubleT.doubleOf(.1), | ||
| DoubleT.doubleOf(1.002), | ||
| DoubleT.doubleOf(1.230), | ||
| DoubleT.doubleOf(1.20000000000) | ||
| }); | ||
| String formatted = | ||
| Format.format("%d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d, %d", val); | ||
| assertThat(formatted) | ||
| .isEqualTo( | ||
| "-1.2, -1.2, -1.23, -1.002, -0.1, -0.1, -1, -0, 0, 0, 1, 0.1, 0.1, 1.002, 1.23, 1.2"); | ||
| } | ||
| | ||
| @Test | ||
| void testLargeDecimalValuesAreProperlyFormatted() { | ||
| UintT largeDecimal = UintT.uintOf(999999999999L); | ||
| ListT val = (ListT) ListT.newValArrayList(null, new Val[] {largeDecimal}); | ||
| String formatted = Format.format("%s", val); | ||
| assertThat(formatted).isEqualTo("999999999999"); | ||
| } | ||
| | ||
| @Test | ||
| void testDuration() { | ||
| Duration duration = Duration.newBuilder().setSeconds(123).setNanos(45678).build(); | ||
| | ||
| ListT val = | ||
| (ListT) ListT.newGenericArrayList(DefaultTypeAdapter.Instance, new Duration[] {duration}); | ||
| String formatted = Format.format("%s", val); | ||
| assertThat(formatted).isEqualTo("123.000045678s"); | ||
| } | ||
| | ||
| @Test | ||
| void testEmptyDuration() { | ||
| Duration duration = Duration.newBuilder().build(); | ||
| ListT val = | ||
| (ListT) ListT.newGenericArrayList(DefaultTypeAdapter.Instance, new Duration[] {duration}); | ||
| String formatted = Format.format("%s", val); | ||
| assertThat(formatted).isEqualTo("0s"); | ||
| } | ||
| | ||
| @Test | ||
| void testDurationSecondsOnly() { | ||
| Duration duration = Duration.newBuilder().setSeconds(123).build(); | ||
| | ||
| ListT val = | ||
| (ListT) ListT.newGenericArrayList(DefaultTypeAdapter.Instance, new Duration[] {duration}); | ||
| String formatted = Format.format("%s", val); | ||
| assertThat(formatted).isEqualTo("123s"); | ||
| } | ||
| | ||
| @Test | ||
| void testDurationNanosOnly() { | ||
| Duration duration = Duration.newBuilder().setNanos(42).build(); | ||
| | ||
| ListT val = | ||
| (ListT) ListT.newGenericArrayList(DefaultTypeAdapter.Instance, new Duration[] {duration}); | ||
| String formatted = Format.format("%s", val); | ||
| assertThat(formatted).isEqualTo("0.000000042s"); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this bc it's much easier to see failing tests in the terminal than having to navigate out to a browser link to see the failing tests.