"required" spec ignored by Marshal, Unmarshal

[gogo]

"required" spec ignored by Marshal, Unmarshal

[awalterschulze]

From awalterschulze on August 28, 2014 01:45:12

When marshaling and unmarshaling code is not generated an error is returned on marshaling and unmarshaling when a required field is not set.
proto: required field "Field1" not set
This will need to fixed in plugin/marshalto and plugin/unmarshal.
This seems to be already implemented in plugin/populate.

I proposed a test adding the following struct to thetest.proto:

message RequiredNinOptNative {
required double Field1 = 1;
required float Field2 = 2;
required int32 Field3 = 3;
required int64 Field4 = 4;
required uint32 Field5 = 5;
required uint64 Field6 = 6;
required sint32 Field7 = 7;
required sint64 Field8 = 8;
required fixed32 Field9 = 9;
required sfixed32 Field10 = 10;
required fixed64 Field11 = 11;
required sfixed64 Field12 = 12;
required bool Field13 = 13;
required string Field14 = 14;
required bytes Field15 = 15;
}

message RequiredNinOptStruct {
required double Field1 = 1;
required float Field2 = 2;
required NidOptNative Field3 = 3;
required RequiredNinOptNative Field4 = 4;
required uint64 Field6 = 6;
required sint32 Field7 = 7;
required NidOptNative Field8 = 8;
required bool Field13 = 13;
required string Field14 = 14;
required bytes Field15 = 15;
}

Then use NewPopulatedNinOptStruct, which will be equivalent to RequiredNinOptStruct except that some values will not be filled in, to create a marshal and unmarshal test that expects an error.

The tests should pass.

Then add the marshaler and unmarshaler extension to the two Required messages defined above.

The tests should then fail.

Then start implementing the fix in plugin/marshal and plugin/unmarshal.

How does this sound to you?

field.IsRequired() is a helper method which should prove useful.

[awalterschulze]

From awalterschulze on August 28, 2014 04:56:09

Status: Accepted

[anton-povarov]

Hello. Any news on this issue ?

[awalterschulze]

I still have not had any time to work on this.
If you would like to tackle this it would be great, otherwise I will get to it eventually.

[anton-povarov]

So i've hacked together this tentative implementation over the last few hours, tested it by hand on messages with 1 to 65 required fiends and various field id combos.
If you like it - i can go ahead and implement it properly (with Marshal() and tests).

please take a look at https://github.com/anton-povarov/protobuf/compare/unmarshaller_required_fields.

[anton-povarov]

And of course - none of this would ever work with JSON + nullable = false.
got some ideas on that, will post a bit later after i've tried them all, if you're interested.

[awalterschulze]

Please move the NewRequiredNotSetError to encode_gogo.go
I try to keep the gogo/protobuf code and golang/protobuf code as seperate as possible.
This makes merging in any new diffs and keeping the copyright correct a little easier.

[awalterschulze]

I like the how you check for the max required field and that you only generate the hasFields variable for the cases where there are required fields.

[awalterschulze]

I think this might work :)
I think the next step is adding some tests.

[awalterschulze]

The only thing I am a little afraid of is merging protobufs.
This would be a little bit more of a specialised test.
I don't even know how the goprotobuf code would handle this.
The goal would be to handle it the same way.

message A {
optional B B = 1;
}

message B {
optional string c = 2;
required string d = 3;
}

Protobufs allow you to merge two messages.

firstA := &A{B: &B{C: "cc"}}
secondA := &A{B: &B{D: "dd"}}
firstBuf, _ := proto.Marshal(firstA)
secondBuf, _ := proto.Marshal(secondA)
buf := append(firstBuf, secondBuf...)

got := &A{}
err := proto.Unmarshal(buf, got)
if err != nil {
//I don't know what the defined behaviour is in this case
//is this a required error
}

if err == nil {
expected := &A{B: &B{C: "cc", D: "dd"}}
if !expected.Equal(got) {
//These should be equal
}
}

I guess I could just write a test now and check.

[awalterschulze]

I don't think goprotobuf handles JSON so I don't think we have to worry about that. I think this is one of those cases where we prefer consistency over correctness.

[awalterschulze]

I am very interested in what other things you have tried :)

[awalterschulze]

Ok great :)
I've tested it, it seems atleast the goprotobuf implementation is quite naive.

The code below gives the following error:
proto: required field "Inner.{Unknown}" not set

This means that each message is handled separately and merging is not taken into account when checking for required fields.
This means you can continue with your implementation and ignore any merging complications :)

---

message OuterRequired {
message InnerRequired {
optional string OptionalFieldC = 1;
required string RequiredFieldD = 2;
}
optional InnerRequired Inner = 1;
}

message OuterOptional {
message InnerOptional {
optional string OptionalFieldC = 1;
optional string OptionalFieldD = 2;
}
optional InnerOptional Inner = 1;
}

package test

import (
"github.com/gogo/protobuf/proto"
"testing"
)

func TestRequiredMerge(t *testing.T) {
first := &OuterOptional{
Inner: &OuterOptional_InnerOptional{
OptionalFieldC: proto.String("cc"),
},
}
second := &OuterOptional{
Inner: &OuterOptional_InnerOptional{
OptionalFieldD: proto.String("dd"),
},
}
firstBuf, err := proto.Marshal(first)
if err != nil {
t.Fatal(err)
}
secondBuf, err := proto.Marshal(second)
if err != nil {
t.Fatal(err)
}
buf := append(firstBuf, secondBuf...)
m := &OuterRequired{}
if err := proto.Unmarshal(buf, m); err != nil {
t.Fatal(err)
}
}