Merge pull request #668 from jskeet/json_ordering

Fix C# JSON field ordering

csharp/protos/extest/unittest_issues.proto

@@ -88,4 +88,29 @@ message ReservedNames {
 
   int32 types = 1;
   int32 descriptor = 2;
+}
+
+message TestJsonFieldOrdering {
+  // These fields are deliberately not declared in numeric
+  // order, and the oneof fields aren't contiguous either.
+  // This allows for reasonably robust tests of JSON output
+  // ordering.
+  // TestFieldOrderings in unittest_proto3.proto is similar,
+  // but doesn't include oneofs.
+  // TODO: Consider adding 
+  
+  int32 plain_int32 = 4;
+
+  oneof o1 {
+    string o1_string = 2;
+    int32 o1_int32 = 5;
+  }
+  
+  string plain_string = 1;
+  
+  oneof o2 {
+    int32 o2_int32 = 6;
+    string o2_string = 3;
+  }
+  
 }
\ No newline at end of file

csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs

@@ -37,6 +37,7 @@ using System.Text;
 using System.Threading.Tasks;
 using Google.Protobuf.TestProtos;
 using NUnit.Framework;
+using UnitTest.Issues.TestProtos;
 
 namespace Google.Protobuf
 {
@@ -284,5 +285,29 @@ namespace Google.Protobuf
             Assert.IsTrue(actualJson.Contains("\"int64Field\": null"));
             Assert.IsFalse(actualJson.Contains("\"int32Field\": null"));
         }
+
+        [Test]
+        public void OutputIsInNumericFieldOrder_NoDefaults()
+        {
+            var formatter = new JsonFormatter(new JsonFormatter.Settings(false));
+            var message = new TestJsonFieldOrdering { PlainString = "p1", PlainInt32 = 2 };
+            Assert.AreEqual("{ \"plainString\": \"p1\", \"plainInt32\": 2 }", formatter.Format(message));
+            message = new TestJsonFieldOrdering { O1Int32 = 5, O2String = "o2", PlainInt32 = 10, PlainString = "plain" };
+            Assert.AreEqual("{ \"plainString\": \"plain\", \"o2String\": \"o2\", \"plainInt32\": 10, \"o1Int32\": 5 }", formatter.Format(message));
+            message = new TestJsonFieldOrdering { O1String = "", O2Int32 = 0, PlainInt32 = 10, PlainString = "plain" };
+            Assert.AreEqual("{ \"plainString\": \"plain\", \"o1String\": \"\", \"plainInt32\": 10, \"o2Int32\": 0 }", formatter.Format(message));
+        }
+
+        [Test]
+        public void OutputIsInNumericFieldOrder_WithDefaults()
+        {
+            var formatter = new JsonFormatter(new JsonFormatter.Settings(true));
+            var message = new TestJsonFieldOrdering();
+            Assert.AreEqual("{ \"plainString\": \"\", \"plainInt32\": 0 }", formatter.Format(message));
+            message = new TestJsonFieldOrdering { O1Int32 = 5, O2String = "o2", PlainInt32 = 10, PlainString = "plain" };
+            Assert.AreEqual("{ \"plainString\": \"plain\", \"o2String\": \"o2\", \"plainInt32\": 10, \"o1Int32\": 5 }", formatter.Format(message));
+            message = new TestJsonFieldOrdering { O1String = "", O2Int32 = 0, PlainInt32 = 10, PlainString = "plain" };
+            Assert.AreEqual("{ \"plainString\": \"plain\", \"o1String\": \"\", \"plainInt32\": 10, \"o2Int32\": 0 }", formatter.Format(message));
+        }
     }
 }

csharp/src/Google.Protobuf/JsonFormatter.cs

@@ -145,13 +145,14 @@ namespace Google.Protobuf
                 var accessor = field.Accessor;
                 // Oneofs are written later
                 // TODO: Change to write out fields in order, interleaving oneofs appropriately (as per binary format)
-                if (field.ContainingOneof != null)
+                if (field.ContainingOneof != null && field.ContainingOneof.Accessor.GetCaseFieldDescriptor(message) != field)
                 {
                     continue;
                 }
-                // Omit default values unless we're asked to format them
+                // Omit default values unless we're asked to format them, or they're oneofs (where the default
+                // value is still formatted regardless, because that's how we preserve the oneof case).
                 object value = accessor.GetValue(message);
-                if (!settings.FormatDefaultValues && IsDefaultValue(accessor, value))
+                if (field.ContainingOneof == null && !settings.FormatDefaultValues && IsDefaultValue(accessor, value))
                 {
                     continue;
                 }
@@ -170,33 +171,7 @@ namespace Google.Protobuf
                 builder.Append(": ");
                 WriteValue(builder, accessor, value);
                 first = false;
-            }
-
-            // Now oneofs
-            foreach (var oneof in message.Descriptor.Oneofs)
-            {
-                var accessor = oneof.Accessor;
-                var fieldDescriptor = accessor.GetCaseFieldDescriptor(message);
-                if (fieldDescriptor == null)
-                {
-                    continue;
-                }
-                object value = fieldDescriptor.Accessor.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, fieldDescriptor.Accessor, value);
-                first = false;
-            }
+            }            
             builder.Append(first ? "}" : " }");
         }