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

[robin-anil]

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]

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.

[robin-anil]

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]

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).

[robin-anil]

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]

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

[robin-anil]

verify is returning null for me.

[robin-anil]

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

[dcodeIO]

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

[dcodeIO]

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.

[robin-anil]

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.

[robin-anil]

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]

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.

[robin-anil]

@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