jsonpb Marshal and Unmarshal should return error on missing required field

[mkevac]

If I have a message in *.proto file with required field and I try to convert json representation of this message to message itself, I would expect my Unmarshaller to check for required fields and to give an error if required fields are not in json representation. But jsonpb does not check that.

package main

import (
	gsp "lala/lala/proto"
	"log"
	"github.com/golang/protobuf/jsonpb"
)

func main() {
	jsonMsg := "{\"age_from\": 18}"
	msg := gsp.RequestSearch{}
	if err := jsonpb.UnmarshalString(jsonMsg, &msg); err != nil {
		log.Printf("error unmarshalling json: %s", err)
	}
	log.Println(msg)
}

type RequestSearch struct {
	UserId                *uint32     `protobuf:"varint,1,req,name=user_id" json:"user_id,omitempty"`
	AgeFrom               *uint32     `protobuf:"varint,2,opt,name=age_from" json:"age_from,omitempty"`
	AgeTo                 *uint32     `protobuf:"varint,3,opt,name=age_to" json:"age_to,omitempty"`
	Gender                *UserGender `protobuf:"varint,4,opt,name=gender,enum=badoo.gophersearch.UserGender" json:"gender,omitempty"`
	Lon                   *float32    `protobuf:"fixed32,5,opt,name=lon" json:"lon,omitempty"`
	Lat                   *float32    `protobuf:"fixed32,6,opt,name=lat" json:"lat,omitempty"`
	ExcludeList           []uint32    `protobuf:"fixed32,7,rep,name=exclude_list" json:"exclude_list,omitempty"`
	MainVectorNeeded      *uint32     `protobuf:"varint,8,opt,name=main_vector_needed" json:"main_vector_needed,omitempty"`
	BeautifulVectorNeeded *uint32     `protobuf:"varint,9,opt,name=beautiful_vector_needed" json:"beautiful_vector_needed,omitempty"`
	SearchDistance        *uint32     `protobuf:"varint,10,opt,name=search_distance" json:"search_distance,omitempty"`
	XXX_unrecognized      []byte      `json:"-"`
}

[dsnet]

\cc @cybrcodr

[cybrcodr]

JSON serialization is only described in proto3 page --
https://developers.google.com/protocol-buffers/docs/proto3#json

Sadly a "required" field does not apply in proto3. If we do change the unmarshaler to respect required for proto2, it may break current users. We can add an option to check for required fields instead.

[mkevac]

@cybrcodr, how would it break current users?

[cybrcodr]

If an existing application is relying on the current behavior of not returning error on missing required fields, and it switches to returning an error, then it may break that app.

Anyways, I think chances are probably small though I could be totally wrong on this. I don't have the bandwidth to work on this, but am open to review a PR.

[cybrcodr]

Btw, for consistency, if we do change this behavior to return an error value if required field is not in set on Unmarshal, we should also change Marshal to return error if required field is not set before Marshal'ing.

[mkevac]

Sure. And that is how it is supposed to be IMO :-\

[cybrcodr]

Fixed in #472