Make GetPropertyCount public and fix its return value

[etemi]

Should fix #104692

@WeihanLi was the first to submit a PR for this issue.
If there should be only one PR per issue feel free to close this as I created my PR later.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[WeihanLi]

👍 thanks, I've closed my PR

[eiriktsarpalis]

The changes look good to me superficially. Paging @bartonjs who is probably more familiar with the code that is being changed.

[eiriktsarpalis]

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.

[etemi]

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);

[bartonjs]

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.

[eiriktsarpalis]

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.

[etemi]

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.

[eiriktsarpalis]

Thanks, I think we should try to backport this.

[bartonjs]

Should there also be something like

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

?

[etemi]

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)

[eiriktsarpalis]

Thanks!

[eiriktsarpalis]

/backport to release/9.0

[github-actions]

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10434784757