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.

Sign up for GitHub

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

Jump to bottom

Protobuf Validation & Conversion #691

Merged
merged 6 commits into from
Jan 10, 2024
Merged

Protobuf Validation & Conversion #691

merged 6 commits into from
Jan 10, 2024

Conversation

ankitk-me
Copy link
Contributor

@ankitk-me ankitk-me commented Jan 2, 2024 •
edited by jfallows
Loading

Fixes #457
Fixes #458

@ankitk-me ankitk-me marked this pull request as draft January 2, 2024 18:14
@ankitk-me ankitk-me self-assigned this Jan 2, 2024
jfallows
jfallows requested changes Jan 5, 2024
View reviewed changes
Copy link
Contributor

@jfallows jfallows left a comment

Choose a reason for hiding this comment

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

Looks pretty good so far!

...tobuf.spec/src/main/scripts/io/aklivity/zilla/specs/validator/protobuf/config/validator.yaml Outdated
catalog0:
- subject: test0
version: latest
record: SimpleMessage
Copy link
Contributor

Choose a reason for hiding this comment

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

Does SimpleMessage not need to align with one of the message names, like example?
Also, perhaps it needs a package prefix too?

@ankitk-me ankitk-me marked this pull request as ready for review January 8, 2024 17:25
jfallows
jfallows requested changes Jan 9, 2024
View reviewed changes
...otobuf/src/main/java/io/aklivity/zilla/runtime/validator/protobuf/ProtobufReadValidator.java Outdated
Comment on lines 66 to 73
if (catalog.id != NO_SCHEMA_ID)
{
schemaId = catalog.id;
}
else
{
schemaId = handler.resolve(subject, catalog.version);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (catalog.id != NO_SCHEMA_ID)
{
schemaId = catalog.id;
}
else
{
schemaId = handler.resolve(subject, catalog.version);
}
schemaId = catalog.id != NO_SCHEMA_ID ? catalog.id : handler.resolve(subject, catalog.version);

...otobuf/src/main/java/io/aklivity/zilla/runtime/validator/protobuf/ProtobufReadValidator.java Outdated
DescriptorTree tree = supplyDescriptorTree(schemaId);
if (tree != null)
{
tree = tree.findByIndexes(indexes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps findByIndexes should return Descriptor directly?

...otobuf/src/main/java/io/aklivity/zilla/runtime/validator/protobuf/ProtobufReadValidator.java Outdated
Comment on lines 156 to 157
builder.mergeFrom(in);
DynamicMessage message = builder.build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
builder.mergeFrom(in);
DynamicMessage message = builder.build();
DynamicMessage message = builder.mergeFrom(in).build();

...otobuf/src/main/java/io/aklivity/zilla/runtime/validator/protobuf/ProtobufReadValidator.java
builder.mergeFrom(in);
DynamicMessage message = builder.build();
builder.clear();
if (!message.getUnknownFields().asMap().isEmpty())
Copy link
Contributor

Choose a reason for hiding this comment

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

This .asMap() still has allocation logic, but it may be the best we can do with these APIs.

...otobuf/src/main/java/io/aklivity/zilla/runtime/validator/protobuf/ProtobufReadValidator.java Outdated
Comment on lines 178 to 181
catch (IOException e)
{
e.printStackTrace();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
catch (IOException e)
{
e.printStackTrace();
}
catch (IOException ex)
{
ex.printStackTrace();
}

We typically use ex for exception variables.

...r-protobuf/src/main/java/io/aklivity/zilla/runtime/validator/protobuf/ProtobufValidator.java Outdated
Comment on lines 220 to 223
catch (InvalidProtocolBufferException e)
{
e.printStackTrace();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
catch (InvalidProtocolBufferException e)
{
e.printStackTrace();
}
catch (InvalidProtocolBufferException ex)
{
ex.printStackTrace();
}

...r-protobuf/src/main/java/io/aklivity/zilla/runtime/validator/protobuf/ProtobufValidator.java Outdated
in.wrap(buffer, index, length);
DynamicMessage message = DynamicMessage.parseFrom(descriptor, in);
status = message.getUnknownFields().asMap().isEmpty();
padding = 2 + JsonFormat.printer().print(descriptor.toProto()).length();
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should reuse this printer?
How often is this method called when messages are flowing?
Why 2 + ...? Suggest using a constant to better document the intent.

...tobuf/src/main/java/io/aklivity/zilla/runtime/validator/protobuf/ProtobufWriteValidator.java Outdated
Comment on lines 138 to 141
catch (IOException e)
{
e.printStackTrace();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
catch (IOException e)
{
e.printStackTrace();
}
catch (IOException ex)
{
ex.printStackTrace();
}

...tobuf/src/main/java/io/aklivity/zilla/runtime/validator/protobuf/ProtobufWriteValidator.java Outdated
int valLength = 0;
if (indexes.size() == 2 && indexes.get(0) == 1 && indexes.get(1) == 0)
{
indexesRO.wrap(new byte[]{ZERO_INDEX});
Copy link
Contributor

Choose a reason for hiding this comment

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

This byte[] should be a constant.

...tobuf/src/main/java/io/aklivity/zilla/runtime/validator/protobuf/ProtobufWriteValidator.java Outdated
Comment on lines 202 to 205
catch (IOException e)
{
e.printStackTrace();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
catch (IOException e)
{
e.printStackTrace();
}
catch (IOException ex)
{
ex.printStackTrace();
}

This was linked to issues Jan 10, 2024
Support outbound message transformation from protobuf to json #458
Closed
Support inbound message transformation from json to protobuf #457
Closed
@jfallows jfallows merged commit 47478cd into aklivity:feature/schema-registry Jan 10, 2024
3 checks passed
@ankitk-me ankitk-me deleted the protobuf branch January 25, 2024 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfallows jfallows jfallows approved these changes

Assignees

@ankitk-me ankitk-me

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@ankitk-me @jfallows