Add specification for ReferenceHandling #354

jozkee · 2019-11-27T20:09:53Z

No description provided.

ahsonkhan · 2019-11-28T01:18:25Z

I had already provided most of the feedback here: jozkee/corefx#1

I will review the revisions made to this doc soon.

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

Add note about immutable types

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

ahsonkhan

A bunch of nits and re-wording suggestions. Otherwise, looks great!

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

ahsonkhan · 2019-12-03T04:48:11Z

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

+3. While Immutable types and `System.Array`s can be Serialized with Preserve semantics, they will not be supported when trying to Deserialize them as a reference; those types are created with the help of an internal converter, and they are not parsed until the entire block of JSON finishes, nested reference to these types is impossible to identify, unless you re-scan the resulting object, which is too expensive.
+4. Value types, such as structs that contain preserve semantics, will not be supported when Deserialized as well; this is because the serializer will never signal a reference object to those types, doing such thing implies boxing of value types.
+5. Additional features, such as Converter support, `ReferenceResolver`, `JsonPropertyAttribute.IsReference` and `JsonPropertyAttribute.ReferenceLoopHandling`,  that build on top of `ReferenceLoopHandling` and `PreserveReferencesHandling` were considered but they can be added in the future based on customer requests.
+6. We are still looking for evidence that backs up supporting `ReferenceHandling.Ignore`, this option will not ship if said evidence is not found.


Suggested change

6. We are still looking for evidence that backs up supporting `ReferenceHandling.Ignore`, this option will not ship if said evidence is not found.

6. We are still looking for evidence that backs up supporting `ReferenceHandling.Ignore`. This option will not ship if said evidence is not found.

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

Co-Authored-By: Ahson Khan <[email protected]>

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

Co-Authored-By: Ahson Khan <[email protected]>

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

Co-Authored-By: Ahson Khan <[email protected]>

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

ahsonkhan · 2019-12-03T06:43:12Z

cc @layomia, @steveharter, @ericstj, @terrajobst

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

…$values Co-Authored-By: Ahson Khan <[email protected]>

ghost · 2019-12-03T06:49:36Z

Hello @ahsonkhan!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

layomia · 2019-12-03T06:51:00Z

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

+
+* Expose the `ReferenceResolver` in `Converters` to have access to the map of references.
+
+* Create `JsonReferenceHandlingAttribute` to enable annotating properties and classes with their own isolated `ReferenceHandling` behavior (I am deferring this feature because the constant checking for attributes was causing too much perf overhead on the main path, but maybe we can try moving the attribute check to the warm-up method to reduce the runtime increase).


Yes - in the future, checking the attributes once during warm-up is the way to go.

layomia · 2019-12-03T06:51:38Z

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

+        ReferenceHandling = ReferenceHandling.Preserve
+    };
+
+    //Throw JsonException - PropertyName cannot start with '$' when Preserve References is enabled.


Suggested change

//Throw JsonException - PropertyName cannot start with '$' when Preserve References is enabled.

// Throw JsonException - PropertyName cannot start with '$' when Preserve References is enabled.

layomia · 2019-12-03T06:52:14Z

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

+    //Throw JsonException - PropertyName cannot start with '$' when Preserve References is enabled.
+    string json = JsonSerializer.Serialize(root, opts);
+
+    //Also throws the same exception.


Suggested change

//Also throws the same exception.

// Throws the same exception as above.

layomia · 2019-12-03T06:54:15Z

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

+
+**Reference loops**: Also referred as circular references, loops occur when a property of a .NET object refers to the object itself, either directly (a -> a) or indirectly (a -> b -> a). They also occur when the element of an array refers to the array itself (arr[0] -> arr). Multiple occurrences of the same reference do not imply a cycle.
+
+**Preserve duplicated references**: Semantically represent objects and/or arrays that have been previously written, with a reference to them when found again in the object graph (using reference equality for comparison).


Provide the definition for deserialization?

layomia · 2019-12-03T06:58:53Z

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

+
+**Metadata**: Extra properties on JSON objects and/or arrays (that may change their schema) to enable reference preservation when round-tripping. These additional properties are only meant to be understood by the `JsonSerializer`.  
+
+# Motivation


I'd also address the motivation for deserialization - to preserve references.

layomia · 2019-12-03T07:05:19Z

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

+```
+See also the [internal implementation details](https://gist.github.com/Jozkee/b0922ef609f7a942f00ac2c93a976ff1).
+
+## In depth


nit: this title is a bit confusing - my mind first went to serialization depth (maybe it's just me)

Consider "A closer look Default, Preserve, and Ignore reference handling options" or something...

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

layomia · 2019-12-03T07:24:53Z

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

+
+* `$.*` **Valid**
+
+Note: For Dictionary keys on serialize, should we allow serializing keys `$id`, `$ref` and `$values`? If we allow it, then there is a potential round-tripping issue.


In addition to the few questions sprinkled around the doc in their respective discussion contexts, I'd add an open questions recap at the end of the doc so we can recall them.

layomia · 2019-12-03T07:25:56Z

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

+Note: By the same principle, `Newtonsoft.Json` does not support parsing JSON arrays into immutables as well.
+Note 2: When using immutable types and `ReferenceHandling.Preserve`, you will not be able to generate payloads that are capables of round-tripping.
+
+* **Immutable types**: i.e: `ImmutableList` and `ImmutableDictionary`


Suggested change

* **Immutable types**: i.e: `ImmutableList` and `ImmutableDictionary`

* **Immutable types**: e.g. `ImmutableList` and `ImmutableDictionary`

I always though i.e. stands for "in example".

layomia · 2019-12-03T07:28:24Z

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md

+* **Immutable types**: i.e: `ImmutableList` and `ImmutableDictionary`
+* **System.Array**: i.e: `Array<T>` and `T[]`


Are collections that don't implement IList/IDictionary, but don't use internal converters--like Stack<T> and HashSet<T>--supported for this feature?

Yes, all collections, as long as they don't rely on a converter, should be compatible with the feature.

…ensionData.

stephentoub · 2020-01-29T19:35:52Z

@jozkee, what's the status of this PR?

stephentoub · 2020-02-06T15:54:00Z

This has been approved, marked as auto-merge, and open for months. I'm going to merge it. If you need to make more changes to the doc, please open a new PR. Thanks.

Add specification for ReferenceHandling

85259e9

jozkee added the area-System.Text.Json label Nov 27, 2019

jozkee added this to the 5.0 milestone Nov 27, 2019

jozkee requested a review from ahsonkhan November 27, 2019 20:09

ahsonkhan added the documentation Documentation bug or enhancement, does not impact product or test code label Nov 27, 2019

ahsonkhan reviewed Nov 28, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

ahsonkhan reviewed Nov 28, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Show resolved Hide resolved

ahsonkhan reviewed Nov 28, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

ahsonkhan reviewed Nov 28, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Show resolved Hide resolved

ahsonkhan reviewed Nov 28, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Show resolved Hide resolved

jozkee added 2 commits November 27, 2019 18:43

Address more feedback, corrected types, and included table of contents

f5f85c1

Add missing access modifiers to class properties

e12a8f6

Add note about immutable types

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

ahsonkhan approved these changes Dec 3, 2019

View reviewed changes

jozkee commented Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Show resolved Hide resolved

Apply suggestions from code review

24c6d17

Co-Authored-By: Ahson Khan <[email protected]>

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

Apply more suggestions from code review

6b4a91b

Co-Authored-By: Ahson Khan <[email protected]>

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

Add PR suggestions.

095d5a9

jozkee commented Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Show resolved Hide resolved

Apply even more suggestions from code review

bc2701c

Co-Authored-By: Ahson Khan <[email protected]>

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

ahsonkhan reviewed Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

jozkee commented Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

jozkee commented Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

jozkee commented Dec 3, 2019

View reviewed changes

src/libraries/System.Text.Json/docs/ReferenceHandling_spec.md Outdated Show resolved Hide resolved

Add missing semi colons on samples and standardize error messages on …

cc1ac66

…$values Co-Authored-By: Ahson Khan <[email protected]>

ahsonkhan approved these changes Dec 3, 2019

View reviewed changes

ahsonkhan added the auto-merge label Dec 3, 2019

layomia reviewed Dec 3, 2019

View reviewed changes

ahsonkhan mentioned this pull request Dec 9, 2019

JsonSerializer falls into cycle #432

Closed

Address changes discussed on API review

95d2636

ahsonkhan mentioned this pull request Jan 1, 2020

New article - Migrate from Newtonsoft.Json dotnet/docs#16225

Merged

jozkee added 2 commits January 14, 2020 22:17

Add note to describe unsupported round-tripping capability on JsonExt…

94021e1

…ensionData.

Replace i.e. for e.g. where meant to quote an example.

c23959d

layomia mentioned this pull request Feb 5, 2020

Added field support to JSON serializer #2192

Closed

stephentoub merged commit 1a1a541 into dotnet:master Feb 6, 2020

layomia assigned jozkee May 15, 2020

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

jozkee deleted the ReferenceHandlingSpec branch March 24, 2021 19:36

	6. We are still looking for evidence that backs up supporting `ReferenceHandling.Ignore`, this option will not ship if said evidence is not found.
	6. We are still looking for evidence that backs up supporting `ReferenceHandling.Ignore`. This option will not ship if said evidence is not found.


		* Expose the `ReferenceResolver` in `Converters` to have access to the map of references.

		* Create `JsonReferenceHandlingAttribute` to enable annotating properties and classes with their own isolated `ReferenceHandling` behavior (I am deferring this feature because the constant checking for attributes was causing too much perf overhead on the main path, but maybe we can try moving the attribute check to the warm-up method to reduce the runtime increase).

	//Throw JsonException - PropertyName cannot start with '$' when Preserve References is enabled.
	// Throw JsonException - PropertyName cannot start with '$' when Preserve References is enabled.

	//Also throws the same exception.
	// Throws the same exception as above.


		Reference loops: Also referred as circular references, loops occur when a property of a .NET object refers to the object itself, either directly (a -> a) or indirectly (a -> b -> a). They also occur when the element of an array refers to the array itself (arr[0] -> arr). Multiple occurrences of the same reference do not imply a cycle.

		Preserve duplicated references: Semantically represent objects and/or arrays that have been previously written, with a reference to them when found again in the object graph (using reference equality for comparison).


		Metadata: Extra properties on JSON objects and/or arrays (that may change their schema) to enable reference preservation when round-tripping. These additional properties are only meant to be understood by the `JsonSerializer`.

		# Motivation


		* `$.` Valid*

		Note: For Dictionary keys on serialize, should we allow serializing keys `$id`, `$ref` and `$values`? If we allow it, then there is a potential round-tripping issue.

	* Immutable types: i.e: `ImmutableList` and `ImmutableDictionary`
	* Immutable types: e.g. `ImmutableList` and `ImmutableDictionary`

		* Immutable types: i.e: `ImmutableList` and `ImmutableDictionary`
		* System.Array: i.e: `Array<T>` and `T[]`

Add specification for ReferenceHandling #354

Add specification for ReferenceHandling #354

Uh oh!

Conversation

jozkee commented Nov 27, 2019

Uh oh!

ahsonkhan commented Nov 28, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Dec 3, 2019

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ahsonkhan commented Dec 3, 2019 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.