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

fix: google.protobuf.Any type_url fixes #1068

Merged
merged 4 commits into from
May 29, 2020

Conversation

nickpestov
Copy link
Contributor

@nickpestov nickpestov commented Jun 8, 2018

Background

While using protobuf.js we've come across compatibility issues to do with google.protobuf.Any. Namely, when dealing with Any types, according to Google's documentation (referenced below), types.googleapis.com should be used as a default prefix within the type_url. The fromObject function referenced below failed to lookup types that had a prefix in their @type when converting from JSON to protobuf.

Addresses #850.

From Google's google.protobuf.Any docs

// The pack methods provided by protobuf library will by default use
// 'type.googleapis.com/full.type.name' as the type URL and the unpack
// methods only use the fully qualified type name after the last '/'
// in the type URL, for example "foo.bar.com/x/y.z" will yield type
// name "y.z".

And, regarding JSON

// The JSON representation of an `Any` value uses the regular
// representation of the deserialized, embedded message, with an
// additional field `@type` which contains the type URL. Example:
//
//     package google.profile;
//     message Person {
//       string first_name = 1;
//       string last_name = 2;
//     }
//
//     {
//       "@type": "type.googleapis.com/google.profile.Person",
//       "firstName": <string>,
//       "lastName": <string>
//     }
//
// If the embedded message type is well-known and has a custom JSON
// representation, that representation will be embedded adding a field
// `value` which holds the custom JSON in addition to the `@type`
// field. Example (for message [google.protobuf.Duration][]):
//
//     {
//       "@type": "type.googleapis.com/google.protobuf.Duration",
//       "value": "1.212s"
//     }
//

In this PR

  • In fromObject, only use type name after the last '/' when doing a lookup and make sure the type_url prefix is preserved. Otherwise, lookups that included type_url path prefixes (such as the default type.googleapis.com prefix) would fail.
  • In toObject, use type.googleapis.com default prefix if no type_url prefix is used.
  • Adjusted 2 tests to comply with the above and the below statement from Google's docs:
  // * The last segment of the URL's path must represent the fully
  //   qualified name of the type (as in `path/google.protobuf.Duration`).
  //   The name should be in a canonical form (e.g., leading "." is
  //   not accepted).

In `fromObject`, only use type name after the last '/' when doing a lookup and make sure the `type_url` prefix is preserved.
In `toObject`, use `type.googleapis.com` default prefix if no `type_url` prefix is used.
@kheyse-werk
Copy link

I would love to have this reviewed and merged. I'm currently dealing with the same issue. Thanks for the PR.

rclark added a commit to rclark/protobuf.js that referenced this pull request Feb 3, 2019
@alexander-fenster alexander-fenster changed the title google.protobuf.Any type_url fixes fix: google.protobuf.Any type_url fixes May 29, 2020
@alexander-fenster
Copy link
Contributor

@murgatroid99 I fixed linting problems, this PR is ready to be reviewed now.

@alexander-fenster alexander-fenster merged commit 192f5f1 into protobufjs:master May 29, 2020
taylorcode pushed a commit to taylorcode/protobuf.js that referenced this pull request Oct 16, 2020
* google.protobuf.Any fixes
In `fromObject`, only use type name after the last '/' when doing a lookup and make sure the `type_url` prefix is preserved.
In `toObject`, use `type.googleapis.com` default prefix if no `type_url` prefix is used.

* fix: lint

* fix: more lint

Co-authored-by: Michael Lumish <[email protected]>
Co-authored-by: Alexander Fenster <[email protected]>
This was referenced May 20, 2022
@github-actions github-actions bot mentioned this pull request Jul 8, 2022
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.

4 participants