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

feat: Support for Google.Protobuf.Value, ListValue and Struct #396

Merged
merged 8 commits into from
Nov 22, 2021

Conversation

boukeversteegh
Copy link
Collaborator

@boukeversteegh boukeversteegh commented Nov 20, 2021

Summary

Dear friends of TsProto,

I've worked the last two days to built an implementation to support some important remaining well known types.
Tests are included that demonstrate it is working.

Benefits

If you are using ts-proto with another server or client that supports Google's Well Known Types, it may send you lists such as [1, true, "hello"], using the ListValue type. Currently, ts-proto will generate a complex object structure for such a type, and will fail while trying to extract the data from the simplified list.

With this PR, you can now read the implicitly converted JSON objects sent by your server, or client with WKT support, and write them back as well, both in binary or JSON.

Added Well Known Values:

  • Value — translates to any in Typescript.

    Referred to as AnyValue in the code, to avoid the confusing term ValueValue, which would be more consistent). However, this may also be confused with the WellKnownValue called 'Any'. Solutions welcome.

  • ListValue — translates to any[] in Typescript.
  • Struct — translates to {[key: string]: any} in Typescript.

Usable areas:

  • as non-repeated simple properties
  • repeated
  • map<_, WellKnownValue>

    This was required because Struct relies on this.

Features:

  • Within typescript, you work with the normalized objects, so a list is just a list.
  • Only when encoding/decoding to binary, the structure is re-converted to/from the Protobuf well-known type.

    This behavior is the same for other currently implemented types, such as BoolValue and StringValue

  • The fromJSON and toJSON don't do any real conversion, because the Typescript data format is essentially already in JSON-compatible format

Usage example:

message ValueMessage {
  google.protobuf.Value value = 1;
  google.protobuf.ListValue anyList = 2;
  repeated google.protobuf.Value repeatedAny = 3;
}
var message = ValueMessage.fromJSON({
    "value": "Hello",
    "anyList": [1, "foo", true],
    "repeatedAny": [2, "bar", false],
});

message.value === "Hello"
message.anyList = [1, "foo", true]
message.repeatedAny: [2, "bar", false]

// will binary encode into correct WellKnownValue objects (Value, ListValue)
ValueMessage.encode(message);
message StructMessage {
  google.protobuf.Struct value = 1;
}

Not included:

  • map<WellKnownValue, _> is not implemented in this PR.
  • A way to disable the normalization of these well-known-values. Currently ts-proto will normalize other WKV's, but not the complex ones, so the option should either toggle all normalization, or just the complex ones. If you think it's essential to have a toggle, a proposed parameter name would be welcome.
  • Normalization of non-normalized JSON objects. So, the implementation expects you receive your data either binary or as normalized JSON. If your server does not normalize (e.g., sends values as {"stringValue": "hello world"} instead of "hello world", you would actually get the entire object where you would have expected a string.

Fixes #336

@boukeversteegh boukeversteegh changed the title Support for Google.Protobuf.Value, ListValue and Struct feat: Support for Google.Protobuf.Value, ListValue and Struct Nov 20, 2021
Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

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

@boukeversteegh this is great!

I think the CI build will fail b/c you didn't include the generated code for the struct test in the PR (it's a little odd, but checking in the generated code is a nice way to see in the diffs "did I unintentionally change the codegen output?").

And maybe throw in an update to the readme somewhere to advertise the well-known types we support, but otherwise looks great! Thanks!

@boukeversteegh
Copy link
Collaborator Author

boukeversteegh commented Nov 21, 2021

:-) Alright! Should I also include the .bins, or just the .ts files? The bin files are all changed... I ran update-bins on windows, using

$ protoc --version
libprotoc 3.14.0

edit: obviously i will include the bin files for the new tests

@stephenh
Copy link
Owner

@boukeversteegh yeah, just the new bins would be great. The whole .bins approach has been flakier than I'd like, need to eventually use like docker-something-something so that we don't rely on each individual users version/platform of protoc to create the files.

@boukeversteegh
Copy link
Collaborator Author

@stephenh ok, no problem. I saw the discussion in another thread.

Then does this mean that the proto -> binary conversion is unstable? I was thinking to use hashes of protobuf files one day for consistency checking, but now it seems like a bad idea 😕

I am also interested in dockerizing protoc for my own project. I'll share my solution once I have it.

@stephenh
Copy link
Owner

stephenh commented Nov 21, 2021

Then does this mean that the proto -> binary conversion is unstable?

It is stable for a given platform + version of protoc. I.e. if I continually run update-bins.sh on a single machine, the output is only deterministic.

And, really, even the version changes are "real" differences, i.e. new versions of protoc will bring in new versions of the various google/protobuf/* included *.proto files, and they'll have new/updated comments (that is almost always the change). But since comments are included into the CodeGeneratorRequest message sent to plugins, they are captured in the .bin files, which is what we want b/c we also test that we echo comments into the source code.

So, that's really it, I think just slightly different versions of comments across protoc versions.

If you hash .proto files themselves, you should be good, again maybe modulo the wrinkle about comment changes will show up as hash changes. If you wanted to avoid those, you could strip comments from the CodeGeneratorRequest message that's sent to the plugin, and then hash the version of that. That way you'd only have structural changes bump your hash.

I am also interested in dockerizing protoc for my own project. I'll share my solution once I have it.

Thanks! That'd be great.

@boukeversteegh
Copy link
Collaborator Author

Then does this mean that the proto -> binary conversion is unstable?

It is stable for a given platform + version of protoc. I.e. if I continually run update-bins.sh on a single machine, the output is only deterministic.

Ah, I see. Then this is more about the stability of the ProtoMessageDescriptors, and not about how proto messages themselves are encoded in binary. It makes sense that this would change from time to time yes, but between platforms is a bit surprising, considering they should be the same implementation.

Just fixed the remaining build errors. I will update the readme in a bit.

@boukeversteegh
Copy link
Collaborator Author

Hi there! I think I'm done for now. I still see possible improvements but I will keep them for another PR (moving the generated unwrapping-util functions to inside Value, ListValue, and Struct, so that they are not generated in multiple files)

LGTM?

@stephenh
Copy link
Owner

@boukeversteegh yep, lgtm, the readme updates look amazing, thank you!

@boukeversteegh boukeversteegh merged commit 7dd9c16 into stephenh:main Nov 22, 2021
stephenh pushed a commit that referenced this pull request Nov 22, 2021
# [1.88.0](v1.87.1...v1.88.0) (2021-11-22)

### Features

* Support for Google.Protobuf.Value, ListValue and Struct ([#396](#396)) ([7dd9c16](7dd9c16))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.88.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@boukeversteegh
Copy link
Collaborator Author

Awesome, thank you for the swift action!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fromJSON doesn't work for Well-Known Types.
2 participants