Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,13 @@ allprojects {
}
tasks.withType<Test>().configureEach {
useJUnitPlatform()
this.testLogging {
events("failed")
exceptionFormat = org.gradle.api.tasks.testing.logging.TestExceptionFormat.FULL
showExceptions = true
showCauses = true
showStackTraces = true
}
Comment on lines +210 to +216
Copy link
Contributor Author

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.

}
}

Expand Down
11 changes: 0 additions & 11 deletions conformance/expected-failures.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,3 @@ custom_rules:
- field_expression/map/int64/invalid
- field_expression/map/uint64/valid
- field_expression/map/uint64/invalid
kitchen_sink:
- field/embedded/invalid
- field/transitive/invalid
- many/all-non-message-fields/invalid
- field/invalid
standard_rules/repeated:
- items/in/invalid
- items/not_in/invalid
standard_rules/well_known_types/duration:
- in/invalid
- not in/invalid
78 changes: 16 additions & 62 deletions src/main/java/build/buf/protovalidate/Format.java
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 %s and will just print hex digits when using %x. It's on the docket to unify all this behavior soon.

} 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");
}
}

Expand All @@ -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(", ");
}
Expand All @@ -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("\"");
}

/**
Expand Down Expand Up @@ -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()));
}
}
91 changes: 90 additions & 1 deletion src/test/java/build/buf/protovalidate/FormatTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link

Choose a reason for hiding this comment

The 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");
}
}