The usage of ICustomDiagnosticMessage here is non-essential - ToDiagnosticString
doesn't actually get called by ToString() in this case, due to JsonFormatter code. It was
intended to make it clearer that it *did* have a custom format... but then arguably I should
do the same for Value, Struct, Any etc.
Moving some of the code out of JsonFormatter and into Duration/Timestamp/FieldMask likewise
feels somewhat nice, somewhat nasty... basically there are JSON-specific bits of formatting, but
also domain-specific bits of computation. <sigh>
Thoughts welcome.
- Tighten up on Infinity/NaN handling in terms of whitespace handling (and test casing)
- Validate that values are genuinely integers when they've been parsed from a JSON number (ignoring the fact that 1.0000000000000000001 == 1 as a double...)
- Allow exponents and decimal points in string representations
The conformance tests now use types which are part of src/google/protobuf, so we need to include src in the proto path.
The notes around "fix-ups" have been out of date for some time now.
This addresses issue #1008, by creating a JsonFormatter which is private and only different
to JsonFormatter.Default in terms of reference equality.
Other plausible designs:
- The same, but expose the diagnostic-only formatter
- Add something to settings to say "I don't have a type registry at all"
- Change the behaviour of JsonFormatter.Default (bad idea IMO, as we really *don't* want the result of this used as regular JSON to be parsed)
Note that just trying to find a separate fix to issue #933 and using that to override Any.ToString() differently wouldn't work for messages that *contain* an Any.
Generated code changes follow in the next commit.
This required a rework of the tokenizer to allow for a "replaying" tokenizer, basically in case the @type value comes after the data itself. This rework is nice in some ways (all the pushback and object depth logic in one place) but is a little fragile in terms of token push-back when using the replay tokenizer. It'll be fine for the scenario we need it for, but we should be careful...
There are corner cases where MessageDescriptor.{ClrType,Parser} will return null, and these are now documented. However, normally they *should* be implemented, even for descriptors of for dynamic messages. Ditto FieldDescriptor.Accessor.
We'll still need a fair amount of work to implement dynamic messages, but this change means that the public API will be remain intact.
Additionally, this change starts making use of C# 6 features in the files that it touches. This is far from exhaustive, and later PRs will have more.
Generated code changes coming in the next commit.
Generated code coming in next commit - in a subsequent PR I want to do a bit of renaming and redocumenting around this, in anticipation of DynamicMessage.
This is only thrown directly by JsonTokenizer, but surfaces from JsonParser as well. I've added doc comments to hopefully make everything clear.
The exception is actually thrown by the reader within JsonTokenizer, in anticipation of keeping track of the location within the document, but that change is not within this PR.
This includes all the well-known types except Any.
Some aspects are likely to require further work when the details of the JSON parsing expectations are hammered out in more detail. Some of these have "ignored" tests already.
Note that the choice *not* to use Json.NET was made for two reasons:
- Going from 0 dependencies to 1 dependency is a big hit, and there's not much benefit here
- Json.NET parses more leniently than we'd want; accommodating that would be nearly as much work as writing the tokenizer
This only really affects the JsonTokenizer, which could be replaced by Json.NET. The JsonParser code would be about the same length with Json.NET... but I wouldn't be as confident in it.
This changes how we approach JSON formatting in general - instead of looking at the field a value came from, we just look at the type of the value. It's possible this *could* be slightly inefficient, but if we start caring about JSON performance deeply, we'll probably want to rewrite all of this anyway. It's definitely simpler this way.
When we support dynamic messages, we'll need to modify JsonFormatter to handle enum values, as they won't come be "real" .NET enums at that point. It shouldn't be hard to do though.
Jon Skeet
Dongjoon Hyun
Jan Tattermusch
Gabriel Kliot