Add WriteTo convenience APIs on JsonDocument and JsonProperty

[ahsonkhan]

Based on the usability study feedback, we realized that having convenience "write" APIs on JsonDocument and JsonProperty would be quite useful for users, especially for discovery.

People expect a Write method on the document itself and having to write the root element feels odd.

We should also align JsonProperty with JsonElement and add Write method that takes a writer to JsonProperty.

Current APIs on JsonElement:

public readonly partial struct JsonElement
{
    public void WriteProperty(ReadOnlySpan<byte> utf8PropertyName, Utf8JsonWriter writer) { }
    public void WriteProperty(ReadOnlySpan<char> propertyName, Utf8JsonWriter writer) { }
    public void WriteProperty(string propertyName, Utf8JsonWriter writer) { }
    public void WriteProperty(JsonEncodedText propertyName, Utf8JsonWriter writer) { }
    public void WriteValue(Utf8JsonWriter writer) { }
}

Proposal:
Part A) Add write APIs to JsonDocument and JsonProperty:

public sealed partial class JsonDocument : System.IDisposable
{
    public void WriteTo(Utf8JsonWriter writer) { }
}
public readonly partial struct JsonProperty
{
    public void WriteTo(Utf8JsonWriter writer) { }
}

Part B) Optionally, update the APIs on JsonElement for consistency

public readonly partial struct JsonElement
{
    // Option 1) Change the API names from "WriteX" to be "WriteAsXTo"
    public void WriteAsPropertyTo(ReadOnlySpan<byte> utf8PropertyName, Utf8JsonWriter writer) { }
    public void WriteAsPropertyTo(ReadOnlySpan<char> propertyName, Utf8JsonWriter writer) { }
    public void WriteAsPropertyTo(string propertyName, Utf8JsonWriter writer) { }
    public void WriteAsPropertyTo(JsonEncodedText propertyName, Utf8JsonWriter writer) { }
    public void WriteAsValueTo(Utf8JsonWriter writer) { }

    // Option 2) Remove "WriteProperty" and only keep "WriteValue", renaming it to "WriteTo"
    public void WriteTo(Utf8JsonWriter writer) { }
}

Sample Usage:
Before:

using (JsonDocument doc = JsonDocument.Parse(jsonString))
{
    JsonElement root = doc.RootElement;
    root.WriteValue(writer);
}

using JsonDocument doc = JsonDocument.Parse(jsonString);
JsonElement root = doc.RootElement;
foreach (JsonProperty property in root.EnumerateObject())
{
    property.Value.WriteProperty(property.Name, writer);
}

After:

using (JsonDocument doc = JsonDocument.Parse(jsonString))
{
    doc.WriteTo(writer);
}

using JsonDocument doc = JsonDocument.Parse(jsonString);
JsonElement root = doc.RootElement;
foreach (JsonProperty property in root.EnumerateObject())
{
    property.WriteTo(writer);
}

Questions:

1. Can we remove WriteProperty APIs on JsonElement and rename WriteValue to be WriteTo? That is the most consistent/cleanest API shape (both internally with JsonDocument, JsonProperty, etc., and externally with Newtonsoft Json's JToken, and Xml APIs).
2. If we are keeping WriteProperty APIs on JsonElement, should we re-order the parameters so that the writer comes up first?

Sample implementation:
dotnet/corefx@78615e1

cc @bartonjs, @terrajobst, @steveharter, @JeremyKuhne

[sasivishnu]

@ahsonkhan Hi, I would like to contribute. Do you think its good issue to get started? Since it marked as "api-ready-for-review", I need to wait till API getting approved before working on it, right?

[terrajobst]

@sasivishnu This issue isn't ready yet.

@ahsonkhan I don't think we should mark API suggestions that weren't approved as up-for-grabs. /cc @karelz

[ericstj]

Removing up-for-grabs until the API change has been approved.

[scalablecory]

Can we instead add Write methods to Utf8JsonWriter?

[terrajobst]

Video

We liked option Part A) + Part B) 2, i.e. a single WriteTo(writer) on all three elements.

Can we instead add Write methods to Utf8JsonWriter?

We don't want this for layering reasons, i.e. the DOM knows about the writer, but the writer shouldn't know about the DOM.

[sasivishnu]

@terrajobst I was listening to Design Review video. One thing I can't able to catch clear is "Are we going to remove all existing WriteProperty APIs?

Btw, is this now "up for grabs" since API is approved?

[bartonjs]

@sasivishnu Yes, we're removing JsonElement.WriteProperty (all overloads) and renaming JsonElement.WriteValue.

Consider the issue yours (though be advised the PR can't be merged until we work out some "late breaking change" paperwork on our side...)

[sasivishnu]

@sasivishnu Yes, we're removing JsonElement.WriteProperty (all overloads) and renaming JsonElement.WriteValue.

Consider the issue yours (though be advised the PR can't be merged until we work out some "late breaking change" paperwork on our side...)

@ahsonkhan @bartonjs
Thanks. Since this will be my first issue in corefx, please do let me know if I am missing something,

1. Add WriteTo API in JsonDocument class and implement it (call WriteTo method of RootElement which is of JsonElement type).
2. Remove all 'WriteProperty' API in JsonElement struct and its references in System.Text.Json.
3. Rename WriteValue to WriteTo in JsonElement and update corresponding references.
4. Update all test methods affecting due to this change.
5. Add or update test method to cover new API WriteTo introduced in JsonDocument class.

[bartonjs]

A couple of additions / clarifications

1.5) Add WriteTo on JsonProperty which calls writer.WritePropertyName(Name); Value.WriteTo(writer);
2.5) You should also be able to delete some code in JsonDocument for writing the value with a property name.
3.5) Run msbuild /t:GenerateReferenceSource from src/System.Text.Json/src (this should rewrite the file in src/System.Text.Json/ref for you)
5.5) Also JsonProperty.WriteTo.

[sasivishnu]

@bartonjs Thanks. Couple of clarifications on testing,

1. What to do with WriteProperty() references in test methods? There are nearly 20 odd test methods in JsonElementWriteTests.cs written for WriteProperty() methods in JsonElement.
   a) Should I fix those test methods by changing target.WritePropertyName(propertyName) to writer.WritePropertyName(propertyName); target.WriteTo(writer);
   b) Or remove those test methods since writing property name no longer belongs to JsonElement responsibility?

2. For writing test methods for JsonProperty.WriteTo(), I should create new file, JsonPropertyTests.cs right? Or should I include a test in JsonDocumentTest.cs?

[ahsonkhan]

I would say, yes, do 1a). I don't see any harm in keeping existing tests which cover more of utf8jsonwriter/jsonelement interop scenarios.

For 2, I prefer a new file, but will defer to @bartonjs

[bartonjs]

Hm, apparently I have this half filled out on a different computer.

What to do with WriteProperty() references in test methods?

My original answer was "delete them". but then I was going to say a nuanced thing that I think is so nuanced that I can't explain it (of trying to ensure the scenario coverage).

So just changing them to be writer.WritePropertyName and element.WriteTo is the easiest answer.

For writing test methods for JsonProperty.WriteTo(), I should create new file, JsonPropertyTests.cs right?

For JsonElement the write tests are in JsonElementWriteTests.cs, because it's a big enough scenario that it's worth testing on its own (class filter), and a big file). For JsonProperty the white-box testing is pretty small in this scenario, but the black-box is pretty big (write a true, a false, a null, a string, a number, a shallow object, a deep object, a shallow array, a deep array).

Existing JsonProperty tests are just in JsonDocumentTests (because there weren't many of them).

Having looked at JsonDocumentTests again just now, I think that JsonPropertyTests.cs makes sense, and ideally the following tests should be moved from JsonDocumentTests to JsonPropertyTests:

• NameEquals_InvalidInstance_Throws
• NameEquals_UseGoodMatches_True
• NameEquals_GivenPropertyAndValue_TrueForPropertyName

[sasivishnu]

Thanks. I will get PR ready by next week (traveling till next Wednesday).