From 351fb30982af4f2d967d2216b11268aa821a551b Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Sun, 28 Jul 2019 21:06:03 -0700 Subject: [PATCH] Fix validation check within Utf8JsonWriter for writing a primitive value after another complete value has been written. (dotnet/corefx#39796) * Fix validation check within Utf8JsonWriter for writing a primitive value. * Update the unit test to pass in JsonValueKind as an inlinedata option. * Test cleanup. * Remove use of _isNotPrimitive as that is redundant and unnecessary. * Some more test clean up - validate bytespending moves. * More test clean up - ensure bytespending remains as is when we throw. * Invert check order to rely on short-circuiting. * Address feedback and add comment related tests. Commit migrated from https://github.com/dotnet/corefx/commit/47c35fe385c18d5f0a4ceacea381f790db472ba2 --- .../System.Text.Json/src/Resources/Strings.resx | 6 +- .../src/System/Text/Json/ThrowHelper.cs | 6 +- .../Writer/Utf8JsonWriter.WriteValues.Helpers.cs | 6 +- .../src/System/Text/Json/Writer/Utf8JsonWriter.cs | 14 +- .../System.Text.Json/tests/Utf8JsonWriterTests.cs | 343 ++++++++++++++++++++- 5 files changed, 353 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index b40292d..de53558 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -156,8 +156,8 @@ Cannot write a JSON property name following another property name. A JSON value is missing. - - Cannot write a JSON value after a single JSON value. Current token type is '{0}'. + + Cannot write a JSON value after a single JSON value or outside of an existing closed object/array. Current token type is '{0}'. Cannot write a JSON value within an object without a property name. Current token type is '{0}'. @@ -423,4 +423,4 @@ Either the JSON value is not in a supported format, or is out of bounds for a UInt16. - \ No newline at end of file + diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs index 267cad6..cbab677 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs @@ -496,8 +496,8 @@ namespace System.Text.Json SR.Format(SR.CannotWritePropertyAfterProperty) : SR.Format(SR.CannotWritePropertyWithinArray, tokenType); break; - case ExceptionResource.CannotWriteValueAfterPrimitive: - message = SR.Format(SR.CannotWriteValueAfterPrimitive, tokenType); + case ExceptionResource.CannotWriteValueAfterPrimitiveOrClose: + message = SR.Format(SR.CannotWriteValueAfterPrimitiveOrClose, tokenType); break; default: Debug.Fail($"The ExceptionResource enum value: {resource} is not part of the switch. Add the appropriate case and exception message."); @@ -623,7 +623,7 @@ namespace System.Text.Json CannotStartObjectArrayWithoutProperty, CannotStartObjectArrayAfterPrimitiveOrClose, CannotWriteValueWithinObject, - CannotWriteValueAfterPrimitive, + CannotWriteValueAfterPrimitiveOrClose, CannotWritePropertyWithinArray, ExpectedJsonTokens, TrailingCommaNotAllowedBeforeArrayEnd, diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.Helpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.Helpers.cs index 7afbce4..04f5793 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.Helpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.WriteValues.Helpers.cs @@ -26,9 +26,11 @@ namespace System.Text.Json else { Debug.Assert(_tokenType != JsonTokenType.PropertyName); - if (!_isNotPrimitive && _tokenType != JsonTokenType.None) + + // It is more likely for CurrentDepth to not equal 0 when writing valid JSON, so check that first to rely on short-circuiting and return quickly. + if (CurrentDepth == 0 && _tokenType != JsonTokenType.None) { - ThrowHelper.ThrowInvalidOperationException(ExceptionResource.CannotWriteValueAfterPrimitive, currentDepth: default, token: default, _tokenType); + ThrowHelper.ThrowInvalidOperationException(ExceptionResource.CannotWriteValueAfterPrimitiveOrClose, currentDepth: default, token: default, _tokenType); } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs index 07df584..329eab4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Writer/Utf8JsonWriter.cs @@ -48,7 +48,6 @@ namespace System.Text.Json private Memory _memory; private bool _inObject; - private bool _isNotPrimitive; private JsonTokenType _tokenType; private BitStack _bitStack; @@ -116,7 +115,6 @@ namespace System.Text.Json _memory = default; _inObject = default; - _isNotPrimitive = default; _tokenType = default; _currentDepth = default; _options = options; @@ -152,7 +150,6 @@ namespace System.Text.Json _memory = default; _inObject = default; - _isNotPrimitive = default; _tokenType = default; _currentDepth = default; _options = options; @@ -249,7 +246,6 @@ namespace System.Text.Json _memory = default; _inObject = default; - _isNotPrimitive = default; _tokenType = default; _currentDepth = default; @@ -470,7 +466,6 @@ namespace System.Text.Json _currentDepth &= JsonConstants.RemoveFlagsBitMask; _currentDepth++; - _isNotPrimitive = true; } private void WriteStartMinimized(byte token) @@ -524,7 +519,9 @@ namespace System.Text.Json { Debug.Assert(_tokenType != JsonTokenType.PropertyName); Debug.Assert(_tokenType != JsonTokenType.StartObject); - if (_tokenType != JsonTokenType.None && (!_isNotPrimitive || CurrentDepth == 0)) + + // It is more likely for CurrentDepth to not equal 0 when writing valid JSON, so check that first to rely on short-circuiting and return quickly. + if (CurrentDepth == 0 && _tokenType != JsonTokenType.None) { ThrowHelper.ThrowInvalidOperationException(ExceptionResource.CannotStartObjectArrayAfterPrimitiveOrClose, currentDepth: default, token: default, _tokenType); } @@ -602,7 +599,6 @@ namespace System.Text.Json _currentDepth &= JsonConstants.RemoveFlagsBitMask; _currentDepth++; - _isNotPrimitive = true; } /// @@ -627,7 +623,6 @@ namespace System.Text.Json _currentDepth &= JsonConstants.RemoveFlagsBitMask; _currentDepth++; - _isNotPrimitive = true; _tokenType = JsonTokenType.StartArray; } @@ -653,7 +648,6 @@ namespace System.Text.Json _currentDepth &= JsonConstants.RemoveFlagsBitMask; _currentDepth++; - _isNotPrimitive = true; _tokenType = JsonTokenType.StartObject; } @@ -772,7 +766,6 @@ namespace System.Text.Json _currentDepth &= JsonConstants.RemoveFlagsBitMask; _currentDepth++; - _isNotPrimitive = true; _tokenType = JsonTokenType.StartArray; } @@ -798,7 +791,6 @@ namespace System.Text.Json _currentDepth &= JsonConstants.RemoveFlagsBitMask; _currentDepth++; - _isNotPrimitive = true; _tokenType = JsonTokenType.StartObject; } diff --git a/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs b/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs index eeeb70b..2e290ed 100644 --- a/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs +++ b/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs @@ -12,6 +12,7 @@ using System.IO.Pipelines; using System.Collections.Generic; using System.Text.Encodings.Web; using System.Text.Unicode; +using System.Diagnostics; namespace System.Text.Json.Tests { @@ -858,6 +859,342 @@ namespace System.Text.Json.Tests } [Theory] + [InlineData(JsonValueKind.Array, true, true)] + [InlineData(JsonValueKind.Array, true, false)] + [InlineData(JsonValueKind.Array, false, true)] + [InlineData(JsonValueKind.Array, false, false)] + [InlineData(JsonValueKind.Object, true, true)] + [InlineData(JsonValueKind.Object, true, false)] + [InlineData(JsonValueKind.Object, false, true)] + [InlineData(JsonValueKind.Object, false, false)] + [InlineData(JsonValueKind.String, true, true)] + [InlineData(JsonValueKind.String, true, false)] + [InlineData(JsonValueKind.String, false, true)] + [InlineData(JsonValueKind.String, false, false)] + [InlineData(JsonValueKind.Number, true, true)] + [InlineData(JsonValueKind.Number, true, false)] + [InlineData(JsonValueKind.Number, false, true)] + [InlineData(JsonValueKind.Number, false, false)] + [InlineData(JsonValueKind.True, true, true)] + [InlineData(JsonValueKind.True, true, false)] + [InlineData(JsonValueKind.True, false, true)] + [InlineData(JsonValueKind.True, false, false)] + [InlineData(JsonValueKind.False, true, true)] + [InlineData(JsonValueKind.False, true, false)] + [InlineData(JsonValueKind.False, false, true)] + [InlineData(JsonValueKind.False, false, false)] + [InlineData(JsonValueKind.Null, true, true)] + [InlineData(JsonValueKind.Null, true, false)] + [InlineData(JsonValueKind.Null, false, true)] + [InlineData(JsonValueKind.Null, false, false)] + public void InvalidJsonDueToWritingMultipleValues(JsonValueKind kind, bool formatted, bool skipValidation) + { + var options = new JsonWriterOptions { Indented = formatted, SkipValidation = skipValidation }; + var output = new ArrayBufferWriter(1024); + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteStartObject(), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteStartObject("foo"), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteStartArray(), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteEndObject(), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteEndArray(), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WritePropertyName("foo"), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteString("key", "foo"), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteStringValue("foo"), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteNumber("key", 123), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteNumberValue(123), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteBoolean("key", true), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteBooleanValue(true), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteBoolean("key", false), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteBooleanValue(false), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteNull("key"), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteNullValue(), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind); + // Writing a comment after any preamable is valid (even when skipValidation is false) + ValidateAction(jsonUtf8, () => jsonUtf8.WriteCommentValue("some comment"), skipValidation: true); + } + } + + [Theory] + [InlineData(JsonValueKind.Array, true, true)] + [InlineData(JsonValueKind.Array, true, false)] + [InlineData(JsonValueKind.Array, false, true)] + [InlineData(JsonValueKind.Array, false, false)] + [InlineData(JsonValueKind.Object, true, true)] + [InlineData(JsonValueKind.Object, true, false)] + [InlineData(JsonValueKind.Object, false, true)] + [InlineData(JsonValueKind.Object, false, false)] + [InlineData(JsonValueKind.String, true, true)] + [InlineData(JsonValueKind.String, true, false)] + [InlineData(JsonValueKind.String, false, true)] + [InlineData(JsonValueKind.String, false, false)] + [InlineData(JsonValueKind.Number, true, true)] + [InlineData(JsonValueKind.Number, true, false)] + [InlineData(JsonValueKind.Number, false, true)] + [InlineData(JsonValueKind.Number, false, false)] + [InlineData(JsonValueKind.True, true, true)] + [InlineData(JsonValueKind.True, true, false)] + [InlineData(JsonValueKind.True, false, true)] + [InlineData(JsonValueKind.True, false, false)] + [InlineData(JsonValueKind.False, true, true)] + [InlineData(JsonValueKind.False, true, false)] + [InlineData(JsonValueKind.False, false, true)] + [InlineData(JsonValueKind.False, false, false)] + [InlineData(JsonValueKind.Null, true, true)] + [InlineData(JsonValueKind.Null, true, false)] + [InlineData(JsonValueKind.Null, false, true)] + [InlineData(JsonValueKind.Null, false, false)] + public void InvalidJsonDueToWritingMultipleValuesWithComments(JsonValueKind kind, bool formatted, bool skipValidation) + { + var options = new JsonWriterOptions { Indented = formatted, SkipValidation = skipValidation }; + var output = new ArrayBufferWriter(1024); + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteStartObject(), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteStartObject("foo"), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteStartArray(), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteEndObject(), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteEndArray(), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WritePropertyName("foo"), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteString("key", "foo"), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteStringValue("foo"), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteNumber("key", 123), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteNumberValue(123), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteBoolean("key", true), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteBooleanValue(true), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteBoolean("key", false), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteBooleanValue(false), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteNull("key"), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + ValidateAction(jsonUtf8, () => jsonUtf8.WriteNullValue(), skipValidation); + } + + using (var jsonUtf8 = new Utf8JsonWriter(output, options)) + { + WritePreamble(jsonUtf8, kind, addComments: true); + // Writing a comment after any preamable is valid (even when skipValidation is false) + ValidateAction(jsonUtf8, () => jsonUtf8.WriteCommentValue("some comment"), skipValidation: true); + } + } + + private void WritePreamble(Utf8JsonWriter writer, JsonValueKind kind, bool addComments = false) + { + Debug.Assert(writer.BytesCommitted == 0 && writer.BytesPending == 0 && writer.CurrentDepth == 0 && kind != JsonValueKind.Undefined); + + if (addComments) + { + writer.WriteCommentValue(" comment value before "); + } + + switch (kind) + { + case JsonValueKind.Object: + writer.WriteStartObject(); + writer.WriteEndObject(); + break; + case JsonValueKind.Array: + writer.WriteStartArray(); + writer.WriteEndArray(); + break; + case JsonValueKind.String: + writer.WriteStringValue("foo"); + break; + case JsonValueKind.Number: + writer.WriteNumberValue(1); + break; + case JsonValueKind.True: + writer.WriteBooleanValue(true); + break; + case JsonValueKind.False: + writer.WriteBooleanValue(false); + break; + case JsonValueKind.Null: + writer.WriteNullValue(); + break; + default: + Debug.Fail($"Invalid JsonValueKind passed in '{kind}'."); + break; + } + + if (addComments) + { + writer.WriteCommentValue(" comment value after "); + } + } + + private void ValidateAction(Utf8JsonWriter writer, Action action, bool skipValidation) + { + int originalBytesPending = writer.BytesPending; + if (skipValidation) + { + action.Invoke(); + Assert.NotEqual(originalBytesPending, writer.BytesPending); + } + else + { + Assert.Throws(action); + Assert.Equal(originalBytesPending, writer.BytesPending); + } + } + + [Theory] [InlineData(true, true)] [InlineData(true, false)] [InlineData(false, true)] @@ -5295,9 +5632,9 @@ namespace System.Text.Json.Tests ReadOnlySpan nullStringSpan = nullString.AsSpan(); charSpanAction(writer, nullStringSpan, value); - + byteSpanAction(writer, ReadOnlySpan.Empty, value); - + writer.WriteEndObject(); writer.Flush(); } @@ -5385,7 +5722,7 @@ namespace System.Text.Json.Tests { var output = new ArrayBufferWriter(1024); string nullString = null; - + using (var writer = new Utf8JsonWriter(output)) { writer.WriteStartArray(); -- 2.7.4