Make json_name take priority over name (fully) in C# parsing#12262
Make json_name take priority over name (fully) in C# parsing#12262jskeet wants to merge 1 commit intoprotocolbuffers:mainfrom
Conversation
Fixes protocolbuffers#11987 (testprotos.pb and UnittestIssues.pb.cs are both generated; no need to review.)
jskeet
added
bug
c#
🅰️ safe for tests
github-actions
bot
removed
the
🅰️ safe for tests
| var original = new Issue11987Message { A = 10, B = 20, C = 30 }; | ||
| var json = JsonFormatter.Default.Format(original); | ||
| AssertJson("{ 'b': 10, 'a': 20, 'd': 30 }", json); | ||
| var parsed = Issue11987Message.Parser.ParseJson(json); |
There was a problem hiding this comment.
I'm curios, what happens if you try to parse "{ 'b': 10, 'a': 20, 'd': 30, 'c': 40 }"? I'm just wondering if on the map, there should only be a single entry per field with the key being JsonName if any, else Name. Or basically just have an internal EffectiveJsonName in the field descriptor?
There was a problem hiding this comment.
Good question - will check on Monday
There was a problem hiding this comment.
Right... our draft specification says that should be rejected, but currently the later field in the JSON will overwrite the earlier field. (For fields of a message type, it'll actually merge the two values.)
So the C# parser is in strict violation of the spec in that sense... as it happens, we need that non-compliant behavior for one CloudEvent parsing scenario at least. (We'll need to work out what to do about either having a "strict" parser setting or change the default in a new major version at some point.)
But I think the behavior with this PR is strictly better than before :)
There was a problem hiding this comment.
Yep, that all makes sense. Thanks!
|
Reminder: #11987 effects C# and Java. Remember to reopen the issue after this is merged, so Java has an open issue to fix it as well. |
4 participants
Fixes #11987
(testprotos.pb and UnittestIssues.pb.cs are both generated; no need to review.)