Fix JSON formatting to always emit fields in field order, including oneofs

4 files changed, 353 insertions, 36 deletions

diff --git a/csharp/protos/extest/unittest_issues.proto b/csharp/protos/extest/unittest_issues.proto
index b0c92fa2..b66da471 100644
--- a/csharp/protos/extest/unittest_issues.proto
+++ b/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
diff --git a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs
index a6715698..b3f6041e 100644
--- a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs
+++ b/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));
+        }
     }
 }
diff --git a/csharp/src/Google.Protobuf.Test/TestProtos/UnittestIssues.cs b/csharp/src/Google.Protobuf.Test/TestProtos/UnittestIssues.cs
index 92485c51..8c0dc4e9 100644
--- a/csharp/src/Google.Protobuf.Test/TestProtos/UnittestIssues.cs
+++ b/csharp/src/Google.Protobuf.Test/TestProtos/UnittestIssues.cs
@@ -36,11 +36,14 @@ namespace UnitTest.Issues.TestProtos {
             "NgoJRW51bUFycmF5GAYgAygOMh8udW5pdHRlc3RfaXNzdWVzLkRlcHJlY2F0", 
             "ZWRFbnVtQgIYASIZCglJdGVtRmllbGQSDAoEaXRlbRgBIAEoBSJECg1SZXNl", 
             "cnZlZE5hbWVzEg0KBXR5cGVzGAEgASgFEhIKCmRlc2NyaXB0b3IYAiABKAUa", 
-            "EAoOU29tZU5lc3RlZFR5cGUqVQoMTmVnYXRpdmVFbnVtEhYKEk5FR0FUSVZF", 
-            "X0VOVU1fWkVSTxAAEhYKCUZpdmVCZWxvdxD7//////////8BEhUKCE1pbnVz", 
-            "T25lEP///////////wEqLgoORGVwcmVjYXRlZEVudW0SEwoPREVQUkVDQVRF", 
-            "RF9aRVJPEAASBwoDb25lEAFCH0gBqgIaVW5pdFRlc3QuSXNzdWVzLlRlc3RQ", 
-            "cm90b3NiBnByb3RvMw=="));
+            "EAoOU29tZU5lc3RlZFR5cGUioAEKFVRlc3RKc29uRmllbGRPcmRlcmluZxIT", 
+            "CgtwbGFpbl9pbnQzMhgEIAEoBRITCglvMV9zdHJpbmcYAiABKAlIABISCghv", 
+            "MV9pbnQzMhgFIAEoBUgAEhQKDHBsYWluX3N0cmluZxgBIAEoCRISCghvMl9p", 
+            "bnQzMhgGIAEoBUgBEhMKCW8yX3N0cmluZxgDIAEoCUgBQgQKAm8xQgQKAm8y", 
+            "KlUKDE5lZ2F0aXZlRW51bRIWChJORUdBVElWRV9FTlVNX1pFUk8QABIWCglG", 
+            "aXZlQmVsb3cQ+///////////ARIVCghNaW51c09uZRD///////////8BKi4K", 
+            "DkRlcHJlY2F0ZWRFbnVtEhMKD0RFUFJFQ0FURURfWkVSTxAAEgcKA29uZRAB", 
+            "Qh9IAaoCGlVuaXRUZXN0Lklzc3Vlcy5UZXN0UHJvdG9zYgZwcm90bzM="));
       descriptor = pbr::FileDescriptor.InternalBuildGeneratedFileFrom(descriptorData,
           new pbr::FileDescriptor[] { },
           new pbr::GeneratedCodeInfo(new[] {typeof(global::UnitTest.Issues.TestProtos.NegativeEnum), typeof(global::UnitTest.Issues.TestProtos.DeprecatedEnum), }, new pbr::GeneratedCodeInfo[] {
@@ -49,7 +52,8 @@ namespace UnitTest.Issues.TestProtos {
             new pbr::GeneratedCodeInfo(typeof(global::UnitTest.Issues.TestProtos.DeprecatedChild), null, null, null, null),
             new pbr::GeneratedCodeInfo(typeof(global::UnitTest.Issues.TestProtos.DeprecatedFieldsMessage), new[]{ "PrimitiveValue", "PrimitiveArray", "MessageValue", "MessageArray", "EnumValue", "EnumArray" }, null, null, null),
             new pbr::GeneratedCodeInfo(typeof(global::UnitTest.Issues.TestProtos.ItemField), new[]{ "Item" }, null, null, null),
-            new pbr::GeneratedCodeInfo(typeof(global::UnitTest.Issues.TestProtos.ReservedNames), new[]{ "Types_", "Descriptor_" }, null, null, new pbr::GeneratedCodeInfo[] { new pbr::GeneratedCodeInfo(typeof(global::UnitTest.Issues.TestProtos.ReservedNames.Types.SomeNestedType), null, null, null, null)})
+            new pbr::GeneratedCodeInfo(typeof(global::UnitTest.Issues.TestProtos.ReservedNames), new[]{ "Types_", "Descriptor_" }, null, null, new pbr::GeneratedCodeInfo[] { new pbr::GeneratedCodeInfo(typeof(global::UnitTest.Issues.TestProtos.ReservedNames.Types.SomeNestedType), null, null, null, null)}),
+            new pbr::GeneratedCodeInfo(typeof(global::UnitTest.Issues.TestProtos.TestJsonFieldOrdering), new[]{ "PlainInt32", "O1String", "O1Int32", "PlainString", "O2Int32", "O2String" }, new[]{ "O1", "O2" }, null, null)
           }));
     }
     #endregion
@@ -1096,6 +1100,294 @@ namespace UnitTest.Issues.TestProtos {
 
   }
 
+  [global::System.Diagnostics.DebuggerNonUserCodeAttribute()]
+  public sealed partial class TestJsonFieldOrdering : pb::IMessage<TestJsonFieldOrdering> {
+    private static readonly pb::MessageParser<TestJsonFieldOrdering> _parser = new pb::MessageParser<TestJsonFieldOrdering>(() => new TestJsonFieldOrdering());
+    public static pb::MessageParser<TestJsonFieldOrdering> Parser { get { return _parser; } }
+
+    public static pbr::MessageDescriptor Descriptor {
+      get { return global::UnitTest.Issues.TestProtos.UnittestIssues.Descriptor.MessageTypes[6]; }
+    }
+
+    pbr::MessageDescriptor pb::IMessage.Descriptor {
+      get { return Descriptor; }
+    }
+
+    public TestJsonFieldOrdering() {
+      OnConstruction();
+    }
+
+    partial void OnConstruction();
+
+    public TestJsonFieldOrdering(TestJsonFieldOrdering other) : this() {
+      plainInt32_ = other.plainInt32_;
+      plainString_ = other.plainString_;
+      switch (other.O1Case) {
+        case O1OneofCase.O1String:
+          O1String = other.O1String;
+          break;
+        case O1OneofCase.O1Int32:
+          O1Int32 = other.O1Int32;
+          break;
+      }
+
+      switch (other.O2Case) {
+        case O2OneofCase.O2Int32:
+          O2Int32 = other.O2Int32;
+          break;
+        case O2OneofCase.O2String:
+          O2String = other.O2String;
+          break;
+      }
+
+    }
+
+    public TestJsonFieldOrdering Clone() {
+      return new TestJsonFieldOrdering(this);
+    }
+
+    public const int PlainInt32FieldNumber = 4;
+    private int plainInt32_;
+    public int PlainInt32 {
+      get { return plainInt32_; }
+      set {
+        plainInt32_ = value;
+      }
+    }
+
+    public const int O1StringFieldNumber = 2;
+    public string O1String {
+      get { return o1Case_ == O1OneofCase.O1String ? (string) o1_ : ""; }
+      set {
+        o1_ = pb::Preconditions.CheckNotNull(value, "value");
+        o1Case_ = O1OneofCase.O1String;
+      }
+    }
+
+    public const int O1Int32FieldNumber = 5;
+    public int O1Int32 {
+      get { return o1Case_ == O1OneofCase.O1Int32 ? (int) o1_ : 0; }
+      set {
+        o1_ = value;
+        o1Case_ = O1OneofCase.O1Int32;
+      }
+    }
+
+    public const int PlainStringFieldNumber = 1;
+    private string plainString_ = "";
+    public string PlainString {
+      get { return plainString_; }
+      set {
+        plainString_ = pb::Preconditions.CheckNotNull(value, "value");
+      }
+    }
+
+    public const int O2Int32FieldNumber = 6;
+    public int O2Int32 {
+      get { return o2Case_ == O2OneofCase.O2Int32 ? (int) o2_ : 0; }
+      set {
+        o2_ = value;
+        o2Case_ = O2OneofCase.O2Int32;
+      }
+    }
+
+    public const int O2StringFieldNumber = 3;
+    public string O2String {
+      get { return o2Case_ == O2OneofCase.O2String ? (string) o2_ : ""; }
+      set {
+        o2_ = pb::Preconditions.CheckNotNull(value, "value");
+        o2Case_ = O2OneofCase.O2String;
+      }
+    }
+
+    private object o1_;
+    public enum O1OneofCase {
+      None = 0,
+      O1String = 2,
+      O1Int32 = 5,
+    }
+    private O1OneofCase o1Case_ = O1OneofCase.None;
+    public O1OneofCase O1Case {
+      get { return o1Case_; }
+    }
+
+    public void ClearO1() {
+      o1Case_ = O1OneofCase.None;
+      o1_ = null;
+    }
+
+    private object o2_;
+    public enum O2OneofCase {
+      None = 0,
+      O2Int32 = 6,
+      O2String = 3,
+    }
+    private O2OneofCase o2Case_ = O2OneofCase.None;
+    public O2OneofCase O2Case {
+      get { return o2Case_; }
+    }
+
+    public void ClearO2() {
+      o2Case_ = O2OneofCase.None;
+      o2_ = null;
+    }
+
+    public override bool Equals(object other) {
+      return Equals(other as TestJsonFieldOrdering);
+    }
+
+    public bool Equals(TestJsonFieldOrdering other) {
+      if (ReferenceEquals(other, null)) {
+        return false;
+      }
+      if (ReferenceEquals(other, this)) {
+        return true;
+      }
+      if (PlainInt32 != other.PlainInt32) return false;
+      if (O1String != other.O1String) return false;
+      if (O1Int32 != other.O1Int32) return false;
+      if (PlainString != other.PlainString) return false;
+      if (O2Int32 != other.O2Int32) return false;
+      if (O2String != other.O2String) return false;
+      return true;
+    }
+
+    public override int GetHashCode() {
+      int hash = 1;
+      if (PlainInt32 != 0) hash ^= PlainInt32.GetHashCode();
+      if (o1Case_ == O1OneofCase.O1String) hash ^= O1String.GetHashCode();
+      if (o1Case_ == O1OneofCase.O1Int32) hash ^= O1Int32.GetHashCode();
+      if (PlainString.Length != 0) hash ^= PlainString.GetHashCode();
+      if (o2Case_ == O2OneofCase.O2Int32) hash ^= O2Int32.GetHashCode();
+      if (o2Case_ == O2OneofCase.O2String) hash ^= O2String.GetHashCode();
+      return hash;
+    }
+
+    public override string ToString() {
+      return pb::JsonFormatter.Default.Format(this);
+    }
+
+    public void WriteTo(pb::CodedOutputStream output) {
+      if (PlainString.Length != 0) {
+        output.WriteRawTag(10);
+        output.WriteString(PlainString);
+      }
+      if (o1Case_ == O1OneofCase.O1String) {
+        output.WriteRawTag(18);
+        output.WriteString(O1String);
+      }
+      if (o2Case_ == O2OneofCase.O2String) {
+        output.WriteRawTag(26);
+        output.WriteString(O2String);
+      }
+      if (PlainInt32 != 0) {
+        output.WriteRawTag(32);
+        output.WriteInt32(PlainInt32);
+      }
+      if (o1Case_ == O1OneofCase.O1Int32) {
+        output.WriteRawTag(40);
+        output.WriteInt32(O1Int32);
+      }
+      if (o2Case_ == O2OneofCase.O2Int32) {
+        output.WriteRawTag(48);
+        output.WriteInt32(O2Int32);
+      }
+    }
+
+    public int CalculateSize() {
+      int size = 0;
+      if (PlainInt32 != 0) {
+        size += 1 + pb::CodedOutputStream.ComputeInt32Size(PlainInt32);
+      }
+      if (o1Case_ == O1OneofCase.O1String) {
+        size += 1 + pb::CodedOutputStream.ComputeStringSize(O1String);
+      }
+      if (o1Case_ == O1OneofCase.O1Int32) {
+        size += 1 + pb::CodedOutputStream.ComputeInt32Size(O1Int32);
+      }
+      if (PlainString.Length != 0) {
+        size += 1 + pb::CodedOutputStream.ComputeStringSize(PlainString);
+      }
+      if (o2Case_ == O2OneofCase.O2Int32) {
+        size += 1 + pb::CodedOutputStream.ComputeInt32Size(O2Int32);
+      }
+      if (o2Case_ == O2OneofCase.O2String) {
+        size += 1 + pb::CodedOutputStream.ComputeStringSize(O2String);
+      }
+      return size;
+    }
+
+    public void MergeFrom(TestJsonFieldOrdering other) {
+      if (other == null) {
+        return;
+      }
+      if (other.PlainInt32 != 0) {
+        PlainInt32 = other.PlainInt32;
+      }
+      if (other.PlainString.Length != 0) {
+        PlainString = other.PlainString;
+      }
+      switch (other.O1Case) {
+        case O1OneofCase.O1String:
+          O1String = other.O1String;
+          break;
+        case O1OneofCase.O1Int32:
+          O1Int32 = other.O1Int32;
+          break;
+      }
+
+      switch (other.O2Case) {
+        case O2OneofCase.O2Int32:
+          O2Int32 = other.O2Int32;
+          break;
+        case O2OneofCase.O2String:
+          O2String = other.O2String;
+          break;
+      }
+
+    }
+
+    public void MergeFrom(pb::CodedInputStream input) {
+      uint tag;
+      while (input.ReadTag(out tag)) {
+        switch(tag) {
+          case 0:
+            throw pb::InvalidProtocolBufferException.InvalidTag();
+          default:
+            if (pb::WireFormat.IsEndGroupTag(tag)) {
+              return;
+            }
+            break;
+          case 10: {
+            PlainString = input.ReadString();
+            break;
+          }
+          case 18: {
+            O1String = input.ReadString();
+            break;
+          }
+          case 26: {
+            O2String = input.ReadString();
+            break;
+          }
+          case 32: {
+            PlainInt32 = input.ReadInt32();
+            break;
+          }
+          case 40: {
+            O1Int32 = input.ReadInt32();
+            break;
+          }
+          case 48: {
+            O2Int32 = input.ReadInt32();
+            break;
+          }
+        }
+      }
+    }
+
+  }
+
   #endregion
 
 }
diff --git a/csharp/src/Google.Protobuf/JsonFormatter.cs b/csharp/src/Google.Protobuf/JsonFormatter.cs
index 438af4df..d9783fc4 100644
--- a/csharp/src/Google.Protobuf/JsonFormatter.cs
+++ b/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 ? "}" : " }");
         }