Format default values for "present" fields regardless of value

conformance/failure_list_csharp.txt

@@ -1,4 +1,3 @@
 Recommended.Proto3.JsonInput.BytesFieldBase64Url.JsonOutput
 Recommended.Proto3.JsonInput.BytesFieldBase64Url.ProtobufOutput
-Required.Proto2.JsonInput.StoresDefaultPrimitive.Validator
 Recommended.Proto2.JsonInput.FieldNameExtension.Validator

csharp/src/Google.Protobuf.Conformance/Program.cs

@@ -83,44 +83,52 @@ private static bool RunTest(BinaryReader input, BinaryWriter output, TypeRegistr
 
         private static ConformanceResponse PerformRequest(ConformanceRequest request, TypeRegistry typeRegistry)
         {
+            ExtensionRegistry proto2ExtensionRegistry = new ExtensionRegistry
+            {
+                ProtobufTestMessages.Proto2.TestMessagesProto2Extensions.ExtensionInt32,
+                ProtobufTestMessages.Proto2.TestAllTypesProto2.Types.MessageSetCorrectExtension1.Extensions.MessageSetExtension,
+                ProtobufTestMessages.Proto2.TestAllTypesProto2.Types.MessageSetCorrectExtension2.Extensions.MessageSetExtension
+            };
             IMessage message;
             try
             {
                 switch (request.PayloadCase)
                 {
                     case ConformanceRequest.PayloadOneofCase.JsonPayload:
-                        if (request.TestCategory == global::Conformance.TestCategory.JsonIgnoreUnknownParsingTest) {
+                        if (request.TestCategory == global::Conformance.TestCategory.JsonIgnoreUnknownParsingTest)
+                        {
                             return new ConformanceResponse { Skipped = "CSharp doesn't support skipping unknown fields in json parsing." };
                         }
                         var parser = new JsonParser(new JsonParser.Settings(20, typeRegistry));
-                        message = parser.Parse<ProtobufTestMessages.Proto3.TestAllTypesProto3>(request.JsonPayload);
-                        break;
-                    case ConformanceRequest.PayloadOneofCase.ProtobufPayload:
-                    {
-                        if (request.MessageType.Equals("protobuf_test_messages.proto3.TestAllTypesProto3"))
+                        switch (request.MessageType)
                         {
-                            message = ProtobufTestMessages.Proto3.TestAllTypesProto3.Parser.ParseFrom(request.ProtobufPayload);
+                            case "protobuf_test_messages.proto3.TestAllTypesProto3":
+                                message = parser.Parse<ProtobufTestMessages.Proto3.TestAllTypesProto3>(request.JsonPayload);
+                                break;
+                            case "protobuf_test_messages.proto2.TestAllTypesProto2":
+                                message = parser.Parse<ProtobufTestMessages.Proto2.TestAllTypesProto2>(request.JsonPayload);
+                                break;
+                            default:
+                                throw new Exception($" Protobuf request doesn't have specific payload type ({request.MessageType})");
                         }
-                        else if (request.MessageType.Equals("protobuf_test_messages.proto2.TestAllTypesProto2"))
-                        {
-                            ExtensionRegistry registry = new ExtensionRegistry()
-                            {
-                                ProtobufTestMessages.Proto2.TestMessagesProto2Extensions.ExtensionInt32,
-                                ProtobufTestMessages.Proto2.TestAllTypesProto2.Types.MessageSetCorrectExtension1.Extensions.MessageSetExtension,
-                                ProtobufTestMessages.Proto2.TestAllTypesProto2.Types.MessageSetCorrectExtension2.Extensions.MessageSetExtension
-                            };
-                            message = ProtobufTestMessages.Proto2.TestAllTypesProto2.Parser.WithExtensionRegistry(registry).ParseFrom(request.ProtobufPayload);
-                        }
-                        else
+                        break;
+                    case ConformanceRequest.PayloadOneofCase.ProtobufPayload:
+                        switch (request.MessageType)
                         {
-                            throw new Exception(" Protobuf request doesn't have specific payload type");
+                            case "protobuf_test_messages.proto3.TestAllTypesProto3":
+                                message = ProtobufTestMessages.Proto3.TestAllTypesProto3.Parser.ParseFrom(request.ProtobufPayload);
+                                break;
+                            case "protobuf_test_messages.proto2.TestAllTypesProto2":
+                                message = ProtobufTestMessages.Proto2.TestAllTypesProto2.Parser
+                                    .WithExtensionRegistry(proto2ExtensionRegistry)
+                                    .ParseFrom(request.ProtobufPayload);
+                                break;
+                            default:
+                                throw new Exception($" Protobuf request doesn't have specific payload type ({request.MessageType})");
                         }
                         break;
-                    }
 					case ConformanceRequest.PayloadOneofCase.TextPayload:
-					{
 						return new ConformanceResponse { Skipped = "CSharp doesn't support text format" };
-					}
                     default:
                         throw new Exception("Unsupported request payload: " + request.PayloadCase);
                 }

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

@@ -40,6 +40,7 @@
 using static Google.Protobuf.JsonParserTest; // For WrapInQuotes
 using System.IO;
 using Google.Protobuf.Collections;
+using ProtobufUnittest;
 
 namespace Google.Protobuf
 {
@@ -153,6 +154,48 @@ public void AllSingleFields()
             AssertJson(expectedText, actualText);
         }
 
+        [Test]
+        public void WithFormatDefaultValues_DoesNotAffectMessageFields()
+        {
+            var message = new TestAllTypes();
+            var formatter = new JsonFormatter(JsonFormatter.Settings.Default.WithFormatDefaultValues(true));
+            var json = formatter.Format(message);
+            Assert.IsFalse(json.Contains("\"singleNestedMessage\""));
+            Assert.IsFalse(json.Contains("\"singleForeignMessage\""));
+            Assert.IsFalse(json.Contains("\"singleImportMessage\""));
+        }
+
+        [Test]
+        public void WithFormatDefaultValues_DoesNotAffectProto3OptionalFields()
+        {
+            var message = new TestProto3Optional();
+            message.OptionalInt32 = 0;
+            var formatter = new JsonFormatter(JsonFormatter.Settings.Default.WithFormatDefaultValues(true));
+            var json = formatter.Format(message);
+            // The non-optional proto3 fields are formatted, as is the optional-but-specified field.
+            AssertJson("{ 'optionalInt32': 0, 'singularInt32': 0, 'singularInt64': '0' }", json);
+        }
+
+        [Test]
+        public void WithFormatDefaultValues_DoesNotAffectProto2Fields()
+        {
+            var message = new TestProtos.Proto2.ForeignMessage();
+            message.C = 0;
+            var formatter = new JsonFormatter(JsonFormatter.Settings.Default.WithFormatDefaultValues(true));
+            var json = formatter.Format(message);
+            // The specified field is formatted, but the non-specified field (d) is not.
+            AssertJson("{ 'c': 0 }", json);
+        }
+
+        [Test]
+        public void WithFormatDefaultValues_DoesNotAffectOneofFields()
+        {
+            var message = new TestOneof();
+            var formatter = new JsonFormatter(JsonFormatter.Settings.Default.WithFormatDefaultValues(true));
+            var json = formatter.Format(message);
+            AssertJson("{ }", json);
+        }
+
         [Test]
         public void RepeatedField()
         {
@@ -313,14 +356,16 @@ public void WrapperFormatting_Message()
         }
 
         [Test]
-        public void WrapperFormatting_IncludeNull()
+        public void WrapperFormatting_FormatDefaultValuesDoesNotFormatNull()
         {
             // The actual JSON here is very large because there are lots of fields. Just test a couple of them.
             var message = new TestWellKnownTypes { Int32Field = 10 };
             var formatter = new JsonFormatter(JsonFormatter.Settings.Default.WithFormatDefaultValues(true));
             var actualJson = formatter.Format(message);
-            Assert.IsTrue(actualJson.Contains("\"int64Field\": null"));
-            Assert.IsFalse(actualJson.Contains("\"int32Field\": null"));
+            // This *used* to include "int64Field": null, but that was a bug.
+            // WithDefaultValues should not affect message fields, including wrapper types.
+            Assert.IsFalse(actualJson.Contains("\"int64Field\": null"));
+            Assert.IsTrue(actualJson.Contains("\"int32Field\": 10"));
         }
 
         [Test]
@@ -602,6 +647,13 @@ public void WriteValue_List()
             AssertWriteValue(value, "[ 1, 2, 3 ]");
         }
 
+        [Test]
+        public void Proto2_DefaultValuesWritten()
+        {
+            var value = new ProtobufTestMessages.Proto2.TestAllTypesProto2() { FieldName13 = 0 };
+            AssertWriteValue(value, "{ 'FieldName13': 0 }");
+        }
+
         private static void AssertWriteValue(object value, string expectedJson)
         {
             var writer = new StringWriter();

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

@@ -34,6 +34,7 @@
 using Google.Protobuf.TestProtos;
 using Google.Protobuf.WellKnownTypes;
 using NUnit.Framework;
+using ProtobufTestMessages.Proto2;
 using System;
 
 namespace Google.Protobuf
@@ -949,6 +950,16 @@ public void UnknownField_NotIgnored()
             Assert.Throws<InvalidProtocolBufferException>(() => TestAllTypes.Parser.ParseJson(json));
         }
 
+        [Test]
+        public void Proto2_DefaultValuesPreserved()
+        {
+            string json = "{ \"FieldName13\": 0 }";
+            var parsed = TestAllTypesProto2.Parser.ParseJson(json);
+            Assert.False(parsed.HasFieldName10);
+            Assert.True(parsed.HasFieldName13);
+            Assert.AreEqual(0, parsed.FieldName13);
+        }
+
         [Test]
         [TestCase("5")]
         [TestCase("\"text\"")]

csharp/src/Google.Protobuf/JsonFormatter.cs

@@ -221,19 +221,12 @@ private bool WriteMessageFields(TextWriter writer, IMessage message, bool assume
             foreach (var field in fields.InFieldNumberOrder())
             {
                 var accessor = field.Accessor;
-                if (field.ContainingOneof != null && field.ContainingOneof.Accessor.GetCaseFieldDescriptor(message) != field)
-                {
-                    continue;
-                }
-                // 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 (field.ContainingOneof == null && !settings.FormatDefaultValues && IsDefaultValue(accessor, value))
+                var value = accessor.GetValue(message);
+                if (!ShouldFormatFieldValue(message, field, value))
                 {
                     continue;
                 }
 
-                // Okay, all tests complete: let's write the field value...
                 if (!first)
                 {
                     writer.Write(PropertySeparator);
@@ -248,6 +241,18 @@ private bool WriteMessageFields(TextWriter writer, IMessage message, bool assume
             return !first;
         }
 
+        /// <summary>
+        /// Determines whether or not a field value should be serialized according to the field,
+        /// its value in the message, and the settings of this formatter.
+        /// </summary>
+        private bool ShouldFormatFieldValue(IMessage message, FieldDescriptor field, object value) =>
+            field.HasPresence
+            // Fields that support presence *just* use that
+            ? field.Accessor.HasValue(message)
+            // Otherwise, format if either we've been asked to format default values, or if it's
+            // not a default value anyway.
+            : settings.FormatDefaultValues || !IsDefaultValue(field, value);
+
         // Converted from java/core/src/main/java/com/google/protobuf/Descriptors.java
         internal static string ToJsonName(string name)
         {
@@ -295,19 +300,19 @@ private static void WriteNull(TextWriter writer)
             writer.Write("null");
         }
 
-        private static bool IsDefaultValue(IFieldAccessor accessor, object value)
+        private static bool IsDefaultValue(FieldDescriptor descriptor, object value)
         {
-            if (accessor.Descriptor.IsMap)
+            if (descriptor.IsMap)
             {
                 IDictionary dictionary = (IDictionary) value;
                 return dictionary.Count == 0;
             }
-            if (accessor.Descriptor.IsRepeated)
+            if (descriptor.IsRepeated)
             {
                 IList list = (IList) value;
                 return list.Count == 0;
             }
-            switch (accessor.Descriptor.FieldType)
+            switch (descriptor.FieldType)
             {
                 case FieldType.Bool:
                     return (bool) value == false;
@@ -793,8 +798,10 @@ static Settings()
             }
 
             /// <summary>
-            /// Whether fields whose values are the default for the field type (e.g. 0 for integers)
-            /// should be formatted (true) or omitted (false).
+            /// Whether fields which would otherwise not be included in the formatted data
+            /// should be formatted even when the value is not present, or has the default value.
+            /// This option only affects fields which don't support "presence" (e.g.
+            /// singular non-optional proto3 primitive fields).
             /// </summary>
             public bool FormatDefaultValues { get; }