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

Improve support for dynamic messages #48

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

pkwarren
Copy link
Member

Update protovalidate-java to determine if the field, message, or oneof options contains an unknown field for the protovalidate extension. If so, reparse the options type to correctly interpret the options and enable validation.

This will enable protovalidate to run when the inputs are a FileDescriptorSet (with preserved options).

Update protovalidate-java to determine if the field, message, or oneof
options contains an unknown field for the protovalidate extension. If
so, reparse the options type to correctly interpret the options and
enable validation.

This will enable protovalidate to run when the inputs are a
FileDescriptorSet (with preserved options).
@pkwarren pkwarren requested review from rodaine and Alfus October 16, 2023 20:24
public class CompilationException extends ValidationException {
public CompilationException(String message) {
super(message);
}

public CompilationException(String message, Throwable cause) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added overridden constructor so if we have an Exception cause, we can preserve it for debugging.

throws InvalidProtocolBufferException, CompilationException {
DescriptorProtos.MessageOptions options = desc.getOptions();
// If the protovalidate message extension is unknown, reparse using extension registry.
if (options.getUnknownFields().hasField(ValidateProto.message.getNumber())) {
options = DescriptorProtos.MessageOptions.parseFrom(options.toByteString(), registry);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the key fix for handling dynamic messages created from a file descriptor set. If we determine that the protovalidate extension is in the unknown fields, we'll reparse it using our extension registry.

public class ValidatorDynamicMessageTest {

@Test
public void testFieldConstraintDynamicMessage() throws Exception {
Copy link
Member Author

Choose a reason for hiding this comment

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

All of these tests previously failed for field, message, and oneof constraints.

return Descriptors.FileDescriptor.buildFrom(fdProto, dependencies);
}

private static DynamicMessage.Builder createMessageWithUnknownOptions(Message message)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a round trip operation to go from a message created from Java generated code -> FileDescriptorSet -> DynamicMessage.Builder. The key thing is on line 126-127 where we reparse the FileDescriptorSet from binary format (losing any resolved extensions, etc.).

@pkwarren pkwarren merged commit 0f7172b into main Oct 16, 2023
3 checks passed
@pkwarren pkwarren deleted the pkw/better-support-dynamicmessage branch October 16, 2023 20:37
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