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

when a object is set for an array field in json, converting to proto does not throw an error #601

Closed
r-tock opened this Issue Dec 30, 2016 · 13 comments

Comments

Projects
None yet
2 participants
@r-tock
Contributor

r-tock commented Dec 30, 2016

Instead it seems like a bad bytestream is generated which throws error on the server side in Java protobuf.
It would be great to enforce type mismatch checks for all fields.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Dec 30, 2016

Type#verify and Message.verify are meant for this. Internally, encoders just look for a true-ish value with a non-zero .length property. This shouldn't encode to an invalid buffer, though.

@dcodeIO dcodeIO added the question label Dec 31, 2016

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 12, 2017

Even with verify, it is getting encoded to wrong byte stream.

message MyProto {
repeated MyArrayMessage msg = 1;
}

message MyArrayMessage {
optional string astring = 1;
}

MyProto.from({ msg: { astring: "foo" } }); succeeds.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 12, 2017

verify should return an error message.

What I'd expect in your example is that MyProto#MyArrayMessage is not present on the wire at all because { astring: "foo" } does not have a true-ish .length property. As I mentioned previously, this is because the check for repeated fields is if (value && value.length) (see).

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 12, 2017

Hmm, yes, the behavior has changed now at Head from when I originally tested it. Yes, it's silently dropping it from the wire.

Can you throw an error instead in the .from method. verify is not doing anything. Will that be a performance degradation?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 12, 2017

verify doesn't throw but return the error message as a string. You could throw that as an error yourself if you wish.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 12, 2017

verify is returning null for me.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 12, 2017

I presume the value is ignored in the proto at the end of from call. So verify is not doing anything.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 12, 2017

It really should return something if the value does not fulfill Array.isArray.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 12, 2017

Ah, yes, from makes the same assumption, only copying values if (value && value.length) as it needs to convert each value accordingly. Hmm.

What exactly are you using all these conversions for? Just curious, because ideally sticking to create and friends would be the performance-savvy way.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 12, 2017

yeah, would be great if somehow parse errors for from was propagated. It will bring a whole lot of sanity, robustness and maintainability to the code. I am even fine with making this strict errors.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 12, 2017

Just saw your edited question

We have a react-redux app, most of the application state is managed in JSON (because of ease of serializability into local-storage and back). We have built an AJAX middleware which deals with proto byte-stream request/response to the server. Before we do an AJAX request, we convert JSON to proto. This is literally everywhere! So when we convert from JSON to proto, if there are mistakes, we would love to have errors thrown at the conversion stage so that we don't send anything invalid value to the server (even silently)

For new code we are using create() but most of the messages are pretty small so dealing with them in JSON and then converting that to proto before calling the middleware is a common pattern.

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 12, 2017

I see, hmm. Well, currently converters really just convert any values as you can also see here for example. Hence, no matter what is set, it is converted through Number, String, Boolean etc. Adding assertions everywhere would probably be too much, considering that numbers for example could be provided as strings intentionally (i.e. because stored as object keys or something) or by mistake, which cannot be reliably distinguished.

One possibility, though, that doesn't conflict with this would be to change how non-arrays are converted to arrays. Let's say you specify an object for a repeated field, then converters could wrap this object in a new array instead. Would that be helpful? - Or, of course, to throw errors just for obviously wrong values (non-arrays for repeated fields and maybe non-objects for inner messages).

Another way to deal with your issue could be utilizing a JSON schema validator before converting, but that'd be another library. It should be possible to generate such a schema from reflection information, though.

@r-tock

This comment has been minimized.

Contributor

r-tock commented Jan 12, 2017

@dcodeIO yes, that was the v5 behavior, which a part of our code unknowingly used and got it working v5 that broke in v6 due to which I found this issue. Every cell in my body is screaming that this is silent conversion is a bad thing, but from a compatibility standpoint it is probably ok to do. I am fine with either throwing errors for non-repeated on a repeated or silently converting them. I prefer the former even if it means more work on my part to clean up our code-base

@dcodeIO dcodeIO closed this in dca26ba Jan 20, 2017

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