Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make GetPropertyCount public and fix its return value #106503

Merged
merged 7 commits into from
Aug 17, 2024
Merged

Make GetPropertyCount public and fix its return value #106503

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
45818a8
Make GetPropertyCount public and fix its return value
etemi Aug 15, 2024
0c6fc09
Change order of assignments to match array counterpart
etemi Aug 16, 2024
ea276ea
Reuse local variable
etemi Aug 16, 2024
eebfb1a
Merge branch 'main' into issue-104692
etemi Aug 16, 2024
77a6f19
Show that GetPropertyCount() and EnumerateObject().Count() are the sa…
etemi Aug 16, 2024
2b0d0fb
Fix test to also check objects inside arrays
etemi Aug 16, 2024
baca560
Merge branch 'main' into issue-104692
etemi Aug 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libraries/System.Text.Json/ref/System.Text.Json.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ public readonly partial struct JsonElement
public System.Text.Json.JsonElement GetProperty(System.ReadOnlySpan<byte> utf8PropertyName) { throw null; }
public System.Text.Json.JsonElement GetProperty(System.ReadOnlySpan<char> propertyName) { throw null; }
public System.Text.Json.JsonElement GetProperty(string propertyName) { throw null; }
public int GetPropertyCount() { throw null; }
public string GetRawText() { throw null; }
[System.CLSCompliantAttribute(false)]
public sbyte GetSByte() { throw null; }
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.MetadataDb.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public sealed partial class JsonDocument
// * 31 bits for token offset
// * Second int
// * Top bit is unassigned / always clear
// * 31 bits for the token length (always 1, effectively unassigned)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might make sense to validate if this comment is consistent with the current implementation, just to make sure we're not accidentally regressing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: For me it seemed that the comment was wrong because previously it wasn't always 1 for StartObject. Instead it was set to the same value as the third int (number of rows until the next value). See first and last line here:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs

Lines 990 to 994 in fc7a8e6

database.SetLength(rowIndex, numberOfRowsForMembers);
int newRowIndex = database.Length;
database.Append(tokenType, tokenStart, reader.ValueSpan.Length);
database.SetNumberOfRows(rowIndex, numberOfRowsForMembers);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the comment was always wrong (at least, was wrong in the initial merge, maybe it was right when I wrote it during feature development).

Nothing noticed or cared that the value was wrong, because nothing read the value until DeepEquals was added... and I guess it didn't notice that it was wrong, because it was the "same wrong" when two things were equal.

Copy link
Member

@eiriktsarpalis eiriktsarpalis Aug 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to decide if this means that we should backport the change to 9. Is it possible for the original values (number of rows) to be equal when the number of properties are not equal? Even if that wouldn't impact functional correctness, it would prompt the comparer to recurse into a more expensive property-wise comparison for differently-sized objects.

Copy link
Contributor Author

@etemi etemi Aug 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to have equal number of rows when the number of properties are not equal:

  • Example 1

    10 rows and 2 properties:

    { "a": [1,2,3,4], "b": 4 }

    10 rows but 3 properties:

    { "a": [1,2], "b": 3, "c": 4 }

  • Example 2

    7 rows and 1 property:

    { "a": [1,2,3] }

    7 rows and 3 properties:

    { "a": 1, "b": 2, "c": true }

But I would say it is very unlikely that two unequal values accidentally have the same number of rows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think we should try to backport this.

// * 31 bits for the number of properties in this object
// * Third int
// * 4 bits JsonTokenType
// * 28 bits for the number of rows until the next value (never 0)
Expand Down
27 changes: 15 additions & 12 deletions src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,7 @@ private static void Parse(
ref StackRowStack stack)
{
bool inArray = false;
int arrayItemsCount = 0;
int arrayItemsOrPropertyCount = 0;
int numberOfRowsForMembers = 0;
int numberOfRowsForValues = 0;

Expand All @@ -972,13 +972,14 @@ private static void Parse(
{
if (inArray)
{
arrayItemsCount++;
arrayItemsOrPropertyCount++;
}

numberOfRowsForValues++;
database.Append(tokenType, tokenStart, DbRow.UnknownSize);
var row = new StackRow(numberOfRowsForMembers + 1);
var row = new StackRow(arrayItemsOrPropertyCount, numberOfRowsForMembers + 1);
stack.Push(row);
arrayItemsOrPropertyCount = 0;
numberOfRowsForMembers = 0;
}
else if (tokenType == JsonTokenType.EndObject)
Expand All @@ -987,28 +988,29 @@ private static void Parse(

numberOfRowsForValues++;
numberOfRowsForMembers++;
database.SetLength(rowIndex, numberOfRowsForMembers);
database.SetLength(rowIndex, arrayItemsOrPropertyCount);

int newRowIndex = database.Length;
database.Append(tokenType, tokenStart, reader.ValueSpan.Length);
database.SetNumberOfRows(rowIndex, numberOfRowsForMembers);
database.SetNumberOfRows(newRowIndex, numberOfRowsForMembers);

StackRow row = stack.Pop();
numberOfRowsForMembers += row.SizeOrLength;
arrayItemsOrPropertyCount = row.SizeOrLength;
numberOfRowsForMembers += row.NumberOfRows;
}
else if (tokenType == JsonTokenType.StartArray)
{
if (inArray)
{
arrayItemsCount++;
arrayItemsOrPropertyCount++;
}

numberOfRowsForMembers++;
database.Append(tokenType, tokenStart, DbRow.UnknownSize);
var row = new StackRow(arrayItemsCount, numberOfRowsForValues + 1);
var row = new StackRow(arrayItemsOrPropertyCount, numberOfRowsForValues + 1);
stack.Push(row);
arrayItemsCount = 0;
arrayItemsOrPropertyCount = 0;
numberOfRowsForValues = 0;
}
else if (tokenType == JsonTokenType.EndArray)
Expand All @@ -1017,7 +1019,7 @@ private static void Parse(

numberOfRowsForValues++;
numberOfRowsForMembers++;
database.SetLength(rowIndex, arrayItemsCount);
database.SetLength(rowIndex, arrayItemsOrPropertyCount);
database.SetNumberOfRows(rowIndex, numberOfRowsForValues);

// If the array item count is (e.g.) 12 and the number of rows is (e.g.) 13
Expand All @@ -1030,7 +1032,7 @@ private static void Parse(
// This check is similar to tracking the start array and painting it when
// StartObject or StartArray is encountered, but avoids the mixed state
// where "UnknownSize" implies "has complex children".
if (arrayItemsCount + 1 != numberOfRowsForValues)
if (arrayItemsOrPropertyCount + 1 != numberOfRowsForValues)
{
database.SetHasComplexChildren(rowIndex);
}
Expand All @@ -1040,13 +1042,14 @@ private static void Parse(
database.SetNumberOfRows(newRowIndex, numberOfRowsForValues);

StackRow row = stack.Pop();
arrayItemsCount = row.SizeOrLength;
arrayItemsOrPropertyCount = row.SizeOrLength;
numberOfRowsForValues += row.NumberOfRows;
}
else if (tokenType == JsonTokenType.PropertyName)
{
numberOfRowsForValues++;
numberOfRowsForMembers++;
arrayItemsOrPropertyCount++;

// Adding 1 to skip the start quote will never overflow
Debug.Assert(tokenStart < int.MaxValue);
Expand All @@ -1068,7 +1071,7 @@ private static void Parse(

if (inArray)
{
arrayItemsCount++;
arrayItemsOrPropertyCount++;
}

if (tokenType == JsonTokenType.String)
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public int GetArrayLength()
/// <exception cref="ObjectDisposedException">
/// The parent <see cref="JsonDocument"/> has been disposed.
/// </exception>
internal int GetPropertyCount()
public int GetPropertyCount()
{
CheckValidInstance();

Expand Down
49 changes: 49 additions & 0 deletions src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonDocumentTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,7 @@ public static void ParseSimpleObject()
string city = parsedObject.GetProperty("city").GetString();
int zip = parsedObject.GetProperty("zip").GetInt32();

Assert.Equal(7, parsedObject.GetPropertyCount());
Assert.True(parsedObject.TryGetProperty("age", out JsonElement age2));
Assert.Equal(30, age2.GetInt32());

Expand All @@ -704,6 +705,7 @@ public static void ParseNestedJson()

Assert.Equal(1, parsedObject.GetArrayLength());
JsonElement person = parsedObject[0];
Assert.Equal(5, person.GetPropertyCount());
double age = person.GetProperty("age").GetDouble();
string first = person.GetProperty("first").GetString();
string last = person.GetProperty("last").GetString();
Expand Down Expand Up @@ -902,6 +904,7 @@ public static void ReadNumber_1Byte(sbyte value)
Assert.Throws<InvalidOperationException>(() => root.GetDateTimeOffset());
Assert.Throws<InvalidOperationException>(() => root.GetGuid());
Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetBoolean());
Expand Down Expand Up @@ -999,6 +1002,7 @@ public static void ReadNumber_2Bytes(short value)
Assert.Throws<InvalidOperationException>(() => root.GetDateTimeOffset());
Assert.Throws<InvalidOperationException>(() => root.GetGuid());
Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetBoolean());
Expand Down Expand Up @@ -1091,6 +1095,7 @@ public static void ReadSmallInteger(int value)
Assert.Throws<InvalidOperationException>(() => root.GetDateTimeOffset());
Assert.Throws<InvalidOperationException>(() => root.GetGuid());
Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetBoolean());
Expand Down Expand Up @@ -1194,6 +1199,7 @@ public static void ReadMediumInteger(long value)
Assert.Throws<InvalidOperationException>(() => root.GetDateTimeOffset());
Assert.Throws<InvalidOperationException>(() => root.GetGuid());
Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetBoolean());
Expand Down Expand Up @@ -1267,6 +1273,7 @@ public static void ReadLargeInteger(ulong value)
Assert.Throws<InvalidOperationException>(() => root.GetDateTimeOffset());
Assert.Throws<InvalidOperationException>(() => root.GetGuid());
Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetBoolean());
Expand Down Expand Up @@ -1340,6 +1347,7 @@ public static void ReadTooLargeInteger()
Assert.Throws<InvalidOperationException>(() => root.GetDateTimeOffset());
Assert.Throws<InvalidOperationException>(() => root.GetGuid());
Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetBoolean());
Expand Down Expand Up @@ -1379,6 +1387,7 @@ public static void ReadDateTimeAndDateTimeOffset(string jsonString, string expec
Assert.Throws<InvalidOperationException>(() => root.GetInt64());
Assert.Throws<InvalidOperationException>(() => root.GetUInt64());
Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetBoolean());
Expand Down Expand Up @@ -1462,6 +1471,7 @@ public static void ReadGuid(string jsonString, string expectedStr)
Assert.Throws<InvalidOperationException>(() => root.GetInt64());
Assert.Throws<InvalidOperationException>(() => root.GetUInt64());
Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetBoolean());
Expand Down Expand Up @@ -1564,6 +1574,7 @@ public static void ReadNonInteger(string str, double expectedDouble, float expec
Assert.Throws<InvalidOperationException>(() => root.GetDateTimeOffset());
Assert.Throws<InvalidOperationException>(() => root.GetGuid());
Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetBoolean());
Expand Down Expand Up @@ -1654,6 +1665,7 @@ public static void ReadTooPreciseDouble()
Assert.Throws<InvalidOperationException>(() => root.GetDateTimeOffset());
Assert.Throws<InvalidOperationException>(() => root.GetGuid());
Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetBoolean());
Expand Down Expand Up @@ -1735,6 +1747,7 @@ public static void CheckUseAfterDispose()

Assert.Throws<ObjectDisposedException>(() => root.ValueKind);
Assert.Throws<ObjectDisposedException>(() => root.GetArrayLength());
Assert.Throws<ObjectDisposedException>(() => root.GetPropertyCount());
Assert.Throws<ObjectDisposedException>(() => root.EnumerateArray());
Assert.Throws<ObjectDisposedException>(() => root.EnumerateObject());
Assert.Throws<ObjectDisposedException>(() => root.GetDouble());
Expand Down Expand Up @@ -1793,6 +1806,7 @@ public static void CheckUseDefault()
Assert.Equal(JsonValueKind.Undefined, root.ValueKind);

Assert.Throws<InvalidOperationException>(() => root.GetArrayLength());
Assert.Throws<InvalidOperationException>(() => root.GetPropertyCount());
Assert.Throws<InvalidOperationException>(() => root.EnumerateArray());
Assert.Throws<InvalidOperationException>(() => root.EnumerateObject());
Assert.Throws<InvalidOperationException>(() => root.GetDouble());
Expand Down Expand Up @@ -2442,13 +2456,15 @@ public static void GetRawText()

using (JsonDocument doc = JsonDocument.Parse(json))
{
Assert.Equal(6, doc.RootElement.GetPropertyCount());
JsonElement.ObjectEnumerator enumerator = doc.RootElement.EnumerateObject();
Assert.True(enumerator.MoveNext(), "Move to first property");
JsonProperty property = enumerator.Current;

Assert.Equal(" weird property name", property.Name);
string rawText = property.ToString();
int crCount = rawText.Count(c => c == '\r');
Assert.Equal(2, property.Value.GetPropertyCount());
Assert.Equal(128 + crCount, rawText.Length);
Assert.Equal('\"', rawText[0]);
Assert.Equal(' ', rawText[1]);
Expand Down Expand Up @@ -3437,6 +3453,39 @@ public static void ParseValue_AllowMultipleValues_TrailingContent()
JsonTestHelper.AssertThrows<JsonException>(ref reader, (ref Utf8JsonReader reader) => reader.Read());
}

[Theory]
[InlineData("""{ "foo" : [1], "test": false, "bar" : { "nested": 3 } }""", 3)]
[InlineData("""{ "foo" : [1,2,3,4] }""", 1)]
[InlineData("""{}""", 0)]
[InlineData("""{ "foo" : {"nested:" : {"nested": 1, "bla": [1, 2, {"bla": 3}] } }, "test": true, "foo2" : {"nested:" : {"nested": 1, "bla": [1, 2, {"bla": 3}] } }}""", 3)]
public static void TestGetPropertyCount(string json, int expectedCount)
{
JsonElement element = JsonSerializer.Deserialize<JsonElement>(json);
Assert.Equal(expectedCount, element.GetPropertyCount());
}

eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
[Fact]
public static void VerifyGetPropertyCountUsingEnumerateObject()
{
using (JsonDocument doc = JsonDocument.Parse(SR.ProjectLockJson))
{
CheckPropertyCountAgainstEnumerateObject(doc.RootElement);
}

void CheckPropertyCountAgainstEnumerateObject(JsonElement obj)
{
Assert.Equal(obj.EnumerateObject().Count(), obj.GetPropertyCount());

foreach (JsonProperty prop in obj.EnumerateObject())
{
if (prop.Value.ValueKind == JsonValueKind.Object)
{
CheckPropertyCountAgainstEnumerateObject(prop.Value);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there also be something like

if (prop.Value.ValueKind == JsonValueKind.Array) {
foreach item, check
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did I miss that... I now also added checks for GetArrayLength (because the local variable is now reused/shared for object property count and array length here: ea276ea)

}
}
}

[Fact]
public static void EnsureResizeSucceeds()
{
Expand Down
3 changes: 3 additions & 0 deletions ...libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Stream.ReadTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,11 +220,14 @@ public async Task TestBOMWithShortAndLongBuffers(Stream stream, int count, int e
void VerifyElement(int index)
{
Assert.Equal(JsonValueKind.Object, value[index].GetProperty("Test").ValueKind);
Assert.Equal(0, value[index].GetProperty("Test").GetPropertyCount());
Assert.False(value[index].GetProperty("Test").EnumerateObject().MoveNext());
Assert.Equal(JsonValueKind.Array, value[index].GetProperty("Test2").ValueKind);
Assert.Equal(0, value[index].GetProperty("Test2").GetArrayLength());
Assert.Equal(JsonValueKind.Object, value[index].GetProperty("Test3").ValueKind);
Assert.Equal(JsonValueKind.Object, value[index].GetProperty("Test3").GetProperty("Value").ValueKind);
Assert.Equal(1, value[index].GetProperty("Test3").GetPropertyCount());
Assert.Equal(0, value[index].GetProperty("Test3").GetProperty("Value").GetPropertyCount());
Assert.False(value[index].GetProperty("Test3").GetProperty("Value").EnumerateObject().MoveNext());
Assert.Equal(0, value[index].GetProperty("PersonType").GetInt32());
Assert.Equal(2, value[index].GetProperty("Id").GetInt32());
Expand Down
Loading