jsonpb: support unmarshaling enums as map values

[jhump]

In PR #630, it looks like unmarshalling of enums from name strings was broken.

It began to break JSON-related tests in my [https://github.com/jhump/protoreflect] project. It also looks like that change may have caused enum marshaling to default to numeric values instead of name strings.

[jhump]

I just realized why existing tests don't catch this. It's only when the enum is the value of a map field. Here's a simple repro:

// in golang/protobuf/proto/test_proto/test.proto, edit GoEnum to look like so:
message GoEnum {
  required FOO foo = 1;
  map<string, FOO> foos = 2;
}

// Add these test cases to golang/protobuf/jsonpb/jsonpb_test.go
func Test644Unmarshal(t *testing.T) {
	js := `{"foo":"FOO1","foos":{"abc":"FOO1"}}`
	var m test_proto.GoEnum
	err := UnmarshalString(js, &m)
	if err != nil {
		t.Fatalf(err.Error())
	}
	expected := map[string]test_proto.FOO{"abc": test_proto.FOO_FOO1}
	if !reflect.DeepEqual(m.Foos, expected) {
		t.Errorf("failed to de-serialize as expected: wanted %v; got %v", expected, m.Foos)
	}
}

func Test644Marshal(t *testing.T) {
	m := &test_proto.GoEnum{
		Foo:  test_proto.FOO_FOO1.Enum(),
		Foos: map[string]test_proto.FOO{"abc": test_proto.FOO_FOO1},
	}
	var jsm Marshaler // Note: EnumsAsInts has defaulted to *false*
	s, err := jsm.MarshalToString(m)
	if err != nil {
		t.Fatal(err.Error())
	}
	// how it used to serialize, before PR 630
	expected := `{"foo":"FOO1","foos":{"abc":"FOO1"}}`
	if s != expected {
		t.Errorf("failed to serialize as expected: wanted `%s`; got `%s`", expected, s)
	}
}

The former (unmarshal) blows up with a parse error where it shouldn't. The latter emits the map value FOO enum as an int.

[kassiansun]

I'll take a look at this as soon as possible.

[kassiansun]

@jhump The issue is not only cause by PR #630, actually:

1. The marshaling behavior has changed before jsonpb: decode int32/uint32/float32/float64 strings #630 , I tried to revert the commit and the marshalling test still fails.
2. The protoc-gen-go generated map[string]enum set enum=XX tag at json, not protobuf, so we will not get proper tag info, and then we'll not detect the Enum. If the enum Type is detected properly, there's a shortcut before handling integer values for Enum.

So to fix the unmarshalling issue, IMO, we should fix protoc-gen-go first, and pass the prop into unmarshalValue case reflect.Map, there's also a TODO for passing prop at here.

For the marshalling issue, I think it's due to the protoc-gen-go, too.

I'm not an expert of protobuf & jsonpb, @dsnet how should we handle this?

[kassiansun]

Actually protoc-gen-go generated protobuf_val tag correctly, but my print shows it did not get it from unmarshalValue, investigating it.

[kassiansun]

@jhump Please take a look at #645 , check if it fixes your issue.

[jhump]

The marshaling behavior has changed before #630 , I tried to revert the commit and the marshalling test still fails.

@kassiansun, ah, sorry for misattributing that to your PR. I was mistakenly thinking that my own protoreflect test's would have failed with that JSON output, and my own tests didn't turn red until #630 landed. I was mistaken: my tests only broke due to the unmarshalling bug.

[kassiansun]

@jhump No problem ;-)

[dsnet]

@jhump, I'm confused how this was working in the first place. PR #645 seems to be addressing items that have been TODOs for quite some time now. Am I misunderstanding something?

(not that we shouldn't fix this)

[jhump]

@dsnet: I acknowledged above that the marshaling part of this bug report (where enum in maps are always marshalled as ints, regardless of the json.Marsahler setting) was in fact already in place. My attributing it to PR #630 was a mistake.

However, the unmarshalling bit was newly introduced. My project, which tests JSON serialization (which includes using jsonpb to verify the dynamic message's JSON output), began failing after #630 landed: https://travis-ci.org/jhump/protoreflect/builds

The cause of my project's test failures was an error during unmarshalling that should not have happened (e.g. the JSON being parsed was valid).

So of the two repro test cases I pasted above, only the first was actually caused recently.