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

Add Protobuf.Text #398

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add Protobuf.Text #398

wants to merge 5 commits into from

Conversation

v0idpwn
Copy link
Collaborator

@v0idpwn v0idpwn commented Jan 15, 2025

This PR introduces support for encoding Protobuf messages into the textproto format.

Motivation

The primary motivation for this feature is to improve the ability to inspect and visualize Protobuf messages, while still conforming to some standard. The inspect protocol default implementation can be painful to work with large or complex structures. Finding the relevant, non-default fields in a inspected struct is not intuitive and takes an unnecessary amount of time. A textproto representation provides a human-readable alternative that is much closer to the actual data transmitted over the wire (specially when using transformers).

This implementation

  • Encoding only: Decoding was deliberately left out as it is less valuable compared to encoding, and has many caveats.
  • Emphasis on readability: pretty-printed by default. We do a best effort for breaking lines, and opted for the most readable and elixir-like alternatives in the spec. Max line width and tab size are is configurable.
  • Limitations:
    • No support for extensions
    • No support for Google.Protobuf.Any.
    • No type validation is performed.

Future Considerations

I'm open to extending this implementation in the future to include:

  1. Adding support for extensions
  2. Adding support for Google.Protobuf.Any.
  3. Implementing type validation.

@v0idpwn v0idpwn requested a review from whatyouhide January 16, 2025 20:07
Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

Should we look at using Inspect.Algebra to simplify the line splitting? Not sure what the spec of the format is and I don't have the bandwidth to go read it now, but it seems flexible enough that we could break lines in a few different places?

lib/protobuf/text.ex Outdated Show resolved Hide resolved
lib/protobuf/text.ex Outdated Show resolved Hide resolved
lib/protobuf/text.ex Outdated Show resolved Hide resolved
lib/protobuf/text.ex Outdated Show resolved Hide resolved
lib/protobuf/text.ex Outdated Show resolved Hide resolved
@v0idpwn
Copy link
Collaborator Author

v0idpwn commented Jan 22, 2025

Thank you, @whatyouhide. I'll explore using Inspect.Algebra, it's probably a good fit here. :)

Makes the code a lot simpler.
@v0idpwn v0idpwn force-pushed the feat/text-encoding branch from dcc90cb to 6c73550 Compare January 23, 2025 03:21
@v0idpwn
Copy link
Collaborator Author

v0idpwn commented Jan 23, 2025

Pushed a new version using Inspect.Algebra. It indeed simplified code a lot, thanks for the suggestion.

Some limitations/changes:

  • I wasn't able to have different separators on container_doc for single line and multi-line. I'm sure it's achievable if I use the lower level functions, but I think having a , after every line isn't that bad (it's optional in the spec), and I like the simplicity of container_doc.
  • I wasn't able to define :pad_size as I was in the hand-crafted implementation, so it's a fixed 2.

I'll address the other comments over the week :)

@whatyouhide
Copy link
Collaborator

@v0idpwn OK sounds good, lmk when this is gonna be ready for re-review. 🙃

Comment on lines +144 to +150
# This is actually new. Should it be ported to Protobuf.Encoder?
defp skip_field?(:proto3, value, %FieldProps{type: {:enum, enum_mod}, oneof: nil}) do
enum_props = enum_mod.__message_props__()
[first_tag | _] = enum_props.ordered_tags
%{name_atom: name_atom, fnum: fnum} = enum_props.field_props[first_tag]
value == name_atom or value == fnum
end
Copy link
Collaborator Author

@v0idpwn v0idpwn Feb 1, 2025

Choose a reason for hiding this comment

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

I tried this on Protobuf.Encoder and it broke conformance tests, despite the fact that in the docs it seems like this is the expected behaviour there as well (just as we skip 0 for non-optional integers, I'd expect it to skip the default enum value...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll investigate this further

@v0idpwn
Copy link
Collaborator Author

v0idpwn commented Feb 1, 2025

I think it's okay to be reviewed, @whatyouhide :)

@v0idpwn
Copy link
Collaborator Author

v0idpwn commented Feb 3, 2025

Actually, I found that there are conformance tests for text encoding. I'll work on them.

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