Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add extension data support to Json serializer#37690

Merged
steveharter merged 3 commits intodotnet:masterfrom
steveharter:Overflow3
May 17, 2019
Merged

Add extension data support to Json serializer#37690
steveharter merged 3 commits intodotnet:masterfrom
steveharter:Overflow3

Conversation

@steveharter
Copy link
Copy Markdown
Contributor

Replaces PR #37578 which had issues with a corrupt branch.

Adds the overflow or data extension support via the new JsonExtensionDataAttribute

Other changes:

Open issues:

  • We do not apply serializer-only options to the reader\writer:
    • A property naming policy, such as a possible ExtensionDataPropertyNaming property. The existing naming policies for normal properties and dictionary keys.
    • IgnoreNullValues.
  • This does not support Stream scenarios yet where the current buffer may run out of data. This will be addressed along with similar cases where this is encountered including the object->JsonElement converter and the extensibility model where the reader is exposed.

Debug.Assert(entry.Key is string);

string propertyName = (string)entry.Key;
element.WriteAsProperty(propertyName, writer);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, I think we may need the explicit call to AsSpan() (since the span to string implicit converter is on core only)

We should just add the WriteAsProperty string-based overload to JsonElement to avoid this concern all together.

System\Text\Json\Serialization\JsonSerializer.Write.HandleDictionary.cs(164,41): error CS1503: Argument 1: cannot convert from 'string' to 'System.ReadOnlySpan'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you start a separate discussion to add the public API?

For now, I'll just do the AsSpan(). When the overload is added we will need to check for all usages and clean up as appropriate.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you start a separate discussion to add the public API?

Yes. Added the overload. See: #37789

Copy link
Copy Markdown

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, LGTM

<value>The data extension property '{0}.{1}' does not match the required signature of IDictionary&lt;string, JsonElement&gt; or IDictionary&lt;string, object&gt;.</value>
</data>
<data name="SerializationDuplicateAttribute" xml:space="preserve">
<value>The attribute '{0}' cannot exist on more than one property.</value>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add, within the same class?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better: A class cannot have more than one property that has the attribute '{0}'.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better

Yes.

However, re-thinking about this, what happens when we add struct support? Is there a generic term that can encapsulate both .NET structs and classes? Type? Object?

Maybe type definition?
A type definition cannot have more than one property that has the attribute '{0}'.

Comment thread src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs Outdated
Comment thread src/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs Outdated
@steveharter steveharter merged commit 71487a4 into dotnet:master May 17, 2019
@steveharter
Copy link
Copy Markdown
Contributor Author

There is some feedback that will be addressed in a future commit.

@steveharter steveharter deleted the Overflow3 branch May 17, 2019 14:56
@karelz karelz added this to the 3.0 milestone May 22, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deserializer should support only getter for IDictionary properties

3 participants