Open dsyer opened 1 year ago
+1
I'm using oneof
in my code. Decoding works great, but encoding is problematic as it creates a default value for each field sets them all.
I have a protobuf that looks like this:
message FlagValue {
oneof value {
bool booleanValue = 1;
double doubleValue = 2;
string stringValue = 3;
}
}
and the generated constructor sets default values for each field (rather than nulls):
constructor(
booleanValue: bool = false,
doubleValue: f64 = 0.0,
stringValue: string = ""
) {
this.booleanValue = booleanValue;
this.doubleValue = doubleValue;
this.stringValue = stringValue;
}
And the encoder sets all three fields.
static encode(message: FlagValue, writer: Writer): void {
writer.uint32(8);
writer.bool(message.booleanValue);
writer.uint32(17);
writer.double(message.doubleValue);
writer.uint32(26);
writer.string(message.stringValue);
}
I think a good solution would be defaulting the fields to null and modifying the encode function to only set non-null fields.
Unfortunately, there is no concept of null
for primitive types, like int32
, in AssemblyScript, so we have to default to values like 0
. I agree that support for oneOf
field is currently broken, it's not trivial to implement and I don't know when I will have time to do that. Feel free to contribute to the project :)
I realize this is a bigger issue than just oneOf
. I can't tell if an optional field is set or not.
I think it's ok for the fields to be set to a default value, but there needs to be a way to tell if the field is set. This could also be used to not write the unset field to the wire protocol.
Since you can't null a primitive type, I'd suggest keeping a setting boolean value isSet
that defaults to false
.
You might need to get rid of the constructor to make that possible. I think it's worthwhile though as settings default values like 0
on unset fields is basically an incorrect implementation in my opinion. Not being able to differentiate being an unset optional
field and a set 0
value is a problem.
Perhaps this could be done with get
, has
, and set
functions. I believe the Java generated classes are similar to this. If a field is unset, calling get
still returns the default value, but calling has
returns false.
The generated class could look something like:
value: int32 = 0;
valueIsSet: boolean = false;
setValue(value: int32): void {
this.value = value;
this.valueIsSet = true;
}
hasValue(): boolean {
return this.valueIsSet;
}
getValue(): int32 {
return this.value;
}
static encode(message: Message, writer: Writer): void {
if(message.valueIsSet) {
writer.uint32(8);
writer.bool(message.value);
}
}
@piotr-oles What do you think about this optional
problem and this solution?
Fixing oneOf
seems like an additional and much more minor issue.
Yes, I agree it's a bigger issue. I'm wondering if we should use boolean
fields, or maybe something more compact, like bit masks (AFAIK boolean will be stored as i32). I think C++ implementation use masks for that. It would have to be a major release because of API changes.
class Message {
value: int32 = 0;
private hasMask1: i32 = 0;
setValue(value: int32): void {
this.value = value;
this.hasMask1 &= 0x1;
}
hasValue(): boolean {
return this.hasMask1 & 0x1 !== 0;
}
getValue(): int32 {
return this.value;
}
static encode(message: Message, writer: Writer): void {
if (message.hasValue()) {
writer.uint32(8);
writer.i32(message.value);
}
}
}
That makes sense. value
can be private too in order to prevent sets without setting the mask.
I'm actually surprised I haven't run into this being a blocker for me yet. I don't have time/motivation to take this on myself (at least for now), but we should probably spin out a separate issue for this.
I'm about to stop using oneOf
because it breaks decoding in Java.
My proto created in AssemblyScript:
message FlagValue {
oneof value {
bool booleanValue = 1;
double doubleValue = 2;
string stringValue = 3;
}
}
The decoding code in Java:
while(!done) {
int tag = input.readTag();
switch (tag) {
case 0:
done = true;
break;
case 8:
this.value_ = input.readBool();
this.valueCase_ = 1;
break;
case 17:
this.value_ = input.readDouble();
this.valueCase_ = 2;
break;
case 26:
String s = input.readStringRequireUtf8();
this.valueCase_ = 3;
this.value_ = s;
break;
default:
if (!super.parseUnknownField(input, extensionRegistry, tag)) {
done = true;
}
}
}
Looks like the Java decoder assumes only one of the fields in a oneOf
is set. What happens is that value
is set to booleanValue
then overwritten with doubleValue
and then overwritten with stringValue
.
So if I have a boolean value set, then value
ends up being ""
from the empty string value.
It's kind of a problem with AssemblyScript, not
as-proto
per se, that it doesn't support union types or undefined, but protobufs often contain "oneof" fields andas-proto
currently doesn't do a great job of transpiling them.Given this:
as-proto
will generate this:The "payload" field isn't even referenced, and the constructor forces the
Simple
type to have all of the "oneof" properties (not one of them).If you try it with
ts-proto
instead it can generate a union type, e.g.but that won't work in AssemblyScript. It would need to be something like this, which I think is what is meant by the very cursory comment on union types in the language guide (https://www.assemblyscript.org/status.html#language-features):