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.

Sign up for GitHub

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

Jump to bottom

Do something better for calling Proto.from with a proto instead of a JSON. Currently all unset fields get expanded #652

Closed
robin-anil opened this issue Jan 19, 2017 · 6 comments
Closed

Do something better for calling Proto.from with a proto instead of a JSON. Currently all unset fields get expanded #652

robin-anil opened this issue Jan 19, 2017 · 6 comments

Comments

@robin-anil
Copy link
Contributor

protobuf.js version: 6.5.2

This is causing failures for us on the server side as the has*() semantics are now broken.
@robin-anil robin-anil mentioned this issue Jan 19, 2017
Closed
@dcodeIO
Copy link
Member

dcodeIO commented Jan 19, 2017

Reference: https://github.com/dcodeIO/protobuf.js/blob/master/src/converter.js#L128

@robin-anil
Copy link
Contributor Author

trying to construct a test case, simple test with a proto with one field passes. So it must be something deeper

@robin-anil
Copy link
Contributor Author

robin-anil commented Jan 19, 2017 •
edited
Loading

Gah!!! Found it. Somewhere in our code we are passing protoObj into a MyProto.from(protoObj) instead of JSON. the from happily takes in the proto, expands all the null fields. 🤦‍♂️

Can you help identify such bad call sites and throw a console.warn or fatal error or something. Or maybe do something smarter.

I think V5 handled this correctly in the contructor. Now in V6, I mechanically changed the ctor to from(), didn't realize this folly.

@robin-anil robin-anil changed the title Unset string fields in JSON are being set as empty string when converting to proto using fromObject Do something better for calling Proto.from with a proto instead of a JSON. Currently all unset fields get expanded Jan 19, 2017
dcodeIO added a commit that referenced this issue Jan 19, 2017
@dcodeIO
Other: In fromObject, check if object is already a runtime message, see
799d030 
#652
@dcodeIO
Copy link
Member

dcodeIO commented Jan 19, 2017

Should now just return the runtime message. That sufficient? 👂

@robin-anil
Copy link
Contributor Author

haha! yes! that is good. A minor release would be great!

@dcodeIO
Copy link
Member

dcodeIO commented Jan 19, 2017

On npm as 6.5.3!

dcodeIO added a commit that referenced this issue Jan 19, 2017
@dcodeIO
Other: Added a test case for #652; Docs: Fixed centered formatting on…
6f0b44a 
… npm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dcodeIO @robin-anil