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

Json parsing with struct and attributes changed from net6 to net7 #14858

Open
JohnnyFasching opened this issue Mar 7, 2023 · 13 comments
Open
Assignees
Labels
Bug Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. Regression Tracking-External
Milestone

Comments

@JohnnyFasching
Copy link

I upgraded my application from .net 6 to .net 7, and after the deployment I got problems, because the newtonsoft json parser returned null values, except of the values represented in the json string.

Repro steps

I provided a sample F# script and the json document, where you can see the difference between the two types I created.
The "TokenResponse_net6" type was working fine in .net 6, but not anymore in .net 7

Expected behavior

However when I modify the type like in my "TokenResponse_net7" the values get parsed properly.
It seems that there was a change in the annotations and probably also how the struct works with those

Newtonsoft.Json.zip

.NET SDK:
Version: 7.0.201
Commit: 68f2d7e7a3

Runtime Environment:
OS Name: Windows
OS Version: 10.0.22621
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\7.0.201\

@github-actions github-actions bot added this to the Backlog milestone Mar 7, 2023
@0101 0101 added Regression Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Needs-Confirmation and removed Needs-Triage labels Mar 13, 2023
@0101 0101 added Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. and removed Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. labels Mar 13, 2023
@vzarytovskii
Copy link
Member

vzarytovskii commented Mar 20, 2023

It sounds like some combination of changes in the deserializer of the nsj and our codegen for net7 (vs net6), since if you change your struct to

[<Struct; DataContract>]
type TokenResponse_net6 =
    {
        [<field: DataMember(Name = "access_token")>]
        access_token: string
        [<field: DataMember(Name = "token_type")>]
        token_type: string
    }

it deserializes just fine.

The following looks related, though seems to be an old issue
JamesNK/Newtonsoft.Json#1295

@vzarytovskii
Copy link
Member

Minimal repro is pretty much:

#r "nuget: Newtonsoft.Json,13.0.3"

open System.Runtime.Serialization

open Newtonsoft.Json

[<NoComparison; NoEquality; Struct; DataContract>]
type DataType =
    {
        [<DataMember(Name = "foo_field")>]
        Foo: string
    }

let json = """{"foo_field": "bar"}"""
printfn $"{JsonConvert.DeserializeObject<DataType>(json)}"

If you change foo_field to foo or Foo it works, as is changing type to reference type.

@vzarytovskii
Copy link
Member

vzarytovskii commented Mar 20, 2023

For the following F# code:

open System.Runtime.Serialization
[<NoComparison; NoEquality; Struct; DataContract>]
type DataType =
    {
        [<DataMember(Name = "foo_field")>]
        Foo: string
    }

Codegen we do in net6:

[Serializable]
[NoComparison]
[NoEquality]
[Struct]
[DataContract]
[CompilationMapping(SourceConstructFlags.RecordType)]
public struct DataType
{
	[DebuggerBrowsable(DebuggerBrowsableState.Never)]
	internal string Foo@;

	[DataMember(Name = "foo_field")]
	[CompilationMapping(SourceConstructFlags.Field, 0)]
	public string Foo
	{
		get
		{
			return Foo@;
		}
	}

	public DataType(string foo)
	{
		Foo@ = foo;
	}

	[CompilerGenerated]
	public override string ToString()
	{
		return ExtraTopLevelOperators.PrintFormatToString(new PrintfFormat<FSharpFunc<DataType, string>, Unit, string, string, DataType>("%+A")).Invoke(this);
	}
}

And net7:

[Serializable]
[NoComparison]
[NoEquality]
[Struct]
[DataContract]
[CompilationMapping(SourceConstructFlags.RecordType)]
public struct DataType
{
	[CompilerGenerated]
	[DebuggerBrowsable(DebuggerBrowsableState.Never)]
	internal string Foo@;

	[DataMember(Name = "foo_field")]
	[CompilationMapping(SourceConstructFlags.Field, 0)]
	public readonly string Foo
	{
		[CompilerGenerated]
		[DebuggerNonUserCode]
		get
		{
			return Foo@;
		}
	}

	public DataType(string foo)
	{
		Foo@ = foo;
	}

	[CompilerGenerated]
	public override string ToString()
	{
		return ExtraTopLevelOperators.PrintFormatToString(new PrintfFormat<FSharpFunc<DataType, string>, Unit, string, string, DataType>("%+A")).Invoke(this);
	}
}

So, the diff is pretty much:

index 99a4d54..d562226 100644
--- "a/.\\net6.cs"
+++ "b/.\\net7.cs"
@@ -6,13 +6,16 @@
 [CompilationMapping(SourceConstructFlags.RecordType)]
 public struct DataType
 {
+       [CompilerGenerated]
        [DebuggerBrowsable(DebuggerBrowsableState.Never)]
        internal string Foo@;

        [DataMember(Name = "foo_field")]
        [CompilationMapping(SourceConstructFlags.Field, 0)]
-       public string Foo
+       public readonly string Foo
        {
+               [CompilerGenerated]
+               [DebuggerNonUserCode]
                get
                {
                        return Foo@;

@vzarytovskii
Copy link
Member

vzarytovskii commented Mar 20, 2023

cc @0101 does this ring a bell? Do you remember where did we start generating those by any chance?

Makes me wonder though, what tips NSJ over though - attribute or readonly modifier on the property.

@vzarytovskii
Copy link
Member

vzarytovskii commented Mar 20, 2023

Although, if we change the field name to comply with the DataMember name, it codegens something like

open System.Runtime.Serialization
[<NoComparison; NoEquality; Struct; DataContract>]
type DataType =
    {
        [<DataMember(Name = "foo_field")>]
        foo_field: string
    }

NET6:

[Serializable]
[NoComparison]
[NoEquality]
[Struct]
[DataContract]
[CompilationMapping(SourceConstructFlags.RecordType)]
public struct DataType
{
    [DebuggerBrowsable(DebuggerBrowsableState.Never)]
    internal string foo_field@;

    [DataMember(Name = "foo_field")]
    [CompilationMapping(SourceConstructFlags.Field, 0)]
    public string foo_field
    {
        get
        {
            return foo_field@;
        }
    }

    public DataType(string foo_field)
    {
        foo_field@ = foo_field;
    }

    [CompilerGenerated]
    public override string ToString()
    {
        return ExtraTopLevelOperators.PrintFormatToString(new PrintfFormat<FSharpFunc<DataType, string>, Unit, string, string, DataType>("%+A")).Invoke(this);
    }
}

NET7:

[Serializable]
[NoComparison]
[NoEquality]
[Struct]
[DataContract]
[CompilationMapping(SourceConstructFlags.RecordType)]
public struct DataType
{
	[CompilerGenerated]
	[DebuggerBrowsable(DebuggerBrowsableState.Never)]
	internal string foo_field@;

	[DataMember(Name = "foo_field")]
	[CompilationMapping(SourceConstructFlags.Field, 0)]
	public readonly string foo_field
	{
		[CompilerGenerated]
		[DebuggerNonUserCode]
		get
		{
			return foo_field@;
		}
	}

	public DataType(string foo_field)
	{
		foo_field@ = foo_field;
	}

	[CompilerGenerated]
	public override string ToString()
	{
		return ExtraTopLevelOperators.PrintFormatToString(new PrintfFormat<FSharpFunc<DataType, string>, Unit, string, string, DataType>("%+A")).Invoke(this);
	}
}

So the diff becomes something like:

 [CompilationMapping(SourceConstructFlags.RecordType)]
 public struct DataType
 {
+    [CompilerGenerated]
     [DebuggerBrowsable(DebuggerBrowsableState.Never)]
     internal string foo_field@;

     [DataMember(Name = "foo_field")]
     [CompilationMapping(SourceConstructFlags.Field, 0)]
-    public string foo_field
+    public readonly string foo_field
     {
+        [CompilerGenerated]
+        [DebuggerNonUserCode]
         get
         {
             return foo_field@;

However, now it deserializes just fine.

@vzarytovskii
Copy link
Member

So, I would guess, in case of readonly fields, it does indeed look at names of the arguments in the .ctor, as suggested in JamesNK/Newtonsoft.Json#1295

Any ideas on how do we workaround it? Well, except the obvious - going back and don't codegen the readonly for the property.

@T-Gro
Copy link
Member

T-Gro commented Mar 22, 2023

I don't see any clean way apart from fixing this in Newtonsoft directly, to cover for this case.

@vzarytovskii
Copy link
Member

I guess we can revert the codegen too.

@JohnnyFasching
Copy link
Author

What was the main reasoning changing the codegen from public to public readonly?

@vzarytovskii
Copy link
Member

What was the main reasoning changing the codegen from public to public readonly?

Making it more runtime-friendly, so it doesn't do defensive copy in this case.

@T-Gro T-Gro modified the milestones: Backlog, April-2023 Apr 6, 2023
@vzarytovskii vzarytovskii removed this from the April-2023 milestone May 4, 2023
@T-Gro T-Gro added this to the May-2023 milestone May 4, 2023
@T-Gro
Copy link
Member

T-Gro commented May 17, 2023

Raised a PR in newtonsoft: JamesNK/Newtonsoft.Json#2860

@auduchinok
Copy link
Member

auduchinok commented Jun 1, 2023

The addition of DebuggerNonUserCode for some symbols also isn't expected. Consider let functions in types:

type T() =
    let f x = x + 1

It gets the attribute despite being a user defined method:
Screenshot 2023-06-01 at 17 41 23

Can this change be reverted for user-defined methods, please? Should I file a separate issue?

@vzarytovskii
Copy link
Member

Should I file a separate issue?

Yes please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Impact-High (Internal MS Team use only) Describes an issue with extreme impact on existing code. Regression Tracking-External
Projects
Status: Done
Development

No branches or pull requests

5 participants