From 4892284ddb91b2b6df7f9eabe39f7246bf0fd1a3 Mon Sep 17 00:00:00 2001 From: Joe Amenta Date: Wed, 24 Apr 2024 15:32:25 -0400 Subject: [PATCH 1/4] This should fix it... needs a test or three, though. --- .../Converters/StjFeatureCollectionConverter.cs | 2 +- .../Converters/StjFeatureConverter.cs | 2 +- .../Converters/StjGeometryConverter.cs | 4 ++-- .../Converters/Utility.cs | 13 +++++++++++++ .../Properties/Resources.Designer.cs | 9 +++++++++ .../Properties/Resources.resx | 5 ++++- 6 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjFeatureCollectionConverter.cs b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjFeatureCollectionConverter.cs index 514070a..828ec78 100644 --- a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjFeatureCollectionConverter.cs +++ b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjFeatureCollectionConverter.cs @@ -65,7 +65,7 @@ public override FeatureCollection Read(ref Utf8JsonReader reader, Type objectTyp else { reader.ReadOrThrow(); - reader.Skip(); + reader.SkipOrThrow(); reader.ReadOrThrow(); } } diff --git a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjFeatureConverter.cs b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjFeatureConverter.cs index 5c2bcda..df1e947 100644 --- a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjFeatureConverter.cs +++ b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjFeatureConverter.cs @@ -162,7 +162,7 @@ public override IFeature Read(ref Utf8JsonReader reader, Type objectType, JsonSe default: // If property name is not one of the above: skip it entirely (foreign member) - reader.Skip(); + reader.SkipOrThrow(); // Advance while (reader.Read()) { diff --git a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjGeometryConverter.cs b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjGeometryConverter.cs index e5d4c1a..eba3089 100644 --- a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjGeometryConverter.cs +++ b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjGeometryConverter.cs @@ -96,7 +96,7 @@ public override Geometry Read(ref Utf8JsonReader reader, Type typeToConvert, Jso break; default: // included "bbox" property //read, but can't do anything with it (see NetTopologySuite.IO.GeoJSON => NetTopologySuite.IO.Converters.GeometryConverter.ParseGeometry) - reader.Skip(); + reader.SkipOrThrow(); reader.Read(); break; } @@ -248,7 +248,7 @@ public override void Write(Utf8JsonWriter writer, Geometry value, JsonSerializer private void WritePolygon(Utf8JsonWriter writer, Polygon value, JsonSerializerOptions options) { - + writer.WriteStartArray(); WriteCoordinateSequence(writer, value.ExteriorRing.CoordinateSequence, options, orientation:_oriExterior); for (int i = 0; i < value.NumInteriorRings; i++) diff --git a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/Utility.cs b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/Utility.cs index 52829e0..254915c 100644 --- a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/Utility.cs +++ b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/Utility.cs @@ -61,6 +61,15 @@ internal static void AssertToken(this ref Utf8JsonReader reader, JsonTokenType r } } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal static void SkipOrThrow(this ref Utf8JsonReader reader) + { + if (!reader.TrySkip()) + { + ThrowForUnexpectedPartialJson(); + } + } + internal static object ObjectFromJsonNode(JsonNode node, JsonSerializerOptions serializerOptions) { switch (node) @@ -93,6 +102,10 @@ internal static JsonNode ObjectToJsonNode(object obj, JsonSerializerOptions seri private static void ThrowForUnexpectedEndOfStream() => throw new JsonException(Resources.EX_UnexpectedEndOfStream); + [MethodImpl(MethodImplOptions.NoInlining)] + private static void ThrowForUnexpectedPartialJson() + => throw new JsonException(Resources.EX_UnexpectedPartialJson); + [MethodImpl(MethodImplOptions.NoInlining)] private static void ThrowForUnexpectedToken(JsonTokenType requiredNextTokenType, ref Utf8JsonReader reader) => throw new JsonException(string.Format(Resources.EX_UnexpectedToken, requiredNextTokenType, reader.TokenType, CurrentTokenAsString(in reader))); diff --git a/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.Designer.cs b/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.Designer.cs index 46224fc..36acb56 100644 --- a/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.Designer.cs +++ b/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.Designer.cs @@ -131,5 +131,14 @@ internal static string EX_UnexpectedToken { return ResourceManager.GetString("EX_UnexpectedToken", resourceCulture); } } + + /// + /// Looks up a localized string similar to JsonConverter received partial JSON. This is likely the result of a bug in the System.Text.Json library.. + /// + internal static string EX_UnexpectedPartialJson { + get { + return ResourceManager.GetString("EX_UnexpectedPartialJson", resourceCulture); + } + } } } diff --git a/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.resx b/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.resx index d0f48e0..ca2b5de 100644 --- a/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.resx +++ b/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.resx @@ -141,4 +141,7 @@ Expected token is '{0}' but was '{1}' (Value '{2}'). - \ No newline at end of file + + JsonConverter received partial JSON. This is likely the result of a bug in the System.Text.Json library. + + From c581d9b9a86fce7d168ec7739e7ba2db7aa526aa Mon Sep 17 00:00:00 2001 From: Joe Amenta Date: Wed, 24 Apr 2024 15:51:51 -0400 Subject: [PATCH 2/4] add some tests that pass with the fix and fail without the fix --- .../FeatureCollectionConverterTest.cs | 17 ++++++++++ .../Converters/FeatureConverterTest.cs | 18 ++++++++++- .../Converters/GeometryConverterTest.cs | 16 ++++++++++ .../SingleByteReadingMemoryStream.cs | 32 +++++++++++++++++++ 4 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 test/NetTopologySuite.IO.GeoJSON4STJ.Test/SingleByteReadingMemoryStream.cs diff --git a/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureCollectionConverterTest.cs b/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureCollectionConverterTest.cs index ef55457..1a1b323 100644 --- a/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureCollectionConverterTest.cs +++ b/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureCollectionConverterTest.cs @@ -1,4 +1,6 @@ using System; +using System.Linq; +using System.Text.Json; using System.Text.Json.Serialization; using NetTopologySuite.Features; using NetTopologySuite.Geometries; @@ -68,5 +70,20 @@ public void TestSanD(OgcGeometryType type, int num, bool threeD) for (int i = 0; i < fc.Count; i++) FeatureConverterTest.CheckEquality(fc[i], d[i]); } + + [Test] + [GeoJsonIssueNumber(143)] + public void SkippedPropertiesShouldNotThrowWithCompleteObjectInPartialBuffer() + { + var options = DefaultOptions; + var node = JsonSerializer.SerializeToNode(new FeatureCollection(), options)!; + node["_skippedProperty"] = "irrelevant"; + var nodes = Enumerable.Repeat(node, 500).ToArray(); + using SingleByteReadingMemoryStream stream = new(); + JsonSerializer.Serialize(stream, nodes, options); + stream.Position = 0; + var roundtrip = JsonSerializer.Deserialize(stream, options); + Assert.That(roundtrip, Has.Length.EqualTo(500)); + } } } diff --git a/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureConverterTest.cs b/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureConverterTest.cs index 7dbc9e3..c0affb9 100644 --- a/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureConverterTest.cs +++ b/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureConverterTest.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Text.Json; using System.Text.Json.Serialization; @@ -128,6 +129,21 @@ public void WriteJsonWithIdTest(string idPropertyName, object id) CheckEquality(value, deserialized, idPropertyName); } + [Test] + [GeoJsonIssueNumber(143)] + public void SkippedPropertiesShouldNotThrowWithCompleteObjectInPartialBuffer() + { + var options = DefaultOptions; + var node = JsonSerializer.SerializeToNode(new Feature(), options)!; + node["_skippedProperty"] = "irrelevant"; + var nodes = Enumerable.Repeat(node, 500).ToArray(); + using SingleByteReadingMemoryStream stream = new(); + JsonSerializer.Serialize(stream, nodes, options); + stream.Position = 0; + var roundtrip = JsonSerializer.Deserialize(stream, options); + Assert.That(roundtrip, Has.Length.EqualTo(500)); + } + public static IEnumerable FeatureIdTestCases { get @@ -244,7 +260,7 @@ public void TestNumericFeatureIdMustBeValidDecimal() }} }} "; - + Assert.That(() => JsonSerializer.Deserialize(serialized, DefaultOptions), Throws.InstanceOf()); } } diff --git a/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/GeometryConverterTest.cs b/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/GeometryConverterTest.cs index 8a1ddf4..efbb1e0 100644 --- a/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/GeometryConverterTest.cs +++ b/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/GeometryConverterTest.cs @@ -1,4 +1,5 @@ using System.IO; +using System.Linq; using System.Text.Json; using NetTopologySuite.Geometries; using NUnit.Framework; @@ -184,5 +185,20 @@ public void TestWriteReadWkt(string wkt) Assert.That(geomS.IsEmpty ? geomD.IsEmpty : geomS.EqualsTopologically(geomD)); } + + [Test] + [GeoJsonIssueNumber(143)] + public void SkippedPropertiesShouldNotThrowWithCompleteObjectInPartialBuffer() + { + var options = DefaultOptions; + var node = JsonSerializer.SerializeToNode(Point.Empty, options)!; + node["_skippedProperty"] = "irrelevant"; + var nodes = Enumerable.Repeat(node, 500).ToArray(); + using SingleByteReadingMemoryStream stream = new(); + JsonSerializer.Serialize(stream, nodes, options); + stream.Position = 0; + var roundtrip = JsonSerializer.Deserialize(stream, options); + Assert.That(roundtrip, Has.Length.EqualTo(500)); + } } } diff --git a/test/NetTopologySuite.IO.GeoJSON4STJ.Test/SingleByteReadingMemoryStream.cs b/test/NetTopologySuite.IO.GeoJSON4STJ.Test/SingleByteReadingMemoryStream.cs new file mode 100644 index 0000000..c132e65 --- /dev/null +++ b/test/NetTopologySuite.IO.GeoJSON4STJ.Test/SingleByteReadingMemoryStream.cs @@ -0,0 +1,32 @@ +using System; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +namespace NetTopologySuite.IO.GeoJSON4STJ.Test; + +/// +/// A implementation that will never read more than one byte at a time. +/// +internal sealed class SingleByteReadingMemoryStream : MemoryStream +{ + public override int Read(byte[] buffer, int offset, int count) + { + return base.Read(buffer, offset, Math.Min(1, count)); + } + + public override int Read(Span buffer) + { + return base.Read(buffer[.. Math.Min(1, buffer.Length)]); + } + + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + return base.ReadAsync(buffer, offset, Math.Min(1, count), cancellationToken); + } + + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + { + return base.ReadAsync(buffer[.. Math.Min(1, buffer.Length)], cancellationToken); + } +} From bbd3fa1eb37d868ea86ac29c36a7b1a49c61f99a Mon Sep 17 00:00:00 2001 From: Joe Amenta Date: Wed, 24 Apr 2024 15:58:30 -0400 Subject: [PATCH 3/4] try to remove some whitespace-only changes --- .../Converters/StjGeometryConverter.cs | 2 +- src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.resx | 2 +- .../Converters/FeatureConverterTest.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjGeometryConverter.cs b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjGeometryConverter.cs index eba3089..a1304d7 100644 --- a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjGeometryConverter.cs +++ b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/StjGeometryConverter.cs @@ -248,7 +248,7 @@ public override void Write(Utf8JsonWriter writer, Geometry value, JsonSerializer private void WritePolygon(Utf8JsonWriter writer, Polygon value, JsonSerializerOptions options) { - + writer.WriteStartArray(); WriteCoordinateSequence(writer, value.ExteriorRing.CoordinateSequence, options, orientation:_oriExterior); for (int i = 0; i < value.NumInteriorRings; i++) diff --git a/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.resx b/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.resx index ca2b5de..163a1bd 100644 --- a/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.resx +++ b/src/NetTopologySuite.IO.GeoJSON4STJ/Properties/Resources.resx @@ -144,4 +144,4 @@ JsonConverter received partial JSON. This is likely the result of a bug in the System.Text.Json library. - + \ No newline at end of file diff --git a/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureConverterTest.cs b/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureConverterTest.cs index c0affb9..d18c9b8 100644 --- a/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureConverterTest.cs +++ b/test/NetTopologySuite.IO.GeoJSON4STJ.Test/Converters/FeatureConverterTest.cs @@ -260,7 +260,7 @@ public void TestNumericFeatureIdMustBeValidDecimal() }} }} "; - + Assert.That(() => JsonSerializer.Deserialize(serialized, DefaultOptions), Throws.InstanceOf()); } } From 0a570c1bc0675aba0d2a5f96447c9d65ab2351ed Mon Sep 17 00:00:00 2001 From: Joe Amenta Date: Wed, 24 Apr 2024 16:08:21 -0400 Subject: [PATCH 4/4] add a comment explaining why we do this instead of Skip() (IMO, Skip() really should have been implemented this way, but whatever) --- src/NetTopologySuite.IO.GeoJSON4STJ/Converters/Utility.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/Utility.cs b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/Utility.cs index 254915c..08c1b02 100644 --- a/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/Utility.cs +++ b/src/NetTopologySuite.IO.GeoJSON4STJ/Converters/Utility.cs @@ -64,6 +64,12 @@ internal static void AssertToken(this ref Utf8JsonReader reader, JsonTokenType r [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static void SkipOrThrow(this ref Utf8JsonReader reader) { + // #143: Even though JsonConverter always seems to see a Utf8JsonReader positioned on + // a full node, that node might be contained in a larger incomplete block. For whatever + // reason, reader.Skip() throws JsonException immediately if the reader is in a partial + // block, even if the current block has plenty of data to read past it. TrySkip will do + // the right thing, but we should still check the return value to make sure that we get + // a JsonException if the reader *does* terminate abruptly (airbreather 2024-04-24). if (!reader.TrySkip()) { ThrowForUnexpectedPartialJson();