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 helper functions #21

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

btkostner
Copy link

@btkostner btkostner commented Jun 14, 2023

I know that helper functions have been discussed before (#8 and #15 (comment)) and rejected because this repository should only keep generated code. I respect that there is a well defined line of what this package handles, but I'd strongly urge to move that line slightly to the right and add helper functions to make these well known Protobuf types more usable in Elixir.

Protobuf handles a lot of the hard work for converting data between languages, but most language implementations have some code to patch over the bad stuff. Take the google.protobuf.Struct message for example. A simple JSON blob like so:

{
  "key_two": [1, 2, 3, null, true, "value"],
  "key_three": {
    "key_four": "value_four",
    "key_five": {
      "key_six": 99,
      "key_seven": {
        "key_eight": "value_eight"
      }
    }
  }
}

will output this Elixir code (removed some lines for readability):

%Google.Protobuf.Struct{
  fields: %{
    "key_three" => %Google.Protobuf.Value{
      kind: {:struct_value,
       %Google.Protobuf.Struct{
         fields: %{
           "key_five" => %Google.Protobuf.Value{
             kind: {:struct_value,
              %Google.Protobuf.Struct{
                fields: %{
                  "key_seven" => %Google.Protobuf.Value{
                    kind: {:struct_value,
                     %Google.Protobuf.Struct{
                       fields: %{
                         "key_eight" => %Google.Protobuf.Value{
                           kind: {:string_value, "value_eight"}
                         }
                       }
                     }}
                  },
                  "key_six" => %Google.Protobuf.Value{
                    kind: {:number_value, 99.0}
                  }
                }
              }}
           },
           "key_four" => %Google.Protobuf.Value{
             kind: {:string_value, "value_four"}
           }
         }
       }}
    },
    "key_two" => %Google.Protobuf.Value{
      kind: {:list_value,
       %Google.Protobuf.ListValue{
         values: [
           %Google.Protobuf.Value{
             kind: {:number_value, 1.0}
           },
           %Google.Protobuf.Value{
             kind: {:number_value, 2.0}
           },
           %Google.Protobuf.Value{
             kind: {:number_value, 3.0}
           },
           %Google.Protobuf.Value{
             kind: {:null_value, :NULL_VALUE}
           },
           %Google.Protobuf.Value{
             kind: {:bool_value, true}
           },
           %Google.Protobuf.Value{
             kind: {:string_value, "value"}
           }
         ]
       }}
    }
  }
}

No sane person would want to work with that in their Elixir code. Here is a list of other languages and how they handle this:

Go

Implements functions like AsMap, AsTime, and AsDuration to convert Protobuf messages to standard Go code.

Java

Implements fromDate and an absurd amount of other functions for handling well known types.

Python

Implements ToDatetime, FromDatetime, ToTimedelta, FromTimedelta and magic functions for Struct to work like a dictionary.

Ruby

Has to and from methods defined.

Rust

Implements into and from Timestamp, into and from Duration, as well as Rust just being able to implement well known struct transforms in user code.

Typescript

The JavaScript world is kinda a mess, so naturally there are 10+ different libraries for Protobuf handling.

Library vs User Code

You could argue that this code would be better lived in each application instead of this library, though I'd suspect most use cases for this library would use the helper functions. If that's the case, it would make more sense to include this code so it can be better documented and tested.

There is an alternative way to make this easier (and something I experimented on earlier) with Transformer modules to loop over all message values and rewrite any Google.Protobuf.Struct and Google.Protobuf.Timestamp values to map() and DateTime.t(), though that comes with a ton of other problems like not being able to see the original data, incorrect types, and just generally being hacky. Ideally you'd be able to encode a DateTime in place of Google.Protobuf.Timestamp transparently, but the Elixir Protobuf library does not have support for this yet.

Implementation

For this PR, I've decided to create new files for duration, struct, and timestamp. The generated files (duration.pb.ex) have been added to the .gitignore to prevent them from being overwritten, while still making it easy to compare the generated output to what was custom edited. I've taken the liberty to update ex_doc and include documentation for those modules so they show up, as well as add more details to the README.md file and add it to the generated documentation. Since none of the encoding or decoding was touched, this is a non breaking change.

You'll notice that I did not use doc tests here. I would have preferred to use them, but compile logic with Protobuf causes errors with the module structs not being defined in time. I've instead written the tests in separate files. I can also add more test cases if requested.

My current employer as well as previous both worked with Elixir and Protobuf. If maintaining is an issue, I'd be happy to help / take over.

@btkostner btkostner changed the title Usability Add helper functions Jun 14, 2023
@ericmj
Copy link
Collaborator

ericmj commented Jun 16, 2023

What should the workflow be to maintain that the manually created modules match the code from the automatically generated modules?

If we do add helper functions to this library I think we need a method to automatically add custom code to generated modules or the helper functions needs to live in a separate modules so they don't interfere with generated code.

@btkostner
Copy link
Author

btkostner commented Jun 16, 2023

Fair point. I’d love if the protobuf library could add custom code, but that’s going to be a while, so I’ll move the functions to a separate module. Is “Google.Protobuf.to_map” and “Google.Protobuf.from_time_unit” acceptable?

Update: It might actually be easy to update the generation script to add the functions 🤔 let me try prototyping something out.

@btkostner
Copy link
Author

Alright, the generate_google_protos.sh script now includes some dark magic to cut and paste code from the priv/templates directory into the generated files. I've confirmed it's working on my Linux and macOS install.

Off the top of my head, two things stand out:

  • Because we are not modifying the generated documentation, the modules still have @moduledoc false and won't generate documentation about these helper functions. Hopefully Add protobuf code comments as Elixir module documentation protobuf#352 fixes this and we get some better docs.
  • I had to use struct/2 due to the ordering of generated modules and undefined structs.

@whatyouhide
Copy link
Contributor

Personally, I don't think this is the way to go.

I have thought about it a lot, and I think the Google stuff belongs in the core library. The core library already supports Google types in its JSON encoding/decoding, so it's already aware of those. We could bake the helpers into the main Protobuf library directly, since these types are supported officially.

I don't personally have time to spend on this at all, so I’m just throwing this out there here 😬

@btkostner
Copy link
Author

I should be able to throw up a PR for that soon then. I’ll split it up into two PRs.

  1. Moving the google protobufs into the protobuf library and adding docs about the deprecation of the google_protobuf package.

  2. Adding helper functions for the well know types (this PR)

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.

3 participants