Skip to content
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

Update to protovalidate 0.6.1 #106

Merged
merged 6 commits into from
Mar 5, 2024
Merged

Update to protovalidate 0.6.1 #106

merged 6 commits into from
Mar 5, 2024

Conversation

pkwarren
Copy link
Member

@pkwarren pkwarren commented Mar 5, 2024

Update protovalidate-java to support the latest rules in protovalidate 0.6.1. Add the isHostAndPort overload and apply additional fixes from protovalidate-go to the Java implementation.

Update protovalidate-java to support the latest rules in protovalidate
0.6.1. Add the isHostAndPort overload and apply additional fixes from
protovalidate-go to the Java implementation.
@pkwarren pkwarren requested review from jhump and drice-buf March 5, 2024 17:05
val stdOptions = options as StandardJavadocDocletOptions
stdOptions.addBooleanOption("Xwerror", true)
// Ignore warnings for generated code.
stdOptions.addBooleanOption("Xdoclint/package:-build.buf.validate,-build.buf.validate.priv", true)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured out how to ignore warnings on Javadoc for generated protoc code but still mark warnings as errors for our library code.

null);
}

private static boolean hostAndPort(String value, boolean portRequired) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based off Go implementation.

@@ -484,7 +535,7 @@ private static boolean validateEmail(String addr) {
try {
InternetAddress emailAddr = new InternetAddress(addr);
emailAddr.validate();
if (addr.contains("<")) {
if (addr.contains("<") || !emailAddr.getAddress().equals(addr)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream Go implementation got this fix for failing on whitespace in email addresses.

@@ -510,7 +561,9 @@ private static boolean validateHostname(String host) {
}
String s = Ascii.toLowerCase(host.endsWith(".") ? host.substring(0, host.length() - 1) : host);
Iterable<String> parts = Splitter.on('.').split(s);
boolean allDigits = false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream Go impl also picked up additional validation for hosts to ensure the last portion isn't all numbers.

@@ -226,16 +233,40 @@ private FieldEvaluator buildField(
FieldDescriptor fieldDescriptor, FieldConstraints fieldConstraints)
throws CompilationException {
ValueEvaluator valueEvaluatorEval = new ValueEvaluator();
boolean ignoreDefault =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates here are mostly equivalent to bufbuild/protovalidate-go@v0.5.2...2268789#diff-70b43cf25409483d9785a2bea15d9d185795803e255330e6562e7dca5112663e (there are some differences in the Java vs. Go generated code).

@@ -226,16 +232,40 @@ private FieldEvaluator buildField(
FieldDescriptor fieldDescriptor, FieldConstraints fieldConstraints)
throws CompilationException {
ValueEvaluator valueEvaluatorEval = new ValueEvaluator();
boolean ignoreDefault =
fieldDescriptor.hasPresence() && shouldIgnoreDefault(fieldConstraints);
Object zero = null;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use ? operator here to initialize

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't make this change - mainly a style thing and I think the ternary operator can make things harder to follow. The only use of the ternary operator so far in this project is in the protoc generated code.

Copy link

@drice-buf drice-buf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some small comments/nits

@pkwarren pkwarren requested a review from drice-buf March 5, 2024 18:01
@pkwarren pkwarren merged commit 9b2dfad into main Mar 5, 2024
4 checks passed
@pkwarren pkwarren deleted the pkw/isHostAndPort branch March 5, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants