From 6ea9bc7aa3af5c392fcd8f4eeebf71342c1ce961 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 10 Jul 2015 14:05:52 +0100 Subject: Fixes to JsonFormatter - Handle oneof properly - Omit unknown enum values --- .../src/ProtocolBuffers.Test/JsonFormatterTest.cs | 48 +++++++++++++- csharp/src/ProtocolBuffers/JsonFormatter.cs | 75 +++++++++++++++++++--- 2 files changed, 112 insertions(+), 11 deletions(-) (limited to 'csharp/src') diff --git a/csharp/src/ProtocolBuffers.Test/JsonFormatterTest.cs b/csharp/src/ProtocolBuffers.Test/JsonFormatterTest.cs index 5f80a499..5441bf47 100644 --- a/csharp/src/ProtocolBuffers.Test/JsonFormatterTest.cs +++ b/csharp/src/ProtocolBuffers.Test/JsonFormatterTest.cs @@ -160,10 +160,34 @@ namespace Google.Protobuf } [Test] - public void UnknownEnumValue() + public void UnknownEnumValueOmitted_SingleField() { var message = new TestAllTypes { SingleForeignEnum = (ForeignEnum) 100 }; - Assert.AreEqual("{ \"singleForeignEnum\": 100 }", JsonFormatter.Default.Format(message)); + Assert.AreEqual("{ }", JsonFormatter.Default.Format(message)); + } + + [Test] + public void UnknownEnumValueOmitted_RepeatedField() + { + var message = new TestAllTypes { RepeatedForeignEnum = { ForeignEnum.FOREIGN_BAZ, (ForeignEnum) 100, ForeignEnum.FOREIGN_FOO } }; + Assert.AreEqual("{ \"repeatedForeignEnum\": [ \"FOREIGN_BAZ\", \"FOREIGN_FOO\" ] }", JsonFormatter.Default.Format(message)); + } + + [Test] + public void UnknownEnumValueOmitted_MapField() + { + // This matches the C++ behaviour. + var message = new TestMap { MapInt32Enum = { { 1, MapEnum.MAP_ENUM_FOO }, { 2, (MapEnum) 100 }, { 3, MapEnum.MAP_ENUM_BAR } } }; + Assert.AreEqual("{ \"mapInt32Enum\": { \"1\": \"MAP_ENUM_FOO\", \"3\": \"MAP_ENUM_BAR\" } }", JsonFormatter.Default.Format(message)); + } + + [Test] + public void UnknownEnumValueOmitted_RepeatedField_AllEntriesUnknown() + { + // *Maybe* we should hold off on writing the "[" until we find that we've got at least one value to write... + // but this is what happens at the moment, and it doesn't seem too awful. + var message = new TestAllTypes { RepeatedForeignEnum = { (ForeignEnum) 200, (ForeignEnum) 100 } }; + Assert.AreEqual("{ \"repeatedForeignEnum\": [ ] }", JsonFormatter.Default.Format(message)); } [Test] @@ -213,5 +237,25 @@ namespace Google.Protobuf { Assert.AreEqual(expected, JsonFormatter.ToCamelCase(original)); } + + [Test] + [TestCase(null, "{ }")] + [TestCase("x", "{ \"fooString\": \"x\" }")] + [TestCase("", "{ \"fooString\": \"\" }")] + [TestCase(null, "{ }")] + public void Oneof(string fooStringValue, string expectedJson) + { + var message = new TestOneof(); + if (fooStringValue != null) + { + message.FooString = fooStringValue; + } + + // We should get the same result both with and without "format default values". + var formatter = new JsonFormatter(new JsonFormatter.Settings(false)); + Assert.AreEqual(expectedJson, formatter.Format(message)); + formatter = new JsonFormatter(new JsonFormatter.Settings(true)); + Assert.AreEqual(expectedJson, formatter.Format(message)); + } } } diff --git a/csharp/src/ProtocolBuffers/JsonFormatter.cs b/csharp/src/ProtocolBuffers/JsonFormatter.cs index a6aa552f..3e2244d6 100644 --- a/csharp/src/ProtocolBuffers/JsonFormatter.cs +++ b/csharp/src/ProtocolBuffers/JsonFormatter.cs @@ -136,13 +136,28 @@ namespace Google.Protobuf builder.Append("{ "); var fields = message.Fields; bool first = true; + // First non-oneof fields foreach (var accessor in fields.Accessors) { + var descriptor = accessor.Descriptor; + // Oneofs are written later + if (descriptor.ContainingOneof != null) + { + continue; + } + // Omit default values unless we're asked to format them object value = accessor.GetValue(message); if (!settings.FormatDefaultValues && IsDefaultValue(accessor, value)) { continue; } + // Omit awkward (single) values such as unknown enum values + if (!descriptor.IsRepeated && !descriptor.IsMap && !CanWriteSingleValue(accessor.Descriptor, value)) + { + continue; + } + + // Okay, all tests complete: let's write the field value... if (!first) { builder.Append(", "); @@ -152,6 +167,32 @@ namespace Google.Protobuf WriteValue(builder, accessor, value); first = false; } + + // Now oneofs + foreach (var accessor in fields.Oneofs) + { + var fieldDescriptor = accessor.GetCaseFieldDescriptor(message); + if (fieldDescriptor == null) + { + continue; + } + var fieldAccessor = fields[fieldDescriptor]; + object value = fieldAccessor.GetValue(message); + // Omit awkward (single) values such as unknown enum values + if (!fieldDescriptor.IsRepeated && !fieldDescriptor.IsMap && !CanWriteSingleValue(fieldDescriptor, value)) + { + continue; + } + + if (!first) + { + builder.Append(", "); + } + WriteString(builder, ToCamelCase(fieldDescriptor.Name)); + builder.Append(": "); + WriteValue(builder, fieldAccessor, value); + first = false; + } builder.Append(first ? "}" : " }"); } @@ -303,15 +344,8 @@ namespace Google.Protobuf } case FieldType.Enum: EnumValueDescriptor enumValue = descriptor.EnumType.FindValueByNumber((int) value); - if (enumValue != null) - { - WriteString(builder, enumValue.Name); - } - else - { - // ??? Need more documentation - builder.Append(((int) value).ToString("d", CultureInfo.InvariantCulture)); - } + // We will already have validated that this is a known value. + WriteString(builder, enumValue.Name); break; case FieldType.Fixed64: case FieldType.UInt64: @@ -354,6 +388,10 @@ namespace Google.Protobuf bool first = true; foreach (var value in list) { + if (!CanWriteSingleValue(accessor.Descriptor, value)) + { + continue; + } if (!first) { builder.Append(", "); @@ -373,6 +411,10 @@ namespace Google.Protobuf // This will box each pair. Could use IDictionaryEnumerator, but that's ugly in terms of disposal. foreach (DictionaryEntry pair in dictionary) { + if (!CanWriteSingleValue(valueType, pair.Value)) + { + continue; + } if (!first) { builder.Append(", "); @@ -409,6 +451,21 @@ namespace Google.Protobuf builder.Append(first ? "}" : " }"); } + /// + /// Returns whether or not a singular value can be represented in JSON. + /// Currently only relevant for enums, where unknown values can't be represented. + /// For repeated/map fields, this always returns true. + /// + private bool CanWriteSingleValue(FieldDescriptor descriptor, object value) + { + if (descriptor.FieldType == FieldType.Enum) + { + EnumValueDescriptor enumValue = descriptor.EnumType.FindValueByNumber((int) value); + return enumValue != null; + } + return true; + } + /// /// Writes a string (including leading and trailing double quotes) to a builder, escaping as required. /// -- cgit v1.2.3