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 MaxDepth to JsonWriterOptions #44947

Closed
Tracked by #63762
amosonn opened this issue Nov 19, 2020 · 2 comments · Fixed by #61608
Closed
Tracked by #63762

Add MaxDepth to JsonWriterOptions #44947

amosonn opened this issue Nov 19, 2020 · 2 comments · Fixed by #61608
Assignees
Labels
api-approved API was approved in API review, it can be implemented api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json Cost:S Work that requires one engineer up to 1 week Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@amosonn
Copy link

amosonn commented Nov 19, 2020

Background and Motivation

Whereas JsonReaderOptions has MaxDepth property, JsonWriterOptions does not, instead taking a constant. While this constant is quite large (1000, compared to the default 64 on reading), there is no easy way to work with larger depths.

Even worse, JsonSerializerOptions has a MaxDepth property, but is not respected on Serialize, as there is no way to pass it on to the JsonWriter.

Proposed API

Add a JsonWriterOptions.MaxDepth property; set that property when called from JsonSerializer.

namespace System.Text.Json
{
    public partial struct JsonWriterOptions
    {
        public bool MaxDepth { get; set; }
    }
}

Alternative Designs

The current API, where this is a constant. This decision is described in #27938 , but I couldn't see a rationale.

@amosonn amosonn added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Nov 19, 2020
@amosonn amosonn changed the title MaxDepth to JsonWriterOptions Add MaxDepth to JsonWriterOptions Nov 19, 2020
@layomia layomia added this to the Future milestone Nov 25, 2020
@layomia layomia added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Nov 25, 2020
@eiriktsarpalis eiriktsarpalis added untriaged New issue has not been triaged by the area owner and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Apr 19, 2021
@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Jul 8, 2021
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 7.0.0 Oct 15, 2021
@eiriktsarpalis eiriktsarpalis added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Oct 15, 2021
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Oct 29, 2021
@eiriktsarpalis
Copy link
Member

I've updated the API proposal and marked it ready for review.

@eiriktsarpalis eiriktsarpalis self-assigned this Oct 29, 2021
@terrajobst
Copy link
Member

terrajobst commented Nov 2, 2021

Video

  • Looks good as proposed
namespace System.Text.Json
{
    public partial struct JsonWriterOptions
    {
        public int MaxDepth { get; set; }
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 2, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 15, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2021
@eiriktsarpalis eiriktsarpalis added User Story A single user-facing feature. Can be grouped under an epic. Cost:S Work that requires one engineer up to 1 week Team:Libraries labels Jan 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json Cost:S Work that requires one engineer up to 1 week Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants