From 4fed0b515fb18b0ffc6f5d382f38e2a1b39912dd Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 31 Jul 2015 10:33:31 +0100 Subject: Fix JSON formatting to always emit fields in field order, including oneofs --- csharp/protos/extest/unittest_issues.proto | 25 ++ .../src/Google.Protobuf.Test/JsonFormatterTest.cs | 25 ++ .../TestProtos/UnittestIssues.cs | 304 ++++++++++++++++++++- csharp/src/Google.Protobuf/JsonFormatter.cs | 35 +-- 4 files changed, 353 insertions(+), 36 deletions(-) (limited to 'csharp') 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 { + private static readonly pb::MessageParser _parser = new pb::MessageParser(() => new TestJsonFieldOrdering()); + public static pb::MessageParser 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 ? "}" : " }"); } -- cgit v1.2.3