Add specification for ReferenceHandling #1

jozkee · 2019-11-25T23:32:46Z

ahsonkhan

Some feedback.

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

ahsonkhan · 2019-11-25T23:40:31Z

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

+
+Community is heavily requesting this feature since is consider by many as a very common scenario, specially when serializing POCOs that came from an ORM Framework, such as Entity Framework; even though JSON specifiacation does not support reference loops by default. Therefore this will be shipped as an opt-in feature.
+
+The current solution to deal with reference loops is to rely in MaxDepth and throw a JsonException after it is exceeded. Now, this is a decent and cheap solution but we will also offer other not-so-cheap options to deal with this problem while keeping the current one in order to not affect the out-of-the-box performance.


Suggested change

The current solution to deal with reference loops is to rely in MaxDepth and throw a JsonException after it is exceeded. Now, this is a decent and cheap solution but we will also offer other not-so-cheap options to deal with this problem while keeping the current one in order to not affect the out-of-the-box performance.

The current solution to deal with cycles in the object graph while serializing is to rely on `MaxDepth` and throw a `JsonException` after it is exceeded. This was done to avoid perf overhead for cycle detection in the common case. The goal is to enable the new opt-in feature with minimal impact to existing performance.

The current solution to deal with cycles in the object graph while serializing is to rely on `MaxDepth` and throw a `JsonException` after it is exceeded. This was done to avoid perf overhead for cycle detection in the common case. The goal is to enable the new opt-in feature with minimal impact to existing performance.

Where did those HTML tags came from?

Don't know. Something broke with the suggestions. That's why I copied the suggestion as regular text.

ahsonkhan · 2019-11-25T23:41:58Z

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

+
+# Prior art
+
+Json.Net contains settings that you can enable to deal with such problems.


Please use Newtonsoft.Json everywhere rather than Json.NET. Json.Net is confusing to readers (they tend to think that's the built-in library).

ahsonkhan · 2019-11-25T23:42:11Z

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

+
+The current solution to deal with reference loops is to rely in MaxDepth and throw a JsonException after it is exceeded. Now, this is a decent and cheap solution but we will also offer other not-so-cheap options to deal with this problem while keeping the current one in order to not affect the out-of-the-box performance.
+
+# Prior art


What about what other languages/libraries do? I would add that here too.

Given we are basically talking about what other libraries do, when you add more info here, consider wording the title as "Other libraries" like you had before.

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

ahsonkhan · 2019-11-26T00:10:31Z

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

+
+1. MaxDepth validation will not be affected by `ReferenceHandling.Preserve`.
+2. We are merging the Json.Net types [`ReferenceLoopHandling`]("https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_ReferenceLoopHandling.htm") and [`PreserveReferencesHandling`]("https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_PreserveReferencesHandling.htm") (we are also not including the granularity on this one) into one single class; `ReferenceHandling`.
+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.


Explain why.

ahsonkhan · 2019-11-26T00:10:40Z

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

+1. MaxDepth validation will not be affected by `ReferenceHandling.Preserve`.
+2. We are merging the Json.Net types [`ReferenceLoopHandling`]("https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_ReferenceLoopHandling.htm") and [`PreserveReferencesHandling`]("https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_PreserveReferencesHandling.htm") (we are also not including the granularity on this one) into one single class; `ReferenceHandling`.
+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.
+4. Value types, such as structs that contain preserve semantics, will not be supported when Deserialized as well.


Same here. Why?

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

ahsonkhan · 2019-11-26T00:14:31Z

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

+
+With ReferenceHandling being a class, we can exclude things that, as of now, we are not sure if they need to be shipped, in example, the `ReferenceLoopHandling.Ignore` option or the Object and Array granularity of Json.Net's `PreserveReferencesHandling`.
+
+In a future, we might also be able to extend the Deserialization behaviors, in order to provide the granularity of Serialization on Deserialization. Or we could also be able to define independant behaviors for both Serialization and Deserialization; i.e: someone may want to emit payloads with preserved references but he does not want to read them.


Add the support matrix here. Along with what the support matrix could look like in the future (also consider including read ahead in your matrix of things we don't support).

https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_MetadataPropertyHandling.htm

Add the support matrix here.

Just to clarify, referring to something like this:
https://github.com/dotnet/corefx/issues/38579#issuecomment-556923546

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

ahsonkhan · 2019-11-26T00:38:49Z

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

+}
+```
+
+* Metadata property **after** `$ref`:


Add rule about JsonExtensionData where metadata won't get added to it ever (even if we ignore it with a setting).

I though we agreed that metadata will be added to JsonExtensionData if we choose to ignore it. There might be people that depend on such properties existing in it.

I though we agreed that metadata will be added to JsonExtensionData if we choose to ignore it.

Is ignoring metadata the default behavior on deserialization? I am fine with that. Please document it as such.

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

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

ahsonkhan · 2019-11-26T01:52:23Z

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

+
+This is a heavily requested feature since it is consider by many as a very common scenario, specially when serializing POCOs that came from an ORM Framework, such as Entity Framework; even though the JSON specification does not support reference loops by default. Therefore, this will be implemented as an opt-in feature (for both serialization and deserialization).
+
+The current solution to deal with reference loops is to rely in MaxDepth and throw a JsonException after it is exceeded. Now, this is a decent and cheap solution but we will also offer other not-so-cheap options to deal with this problem while keeping the current one in order to not affect the out-of-the-box performance.


Suggested change

The current solution to deal with reference loops is to rely in MaxDepth and throw a JsonException after it is exceeded. Now, this is a decent and cheap solution but we will also offer other not-so-cheap options to deal with this problem while keeping the current one in order to not affect the out-of-the-box performance.

The current solution to deal with cycles in the object graph while serializing is to rely on `MaxDepth` and throw a `JsonException` after it is exceeded. This was done to avoid perf overhead for cycle detection in the common case. The goal is to enable the new opt-in feature with minimal impact to existing performance.

ahsonkhan · 2019-11-26T01:53:19Z

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

+
+Currently there is no mechanism to prevent infinite looping in circular objects (while serializing) nor to preserve references that round-trip when using System.Text.Json.
+
+This is a heavily requested feature since it is consider by many as a very common scenario, specially when serializing POCOs that came from an ORM Framework, such as Entity Framework; even though the JSON specification does not support reference loops by default. Therefore, this will be implemented as an opt-in feature (for both serialization and deserialization).


Suggested change

This is a heavily requested feature since it is consider by many as a very common scenario, specially when serializing POCOs that came from an ORM Framework, such as Entity Framework; even though the JSON specification does not support reference loops by default. Therefore, this will be implemented as an opt-in feature (for both serialization and deserialization).

This is a heavily requested feature since it is considered by many as a very common scenario, specially when serializing POCOs that came from an ORM Framework, such as Entity Framework; even though the JSON specification does not support reference loops by default. Therefore, this will be implemented as an opt-in feature (for both serialization and deserialization).

ahsonkhan · 2019-11-26T01:55:31Z

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

+
+This API is exposing the `ReferenceHandling` property as a class, to be extensible in the future; and provide built-in static instances of `Default` and `Preserve` that are useful to enable the most common behaviors by just setting those in `JsonSerializerOptions.ReferenceHandling`.
+
+With `ReferenceHandling` being a class, we can exclude things that, as of now, we are not sure are required and add them later based on customer feedback. For example, the `Object` and `Array` granularity of `Newtonsoft.Json's` ``PreserveReferencesHandling` or the `ReferenceLoopHandling.Ignore` option.


Suggested change

With `ReferenceHandling` being a class, we can exclude things that, as of now, we are not sure are required and add them later based on customer feedback. For example, the `Object` and `Array` granularity of `Newtonsoft.Json's` ``PreserveReferencesHandling` or the `ReferenceLoopHandling.Ignore` option.

With `ReferenceHandling` being a class, we can exclude things that, as of now, we are not sure are required and add them later based on customer feedback. For example, the `Object` and `Array` granularity of `Newtonsoft.Json's` `PreserveReferencesHandling` or the `ReferenceLoopHandling.Ignore` option.

…isions.

Add specification for ReferenceHandling

d284c81

ahsonkhan reviewed Nov 26, 2019

View reviewed changes

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

ahsonkhan reviewed Nov 26, 2019

View reviewed changes

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

ahsonkhan reviewed Nov 26, 2019

View reviewed changes

Apply suggestions from code review

1a8bf26

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

ahsonkhan reviewed Nov 26, 2019

View reviewed changes

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

ahsonkhan reviewed Nov 26, 2019

View reviewed changes

jozkee added 3 commits November 26, 2019 15:32

Address feedback.

81e5c53

Add paragraph to explain (overlap) in compat matrix

9f0b9de

Address more feedback and add spec for JsonPropertyNameAttribute coll…

94ba4e6

…isions.

ahsonkhan mentioned this pull request Nov 28, 2019

Add specification for ReferenceHandling dotnet/runtime#354

Merged

jozkee added 3 commits November 27, 2019 18:40

Address more feedback, corrected types and included table of contents

3f858b0

Add missing access modifiers to class properties.

c452838

Add note about immutable types

58bbff6

jozkee closed this Dec 10, 2019


		Community is heavily requesting this feature since is consider by many as a very common scenario, specially when serializing POCOs that came from an ORM Framework, such as Entity Framework; even though JSON specifiacation does not support reference loops by default. Therefore this will be shipped as an opt-in feature.

		The current solution to deal with reference loops is to rely in MaxDepth and throw a JsonException after it is exceeded. Now, this is a decent and cheap solution but we will also offer other not-so-cheap options to deal with this problem while keeping the current one in order to not affect the out-of-the-box performance.


		# Prior art

		Json.Net contains settings that you can enable to deal with such problems.


		With ReferenceHandling being a class, we can exclude things that, as of now, we are not sure if they need to be shipped, in example, the `ReferenceLoopHandling.Ignore` option or the Object and Array granularity of Json.Net's `PreserveReferencesHandling`.

		In a future, we might also be able to extend the Deserialization behaviors, in order to provide the granularity of Serialization on Deserialization. Or we could also be able to define independant behaviors for both Serialization and Deserialization; i.e: someone may want to emit payloads with preserved references but he does not want to read them.


		This is a heavily requested feature since it is consider by many as a very common scenario, specially when serializing POCOs that came from an ORM Framework, such as Entity Framework; even though the JSON specification does not support reference loops by default. Therefore, this will be implemented as an opt-in feature (for both serialization and deserialization).

		The current solution to deal with reference loops is to rely in MaxDepth and throw a JsonException after it is exceeded. Now, this is a decent and cheap solution but we will also offer other not-so-cheap options to deal with this problem while keeping the current one in order to not affect the out-of-the-box performance.


		Currently there is no mechanism to prevent infinite looping in circular objects (while serializing) nor to preserve references that round-trip when using System.Text.Json.

		This is a heavily requested feature since it is consider by many as a very common scenario, specially when serializing POCOs that came from an ORM Framework, such as Entity Framework; even though the JSON specification does not support reference loops by default. Therefore, this will be implemented as an opt-in feature (for both serialization and deserialization).


		This API is exposing the `ReferenceHandling` property as a class, to be extensible in the future; and provide built-in static instances of `Default` and `Preserve` that are useful to enable the most common behaviors by just setting those in `JsonSerializerOptions.ReferenceHandling`.

		With `ReferenceHandling` being a class, we can exclude things that, as of now, we are not sure are required and add them later based on customer feedback. For example, the `Object` and `Array` granularity of `Newtonsoft.Json's` ``PreserveReferencesHandling` or the `ReferenceLoopHandling.Ignore` option.

Add specification for ReferenceHandling #1

Add specification for ReferenceHandling #1

Uh oh!

Conversation

jozkee commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jozkee Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jozkee commented Nov 25, 2019 •

edited

Loading

ahsonkhan Nov 26, 2019 •

edited

Loading

jozkee Nov 26, 2019 •

edited

Loading