Fixes to JsonFormatter

- Handle oneof properly
- Omit unknown enum values

2 files changed, 112 insertions, 11 deletions

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(", ");
@@ -410,6 +452,21 @@ namespace Google.Protobuf
         }
 
         /// <summary>
+        /// 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.
+        /// </summary>
+        private bool CanWriteSingleValue(FieldDescriptor descriptor, object value)
+        {
+            if (descriptor.FieldType == FieldType.Enum)
+            {
+                EnumValueDescriptor enumValue = descriptor.EnumType.FindValueByNumber((int) value);
+                return enumValue != null;
+            }
+            return true;
+        }
+
+        /// <summary>
         /// Writes a string (including leading and trailing double quotes) to a builder, escaping as required.
         /// </summary>
         /// <remarks>