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.

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

Add JsonElement.GetPropertyCount() #106222

Closed
wants to merge 8 commits into from

Conversation

WeihanLi
Copy link
Contributor

fixes #104692

Copy link

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.

1 similar comment
Copy link

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-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 10, 2024
@WeihanLi WeihanLi marked this pull request as draft August 10, 2024 05:55
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

This looks great. Can I mark this ready for review or do you still have changes you want to make?

@WeihanLi
Copy link
Contributor Author

@eiriktsarpalis I still have some issues with the result of GetPropertyCount
for example, the use case from the issue, JsonSerializer.Deserialize<JsonElement>("""{ "foo" : 1, "bar" : 2 }""").GetPropertyCount() returns 5, does not return 2 as expected, think maybe need some filter from the current filter, do you have any idea about this?

@eiriktsarpalis
Copy link
Member

Oh, interesting. This suggests that there might be an issue with the value of the SizeOrLength property in the context of objects:

internal int GetPropertyCount(int index)
{
CheckNotDisposed();
DbRow row = _parsedData.Get(index);
CheckExpectedType(JsonTokenType.StartObject, row.TokenType);
return row.SizeOrLength;
}

It might be worth investigating what the value represents in objects and see if there's an easy way to obtain the actual property count from it.

int propertyCount = 0;
int objectOffset = index + DbRow.Size;
int innerDepth = 0;
for (; objectOffset < _parsedData.Length; objectOffset += DbRow.Size)
Copy link
Member

Choose a reason for hiding this comment

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

The issue with this approach is that needs to perform a traversal of the entire object to compute the size. This is already possible with EnumerateObject(), we should only include this API if we can guarantee that it's constant-time.

Were you able to determine what the row.SizeOrLength value represents in the case of objects and if so, can be meaningfully used (or changed) so that we can obtain the property count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some test cases failed when I tried to update the SizeOrLength for object properties count, so I tried this way.
Would try to update the SizeOrLength to do some research on the failed cases, thanks for the input

@WeihanLi WeihanLi marked this pull request as draft August 12, 2024 16:04
@WeihanLi
Copy link
Contributor Author

closed in favor of #106503

@WeihanLi WeihanLi closed this Aug 16, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add JsonElement.GetPropertyCount()
2 participants