From fe3135cc4a67901e026fec27318b39741ac71e9b Mon Sep 17 00:00:00 2001 From: Ted Stein Date: Wed, 28 Jun 2017 14:35:56 -0700 Subject: [PATCH] [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 --- CHANGELOG.md | 3 ++ cs/src/core/protocols/SimpleXmlWriter.cs | 20 ++++++++++++ cs/src/json/protocols/SimpleJsonWriter.cs | 32 +++++++++++++++---- cs/test/core/Core.csproj | 20 ++++++------ cs/test/core/JsonSerializationTests.cs | 26 +++++++++++++++ .../core/{XmlTests.cs => XmlParsingTests.cs} | 4 +-- cs/test/core/XmlSerializationTests.cs | 26 +++++++++++++++ 7 files changed, 114 insertions(+), 17 deletions(-) create mode 100644 cs/test/core/JsonSerializationTests.cs rename cs/test/core/{XmlTests.cs => XmlParsingTests.cs} (99%) create mode 100644 cs/test/core/XmlSerializationTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e444fb4..81b2cb25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,6 +102,9 @@ get a compiler error. To fix, remove the `` 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 ## diff --git a/cs/src/core/protocols/SimpleXmlWriter.cs b/cs/src/core/protocols/SimpleXmlWriter.cs index f72dc873..38bb99a3 100644 --- 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 diff --git a/cs/src/json/protocols/SimpleJsonWriter.cs b/cs/src/json/protocols/SimpleJsonWriter.cs index 67adadd7..8d0e7e25 100644 --- 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); } diff --git a/cs/test/core/Core.csproj b/cs/test/core/Core.csproj index b2551fe4..3be7e3c4 100644 --- a/cs/test/core/Core.csproj +++ b/cs/test/core/Core.csproj @@ -1,4 +1,4 @@ - + net45 @@ -39,22 +39,24 @@ + + + - - - + + $(BondOptions) --using="Lazy=Lazy<{0}>" --using="OrderedSet=SortedSet<{0}>" --using="Decimal=decimal" --using="EnumString=Alias.EnumString<{0}>" --using="Array={0}[]" --using=ArrayBlob=byte[] --using="CustomList=UnitTest.Aliases.SomeCustomList<{0}>" @@ -73,13 +75,13 @@ - - - + + - + + @@ -114,4 +116,4 @@ - \ No newline at end of file + diff --git a/cs/test/core/JsonSerializationTests.cs b/cs/test/core/JsonSerializationTests.cs new file mode 100644 index 00000000..a886931c --- /dev/null +++ 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(typeof(BasicTypes)); + var stream = new StringWriter(); + var jw = new SimpleJsonWriter(stream); + + var nullString = new BasicTypes {_str = null}; + Assert.Throws(() => ser.Serialize(nullString, jw)); + + var nullWString = new BasicTypes {_wstr = null}; + Assert.Throws(() => ser.Serialize(nullWString, jw)); + } + } +} diff --git a/cs/test/core/XmlTests.cs b/cs/test/core/XmlParsingTests.cs similarity index 99% rename from cs/test/core/XmlTests.cs rename to cs/test/core/XmlParsingTests.cs index 3adac317..bd7774de 100644 --- a/cs/test/core/XmlTests.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 } [Test] - public void XmlParing_Recursive() + public void XmlParsing_Recursive() { const string xml = @" diff --git a/cs/test/core/XmlSerializationTests.cs b/cs/test/core/XmlSerializationTests.cs new file mode 100644 index 00000000..1b969f30 --- /dev/null +++ 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(() => Serialize.To(xmlWriter, nullString)); + + var nullWString = new BasicTypes {_wstr = null}; + Assert.Throws(() => Serialize.To(xmlWriter, nullWString)); + } + } +}