[c#] JSON/XML: Throw on null, non-nullable strings

Resolves https://github.com/Microsoft/bond/issues/417 Closes https://github.com/Microsoft/bond/pull/515
2017-06-28 14:35:56 -07:00 · 2017-06-28 14:35:56 -07:00 · fe3135cc4a
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -102,6 +102,9 @@ get a compiler error. To fix, remove the `<Writer>` part:
  versions.
 * The codegen MSBuild targets will now re-run codegen if gbc itself has been
  changed.
+* Fixed a bug where JSON and XML protocols would permit the serialization of
+  non-nullable string fields that were set to null instead of throwing a
+  NullReferenceException. [Issue #417](https://github.com/Microsoft/bond/issues/417)

 ## 5.3.1: 2017-04-25 ##

--- a/cs/src/core/protocols/SimpleXmlWriter.cs
+++ b/cs/src/core/protocols/SimpleXmlWriter.cs
@ -212,12 +212,32 @@ namespace Bond.Protocols
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void WriteString(string value)
        {
+            // Other protocols depend on expressions such as value.Count to
+            // throw an NRE if we've been asked to serialize a non-nullable
+            // string field that is set to null. Implementations of
+            // System.Xml.XmlWriter may successfully serialize it, so we need
+            // to check and throw explicitly before that.
+            if (value == null)
+            {
+                throw new NullReferenceException(
+                   "Attempted to serialize a null string. This may indicate a non-nullable string field that was set to null.");
+            }
            writer.WriteValue(value);
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void WriteWString(string value)
        {
+            // Other protocols depend on expressions such as value.Count to
+            // throw an NRE if we've been asked to serialize a non-nullable
+            // string field that is set to null. Implementations of
+            // System.Xml.XmlWriter may successfully serialize it, so we need
+            // to check and throw explicitly before that.
+            if (value == null)
+            {
+                throw new NullReferenceException(
+                   "Attempted to serialize a null string. This may indicate a non-nullable string field that was set to null.");
+            }
            writer.WriteValue(value);
        }
        #endregion
--- a/cs/src/json/protocols/SimpleJsonWriter.cs
+++ b/cs/src/json/protocols/SimpleJsonWriter.cs
@ -147,12 +147,6 @@ namespace Bond.Protocols
            writer.WriteValue(value);
        }

-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public void WriteString(string value)
-        {
-            writer.WriteValue(value);
-        }
-
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void WriteUInt16(ushort value)
        {
@ -177,9 +171,35 @@ namespace Bond.Protocols
            writer.WriteValue(value);
        }

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public void WriteString(string value)
+        {
+            // Other protocols depend on expressions such as value.Count to
+            // throw an NRE if we've been asked to serialize a non-nullable
+            // string field that is set to null. Newtonsoft.Json will
+            // successfully serialize it as a JSON null (the unquoted text
+            // null), so we need to check and throw explicitly before that.
+            if (value == null)
+            {
+                throw new NullReferenceException(
+                   "Attempted to serialize a null string. This may indicate a non-nullable string field that was set to null.");
+            }
+            writer.WriteValue(value);
+        }
+
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void WriteWString(string value)
        {
+            // Other protocols depend on expressions such as value.Count to
+            // throw an NRE if we've been asked to serialize a non-nullable
+            // string field that is set to null. Newtonsoft.Json will
+            // successfully serialize it as a JSON null (the unquoted text
+            // null), so we need to check and throw explicitly before that.
+            if (value == null)
+            {
+                throw new NullReferenceException(
+                    "Attempted to serialize a null string. This may indicate a non-nullable string field that was set to null.");
+            }
            writer.WriteValue(value);
        }

--- a/cs/test/core/Core.csproj
+++ b/cs/test/core/Core.csproj
@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <BuildFramework Condition="'$(Configuration)' == 'Net45'">net45</BuildFramework>
@ -39,22 +39,24 @@
    <Compile Include="DeserializerControlsTests.cs" />
    <Compile Include="EnumString.cs" />
    <Compile Include="Equal.cs" />
+    <Compile Include="GenericsTests.cs" />
    <Compile Include="GuidConversionTests.cs" />
    <Compile Include="InterfaceTests.cs" />
    <Compile Include="JsonParsingTests.cs" />
+    <Compile Include="JsonSerializationTests.cs" />
    <Compile Include="MetaInitializationTests.cs" />
    <Compile Include="Partial.cs" />
    <Compile Include="properties\AssemblyInfo.cs" />
+    <Compile Include="ProtocolTests.cs" />
    <Compile Include="Random.cs" />
    <Compile Include="SerializationTests.cs" />
    <Compile Include="SerializerGeneratorFactoryTests.cs" />
-    <Compile Include="GenericsTests.cs" />
    <Compile Include="StreamTests.cs" />
    <Compile Include="Structs.cs" />
    <Compile Include="TypeAliasTests.cs" />
-    <Compile Include="ProtocolTests.cs" />
    <Compile Include="Util.cs" />
-    <Compile Include="XmlTests.cs" />
+    <Compile Include="XmlParsingTests.cs" />
+    <Compile Include="XmlSerializationTests.cs" />
    <BondCodegen Include="Aliases.bond">
      <Options>$(BondOptions) --using="Lazy=Lazy&lt;{0}&gt;" --using="OrderedSet=SortedSet&lt;{0}&gt;" --using="Decimal=decimal" --using="EnumString=Alias.EnumString&lt;{0}&gt;" --using="Array={0}[]" --using=ArrayBlob=byte[] --using="CustomList=UnitTest.Aliases.SomeCustomList&lt;{0}&gt;"</Options>
    </BondCodegen>
@ -73,13 +75,13 @@
    </BondCodegen>
    <BondCodegen Include="import dir with spaces\Bond File With Spaces.bond" />
    <!-- Resharper Workaround -->
-    <Compile Include="$(IntermediateOutputPath)\UnitTest_types.cs" Condition="False" />
-    <Compile Include="$(IntermediateOutputPath)\Containers_types.cs" Condition="False" />
    <Compile Include="$(IntermediateOutputPath)\Aliases_types.cs" Condition="False" />
-    <Compile Include="$(IntermediateOutputPath)\ReadOnly_types.cs" Condition="False" />
+    <Compile Include="$(IntermediateOutputPath)\Bond File With Spaces_types.cs" Condition="False" />
+    <Compile Include="$(IntermediateOutputPath)\Containers_types.cs" Condition="False" />
    <Compile Include="$(IntermediateOutputPath)\NamespaceConflict_types.cs" Condition="False" />
    <Compile Include="$(IntermediateOutputPath)\NamespaceConflictBond_types.cs" Condition="False" />
-    <Compile Include="$(IntermediateOutputPath)\Bond File With Spaces_types.cs" Condition="False" />
+    <Compile Include="$(IntermediateOutputPath)\ReadOnly_types.cs" Condition="False" />
+    <Compile Include="$(IntermediateOutputPath)\UnitTest_types.cs" Condition="False" />
    <!-- End Resharper Workaround -->
  </ItemGroup>
  <ItemGroup>
@ -114,4 +116,4 @@
    <None Include="packages.config" />
  </ItemGroup>
  <Import Project="$(MSBuildThisFileDirectory)\..\..\build\internal\Common.Internal.targets" />
-</Project>
+</Project>
--- a/cs/test/core/JsonSerializationTests.cs
+++ b/cs/test/core/JsonSerializationTests.cs
@ -0,0 +1,26 @@
+namespace UnitTest
+{
+    using System;
+    using System.IO;
+    using Bond;
+    using Bond.Protocols;
+    using NUnit.Framework;
+
+    [TestFixture]
+    class JsonSerializationTests
+    {
+        [Test]
+        public void JsonSerialization_NullNonNullableString_Throws()
+        {
+            var ser = new Serializer<SimpleJsonWriter>(typeof(BasicTypes));
+            var stream = new StringWriter();
+            var jw = new SimpleJsonWriter(stream);
+
+            var nullString = new BasicTypes {_str = null};
+            Assert.Throws<NullReferenceException>(() => ser.Serialize(nullString, jw));
+
+            var nullWString = new BasicTypes {_wstr = null};
+            Assert.Throws<NullReferenceException>(() => ser.Serialize(nullWString, jw));
+        }
+    }
+}
--- a/cs/test/core/XmlParsingTests.cs
+++ b/cs/test/core/XmlParsingTests.cs
@ -10,7 +10,7 @@
    using NUnit.Framework;

    [TestFixture]
-    public class XmlTests
+    public class XmlParsingTests
    {
        static readonly XmlReaderSettings xmlReaderSettings =
            new XmlReaderSettings
@ -602,7 +602,7 @@ World</_str>
        }

        [Test]
-        public void XmlParing_Recursive()
+        public void XmlParsing_Recursive()
        {
            const string xml = @"
 <Tree>
--- a/cs/test/core/XmlSerializationTests.cs
+++ b/cs/test/core/XmlSerializationTests.cs
@ -0,0 +1,26 @@
+namespace UnitTest
+{
+    using System;
+    using System.Text;
+    using System.Xml;
+    using Bond;
+    using Bond.Protocols;
+    using NUnit.Framework;
+
+    [TestFixture]
+    class XmlSerializationTests
+    {
+        [Test]
+        public void XmlSerialization_NullNonNullableString_Throws()
+        {
+            var xmlString = new StringBuilder();
+            var xmlWriter = new SimpleXmlWriter(XmlWriter.Create(xmlString));
+
+            var nullString = new BasicTypes {_str = null};
+            Assert.Throws<NullReferenceException>(() => Serialize.To(xmlWriter, nullString));
+
+            var nullWString = new BasicTypes {_wstr = null};
+            Assert.Throws<NullReferenceException>(() => Serialize.To(xmlWriter, nullWString));
+        }
+    }
+}