Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Add testcase for #61734
  • Loading branch information
krwq authored and github-actions committed Aug 31, 2022
commit 480af474dd0272c97cf8836c33ac8e0d55b4f8a9
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ private void AddSource(string fileName, string source, bool isRootContextDef = f
bool isInGlobalNamespace = @namespace == JsonConstants.GlobalNamespaceValue;

StringBuilder sb = new(@"// <auto-generated/>

#nullable enable annotations
#nullable disable warnings
Copy link
Member

Choose a reason for hiding this comment

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

Are we ever generating public API? With annotations enabled we will still be outputting nullability metadata, and if we have reason to believe that the annotations are incorrect, that'll mean incorrect nullability information surfaced to consumers of the APIs.

If this is all internal to the assembly, it's probably fine. If it's not, we might instead consider doing:

#nullable disable
#pragma warning disable CS8632

instead, or something along those lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eiriktsarpalis do you want to resolve this question before I hit merge?

Copy link
Member

Choose a reason for hiding this comment

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

Yes let's please resolve this q first.

Copy link
Member

Choose a reason for hiding this comment

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

we do produce some public APIs - AFAIK they're correct though

Copy link
Member

Choose a reason for hiding this comment

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

they're correct though

Ok


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,22 @@ public static void ClassWithStringValuesRoundtrips()
Assert.Equal("""{"StringValuesProperty":["abc","def"]}""", json);
}

// Regression test for https://github.com/dotnet/runtime/issues/61734
[Fact]
public static void ClassWithDictionaryPropertyRoundtrips()
{
JsonSerializerOptions options = ClassWithDictionaryPropertyContext.Default.Options;

ClassWithDictionaryProperty obj = new(new Dictionary<string, object?>()
{
["foo"] = "bar",
["test"] = "baz",
});

string json = JsonSerializer.Serialize(obj, options);
Assert.Equal("""{"DictionaryProperty":{"foo":"bar","test":"baz"}}""", json);
}

[JsonConverter(typeof(JsonStringEnumConverter))]
public enum TestEnum
{
Expand All @@ -416,6 +432,11 @@ internal partial class ClassWithStringValuesContext : JsonSerializerContext
{
}

[JsonSerializable(typeof(ClassWithDictionaryProperty))]
internal partial class ClassWithDictionaryPropertyContext : JsonSerializerContext
{
}

internal class ClassWithPocoListDictionaryAndNullable
{
public uint UIntProperty { get; set; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,4 +281,10 @@ public sealed class ClassWithStringValues
{
public StringValues StringValuesProperty { get; set; }
}

public class ClassWithDictionaryProperty
{
public ClassWithDictionaryProperty(Dictionary<string, object?> property) => DictionaryProperty = property;
public Dictionary<string, object?> DictionaryProperty { get; }
}
}