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 a copy constructor to JsonSerializerOptions #30445

Closed
pranavkm opened this issue Aug 1, 2019 · 6 comments · Fixed by #34725
Closed

Add a copy constructor to JsonSerializerOptions #30445

pranavkm opened this issue Aug 1, 2019 · 6 comments · Fixed by #34725
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@pranavkm
Copy link
Contributor

pranavkm commented Aug 1, 2019

(Edit by @jozkee: proposed API is in a comment at #30445 (comment))

MVC has a scenario where it needs to start with an existing options instance, modify a single property on a copy of it (the encoder) and perform serialization. In the absence of a copy constructor, we're at risk of missing out on newly added properties to the type.

Proposed API

namespace System.Text.Json
{
    public sealed partial class JsonSerializerOptions
    {
        public JsonSerializerOptions(JsonSerializerOptions source)
        {
            /* Copy all options from source to a new JsonSerializerOptions instance. */
        }
    }
}
@ahsonkhan
Copy link
Member

MVC has a scenario where it needs to start with an existing options instance, modify a single property on a copy of it (the encoder) and perform serialization.

Can you provide a link to where it would be used in practice? What do you end up doing today? Setting/resetting the options after modifying/using it?

So are you looking for something like:

namespace System.Text.Json
{
   public sealed partial class JsonSerializerOptions
   {
      public JsonSerializerOptions(System.Text.Json.JsonSerializerOptions options) { };
   }
}

With sample usage:

public static object ModifyOptionsCopy(string json, JsonSerializerOptions currentOptions)
{
   if (currentOptions.Encoder != JavascriptEncoder.Default)
   {
      var newOptions = new JsonSerializerOptions (currentOptions);
      newOptions.Encoder = JavascriptEncoder.Default;
      return JsonSerializer.Deserialize(json, newOptions);
   }
   return null;
}

Would the underlying caches also get copied (i.e. full deep copy)?

In the absence of a copy constructor, we're at risk of missing out on newly added properties to the type.

Can you expand on what you mean by this?

@pranavkm
Copy link
Contributor Author

pranavkm commented Aug 1, 2019

Would the underlying caches also get copied (i.e. full deep copy)?

It doesn't need to. Users could independently cache the copied instance.

Can you expand on what you mean by this?

If you add a new property to the serializer options, MVC would need to react to it accordingly. This becomes complicated if the user happens to reference a newer version of the library. Using reflection to copy things over seems like overkill.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@jozkee
Copy link
Member

jozkee commented Mar 21, 2020

This should be very simple to implement and it could be very helpful since will allow our ASP.NET users to try-out preview features that need to be enabled from the options like ReferenceHandling.

Proposed API

namespace System.Text.Json
{
    public sealed partial class JsonSerializerOptions
    {
        public JsonSerializerOptions(JsonSerializerOptions source)
        {
            /* Copy all options from source to a new JsonSerializerOptions instance. */
        }
    }
}

cc @steveharter @layomia @terrajobst

@steveharter
Copy link
Member

steveharter commented Mar 23, 2020

/* Copy all options from source to a new JsonSerializerOptions instance. */

The state on the option instances can diverge after a clone so we need to ensure they don't contaminate eachother by sharing caches (the JsonClassInfo cache) or a direct reference to the converter list. Sharing caches can result in unnecessary references to Types (inability to GC the source options) and sharing the converter list reference can result in incorrect semantics.

For the clone, we should only add the converters that were manually added via options.Converters.Add() which is only allowed before the first (de)serialization.

@jozkee jozkee added api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 23, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work labels Mar 24, 2020
@terrajobst
Copy link
Member

terrajobst commented Mar 24, 2020

Video

  • Looks good
  • This will only copy settings, no cache information
  • Converters that were added automatically (e.g. factories) should not be copied.
  • Let's use options for the parameter name
namespace System.Text.Json
{
    public sealed partial class JsonSerializerOptions
    {
        public JsonSerializerOptions(JsonSerializerOptions options)
        {
            /* Copy all options from source to a new JsonSerializerOptions instance. */
        }
    }
}

@ahsonkhan
Copy link
Member

ahsonkhan commented Mar 24, 2020

Also capturing another note: The copied option that is returned should be "unlocked" and the user should be able to modify it until the first (de)serialization occurs (regardless of whether the original option was locked or unlocked).

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.

7 participants