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

Reduce json max depth default value #2912

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Reduce json max depth default value #2912

merged 1 commit into from
Sep 25, 2023

Conversation

shargon
Copy link
Member

@shargon shargon commented Sep 18, 2023 •
edited
Loading

Related to: #2950

The default value of JsonReaderOptions.MaxDepth is 64 as mentioned in:

Gets or sets the maximum depth allowed when reading JSON, with the default (i.e. 0) indicating a max depth of 64.

We shouldn't have more than that

@shargon shargon requested a review from superboyiii September 18, 2023 10:34
@shargon shargon changed the title Reduce max depth Reduce json max depth default value Sep 18, 2023
vncoelho
vncoelho approved these changes Sep 18, 2023 •
edited
Loading
View reviewed changes
@Jim8y
Copy link
Contributor

Jim8y commented Sep 18, 2023

if (!IsHardforkEnabled(Hardfork.HF_Basilisk)) maybe LOL, kinda allegic to the change now.

@vang1ong7ang
Copy link
Contributor

vang1ong7ang commented Sep 18, 2023 •
edited
Loading

if (!IsHardforkEnabled(Hardfork.HF_Basilisk)) maybe LOL, kinda allegic to the change now.

it would be cynocephalus's turn there is no fork

@vncoelho
Copy link
Member

Hahahaha

This change is necessary because of the limit, anyway.
This should be moved forward.

I do not believe it solves the issue.

@roman-khimov roman-khimov mentioned this pull request Sep 18, 2023
Closed
@shargon
Copy link
Member Author

shargon commented Sep 19, 2023

@superboyiii could you test it? I think that no HF will be needed

@superboyiii
Copy link
Member

@superboyiii could you test it? I think that no HF will be needed

Sure

superboyiii
superboyiii approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@superboyiii superboyiii left a comment

Choose a reason for hiding this comment

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

Tested OK. Data is compatible. @shargon

@shargon shargon merged commit bfe6d13 into master Sep 25, 2023
@shargon shargon deleted the json-max-depth branch September 25, 2023 08:29
@AnnaShaleva AnnaShaleva mentioned this pull request Nov 9, 2023
Closed
@AnnaShaleva AnnaShaleva mentioned this pull request Nov 23, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@superboyiii superboyiii superboyiii approved these changes

@vncoelho vncoelho vncoelho approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shargon @Jim8y @vang1ong7ang @vncoelho @superboyiii