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: add getTypeUrl method to generated code #1463

Merged
merged 4 commits into from
May 21, 2021

Conversation

taylorcode
Copy link
Collaborator

An encoded proto containing a nested Any that is missing a typeUrl cannot be decoded in Python, and perhaps other languages.

js

const anyProto = google.protobuf.Any.create({
   // typeUrl: none provided
   value: arrayBuffer,
});

python

child_message = ChildMessage()
any_msg.Unpack(child_message) # fails because of missing type url on any_msg

It looks like in other languages, e.g. Java a nested Any message has a getTypeUrl method. It would be nice if protobufjs's Any also provided a similar method or property that would return the default type url.

Any childMessage = parentMessage.getChildMessage();
String typeUrl = childMessage.getTypeUrl()

It seems like in order to add this to Any, we would need messages to know their default typeUrl. In other languages this can be determined through reflection, but for JS this doesn't work reliable for at least two reasons. First, we'd need a way to determine the constructor name. instance.constructor.name gives us this, but it does not work in IE, and the workarounds seem wonky. Second, minifiers can change the name of constructors. In practice this usually shouldn't be a problem because protobufjs messages are properties on an object and not local variables (and therefore not mangle-able), but there are minifiers that can mangle properties, such as closure. Therefore I think the default type url needs to be represented as a string.

@alexander-fenster
Copy link
Contributor

@taylorcode Thank you! We can merge it but I can't bring this PR up to date. Can you please press the Update button for me? Thank you!

@alexander-fenster alexander-fenster changed the title Add getTypeUrl method to generated code feat: add getTypeUrl method to generated code Jul 31, 2020
@alexander-fenster
Copy link
Contributor

@taylorcode Also, adding a test would've been appreciated - just to make sure this feature does not get broken by a random change in the future.

@taylorcode
Copy link
Collaborator Author

taylorcode commented Oct 16, 2020

@alexander-fenster I've added a test but I'm not confident I did it correctly. I ran node scripts/gentests.js to regenerate the fixtures, but I don't see any existing tests on the generated code.

I noticed the test files within the tests directory follow a certain naming convention, test files on the protobuf library itself are prefixed with api_, tests on the proto parser are prefixed with comp_, docs with docs_, and some other tests with other_. I prefixed my test file with gen_.

@alexander-fenster
Copy link
Contributor

@taylorcode Mind giving the reviewers a permission to update your branch? Thank you!

@taylorcode
Copy link
Collaborator Author

taylorcode commented Mar 3, 2021

@alexander-fenster sorry, I'm not sure how to grant write access. Could you provide guidance? thanks

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

Successfully merging this pull request may close these issues.

2 participants