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

Add support for POCO serialization callbacks #54528

Closed
steveharter opened this issue Jun 21, 2021 · 4 comments · Fixed by #54709
Closed

Add support for POCO serialization callbacks #54528

steveharter opened this issue Jun 21, 2021 · 4 comments · Fixed by #54709
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@steveharter
Copy link
Member

Background and Motivation

As part of making more extensible POCO converters, a low-risk feature for specifying notification callbacks on a POCO for serialization is possible in the V6 timeframe. The other extensibility POCO features are no longer feasible for V6 since they need 1-2 preview releases to be properly vetted.

These notifications are used primarily for defaulting and validation and are only called for POCOs and not collections or values.

This feature avoids using the non-ideal workaround of using a custom converter. The workaround slows performance (due to re-entry of the serializer and forcing read-ahead for Stream cases) and also breaks the "object chain" in the "JSON Path" which is used when exceptions are thrown.

The new attributes are similar to the ones in [System.Runtime.Serialization](https://docs.microsoft.com/en-us/dotnet/api/system.runtime.serialization.onserializingattribute?view=net-5.0. Like the rest of STJ, it adds its own attributes instead of re-using existing ones to prevent confusion over what is supported and not supported. Note that Newtonsoft's Json.NET also supports these System.Runtime.Serialization attributes.

These will also be supported in source-gen, although the callback must be internal or public.

Proposed API

namespace System.Text.Json.Serialization
{
+    // Most common attribute; can be used to:
+    // - Initialize unassigned properties.
+    // - Throw exception if missing or invalid state.
+    // - Set calculated properties in preparation for consumption.
+    [System.AttributeUsage(System.AttributeTargets.Method, Inherited = false, AllowMultiple = false)]
+    public sealed partial class JsonOnDeserializedAttribute : System.Text.Json.Serialization.JsonAttribute
+    {
+        public JsonOnDeserializedAttribute() { }
+    }

+    // Alternative to constructors to initialize state specific for deserialization.
+    [System.AttributeUsage(System.AttributeTargets.Method, Inherited = false, AllowMultiple = false)]
+    public sealed partial class JsonOnDeserializingAttribute : System.Text.Json.Serialization.JsonAttribute
+    {
+        public JsonOnDeserializingAttribute() { }
+    }

+     // Can be used to clear any temporary variables or state specific to serialization.
+    [System.AttributeUsage(System.AttributeTargets.Method, Inherited = false, AllowMultiple = false)]
+    public sealed partial class JsonOnSerializedAttribute : System.Text.Json.Serialization.JsonAttribute
+    {
+        public JsonOnSerializedAttribute() { }
+    }

+     // Can be used to:
+     // - Assign values to properties that have not been set.
+     // - Throw exception if missing or invalid state.
+     // - Set up any temporary variables used for serialization workarounds.
+    [System.AttributeUsage(System.AttributeTargets.Method, Inherited = false, AllowMultiple = false)]
+    public sealed partial class JsonOnSerializingAttribute : System.Text.Json.Serialization.JsonAttribute
+    {
+        public JsonOnSerializingAttribute() { }
+    }

Note that the method must not contain any return value or parameters and must be non-virtual, and there can only be one callback per notification. These are the same constraints as Json.NET.

Usage Examples

Here's a sample used to validate both incoming and outgoing JSON:

public class CreditAmount
{
    public decimal Amount { get; set; } = -1;
    public string Description { get; set; }

    [JsonOnSerializing]
    internal void OnSerializing()
    {
        Validate();
    }

    [JsonOnDeserialized]
    internal void OnDeserialized()
    {
        Validate();
    }

    private void Validate()
    {
        if (Amount < 0)
        {
            // Re-thrown as JsonException by the serializer
            throw new InvalidOperationException("Amount must be >= 0.");
        }

        if (string.IsNullOrEmpty(Description))
        {
            // Re-thrown as JsonException by the serializer
            throw new InvalidOperationException("Description must contain a value.");
        }
    }
}

Alternative Designs

A nice feature of this is that the logic for defaulting and validation can exist on each POCO for owned types instead of a separate class. However, no mechanism is currently supported to provide callbacks for non-owned types. For these non-owned types, the existing workaround of using a custom converter must be used. In V7+, once the JsonTypeInfo class is generally exposed, a Func can be exposed to handle those scenarios.

In V7+ it is also possible to add a new type of POCO converter that may contain equivalent virtual methods, or a metadata provider where a single callback can receive notifications for all POCO types.

There is no state passed to the callbacks. In V7+, it is possible to pass an object that has access to the JSON path and\or user-defined state that is preserved for a given root-level serialization call to assist with passing state between the "before" and "after" callbacks.

Risks

In general, this is a low-risk approach as other serializers essentially use the same approach.

For reflection cases, the linker already assumes any member on a POCO can be called; for source-gen the callback is generated and thus supports the linker that way.

If we decide to support an object passed to the callbacks, this can be added later. However, for source-gen usages, it that may have limitations if it contains a mechanism to obtain the JSON path since that is unlikely to be supported by source-gen for performance reasons.

No measurable cold startup cost was detected for the reflection case where the startup cost is measured ~20ms for a console app startup. At run-time, once warm-up has occurred, two simple checks for a null Func are necessary (one each for before\after).

@steveharter steveharter added area-System.Text.Json blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 21, 2021
@steveharter steveharter added this to the 6.0.0 milestone Jun 21, 2021
@steveharter steveharter self-assigned this Jun 21, 2021
@ghost
Copy link

ghost commented Jun 21, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

As part of making more extensible POCO converters, a low-risk feature for specifying notification callbacks on a POCO for serialization is possible in the V6 timeframe. The other extensibility POCO features are no longer feasible for V6 since they need 1-2 preview releases to be properly vetted.

These notifications are used primarily for defaulting and validation and are only called for POCOs and not collections or values.

This feature avoids using the non-ideal workaround of using a custom converter. The workaround slows performance (due to re-entry of the serializer and forcing read-ahead for Stream cases) and also breaks the "object chain" in the "JSON Path" which is used when exceptions are thrown.

The new attributes are similar to the ones in [System.Runtime.Serialization](https://docs.microsoft.com/en-us/dotnet/api/system.runtime.serialization.onserializingattribute?view=net-5.0. Like the rest of STJ, it adds its own attributes instead of re-using existing ones to prevent confusion over what is supported and not supported. Note that Newtonsoft's Json.NET also supports these System.Runtime.Serialization attributes.

These will also be supported in source-gen, although the callback must be internal or public.

Proposed API

namespace System.Text.Json.Serialization
{
+    // Most common attribute; can be used to:
+    // - Initialize unassigned properties.
+    // - Throw exception if missing or invalid state.
+    // - Set calculated properties in preparation for consumption.
+    [System.AttributeUsage(System.AttributeTargets.Method, Inherited = false, AllowMultiple = false)]
+    public sealed partial class JsonOnDeserializedAttribute : System.Text.Json.Serialization.JsonAttribute
+    {
+        public JsonOnDeserializedAttribute() { }
+    }

+    // Alternative to constructors to initialize state specific for deserialization.
+    [System.AttributeUsage(System.AttributeTargets.Method, Inherited = false, AllowMultiple = false)]
+    public sealed partial class JsonOnDeserializingAttribute : System.Text.Json.Serialization.JsonAttribute
+    {
+        public JsonOnDeserializingAttribute() { }
+    }

+     // Can be used to clear any temporary variables or state specific to serialization.
+    [System.AttributeUsage(System.AttributeTargets.Method, Inherited = false, AllowMultiple = false)]
+    public sealed partial class JsonOnSerializedAttribute : System.Text.Json.Serialization.JsonAttribute
+    {
+        public JsonOnSerializedAttribute() { }
+    }

+     // Can be used to:
+     // - Assign values to properties that have not been set.
+     // - Throw exception if missing or invalid state.
+     // - Set up any temporary variables used for serialization workarounds.
+    [System.AttributeUsage(System.AttributeTargets.Method, Inherited = false, AllowMultiple = false)]
+    public sealed partial class JsonOnSerializingAttribute : System.Text.Json.Serialization.JsonAttribute
+    {
+        public JsonOnSerializingAttribute() { }
+    }

Note that the method must not contain any return value or parameters and must be non-virtual, and there can only be one callback per notification. These are the same constraints as Json.NET.

Usage Examples

Here's a sample used to validate both incoming and outgoing JSON:

public class CreditAmount
{
    public decimal Amount { get; set; } = -1;
    public string Description { get; set; }

    [JsonOnSerializing]
    internal void OnSerializing()
    {
        Validate();
    }

    [JsonOnDeserialized]
    internal void OnDeserialized()
    {
        Validate();
    }

    private void Validate()
    {
        if (Amount < 0)
        {
            // Re-thrown as JsonException by the serializer
            throw new InvalidOperationException("Amount must be >= 0.");
        }

        if (string.IsNullOrEmpty(Description))
        {
            // Re-thrown as JsonException by the serializer
            throw new InvalidOperationException("Description must contain a value.");
        }
    }
}

Alternative Designs

A nice feature of this is that the logic for defaulting and validation can exist on each POCO for owned types instead of a separate class. However, no mechanism is currently supported to provide callbacks for non-owned types. For these non-owned types, the existing workaround of using a custom converter must be used. In V7+, once the JsonTypeInfo class is generally exposed, a Func can be exposed to handle those scenarios.

In V7+ it is also possible to add a new type of POCO converter that may contain equivalent virtual methods, or a metadata provider where a single callback can receive notifications for all POCO types.

There is no state passed to the callbacks. In V7+, it is possible to pass an object that has access to the JSON path and\or user-defined state that is preserved for a given root-level serialization call to assist with passing state between the "before" and "after" callbacks.

Risks

In general, this is a low-risk approach as other serializers essentially use the same approach.

For reflection cases, the linker already assumes any member on a POCO can be called; for source-gen the callback is generated and thus supports the linker that way.

If we decide to support an object passed to the callbacks, this can be added later. However, for source-gen usages, it that may have limitations if it contains a mechanism to obtain the JSON path since that is unlikely to be supported by source-gen for performance reasons.

No measurable cold startup cost was detected for the reflection case where the startup cost is measured ~20ms for a console app startup. At run-time, once warm-up has occurred, two simple checks for a null Func are necessary (one each for before\after).

Author: steveharter
Assignees: steveharter
Labels:

api-ready-for-review, area-System.Text.Json, blocking

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 21, 2021
@terrajobst
Copy link
Member

  • We should make sure this feature works well in source generation
    • We can either say we don't support private/internal at all
    • We could say we support private/internal only when used in reflection mode
    • Or we could change the shape of the feature for this problem to disappear, for example, by using interfaces instead.
namespace System.Text.Json.Serialization
{
    public interface IJsonOnDeserializing
    {
        void OnDeserializing();
    }

    public interface IJsonOnDeserialized
    {
        void OnDeserialized();
    }

    public interface IJsonOnSerializing
    {
        void OnSerializing();
    }

    public interface IJsonOnSerialized
    {
        void OnSerialized();
    }
}

@terrajobst terrajobst 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 Jun 22, 2021
@GrabYourPitchforks
Copy link
Member

There is no state passed to the callbacks. In V7+, it is possible to pass an object that has access to the JSON path and\or user-defined state that is preserved for a given root-level serialization call to assist with passing state between the "before" and "after" callbacks.

If you go with an interface-based approach, how does this mesh? Would you need to define an IJsonOnSerializing2 interface with a parameterful callback routine?

@steveharter steveharter removed blocking Marks issues that we want to fast track in order to unblock other important work untriaged New issue has not been triaged by the area owner labels Jun 24, 2021
@steveharter
Copy link
Member Author

steveharter commented Jun 24, 2021

If you go with an interface-based approach, how does this mesh? Would you need to define an IJsonOnSerializing2 interface with a parameterful callback routine?

Yes. I mentioned this during the review that one advantage to attributes is that the same attribute could be applied to different signatures in the future.

The most likely parameter that would be passed is the existing ReadStack and WriteStack which should get renamed to DeserializeState and SerializeState. These are also value types, so they would have to be passed via ref.

So we can either:

  1. Create the interfaces as-is above and add IJsonOnSerializingWithState() etc later if necessary.
  2. Create the interfaces above with a ref DeserializeState state or ref SerializeState state and make those state classes public. For now, the state classes don't need to expose any public members, but in the future may allow a general-use property bag and a way to get the JsonPath for throwing exceptions.

However, these state classes wouldn't likely work with source-gen since source-gen by design does not want to pass these state references around (mostly since it implies tracking the JSON property paths including the parent property name)...

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 24, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 9, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 8, 2021
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants