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

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

Closed
r-tock opened this Issue Jan 19, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@r-tock
Contributor

r-tock commented Jan 19, 2017

protobuf.js version: 6.5.2

This is causing failures for us on the server side as the has*() semantics are now broken.

@dcodeIO

This comment has been minimized.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 19, 2017

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

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 19, 2017

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.

@r-tock r-tock changed the title from Unset string fields in JSON are being set as empty string when converting to proto using fromObject to 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

This comment has been minimized.

Owner

dcodeIO commented Jan 19, 2017

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

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 19, 2017

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

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 19, 2017

On npm as 6.5.3!

@r-tock r-tock closed this Jan 19, 2017

dcodeIO added a commit that referenced this issue Jan 19, 2017

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