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

[API Proposal]: Add the ability to specify line endings when serializing Json #84117

Closed
aodpi opened this issue Mar 30, 2023 · 18 comments · Fixed by #100890
Closed

[API Proposal]: Add the ability to specify line endings when serializing Json #84117

aodpi opened this issue Mar 30, 2023 · 18 comments · Fixed by #100890
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@aodpi
Copy link

aodpi commented Mar 30, 2023

Background and motivation

Currently when serializing Json you have the option to write it Indented. But the new line character seems to be picked up based on the operating system through Environment.NewLine. This is fair but I've encountered a situation when a client wanted Line endings to be Line Feed character ('\n') instead of Carriage Return and Line Feed (\r\n). Proposal is to add the option to specify the line ending character to be used when serializing json with indentation.

When using popular package Newtonsoft.Json you have the ability to pass a StreamWriter instance which will be used to write json, with an additional ability to specify the NewLine Character when instantiating a StreamWriter

API Proposal

Extend JsonSerializerOptions and give users ability what line terminator to use when writing Indented json so we would have:

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public bool WriteIndented { get; set; }
    public char IndentChar { get; set; }
    public int IndentSize { get; set; }
+   public string NewLine { get; set; } = Environment.NewLine; // only "\n" or "\r\n" are permitted values.

public partial struct JsonWriterOptions
{
    public bool Indented { get; set; }
    public char IndentChar { get; set; }
    public int IndentSize { get; set; }
+   public string NewLine { get; set; } = Environment.NewLine; // only "\n" or "\r\n" are permitted values.
}

By default the NewLine property will have the line terminator based on the current OS as it's treated at the moment.

API Usage

User would initialize JsonSerializerOptions by specifying the line terminator

var jsonOptions = new JsonSerializerOptions
{
    WriteIndented = true,
    NewLine = "\n"
};

User would pass these options when serializing json as follows:

using System.Text.Json;

JsonSerializer.Serialize(stream, value, jsonOptions);

The result would be a json string serialized with specified line terminator written to the specified stream.

Alternative Designs

No response

Risks

No identified risks or breaking changes.

@aodpi aodpi added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 30, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 30, 2023
@ghost
Copy link

ghost commented Mar 30, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Currently when serializing Json you have the option to write it Indented. But the new line character seems to be picked up based on the operating system through Environment.NewLine. This is fair but I've encountered a situation when a client wanted Line endings to be Line Feed character ('\n') instead of Carriage Return and Line Feed (\r\n). Proposal is to add the option to specify the line ending character to be used when serializing json with indentation.

When using popular package Newtonsoft.Json you have the ability to pass a StreamWriter instance which will be used to write json, with an additional ability to specify the NewLine Character when instantiating a StreamWriter

API Proposal

Extend JsonSerializerOptions and give users ability what line terminator to use when writing Indented json so we would have:

using System.Text.Json;

var jsonOptions = new JsonSerializerOptions
{
    WriteIndented = true,
    NewLine = "\n"
}

By default the NewLine property will have the line terminator based on the current OS as it's treated at the moment.

API Usage

User would initialize JsonSerializerOptions by specifying the line terminator

var jsonOptions = new JsonSerializerOptions
{
    WriteIndented = true,
    NewLine = "\n"
};

User would pass these options when serializing json as follows:

using System.Text.Json;

JsonSerializer.Serialize(stream, value, jsonOptions);

The result would be a json string serialized with specified line terminator written to the specified stream.

Alternative Designs

No response

Risks

No identified risks or breaking changes.

Author: aodpi
Assignees: -
Labels:

api-suggestion, area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

Sounds reasonable, we could consider this. Related to #63882.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Mar 30, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Mar 30, 2023
@gregsdennis
Copy link
Contributor

I think having an enumeration for this instead of an open-ended string would be better.

JsonSerializationOptions.NewLine = LineTermination.CarriageReturn;

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Mar 31, 2023

FWIW this would also need an equivalent option in JsonWriterOptions which JsonSerializerOptions forwards to.

@Ayazcan3333
Copy link

using System.Text.Json;

var jsonOptions = new JsonSerializerOptions
{
WriteIndented = true,
NewLine = "\n"
}

1 similar comment
@Ayazcan3333
Copy link

using System.Text.Json;

var jsonOptions = new JsonSerializerOptions
{
WriteIndented = true,
NewLine = "\n"
}

@msoler8785
Copy link

Yes please add this, because I did a code review today and I saw the following and I just went home for the rest of the day:

JsonSerializer.Serialize(settings).Replace(Environment.NewLine, "\r\n\t")

@aodpi
Copy link
Author

aodpi commented Aug 9, 2023

Sounds reasonable, we could consider this. Related to #63882.

I don't think it's related, that issue describes indentation size and indent characters. This is more about line endings in the result json string.

@rogerfar
Copy link

rogerfar commented Feb 21, 2024

I ran into this issue because we're doing unit tests where we compare raw json to a resulting json:

        const String json1 =
            """
            [{
              "name": "test"
            }]
            """;

        const String json2 =
            """
            [{
              "name": "test"
            }]
            """;
			
Assert.Equal(json1, json2); // works

var json3 = JsonSerializer.Deserialize<>(JsonSerializer.Serialize(json2));
Assert.Equal(json1, resultJson); // works on Windows, but not on the linux build server
Expected: "{\r\n  "name": "test",\r\n...
Actual:   "{\n  "name": "test",\n...

@eiriktsarpalis
Copy link
Member

@rogerfar This type of comparison would always be susceptible to whitespace issues. I would recommend creating a JsonNode instance for each JSON document and then compare using the newly added JsonNode.DeepEquals method.

@martincostello
Copy link
Member

+1 for this feature - this week I've been working on a tool what makes targeted edits to users' JSON files to update specific nodes that meet specific criteria.

I want these changes to be as minimally invasive as possible, so as retaining the file's original file endings (assuming they're consistent). With .NET 8 today, without resorting to custom code (unless there's a better way I'm not aware of), I have to live with the compromise of the line endings being whatever the operating system uses when the edits are saved and hoping they're the same as the original.

I could then re-read/edit the file post JSON processing to fix-up the line endings again, but that's a bit bleurgh. I might do that anyway to as it's simple enough as I already detect line endings for other types of edits, but the ability to override it like you can with TextWriter.NewLine would be most welcome.

I'd be happy to take a stab at it for .NET 9 if it goes through API review.

@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 23, 2024
@eiriktsarpalis eiriktsarpalis self-assigned this Feb 23, 2024
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Feb 23, 2024

API Proposal

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public bool WriteIndented { get; set; }
    public char IndentChar { get; set; }
    public int IndentSize { get; set; }
+   public string NewLine { get; set; } = Environment.NewLine; // only "\n" or "\r\n" are permitted values.

public partial struct JsonWriterOptions
{
    public bool Indented { get; set; }
    public char IndentChar { get; set; }
    public int IndentSize { get; set; }
+   public string NewLine { get; set; } = Environment.NewLine; // only "\n" or "\r\n" are permitted values.
}

I've marked it ready for API review.

@baronfel
Copy link
Member

baronfel commented Mar 5, 2024

Adding a +1 to this - this is very helpful for calculating things like OCI Digests. In many container scenarios, json is returned with both indentation and \n newlines only, and since the digest calculations use the raw json as input, STJ inserting \r\n newlines makes my calculations incorrect by construction - even if I opt into indented writing. Specifically I discovered this while implementing dotnet/sdk#39160 - adding container label metadata for the 'base' image's digest, which container tooling will use to give you good provenance tooling/tracing.

@eiriktsarpalis eiriktsarpalis added the partner-impact This issue impacts a partner who needs to be kept updated label Mar 5, 2024
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 9.0.0 Mar 5, 2024
@bartonjs
Copy link
Member

bartonjs commented Apr 9, 2024

Video

  • JsonSourceGenerationOptionsAttribute was missing, but should have parity with JsonSerializerOptions, so it was added.
namespace System.Text.Json
{
    public partial class JsonSerializerOptions
    {
        public string NewLine { get; set; } = Environment.NewLine; // only "\n" or "\r\n" are permitted values.
    }

    public partial struct JsonWriterOptions
    {
        public string NewLine { get; set; } = Environment.NewLine; // only "\n" or "\r\n" are permitted values.
    }
}

namespace System.Text.Json.Serialization
{
    public partial class JsonSourceGenerationOptionsAttribute
    {
        public string? NewLine { get; set; }
    }
}

@bartonjs bartonjs 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 Apr 9, 2024
@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Apr 9, 2024
@martincostello
Copy link
Member

@eiriktsarpalis I'll look at starting on this later today.

@martincostello
Copy link
Member

@bartonjs Looks like API review bot didn't include the timestamp in the YouTube link 😿

@bartonjs
Copy link
Member

@martincostello The bot/tool embeds the time that the previous issue got marked in a way that says we're moving on (approved, needs more info, closed), which is usually conveniently just a few seconds before we start the issue after it. This was the first issue of the meeting, so no previous issue, so 0h0m0s is the best it can come up with.

If you have a better algorithm that doesn't require me or Immo to need to remember to do paperwork or push buttons while driving the meeting, we'd be happy to hear it 😄.

@martincostello
Copy link
Member

Ah, I just heard the intro music and assumed it had gone awry, rather than that it was the first thing discussed 😄

martincostello added a commit to martincostello/runtime that referenced this issue Apr 10, 2024
Allow the new line string to use for indented JSON to be specified through options.
Resolves dotnet#84117.
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Apr 10, 2024
eiriktsarpalis pushed a commit that referenced this issue Apr 16, 2024
* Add new line to be specified for JSON formatting

Allow the new line string to use for indented JSON to be specified through options.
Resolves #84117.

* Address review feedback

- Cater for `_newLine` in JsonSerializerOptions caching.
- Lazily initialize field.
- Allow null to reset to default.
- Add assertions.
- Add/update comments.
- Use `nameof()`.
- Remove redundant field.
- Extend tests.

* Update Logging.Console tests

- Update property count to fix assertion.
- Update test to validate `NewLine` can be set/bound.

* Only normalize line endings if needed

Only normalize the line endings if the `JsonWriterOptions` are not using the defaults.

* Address feedback

- Access lazily initialized field through property.
- Update hash code assertion.

* Update exception message

Use similar format string to `Format_InvalidGuidFormatSpecification`/

* Address feedback

Reword comment as suggested.

* Address feedback

- Simplify condition.
- Disallow null for `string NewLine` properties.
matouskozak pushed a commit to matouskozak/runtime that referenced this issue Apr 30, 2024
* Add new line to be specified for JSON formatting

Allow the new line string to use for indented JSON to be specified through options.
Resolves dotnet#84117.

* Address review feedback

- Cater for `_newLine` in JsonSerializerOptions caching.
- Lazily initialize field.
- Allow null to reset to default.
- Add assertions.
- Add/update comments.
- Use `nameof()`.
- Remove redundant field.
- Extend tests.

* Update Logging.Console tests

- Update property count to fix assertion.
- Update test to validate `NewLine` can be set/bound.

* Only normalize line endings if needed

Only normalize the line endings if the `JsonWriterOptions` are not using the defaults.

* Address feedback

- Access lazily initialized field through property.
- Update hash code assertion.

* Update exception message

Use similar format string to `Format_InvalidGuidFormatSpecification`/

* Address feedback

Reword comment as suggested.

* Address feedback

- Simplify condition.
- Disallow null for `string NewLine` properties.
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2024
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this issue May 30, 2024
* Add new line to be specified for JSON formatting

Allow the new line string to use for indented JSON to be specified through options.
Resolves dotnet#84117.

* Address review feedback

- Cater for `_newLine` in JsonSerializerOptions caching.
- Lazily initialize field.
- Allow null to reset to default.
- Add assertions.
- Add/update comments.
- Use `nameof()`.
- Remove redundant field.
- Extend tests.

* Update Logging.Console tests

- Update property count to fix assertion.
- Update test to validate `NewLine` can be set/bound.

* Only normalize line endings if needed

Only normalize the line endings if the `JsonWriterOptions` are not using the defaults.

* Address feedback

- Access lazily initialized field through property.
- Update hash code assertion.

* Update exception message

Use similar format string to `Format_InvalidGuidFormatSpecification`/

* Address feedback

Reword comment as suggested.

* Address feedback

- Simplify condition.
- Disallow null for `string NewLine` properties.
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 area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants