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

Do not deserialize using internal or private default ctors for all supported TFMs. #32213

Merged
merged 4 commits into from Feb 14, 2020

Conversation

ahsonkhan
Copy link
Member

@ahsonkhan ahsonkhan commented Feb 13, 2020

We have a discrepancy in S.T.Json behavior between the netstandard2.0 /2.1 implementation (including netcoreapp2.1 and below), and the NC3.0+ one, when deserializing JSON into POCOs with no public default parameterless ctors.

  • On NC3.0+, we allow internal/private ctors of POCOs and call them while deserializing.
  • On netstandard2.0/2.1, we don’t and throw a MissingMethodException: No parameterless constructor defined for this object. (from Activator.CreateInstance).
using System;
using System.Text.Json;

namespace NS20
{
    class Program
    {
        static void Main(string[] args)
        {
            Foo f = JsonSerializer.Deserialize<Foo>("{}");

            // Currently:
            // On netcoreapp3.0+, this outputs the type name: "NS20.Foo"
            // On netcoreapp2.1 and below, this throws: MissingMethodException
            Console.WriteLine(f.ToString());
        }
    }

    public class Foo
    {
        internal Foo()
        {

        }
    }
}

This happens because of the difference between the reflection-based MemberAccessor and the refemit-based MemberAccessor.

ReflectionMemberAccessor calls Activator.CreateInstance without any binding flags, so the default is public ctors only:

ReflectionEmitMemberAccessor gets the constructor using different binding flags (and hence ends up calling internal/private parameterless ctors as well):

ConstructorInfo? realMethod = type.GetConstructor(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance, binder: null, Type.EmptyTypes, modifiers: null);

The JsonSerializer shouldn't be calling non-public surface area of types, especially by default (whether that's ctor, properties, or fields). Hence, this change is making the behavior consistent across the board by removing support for internal/private default ctors on all supported TFMs (regardless of whether the implementation is using reflection or ref emit). This is also consistent with the rest of the deserializer behavior, where only public properties are supported by default with public getter/setters. We will do the same for fields when they are supported.

It is unlikely that many folks are relying on this behavior. However, as a workaround, if the user controls their own type and don't want to make the default ctor public for some reason, they could implement a JsonConverter<T> for their type and control the deserialization behavior. Additionally, we are currently working to provide an attribute based opt-in support for non-paramterless/default ctors, so it is possible that we extend that feature to also support internal/private default ctors, by having the user place an attribute on the ctor they want the JsonSerializer to use. See #29895

Note: This only affects deserialization/reading and doesn't change the serialization/writing behavior.

@ahsonkhan ahsonkhan added area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. labels Feb 13, 2020
@ahsonkhan ahsonkhan added this to the 5.0 milestone Feb 13, 2020
@ahsonkhan ahsonkhan self-assigned this Feb 13, 2020
@ahsonkhan
Copy link
Member Author

Waiting on review/approval to get this merged.

@ahsonkhan
Copy link
Member Author

The test failures are unrelated and logs don't indicate much:
https://helix.dot.net/api/2019-06-17/jobs/a8559cb9-fb40-4003-8814-b2ad05ebb4cc/workitems/System.Net.Quic.Tests/console

Executed on dci-mac-build-059
/bin/sh: /tmp/helix/working/AA850998/p/scripts/87031322f21940bb9077b4a77283c03d/execute.sh: Permission denied

Other test failure: #32335

@ahsonkhan
Copy link
Member Author

cc @dotnet/dnceng about the execute.sh: Permission denied error on mac machines

@ahsonkhan ahsonkhan merged commit 0174b98 into dotnet:master Feb 14, 2020
@ahsonkhan ahsonkhan deleted the OnlySupportPublic branch February 14, 2020 22:44
@jonfortescue
Copy link
Member

@ahsonkhan We think we have that fixed in the Mac queues as of this afternoon. Let us know if you see it again!

@danmoseley
Copy link
Member

@layomia this is labeled breaking change. Can you please open an issue if needed with https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md

I don't see an existing one

@danmoseley danmoseley added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 30, 2020
@layomia
Copy link
Contributor

layomia commented Oct 2, 2020

Breaking change doc filed - dotnet/docs#20887.

@layomia layomia removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 2, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants